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: Move the textures.updateRenderTarget logic to Renderer.setRenderTarget #28392

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented May 16, 2024

Related issue: #28289 #28372

Description

This PR moves the logic of updating the textures bound to a renderTarget into the setRenderTarget method:

setRenderTarget( renderTarget, activeCubeFace = 0, activeMipmapLevel = 0 ) {
    
    this._renderTarget = renderTarget;
    this._activeCubeFace = activeCubeFace;
    this._activeMipmapLevel = activeMipmapLevel;
    
    if ( renderTarget !== null ) {
    
	    this._textures.updateRenderTarget( renderTarget, activeMipmapLevel );
    
    }

}

At my company, we are currently working on a complex WebGPU project since months and have a deeply nested postprocessing setup. Occasionally, when the FPS drops or the CPU/GPU are under heavy load, when resizing the window a rare error occurs when trying to access the texture bound to the RTT in WebGPUBindingUtils, notably using RTT inside an node.updateBefore call:

//  textureData.texture is not defined since textureData = {} as it was never initialized by the `Textures` class.
resourceGPU = textureData.texture.createView( { aspect: aspectGPU, dimension: dimensionViewGPU, mipLevelCount: binding.store ? 1 : textureData.mipLevelCount } );`

This PR is the 3rd attempt of trying to fix this issue as I'm now pretty sure it is an issue inside the core of the WebGPURenderer pipeline.
I believe relocating this logic to the setRenderTarget method of the Renderer pipeline make more sense as it is where we want to bind and allocate the proper ressources associated to a RenderTarget, additionally, this change appears to resolve my issue.

This contribution is funded by Utsubo

@mrdoob mrdoob added this to the r165 milestone May 16, 2024
@sunag
Copy link
Collaborator

sunag commented May 16, 2024

Do you think we can check this inside WebGPUBindingUtils for example? Adding updateRenderTarget in setRenderTarget seems require that the device is initialized.

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented May 16, 2024

For the moment I simply handle the issue in WebGPUBindingUtils in a way that it will help us debug the issue and prevent the renderer to crash.

In any case, the original error might be related to an issue even higher in the pipeline such as bindings themselves not being properly updated or checked for update in Bindings.getForRender().

As mentioned this issue is very difficult to isolate and to reproduce, and debugging might be a real adventure, so finding all potential improvement along the way in the pipeline to solidify it still a good thing.

Anyway, this PR still improve the renderer logic, and I would push the fact that we add functions similar to clearAsync, computeAsync, compileAsync, hasFeatureAsync, and renderAsync by introducing setRenderTargetAsync.

This would align it with the same logic as the rest of the renderer utilities functions and handle cases where the device has not been initialized via:

if ( this._initialized === false ) await this.init();

@RenaudRohlinger
Copy link
Collaborator Author

I updated the PR consequently.

@sunag
Copy link
Collaborator

sunag commented May 16, 2024

I think clear(), renderer(), compute() ... are execution stages, while setRenderTarget is preparation/configuration stage. I think this bug can be resolved without having to change this logic. From what I understand, the hardest part is reproducing the problem?

I would like to make sure whether or not we can avoid adding another async function. Do you think it might be related to some dispose() in the rendering pass process?

@RenaudRohlinger
Copy link
Collaborator Author

I see, and yes I'm 100% affirmative on the issue being related to the dispose() call triggered by the setSize() call of the RenderTarget class in the rendering pass process.

@RenaudRohlinger
Copy link
Collaborator Author

In the meantime I will handle the error in user-land I guess. I lack of knowledge regarding all the binding setup part. I will open an issue if I find anything more verbose leading to a potential real fix. Sorry about spamming on this subject, this issue is quite a rabbit hole, but we're getting somewhere!

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented May 17, 2024

As demonstrated here, the issue comes from the Postprocessing texture of the PassNode. Somehow in the WebGPU Backend binding.update() will return false so it assumes the texture is indeed already available in the backend while it's not and generates an error that breaks the app.
image

This small condition added in Bindings.js line 140 patch the issue and prevent the app to break while properly rebinding the texture so that everything runs fine.

const textureData = backend.get( binding.texture );

const isTextureGPUNotAvailable = textureData.texture === undefined && textureData.externalTexture === undefined;

if ( isTextureGPUNotAvailable ) {

	// TODO: Remove this once we found why updated === false doesn't generate a texture in the WebGPU backend
	console.error( 'Bindings._update: should be available binding:', binding, updated, binding.texture, binding.textureNode.value );

	this.textures.updateTexture( binding.texture );
	needsBindingsUpdate = true;

}

Should I open a PR with this small patch so that it does fix our issue while adding a log to help fix the potential real issue in the future? /cc @sunag

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