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] Fix error when having BoneAttachment before PhysicalBone #67698

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Oct 21, 2022

Fixes #42010

bind_child_node_to_bone() allows re-adding an existing bind. But unbind_child_node_from_bone() does not have extra checks for unbinding a non-existing bindings. So List::erase() complains about erasing something that does not exist.

There's no error when no BoneAttachment node comes before a PhysicalBone. This is because calling List::erase() on an empty list won't print an error. (Bone attachments are also bind to bones, so when there's a bone attachment node before the physical bone, the bind node list is not empty when it enters tree.)

It's inconsistent that List::erase() behaves differently when empty and non-empty. But I think fixing this on Skeleton level is acceptable since we prefer local solutions 😛

There're no such methods on master.

@timothyqiu timothyqiu added bug topic:3d cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Oct 21, 2022
@timothyqiu timothyqiu added this to the 3.6 milestone Oct 21, 2022
@timothyqiu timothyqiu requested a review from a team as a code owner October 21, 2022 02:23
@lawnjelly
Copy link
Member

Might be worth reviewing at the same time whether this should be a List at all versus a LocalVector. There's a whole bunch of stuff in Godot using Lists that really should be LocalVectors (due to linked lists being so bad for cache etc). At some point it would be nice to go through these.

@timothyqiu
Copy link
Member Author

Updated and replaced the List with LocalVectori.

scene/3d/skeleton.cpp Outdated Show resolved Hide resolved
return; // Doesn't exist in the first place.
}

bones.write[p_bone].nodes_bound.remove(index);
Copy link
Member

Choose a reason for hiding this comment

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

Are the nodes_bound required to be in order? If not, we can maybe use remove_unordered().. 🤔

Copy link
Member Author

@timothyqiu timothyqiu Nov 14, 2022

Choose a reason for hiding this comment

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

Skeleton itself does not require some specific order. But changing nodes_bound order affect the order of the returned array from get_bound_child_nodes_to_bone() and the bound_children property. So using remove_unordered() might break user code I think.

@akien-mga akien-mga merged commit 861943e into godotengine:3.x Nov 14, 2022
@akien-mga
Copy link
Member

Thanks!

@timothyqiu
Copy link
Member Author

Cherry-picked for 3.5.2

@timothyqiu timothyqiu removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Dec 5, 2022
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.

3 participants