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: Relocate VAO handling in WebGLBackend #26798

Closed
wants to merge 3 commits into from

Conversation

aardgoose
Copy link
Contributor

@aardgoose aardgoose commented Sep 18, 2023

Associating the VAO with the WebGL pipeline causes the wrong attributes to be used in a draw call when materials are shared.

A previous PR added geometry id as an element to the pipeline cache but that has the side effect of excessive shader program compilations etc.

Move the VAO creation into a separate method and cache against the renderObject. This aligns more with the WebGPU backend.

With the previous PR enabling video textures this enables the example 'material / video' to work correctly with the WebGL backend

@mrdoob mrdoob added this to the r157 milestone Sep 19, 2023
@mrdoob mrdoob modified the milestones: r157, r158 Sep 28, 2023
@sunag
Copy link
Collaborator

sunag commented Sep 29, 2023

I had to rebase to add support for InterleavedBufferAttribute to the existing code...

Would you have an example to reproduce the bug?

One of the reasons I delayed the merge of the previous PR was the geometry.id, other thing is that NodeMaterial can also add anonymous attributes. I think it's better to improve the caching system instead of bringing this logic to the draw().

@aardgoose
Copy link
Contributor Author

The addition of the geometry.id in the previous PR solves the problem in the occlusion example where both objects use the same material and the second object gets the VAO from the previous object unless they use different pipeline objects.

I spotted the problem of additional non geometry attributes so I cached VAO by RenderObject rather than using geometry.id in this PR. I still think the draw call is the correct place because it should allow pipelines to be used for multiple objects. Attributes are set up per draw call in the WebGPU equivalent code.

@sunag
Copy link
Collaborator

sunag commented Oct 16, 2023

The problem is that this is not passing through the cache logic, I agree with you about geometry.id and I explained in the comment above, other reasons, I think replacing renderObject.geometry.id with renderObject.id in getCacheKey() could give the same result provisionally and saves this check in draw(), I would merge this if it works until we improve the cache.

If we follow that each RenderObject will have a VAO independent, we will never have shared VAO, as we have a RenderObject for each Mesh, regardless of whether it has the same Geometry and Material.

@mrdoob mrdoob modified the milestones: r158, r159 Oct 27, 2023
@aardgoose
Copy link
Contributor Author

I have finally worked what problem this addresses. The webgpu_materials_video example required a new gl.linkProgram() for each cube rendered, although only two shader objects were created (vertex and fragment) which results in a noticeable delay when starting the video.

I agree ideally vaos would be cached by something that captured the combination of geometry and material generated attributes.

@sunag sunag removed this from the r159 milestone Nov 20, 2023
@sunag sunag closed this Nov 20, 2023
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