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

Fix for issue where Godot crashes on reparenting bones in the skeleton gizmo editor #52575

Conversation

TwistedTwigleg
Copy link
Contributor

Fixes #52533

The issue was that it was trying to access bones before updating the process order, causing the issue. Calling _update_process_order right before the gizmo tries to build will cause it to update if it needs it and fixes the crash.

@fire
Copy link
Member

fire commented Sep 11, 2021

Doing the operation doesn't crash Godot anymore. Tested a build.

@reduz
Copy link
Member

reduz commented Sep 12, 2021

This kind of does not feel like the right fix, to me it sounds like this should be transparent within Skeleton. It sounds like something is missing within skeleton that leads to an inconsistent state, this inconsistent state should never happen.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Comment from Reduz requesting changes.

@TwistedTwigleg
Copy link
Contributor Author

This kind of does not feel like the right fix, to me it sounds like this should be transparent within Skeleton. It sounds like something is missing within skeleton that leads to an inconsistent state, this inconsistent state should never happen.

What I think is happening is that the bone order is changed/manipulated, which leads to a boolean for updating the process order to be turned to true, and then right after the code tries to get the bone’s parent. Normally this isn’t too much of an issue, as after a single process call the boolean is found in a condition and then the process order updated, but because the gizmo code calls one right after the other, the skeleton hasn’t had a chance to update the process order yet.

I do agree that, upon reflection, it does seem like something the Skeleton should handle internally rather than relying on the user to know to wait a single process before trying to access bone parents after manipulation. Perhaps the get_bone_parent function (and similar functions that manipulate the bone order) should check the state of the boolean to update the process order and, if the boolean is true, manually update the process order?

@mhilbrunner mhilbrunner modified the milestones: 4.0, 4.x Aug 25, 2022
@mhilbrunner
Copy link
Member

cc @TokageItLab @fire please assess whether this needs to go into 4.0/beta :)

@TokageItLab
Copy link
Member

It is not critical and does not need to be fixed before BETA, but it must be fixed before RC. We will need to re-visit this after refactoring skeleton by #63588.

@TokageItLab TokageItLab modified the milestones: 4.x, 4.0 Aug 26, 2022
@mhilbrunner
Copy link
Member

@TokageItLab What is the status of this? Beta is way underway and we'll need to move to RC status eventually. :)

@TokageItLab
Copy link
Member

@mhilbrunner For now, @fire is working hard, IK is the only release blocker on AnimationTeam so far.

@TokageItLab
Copy link
Member

#52533 is already fixed by something else, so this is unnecessary.

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.

Dragging a skeleton bone root to be the immedate bone under it crashes
7 participants