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

Use vertex input mask for creating vertex arrays #85092

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

clayjohn
Copy link
Member

Fixes: #85053

The issue is that the calculation of skeletons became super slow after #81138. The root cause was how we cache and retrieve vertex array objects. Prior to #81138 we created a special vertex array object for skeletons which only ever contained vertices, normals, and tangents. After #81138 we used the regular mesh caching approach so we could automatically get the correct vertex array object.

However, the caching approach was subtly broken. It always returned a vertex array object with all available attributes enabled. That means, if the mesh has colors, UVs, etc. The VAO would also contain them. For skeletons, this created a huge amount of unnecessary overhead as only positions, normals, and tangents (at most) are needed.

Once I added the code to take the vertex input mask into account. I noticed it was broken as the indices were off by one.

This PR fixes the performance regression in #85053 and might also increase performance in other cases as well.

This PR is slightly too risky for RCs, so let's plan on merging for 4.3 and cherry picking to 4.2.1

Also fix bug in creation of vertex input mask
@clayjohn clayjohn added bug topic:rendering regression performance cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 19, 2023
@clayjohn clayjohn added this to the 4.3 milestone Nov 19, 2023
@clayjohn clayjohn requested a review from a team as a code owner November 19, 2023 15:39
@xix-xeaon
Copy link

I can confirm this fixes #85053.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Number of blobs in the MRP from #85053 before going below 60 FPS on i9-13900K + RTX 4090 at the default project window size:

4.1.3 master 5945768 This PR
366 46 366

@akien-mga akien-mga merged commit 74e49b7 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@clayjohn clayjohn deleted the GL-vertex-input-mask branch December 5, 2023 00:19
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cull 3D performance issue with animated model in Compatibility renderer
5 participants