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 Skeleton3D falsely assuming all physical bones will be children of their first bone #67282

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

CheesecakeCG
Copy link
Contributor

physical_bones_start_simulation_on was falsely assuming all physbones will be children of the first bone in their skeleton.

I've changed it so when the list of bones is empty, all physical bones start simulating, which more accurately reflects the documentation.

This issue might also affect 3.x, I vaguely remember running into something that might have been this.

@fire
Copy link
Member

fire commented Oct 12, 2022

Are you able to describe what happens previous and with the fix so I can check it does what it says it does? Like a video or a description.

@CheesecakeCG
Copy link
Contributor Author

Are you able to describe what happens previous and with the fix so I can check it does what it says it does? Like a video or a description.

Before, any PhysicalBone3D's that were assigned to bones that weren't parented to bone ID 0 got ignored by physical_bones_start_simulation() in GDScript. So in the case of this skeleton, it ignores all of the PhysicalBone3D's since none of them are parented to bone ID 0, "Root".
Did not work

In this skeleton, all the bones are parented to "Hips" still, but since "Hips" now is now bone ID 0, the rag-doll sim starts like you'd expect.
Did work

The second one is an older version of the same rig in Blender, but the biggest relevant difference I that I did not enable "Export Deformation Bones Only" in the GLTF export on one character but not the other. However, these aren't off the shelf rigs, so I have no idea how many people actual run into this issue.

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.

@reduz @fabriceci can you check? Your explanation makes sense but I'm not that familiar.

@akien-mga
Copy link
Member

@CheesecakeCG FYI, the Git author details you used to author this commit don't seem to match your GitHub account (see the "From:" line on https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/67282.patch), that's why the commit doesn't appear with your avatar. It's not a problem for Git but if you want to be credited as contributor if/when this is merged, you might want to change that (you can change your Git identity locally, then edit the commit with git commit --amend --reset-author and force push with git push --force to update the PR).

@CheesecakeCG
Copy link
Contributor Author

FYI, the Git author details you used to author this commit don't seem to match your GitHub account

Thanks for letting me know! Should be fixed now.

@akien-mga akien-mga merged commit 1518bb7 into godotengine:master Oct 31, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@CheesecakeCG CheesecakeCG deleted the fix-pb-start-sim branch October 31, 2022 11:34
@timothyqiu timothyqiu added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Nov 4, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 30, 2022
@timothyqiu
Copy link
Member

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.

5 participants