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

WebGPURenderer: Fix SampledTexture not correctly bound #28289

Merged
merged 4 commits into from May 8, 2024

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented May 6, 2024

Related issue: #28268

Description

When resizing a RenderTarget, for example using a PostProcess pipeline with a PassNode, the resize will trigger this.renderTarget.setSize() which will dispatch dispose destroying the associated textures in the backend and then lose the proper binding on the GPU side:

_destroyTexture( texture ) {

		this.backend.destroySampler( texture );
		this.backend.destroyTexture( texture );

		this.delete( texture );

}

This PR fix SampledTexture binding update. Related comment #28268 (comment)
With this fix, the error seems to no longer appears.

This contribution is funded by Utsubo

@RenaudRohlinger
Copy link
Collaborator Author

All E2E testing seems broken on all recent PRs 🤷‍♂️ /cc @Mugen87

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2024

Sorry, I'm not familiar with the E2E tests so I have no idea what's going wrong. I try to disable E2E testing manually for the moment.

@Mugen87 Mugen87 added this to the r165 milestone May 6, 2024
@RenaudRohlinger RenaudRohlinger marked this pull request as draft May 6, 2024 13:22
@RenaudRohlinger RenaudRohlinger marked this pull request as ready for review May 6, 2024 14:02
@RenaudRohlinger RenaudRohlinger changed the title WebGPURenderer: Update texture Sampler when binding is updated WebGPURenderer: Fix Sampler and SampledTexture not correctly bound May 6, 2024
@RenaudRohlinger RenaudRohlinger changed the title WebGPURenderer: Fix Sampler and SampledTexture not correctly bound WebGPURenderer: Fix SampledTexture not correctly bound May 7, 2024
@RenaudRohlinger
Copy link
Collaborator Author

@sunag Does this change seems ok with you? After 2 days of tests I can't reproduce my issue anymore when resizing the window on PostProcess setup. (I would suggest to remove the getter needsBindingsUpdate from NodeSampledTexture too).

@sunag
Copy link
Collaborator

sunag commented May 8, 2024

@sunag Does this change seems ok with you? After 2 days of tests I can't reproduce my issue anymore when resizing the window on PostProcess setup. (I would suggest to remove the getter needsBindingsUpdate from NodeSampledTexture too).

Exactly what I would suggest. This section, if I'm not mistaken, was supposed to load a VideoTexture url outside the declaration, but I don't think it will be necessary anymore.

Another thing that caught my attention. I think const texture = binding.texture variable should be declared after binding.update() to obtain the updated reference.

@RenaudRohlinger
Copy link
Collaborator Author

Make sense! Indeed needsBindingsUpdate wasn't being used anymore. I cleaned up SampledTexture too.

@sunag sunag merged commit 2ab4c88 into mrdoob:dev May 8, 2024
8 of 12 checks passed
RenaudRohlinger added a commit to RenaudRohlinger/three.js that referenced this pull request May 17, 2024
RenaudRohlinger added a commit that referenced this pull request May 17, 2024
* revert #28289

* log patch should only cover webgpu

* prettify code
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.

None yet

3 participants