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: Improve Infos logic #27889

Merged

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented Mar 9, 2024

Description:
This PR enhances the analytics system by introducing support for postprocessing and by ensuring it operates based on the user's application main loop, rather than resetting at each step of the code execution. For instance, in the previous setup, a postprocessing routine that calls render() multiple times would reset the analytics data within each step, leading to incorrect information.

In order to work correctly WebGPURenderer requires the usage of setAnimationLoop (to get this.nodes.nodeFrame.update(); properly called), or at least the developer has to copy the logic of the Animation class.

The reset of the draw calls and timestamps informations are now simplified and handled before the main loop gets called.
Additionally, the system for managing timestamps has been improved.

This is something I wanted to tackle for a long time in the WebGLRenderer but I understand it might be breaking change, so that's why I think it could be good to try it on the WebGPURenderer.

Alternative:

  • Make a new property into the Info Class this.resetMode = 'render||loop'
  • We could also introduce a total information along to the current Info system but it seems confusing to me since trying to isolate a specific function would resume to do the same in the end by commenting all other methods in an application.

Demo:
https://raw.githack.com/renaudrohlinger/three.js/utsubo/fix/refactor-improve-infos/examples/webgpu_compute_particles_snow.html
image

Related:
#27870
#27597

This contribution is funded by Utsubo

@RenaudRohlinger RenaudRohlinger marked this pull request as ready for review March 10, 2024 09:52
@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Mar 10, 2024

If you think changing the way we reset the infos is too much of a breaking change I can split this part of the PR into another.

I also like the this.resetMode = 'render||loop' alternative which would still support both solutions pretty easily without any breaking change.

However, as previously mentioned and poorly explained (apologies for that), not handling the reset logic directly within the main loop of the Animation Class and keeping it in the Renderer will result in developers having to almost always manually manage info.autoReset = false and info.reset() if they want accurate informations.

This is because the current info.drawCalls and other infos are constantly being erased by postprocessing setups, mirrors, or any RenderTarget setup, making them almost always incorrect as it's simply returning the last render call infos (which is almost always the postprocessing step so a single draw call).

Again, this change will exclusively affect the WebGPURenderer and will differ from the WebGLRenderer, which, unlike the former, does not make use of an animationLoop.

/cc @mrdoob @Mugen87 @sunag

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 11, 2024

IMO, we should give this new design a try since in almost all cases I have seen so far the manual reset was never done by developers although it would be required for certain use cases. We want to migrate the examples to setAnimationLoop() in any event so I think it's okay if the new system relies on it.

That said, we want to ensure WebGPURenderer can be used without setAnimationLoop() as well but I think it's okay if certain features require it.

@sunag
Copy link
Collaborator

sunag commented Mar 11, 2024

@RenaudRohlinger Apparently even requestAnimationFrame() should work since setAnimationLoop() is not a trigger for the frame count as could happen in WebGLRenderer, or would there be something else related?

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Mar 12, 2024

Nice! And developers not wanting to use setAnimationLoop() are using the renderer in a more advanced way so it would make sense if they need to also handle manually the info system.

As far as I know WebGPURenderer works without setAnimationLoop() but some Nodes seems to rely on this.nodes.nodeFrame.update(); and might not work correctly I guess @sunag?

@sunag
Copy link
Collaborator

sunag commented Mar 12, 2024

Yeah, exactly that. Do you know that with your PR we are very close to having information control based on hierarchy 🤔

Keep thinking about adding some functions to Info so that we can better control the RenderContext and thus add classes like: renderer.info = new AdvancedInfo().

@sunag sunag added this to the r163 milestone Mar 12, 2024
@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Mar 12, 2024

Do you know that with your PR we are very close to having information control based on hierarchy 🤔

Damn, that sound actually awesome, now I'm interested into diving more into this. 🤓

@sunag
Copy link
Collaborator

sunag commented Mar 12, 2024

Damn, that sound actually awesome, now I'm interested into diving more into this.

I thought about use info.beginRender( renderContext ) and info.beginCompute( computeContext|Node ) and info.end*( .. ) we could make a stack and registers just the current info per context and create the hierarchy. We can try it after merge this PR, what do you think?

About the info structure, what do you think of unify the timestamp within the renderer and compute structure, like info.renderer.timestamp instead of info.timestamp.renderer?

@RenaudRohlinger
Copy link
Collaborator Author

Updated the snow example to import an external dependency stats-gl which is a fork project of stats.js I've been working on since some time that makes complex high and frequency data human readable. stats-gl now supports this new info.timestamp.renderer API.

Here an example on how it looks:
image

@RenaudRohlinger
Copy link
Collaborator Author

Let's give it a try, it should help test the WebGPU vs WebGL performances worries in #27880

I will continue the information control based on hierarchy idea in another PR.
/cc @sunag

@RenaudRohlinger RenaudRohlinger merged commit f4a695c into mrdoob:dev Mar 13, 2024
11 checks passed
renderContextData.activeQuery = null;
this.queryRunning = false;

if ( renderContextData.queryQueue && renderContextData.queryQueue.length > 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between .gpuQueries and .queryQueue?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 25, 2024

During testing I have realized that this PR might broke: https://rawcdn.githack.com/mrdoob/three.js/dev/examples/webgpu_storage_buffer.html

At least it seems with previous commits in the dev history the example is still working. There is also a runtime error in the console:

webgpu_storage_buffer.html:220 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'compute')
at stepAnimation (webgpu_storage_buffer.html:220:88)

@RenaudRohlinger
Copy link
Collaborator Author

Ah thanks it's because we renamed the info logic from timestamp.compute to compute.timestamp! I will PR a fix. Too bad the CI still cannot run properly WebGPU examples to catch this kind of issues 🥲

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.

4 participants