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 Curve3D baking up vectors for nontrivial curves. #81885

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

rmmh
Copy link
Contributor

@rmmh rmmh commented Sep 18, 2023

The code was modified in 42aa539 to have a different basis vector, but this line was missed and caused up vectors to invert sometimes.

Fixes #81879

One-character bugfix by @kleonc, bisection and test case by me.

@rmmh rmmh requested a review from a team as a code owner September 18, 2023 22:36
@rmmh
Copy link
Contributor Author

rmmh commented Sep 18, 2023

Is there a better macro to test for vectors being nearly equal? CHECK_EQ is nice because it shows the precise values that are incorrect, but it's inappropriate for precise floating checks.

@TokageItLab TokageItLab requested a review from a team September 18, 2023 22:45
@TokageItLab TokageItLab added this to the 4.x milestone Sep 18, 2023
@kleonc kleonc modified the milestones: 4.x, 4.2 Sep 18, 2023
@kleonc
Copy link
Member

kleonc commented Sep 18, 2023

The bug is also present in 4.1, should be safe for cherry-picking? 🤔

@kleonc kleonc added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 18, 2023
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

Sorry, the my code got confused when I was testing. This fix is probably fine, but we should fix the comment in Line 1656:

		Basis frame; // X-right, Y-up, Z-forward.

to

		Basis frame; // X-right, Y-up, -Z-forward.

@rmmh rmmh force-pushed the fix-curve3d-twisting branch from 9076147 to 35add14 Compare September 19, 2023 00:49
The code was modified in 42aa539 to have a different basis vector, but
this line was missed and caused up vectors to invert sometimes.

Fixes godotengine#81879
@rmmh rmmh force-pushed the fix-curve3d-twisting branch from 35add14 to 734b9d2 Compare September 19, 2023 00:50
Copy link
Member

@akien-mga akien-mga 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 to me.

As this bug got popular on Twitter, I'll clarify that even though this will be merged in a bit, it won't make the cut for 4.2-dev5 which was built yesterday (even though we'll release it today).

It should be in 4.2-dev6 and likely 4.1.2 RC 1.

@akien-mga akien-mga merged commit 571cd0e into godotengine:master Sep 19, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
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.

Path3D Up Vector miscalculation
5 participants