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

Remove mesh bone_aabbs as they are not used anywhere and calculating them is a pain #70091

Merged
merged 1 commit into from
Dec 17, 2022

Conversation

clayjohn
Copy link
Member

Fixes #56458 once and for all

Tested with MRPs from: #56458, #69516, and #64416 (comment)

The bone_aabbs array was added in 449df8f and was only used in one place. That one place was removed in #44649 and since then bone_aabbs has never been used. We do however, use surface[i].bone_aabbs.

There is no point in merging together the various bone_aabbs as the bone indices don't necessarily correlate with each other at all. At best this resulted in a fairly chaotic array of AABBs.

I will need to discuss with @reduz, but my guess behind the original intention of this code was that it should expand the mesh->aabb to cover all the individual bone AABBS. If that is the case, then we will be better off doing that during the original calculation of the bone_aabbs in the RenderingServer

@clayjohn clayjohn added this to the 4.0 milestone Dec 15, 2022
@clayjohn clayjohn requested a review from a team as a code owner December 15, 2022 03:41
@fire
Copy link
Member

fire commented Dec 15, 2022

Your reasoning makes sense, and if nothing is using it... it follows to remove it.

@reduz
Copy link
Member

reduz commented Dec 15, 2022

Unless I am misunderstanding something, Bone AABBs are used for updating the AABB of a mesh instance when a skeleton using it changes. This in itself should probably not be the default behavior due to performance, but should be optional for games that really need this AABB precision (had a couple in the past). If bone AABBs are not supplied, then the regular mesh AABB should be used (and/or a custom override).

@clayjohn
Copy link
Member Author

Unless I am misunderstanding something, Bone AABBs are used for updating the AABB of a mesh instance when a skeleton using it changes. This in itself should probably not be the default behavior due to performance, but should be optional for games that really need this AABB precision (had a couple in the past). If bone AABBs are not supplied, then the regular mesh AABB should be used (and/or a custom override).

@reduz That code uses the per-surface bone_aabbs, not the merged mesh->bone_aabbs which is all of the surface's bone_aabbs merged together

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

This is unused code currently so the removal makes sense.

@akien-mga akien-mga merged commit 2b05611 into godotengine:master Dec 17, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Warnings "AABB size is negative" when importing glTF file with zero-weight bones
4 participants