Skip to content
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.

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

Docs: Improve dispose guide. #30524

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

catalin-enache
Copy link
Contributor

@catalin-enache catalin-enache commented Feb 14, 2025

Related issue: #30516

Description

Disposing pmremGenerator in WebGLCubeUVMaps
added backgroundDispose method WebGLRenderer.

Both are needed so that when deeply cleaning the scene
no texture and geometry remain hanging around, reported as being in use by renderer.info.memory

the geometry created by WebGLBackground can be disposed now like: renderer.backgroundDispose()

EDIT:
I reverted all proposed changes replacing them with a new mention in the doc about internal geometries/textures that cannot be disposed.

Copy link

github-actions bot commented Feb 14, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.42
78.33
336.49
78.36
+67 B
+33 B
WebGPU 518.01
143.8
518.01
143.8
+0 B
+0 B
WebGPU Nodes 517.48
143.69
517.48
143.69
+0 B
+0 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.48
112.2
+67 B
+33 B
WebGPU 589.88
159.76
589.88
159.76
+0 B
+0 B
WebGPU Nodes 545.25
149.23
545.25
149.23
+0 B
+0 B

@@ -331,6 +331,7 @@ class WebGLRenderer {
_this.shadowMap = shadowMap;
_this.state = state;
_this.info = info;
_this.background = background; // allows using renderer.background.dispose() when deep cleaning the scene
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm not voting doing that since the entire component becomes public and that is not wanted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I said before it is not our goal to make everything disposable. The backgrounds resources do not need a clean up.

Copy link
Contributor Author

@catalin-enache catalin-enache Feb 14, 2025

Choose a reason for hiding this comment

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

Ok, added a new commit replacing exposed background with disposing method, not exposing the background itself anymore.

Copy link
Collaborator

@Mugen87 Mugen87 Feb 14, 2025

Choose a reason for hiding this comment

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

TBH, I'm not in favor of disposeBackground(). I would rather hide the internal objects from the statistics.

There is really no reason for users to care about a few internal geometries or materials. They do not harm the runtime behavior of your app in any means. I'm afraid you are taking this issue too precisely.

@@ -106,6 +106,7 @@ function WebGLCubeUVMaps( renderer ) {

cubeUVmaps.delete( texture );
cubemapUV.dispose();
pmremGenerator.dispose();
Copy link
Collaborator

@Mugen87 Mugen87 Feb 14, 2025

Choose a reason for hiding this comment

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

If you call dispose() the generator it's internal resource must be created from scratch during the next usage. That is not a correct fix. We must find out why the generator adds up texture references and apply a fix inside PMREMGenerator.

Copy link
Collaborator

@Mugen87 Mugen87 Feb 14, 2025

Choose a reason for hiding this comment

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

Is it possible for you to share a fiddle that demonstrate the leak in PMREMGenerator with an isolated test case?

Copy link
Contributor Author

@catalin-enache catalin-enache Feb 14, 2025

Choose a reason for hiding this comment

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

https://jsfiddle.net/catalin_enache/rfyc9jmo/44/

The fiddle is just about a plane and envMap or scene.environment.
I had to let deepCleanScene helper in the fiddle (I know is large, but after checking it does the right job it can be collapsed).

THe PMREMGenerator itself does not leak.
The fact that it cannot be disposed leaks.
The texture that leaks is in _pingPongRenderTarget and it adds up.
The 12 geometries leak but do not add up, however they should be disposed along with the texture.

Also added a small update which should recreated pmremGenerator next time

Copy link
Collaborator

@Mugen87 Mugen87 Feb 14, 2025

Choose a reason for hiding this comment

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

The texture that leaks is in _pingPongRenderTarget and it adds up.

I have simplified your fiddle a bit and there are no signs that PMREMGenerator leaks:

https://jsfiddle.net/4kqbgmse/

Copy link
Contributor Author

@catalin-enache catalin-enache Feb 14, 2025

Choose a reason for hiding this comment

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

Thank you for taking the time to look int it.
Interesting.
It seems that it leaks only when doing this inside the loop:

renderer.renderLists.dispose();
renderer.properties.dispose();
renderer.info.reset();

which is part of renderer.dispose() using exposed things on the renderer.

https://jsfiddle.net/catalin_enache/ndku0pbw/6/

So, the idea is to not touch those in the process of deep cleaning the scene to force going close to zero the geometries and textures.

@catalin-enache
Copy link
Contributor Author

Wait until Monday please, I want to do more tests in my app.
But basically if disposing the other things exposed on the renderer were the reason of leaking, and with having this in mind to not touch them I could say it is no real bug (just something not ideal, but not harming).
Monday I'll close the ticket, after doing some other tests.
Thank you so much again.

@catalin-enache catalin-enache force-pushed the 30516-scene-cleaning-improvements branch from 6cb16ca to 9c01215 Compare February 17, 2025 10:15
@catalin-enache catalin-enache force-pushed the 30516-scene-cleaning-improvements branch 3 times, most recently from 054356a to 7c67239 Compare February 17, 2025 11:19
@catalin-enache
Copy link
Contributor Author

catalin-enache commented Feb 17, 2025

I reverted all proposed changes replacing them with a new mention in the doc about internal geometries/textures that cannot be disposed.

@catalin-enache catalin-enache force-pushed the 30516-scene-cleaning-improvements branch 2 times, most recently from 9aa9a30 to a1eb29a Compare February 17, 2025 11:42
@catalin-enache catalin-enache force-pushed the 30516-scene-cleaning-improvements branch from a1eb29a to e9c72ff Compare February 17, 2025 11:45
@Mugen87 Mugen87 merged commit 517e9b7 into mrdoob:dev Feb 17, 2025
1 of 2 checks passed
@Mugen87 Mugen87 added this to the r174 milestone Feb 17, 2025
@Mugen87 Mugen87 changed the title 30516 scene cleaning improvements Docs: Improve dispose guide. Feb 17, 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.

2 participants