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.

Sign up for GitHub

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

Jump to bottom

Gizmos Circle does not work with non-unit normal vectors #11401

Closed
atlv24 opened this issue Jan 18, 2024 · 1 comment · Fixed by #11422
Closed

Gizmos Circle does not work with non-unit normal vectors #11401

atlv24 opened this issue Jan 18, 2024 · 1 comment · Fixed by #11422
Labels
A-Gizmos Visual editor and debug gizmos C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@atlv24
Copy link
Contributor

atlv24 commented Jan 18, 2024

Function in question: https://github.com/bevyengine/bevy/blob/main/crates/bevy_gizmos/src/circles.rs#L21

Docs do not mention normal needing to be unit length, and code does not check for it either. It just renders weird and kinda wrong most of the time. I think having glam asserts on would catch it, but ideally the docs are either updated to make this requirement clear, or the bevy normalizes the vector silently.
@atlv24 atlv24 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jan 18, 2024
@alice-i-cecile alice-i-cecile added A-Gizmos Visual editor and debug gizmos D-Trivial Nice and easy! A great choice to get started with Bevy and removed S-Needs-Triage This issue needs to be labelled labels Jan 18, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 18, 2024 •
edited
Loading

We should silently normalize this IMO (and document that we do so). The perf constraints aren't bad enough to complicate things.

@ArthurBrussee ArthurBrussee mentioned this issue Jan 19, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 21, 2024
@ArthurBrussee
Use Direction3d for gizmos.circle normal (#11422)
ffb6faa 
# 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Gizmos Visual editor and debug gizmos C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use Direction3d for gizmos.circle normal ArthurBrussee/bevy
2 participants
@alice-i-cecile @atlv24