Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 11, 2025

Fixed #30503.

Description

Managing a global instance of PMREMGenerator in the renderer solves issues when more than one renderer is used in a multi-canvas setup.

I have updated the PR since integrating PMREMGenerator in the renderer is bad for tree-shaking. Since there are usually not many PMREMNode instances per scene it should be safe when each node manages its own generator.

@github-actions
Copy link

github-actions bot commented Feb 11, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.42
78.33
336.42
78.33
+0 B
+0 B
WebGPU 518
143.91
518.01
143.8
+12 B
-110 B
WebGPU Nodes 517.46
143.8
517.48
143.69
+12 B
-111 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.41
112.17
465.41
112.17
+0 B
+0 B
WebGPU 589.87
160
589.88
159.76
+11 B
-240 B
WebGPU Nodes 545.24
149.54
545.25
149.23
+11 B
-314 B

@Mugen87 Mugen87 added this to the r174 milestone Feb 12, 2025
@x0YYYY0
Copy link

x0YYYY0 commented Feb 12, 2025

Thank you 💯 !

@Mugen87 Mugen87 changed the title Renderer: Manage PMREMGenerator in renderer instead of node builder. PMREMNode: Manage own generator. Feb 12, 2025
@Mugen87 Mugen87 marked this pull request as draft February 12, 2025 10:05

const texture = this.value;

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

@Mugen87 Mugen87 Feb 12, 2025

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.

@Mugen87 Mugen87 marked this pull request as ready for review February 12, 2025 22:48
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 12, 2025

The PR also moves the texture cache in the class scope since otherwise a similar error like #30503 can occur when the same instance of Texture is used in two renderers. One renderer would generate a PMREM texture that would be shared by another one which does not work. PMREM textures are render target textures and WebGL/WebGPU resources can't be shared across contexts.

@sunag
Copy link
Collaborator

sunag commented Feb 13, 2025

The PR also moves the texture cache in the class scope since otherwise a similar error like #30503 can occur when the same instance of Texture is used in two renderers.

I think it's better to relate the cache to the renderer, for example cache[ renderer ][ texture ], if we leave it in class scope we will probably have duplicate PMREM.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 13, 2025

Great idea! I've added the cache back which is now hierarchical and respects the current active renderer.

@Mugen87 Mugen87 merged commit f27108e into mrdoob:dev Feb 13, 2025
12 checks passed
@Mugen87 Mugen87 changed the title PMREMNode: Manage own generator. PMREMNode: Manage own generator, fix caching. Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Environments with more than one WebGPURenderers / canvases do not work

3 participants