Skip to content

PMREMNode: Manage own generator, fix caching. #30504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Mugen87 merged 5 commits into from
Feb 13, 2025
Merged

PMREMNode: Manage own generator, fix caching. #30504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5aaf6ee
PMREMNode: Manage own generator.
Mugen87 Feb 12, 2025
10a04d7
PMREMNode: Fix import.
Mugen87 Feb 12, 2025
54b4247
PMREMNode: Simplify code.
Mugen87 Feb 12, 2025
370b8ee
PMREMNode: Clean up.
Mugen87 Feb 12, 2025
bd4cf40
PMREMNode: Fix cache.
Mugen87 Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions src/nodes/core/NodeBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ import { getCurrentStack, setCurrentStack } from '../tsl/TSLBase.js';
import CubeRenderTarget from '../../renderers/common/CubeRenderTarget.js';
import ChainMap from '../../renderers/common/ChainMap.js';

import PMREMGenerator from '../../renderers/common/extras/PMREMGenerator.js';

import BindGroup from '../../renderers/common/BindGroup.js';

import { REVISION, IntType, UnsignedIntType, LinearFilter, LinearMipmapNearestFilter, NearestMipmapLinearFilter, LinearMipmapLinearFilter } from '../../constants.js';
Expand Down Expand Up @@ -472,19 +470,6 @@ class NodeBuilder {

}

/**
* Factory method for creating an instance of {@link PMREMGenerator}.
*
* @return {PMREMGenerator} The PMREM generator.
*/
createPMREMGenerator() {

// TODO: Move Materials.js to outside of the Nodes.js in order to remove this function and improve tree-shaking support

return new PMREMGenerator( this.renderer );

}

/**
* Whether the given node is included in the internal array of nodes or not.
*
Expand Down
70 changes: 47 additions & 23 deletions src/nodes/pmrem/PMREMNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ import { uniform } from '../core/UniformNode.js';
import { NodeUpdateType } from '../core/constants.js';
import { nodeProxy, vec3 } from '../tsl/TSLBase.js';

import { WebGLCoordinateSystem } from '../../constants.js';
import { Texture } from '../../textures/Texture.js';

let _generator = null;
import PMREMGenerator from '../../renderers/common/extras/PMREMGenerator.js';

const _cache = new WeakMap();

Expand All @@ -32,15 +30,19 @@ function _generateCubeUVSize( imageHeight ) {
}

/**
* Generates a PMREM from the given texture .
* Generates a PMREM from the given texture.
*
* @private
* @param {Texture} texture - The texture to create the PMREM for.
* @param {Renderer} renderer - The renderer.
* @param {PMREMGenerator} generator - The PMREM generator.
* @return {Texture} The PMREM.
*/
function _getPMREMFromTexture( texture ) {
function _getPMREMFromTexture( texture, renderer, generator ) {

const cache = _getCache( renderer );

let cacheTexture = _cache.get( texture );
let cacheTexture = cache.get( texture );

const pmremVersion = cacheTexture !== undefined ? cacheTexture.pmremVersion : - 1;

Expand All @@ -52,7 +54,7 @@ function _getPMREMFromTexture( texture ) {

if ( isCubeMapReady( image ) ) {

cacheTexture = _generator.fromCubemap( texture, cacheTexture );
cacheTexture = generator.fromCubemap( texture, cacheTexture );

} else {

Expand All @@ -65,7 +67,7 @@ function _getPMREMFromTexture( texture ) {

if ( isEquirectangularMapReady( image ) ) {

cacheTexture = _generator.fromEquirectangular( texture, cacheTexture );
cacheTexture = generator.fromEquirectangular( texture, cacheTexture );

} else {

Expand All @@ -77,14 +79,38 @@ function _getPMREMFromTexture( texture ) {

cacheTexture.pmremVersion = texture.pmremVersion;

_cache.set( texture, cacheTexture );
cache.set( texture, cacheTexture );

}

return cacheTexture.texture;

}

/**
* Returns a cache that stores generated PMREMs for the respective textures.
* A cache must be maintaned per renderer since PMREMs are render target textures
* which can't be shared across render contexts.
*
* @private
* @param {Renderer} renderer - The renderer.
* @return {WeakMap<Texture, Texture>} The PMREM cache.
*/
function _getCache( renderer ) {

let rendererCache = _cache.get( renderer );

if ( rendererCache === undefined ) {

rendererCache = new WeakMap();
_cache.set( renderer, rendererCache );

}

return rendererCache;

}

/**
* This node represents a PMREM which is a special type of preprocessed
* environment map intended for PBR materials.
Expand Down Expand Up @@ -234,7 +260,7 @@ class PMREMNode extends TempNode {

}

updateBefore() {
updateBefore( frame ) {

let pmrem = this._pmrem;

Expand All @@ -249,7 +275,7 @@ class PMREMNode extends TempNode {

} else {

pmrem = _getPMREMFromTexture( texture );
pmrem = _getPMREMFromTexture( texture, frame.renderer, this._generator );

}

Expand All @@ -267,14 +293,12 @@ class PMREMNode extends TempNode {

setup( builder ) {

if ( _generator === null ) {
if ( this._generator === null ) {

_generator = builder.createPMREMGenerator();
this._generator = new PMREMGenerator( builder.renderer );

}

//

this.updateBefore( builder );

//
Expand All @@ -289,14 +313,6 @@ class PMREMNode extends TempNode {

//

const texture = this.value;

if ( builder.renderer.coordinateSystem === WebGLCoordinateSystem && texture.isPMREMTexture !== true && texture.isRenderTargetTexture === true ) {
Copy link
Collaborator Author

@Mugen87 Mugen87 Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunag This check causes issues in the existing implementation (so without this PR) since depending on the scene setup the reflections are incorrectly flipped along the x-axis. Here is a slightly modified version of webgpu_cubemap_dynamic that demonstrates the issue:

https://jsfiddle.net/L7zn6bqs/1/

I understand you have added the check in #27988 to fix webgpu_cubemap_dynamic in the first place but it seems a different approach is needed. When each PMREMNode has its own generator, the check should no longer be required.


uvNode = vec3( uvNode.x.negate(), uvNode.yz );

}

uvNode = vec3( uvNode.x, uvNode.y.negate(), uvNode.z );

//
Expand All @@ -315,6 +331,14 @@ class PMREMNode extends TempNode {

}

dispose() {

super.dispose();

if ( this._generator !== null ) this._generator.dispose();

}

}

export default PMREMNode;
Expand Down