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

Reset to Rest Pose in Skeleton2D menu fails #54793

Closed
irslutz opened this issue Nov 9, 2021 · 9 comments · Fixed by #54851
Closed

Reset to Rest Pose in Skeleton2D menu fails #54793

irslutz opened this issue Nov 9, 2021 · 9 comments · Fixed by #54851

Comments

@irslutz
Copy link

irslutz commented Nov 9, 2021

Godot version

v3.4.stable.official [206ba70]

System information

Windows 10

Issue description

I have rigged a paper doll guy, mapped bones to verts for deformation, and created animations.
image

In Godot v3.3.4.stable.official [faf3f88] I can select the Skeleton2D and use the Skeleton2D menu to "Set Bones to Rest Pose." The result looks good. However, in Godot v3.4.stable.official [206ba70] this does not work as expected.
image

When I select "Reset to Rest Pose" in the newer version, the Polygon2D assets return to rest pose, however the bones do not. This seems to scramble the mapping of verts to bones for all animations.
image

Steps to reproduce

  1. Load a Skeleton that has been mapped to Polygon2Ds authored in v3.3.4.stable.official [faf3f88]
  2. Select the Skeleton2D
  3. Use the Skeleton2D menu to "reset to rest pose"

Observe: Polygon2Ds correctly return to rest pose, bones do not.

  1. Try to play an animation

Observe: Now that bones have new offsets animations no longer look correct.

Minimal reproduction project

RestPoseBug.zip

@Calinou
Copy link
Member

Calinou commented Nov 9, 2021

@irslutz Can you reproduce this in any of the 3.4 betas and RCs to determine when the regression started?

@irslutz
Copy link
Author

irslutz commented Nov 9, 2021

@Calinou I did a little bracketing for the test case. Beta2~beta4 seemed good, the first reproduction of the bug was beta 5.

@akien-mga
Copy link
Member

See this change in beta 5: de3f454

That's only a rename, maybe that's the source of confusion? The 3.4 equivalent of "Set Bones to Rest Pose" is "Overwrite Rest Pose".

@irslutz
Copy link
Author

irslutz commented Nov 9, 2021

OK @akien-mga is correct, functionality between the two options is consistent.

  • "Make Rest Pose (From Bones)" has become "Reset to Rest Pose"
  • "Set Bones to Rest Pose" has become "Overwrite Rest Pose"

In each case the new label provides identical functionality. But it seems that those two labels should be switched, at least in English.

I notice current documentation uses old terms.

@Calinou
Copy link
Member

Calinou commented Nov 9, 2021

The button texts were tweaked in #52317 following the advice from #28124.

@bitbutter Did I get them the wrong way around? I don't really understand right now.

@irslutz
Copy link
Author

irslutz commented Nov 10, 2021

It looks like that suggestion was to change words and order. The words have changed but the order is the same.

@irslutz
Copy link
Author

irslutz commented Nov 10, 2021

@Calinou I have tried to illustrate what it appears the request from #28124
image1724

It appears that the functionality that was tied to Make Rest Pose (From Bones) should have the text Overwrite Rest Pose and be put in the lower position. The functionality tied to Set Bones to Rest Pose should be matched to the text Reset to Rest Pose and be in the upper position.

The text is in the correct position, but the functionality is reversed.

I hope this is helpful.

@Neketek
Copy link

Neketek commented Nov 15, 2021

OK @akien-mga is correct, functionality between the two options is consistent.

  • "Make Rest Pose (From Bones)" has become "Reset to Rest Pose"
  • "Set Bones to Rest Pose" has become "Overwrite Rest Pose"

In each case the new label provides identical functionality. But it seems that those two labels should be switched, at least in English.

I notice current documentation uses old terms.

I agree, this labeling is very confusing.

@akien-mga
Copy link
Member

I notice current documentation uses old terms.

Can you point to the documentation which still uses the old terms?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants