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

[3.x] GLES2: Make GPU skinning more consistent with GLES3 #80652

Merged

Conversation

Chlorobyte-but-real
Copy link

My first contribution here! 👋

Aside from consistency, functionally, this may improve performance, with a single ivec4 cast as opposed to 4 separate int casts for bone_ids, plus calling transpose() 1 time instead of 4.

But this change is mainly motivated by the vec4 indexing in the previous implementation causing the following issue on my Xiaomi Redmi 6A device on WebGL (a desktop and a high-end mobile device, and a native export on the same device worked, so this is entirely a driver+ANGLE issue):
Screenshot_2023-08-15-11-07-43-365_com android chrome

With the GLES3 implementation ported over, the model renders correctly (the issue with her clothes is common in Makehuman, the rendering is technically correct):
Screenshot_2023-08-15-11-08-19-569_com android chrome

The minimal fix for the issue would be changing
bone_transform += transpose(b) * bone_weights[i];
to:
bone_transform += transpose(b) * dot(bone_weights, vec4(i == 0 ? 1.0 : 0.0, i == 1 ? 1.0 : 0.0, i == 2 ? 1.0 : 0.0, i == 3 ? 1.0 : 0.0));
but this would degrade performance, and looks quite ugly.

@Chlorobyte-but-real
Copy link
Author

Eh, sure, if it needs to be that indented...

Chlorobyte-but-real

This comment was marked as resolved.

@lawnjelly
Copy link
Member

Looks fine to me if it is more widely compatible and reduces the number of transposes, the commits will need squashing though and @clayjohn to take a look.

Would be nice to see some performance benchmarks especially on Android just to double check there is no regression with speed (it should in theory be faster, but might depend on how well / whether the old version gets optimized).

@Chlorobyte-but-real
Copy link
Author

Would be nice to see some performance benchmarks especially on Android just to double check there is no regression with speed (it should in theory be faster, but might depend on how well / whether the old version gets optimized).

With 103714 vertices on a native build, I cannot measure a performance difference - both are getting average frame times of about 133.3 ms.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks fine to me too!

@akien-mga akien-mga merged commit 98976f9 into godotengine:3.x Aug 16, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@Chlorobyte-but-real Chlorobyte-but-real deleted the 3.x_fix_gles2_skinning branch August 16, 2023 15:41
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.

5 participants