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

Implement Arc3D for Gizmos #11540

Merged
merged 8 commits into from
Jan 28, 2024
Merged

Implement Arc3D for Gizmos #11540

merged 8 commits into from
Jan 28, 2024

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Jan 26, 2024

Objective

Solution

arc_3d

  • The current arc3d method on gizmos only takes an angle
  • It draws an "standard arc" by default, this is an arc starting at Vec3::X, in the XZ plane, in counter clockwise direction with a normal that is facing up
  • The "standard arc" can be customized with the usual gizmo builder pattern. This way you'll be able to draw arbitrary arcs

short/long_arc_3d_between

  • Given center, from, to draws an arc between from and to

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • Added: Gizmos::arc3d(&mut self, angle) method
  • Added: Gizmos::long_arc_3d_between(&mut self, center, from, to) method
  • Added: Gizmos::short_arc_3d_between(&mut self, center, from, to) method

This PR factors out an orthogonal part of another PR as mentioned in this comment

The API of this implementation mimics unity's API for the same feature,
c.f.

https://docs.unity3d.com/ScriptReference/Handles.DrawWireArc.html

This was done since it seems to be the most flexible design so far
allowing for arcs with arc angles in the range of [-360.0, 360.0]
degrees.

Authored-by: RobWalt <robwalter96@gmail.com>
I noticed some flaws with the original implementation of the API while
creating some examples:

- The orientation via `from` and `rotation_axis` fields felt really
  wonky
- The rotation seemed not to rotate around the center set by the user

This is why I reimplemented the API. It now draws some kind of default
arc in 3D which has properties which are documented in the doc strings.
You can modify this default arc via builder methods which seems to be
the most flexible approach in my eyes.

Authored-by: RobWalt <robwalter96@gmail.com>
Authored-by: RobWalt <robwalter96@gmail.com>
- reuse new factored out function to calculate the default amount of
  segments for arc2d
- add the gizmos active check to arc3d
- also implement optional segments logic for arc3d
- cleanup debug gizmo center drawing
@RobWalt RobWalt mentioned this pull request Jan 26, 2024
4 tasks
@Kanabenki Kanabenki added C-Feature A new feature, making something new possible A-Gizmos Visual editor and debug gizmos labels Jan 26, 2024
Authored-by: RobWalt <robwalter96@gmail.com>
/// ```
///
/// # Notes
/// - This method assumes that the points `from` and `to` are distinct from the `center`. If
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit nervous about using "behavior is undefined" here (and above): it's not UB in the compiler sense and we may confuse readers. Maybe "graphical glitches may occur"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting this!

I just tested these cases (after all we're also part scientists, right? :D). It seems like nothing happens at all. We should probably communicate this to the user.

Do you think an early return might make sense? I'm not really sure. On the one hand we could save some compute in the case that we wouldn't draw anything anyways. On the other hand this is an edge case and adding two checks for every non-edge case might be needless overhead.

I think I'm just going for changing the comment for now, but please let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I think let's just change the comment and leave the behavior for now. I'm really not concerned about the performance in pathological cases and if it doesn't crash I'm quite happy with it as is.

Authored-by: RobWalt <robwalter96@gmail.com>
Thanks for spotting this Alice!

Authored-by: RobWalt <robwalter96@gmail.com>
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

Similar thoughts as #11072 (review), the position, rotation, and color should be given as arguments to the arc methods. All gizmo methods have some kind of position and a color as an argument, and assuming a default color isn't ideal imo.

arc_2d also has the circle center, rotation (direction_angle) and color as arguments, so it's not good to make arc_3d different. The APIs should be consistent both in terms of arguments and naming (if possible).

crates/bevy_gizmos/src/arcs.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/arcs.rs Outdated Show resolved Hide resolved
and also adjust:

- docs
- tests
- examples

Authored-by: RobWalt <robwalter96@gmail.com>
@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 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 28, 2024
Merged via the queue into bevyengine:main with commit bcae8e9 Jan 28, 2024
23 checks passed
@RobWalt RobWalt deleted the feat/arc3d branch January 28, 2024 10:46
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

- Implement an arc3d API for gizmos
- Solves bevyengine#11536

## Solution

### `arc_3d`

- The current `arc3d` method on gizmos only takes an angle
- It draws an "standard arc" by default, this is an arc starting at
`Vec3::X`, in the XZ plane, in counter clockwise direction with a normal
that is facing up
- The "standard arc" can be customized with the usual gizmo builder
pattern. This way you'll be able to draw arbitrary arcs

### `short/long_arc_3d_between`

- Given `center`, `from`, `to` draws an arc between `from` and `to`

---

## Changelog

> This section is optional. If this was a trivial fix, or has no
externally-visible impact, you can delete this section.

- Added: `Gizmos::arc3d(&mut self, angle)` method
- Added: `Gizmos::long_arc_3d_between(&mut self, center, from, to)`
method
- Added: `Gizmos::short_arc_3d_between(&mut self, center, from, to)`
method

---

This PR factors out an orthogonal part of another PR as mentioned in
[this
comment](bevyengine#11072 (comment))
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-Feature A new feature, making something new possible 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.

4 participants