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

Use Direction3d for gizmos.circle normal #11422

Merged
merged 10 commits into from
Jan 21, 2024

Conversation

ArthurBrussee
Copy link
Contributor

@ArthurBrussee ArthurBrussee commented Jan 19, 2024

Objective

Fix weird visuals when drawing a gizmo with a non-normed normal.

Fixes #11401

Solution

Just normalize right before we draw. Could do it when constructing the builder but that seems less consistent.

Changelog

  • gizmos.circle normal is now a Direction3d instead of a Vec3.

Migration Guide

  • Pass a Direction3d for gizmos.circle normal, eg. Direction3d::new(vec).unwrap_or(default) or potentially Direction3d::new_unchecked(vec) if you know your vec is definitely normalized.

@ArthurBrussee ArthurBrussee changed the title Main Fix gizmos.circle when normal isn't unit length Jan 19, 2024
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

Could you use a Direction3d instead of a Vec3 so that it doesn't't have to be normalised? http://dev-docs.bevyengine.org/bevy/prelude/primitives/struct.Direction3d.html

@matiqo15 matiqo15 added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Gizmos Visual editor and debug gizmos labels Jan 19, 2024
@ArthurBrussee
Copy link
Contributor Author

Sure! Gave that a go. This does slightly conflict with @alice-i-cecile comment in #11401 though. Maybe callees using Direction3d::new(vec).unwrap_or(Direction3d::Y) isn't too bad. Maybe could be worth having a Direction3d::new_or(vec, Direction3d::Y) ? Not sure.

@alice-i-cecile
Copy link
Member

I prefer the type-safe validation to my suggestion linked :)

@alice-i-cecile
Copy link
Member

@rodolphito review?

@ArthurBrussee ArthurBrussee changed the title Fix gizmos.circle when normal isn't unit length Use Direction3d for gizmos.circle normal Jan 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 21, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 21, 2024
Merged via the queue into bevyengine:main with commit ffb6faa Jan 21, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gizmos Circle does not work with non-unit normal vectors
4 participants