-
-
Notifications
You must be signed in to change notification settings - Fork 36.2k
WebGPURenderer: Prevent dispose of default textures. #31860
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
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
@Mugen87 thank you, happy it helps! |
|
|
||
| this.backend.destroySampler( texture ); | ||
| this.backend.destroyTexture( texture ); | ||
| this.backend.destroyTexture( texture, isDefaultTexture ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we put the condition here instead of taking it to the backend?
if ( isDefaultTexture === false ) {
this.backend.destroyTexture( texture );
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the backend does not delete the texture data which is done inside the destroyTexture() methods. Meaning:
backend.delete( texture );That was indeed the main reason why I have introduced a new parameter.
We could move backend.delete( texture ); into Textures._destroyTexture() however WebGPUTextureUtils.destroyTexture() is used at other places as well so we would have to copy this line multiple times which seems error prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You tried to add/use a flag like isDefault through the backend method backend.createDefaultTexture() similar to the one used in WebGLBackend?
three.js/src/renderers/webgl-fallback/utils/WebGLTextureUtils.js
Lines 410 to 414 in 8c698fb
| backend.set( texture, { | |
| textureGPU, | |
| glTextureType, | |
| isDefault: true | |
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the same flag in WebGPUTextureUtils for testing but unfortunately this introduces a regression that textures are not destroyed anymore at all since the isDefault flag persists with the texture data. It would be necessary to remove this flag or set it to false when the real texture data are available.
This kind of management already exists in Textures and it seems error prone to duplicate it in both backends.
For consistency reasons, I have removed the isDefault flag from WebGLTextureUtils since it was not evaluated it the code base anyway.
Related issue: #31747
Description
@Makio64's live example from #31747 (comment) was really helpful to fix an edge case in
WebGPURenderer's texture management.When you start loading a texture and call
dispose()during the loading process, the backends delete the GPU texture objects of the default textures. That should not happen since these objects are cached and shared across other compatible textures. The PR fixes this issue my performing thedispose()as usual except for the deletion of the GPU texture object. That requires to delegate theisDefaultTextureflag down to the texture utils components for both backends.