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 Polygon2D to Skeleton2D transform calculation #86557

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Dec 27, 2023

Fixes incorrect calculation made in #84451 (cc @ShirenY).

  • When multiplying transforms they're composed right to left.
  • 3D basis vectors are stored in columns just as in 2D transforms (even though memory-wise it's row-wise vs column-wise layouts).

Fixes #86390.

Before/after examples:

MRP from #86390

  • Before (v4.3.dev1.official [9d1cbab]):
out1pre.mp4
  • After (this PR):
out1post.mp4

MRP from #86547

  • Before (v4.3.dev1.official [9d1cbab]):
out2pre.mp4
  • After (this PR):
out2post.mp4

@kleonc kleonc added this to the 4.3 milestone Dec 27, 2023
@kleonc kleonc requested a review from a team as a code owner December 27, 2023 18:00
@fire fire requested review from a team and lawnjelly December 28, 2023 00:09
@ShirenY
Copy link
Contributor

ShirenY commented Dec 28, 2023

Sorry, it's a shame to missing the transform multiply rule XD.
Yeah, I may figured the 2d to 3d matrix part wrong. Since I was bit confused how they were composed. I actually think we need a global utility function in the engine for converting them back and forth.
I was too depend on the "If it looks right, it's right" theory, some issues just doesn't appears in some cases.
I will do some test on my side, but changes look legit for me. Thanks for the fix.

@ShirenY
Copy link
Contributor

ShirenY commented Dec 31, 2023

Checked my test cases with this fix, they all looks fine. As auther said 2d to 3d transform should be take care like this.
But when reading into 2d transform, I found that they took positive Y as downwards which is in contrast to how Transform3D does. Should we concern this? I guess if every 2d transform calculation handls this in a constant way, it will be fine. But if any code in middle of the 2d-3d-2d process did some calculation with up is positive Y assumption, there will be an issue.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good

@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 6, 2024
@akien-mga akien-mga merged commit 665c3ed into godotengine:master Jan 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the polygon2d-with-skeleton-transform-fix branch January 8, 2024 11:46
@YuriSizov YuriSizov added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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.

Polygon2D mesh disappears when vertices exceed boundaries
5 participants