Drawing Primitives with Gizmos

[RobWalt]

The PR is in a reviewable state now in the sense that the basic implementations are there. There are still some ToDos that I'm aware of:

• docs for all the new structs and traits
• implement Default and derive other useful traits for the new structs
• Take a look at the notes again (Do this after a first round of reviews)
• Take care of the repetition in the circle drawing functions

---

Objective

• TLDR: This PR enables us to quickly draw all the newly added primitives from bevy_math in immediate mode with gizmos
• Addresses Gizmos for primitive shapes #10571

Solution

• This implements the first design idea I had that covered everything that was mentioned in the Issue Gizmos for primitive shapes #10571 (comment)

---

Caveats

• I added the Primitive(2/3)d impls for Direction(2/3)d to make them work with the current solution. We could impose less strict requirements for the gizmoable objects and remove the impls afterwards if the community doesn't like the current approach.

---

Changelog

• implement capabilities to draw ellipses on the gizmo in general (this was required to have some code which is able to draw the ellipse primitive)
• refactored circle drawing code to use the more general ellipse drawing code to keep code duplication low
• implement Primitive2d for Direction2d and impl Primitive3d for Direction3d
• implement trait to draw primitives with specialized details with gizmos
  • GizmoPrimitive2d for all the 2D primitives
  • GizmoPrimitive3d for all the 3D primitives
  • (question while writing this: Does it actually matter if we split this in 2D and 3D? I guess it could be useful in the future if we do something based on the main rendering mode even though atm it's kinda useless)

---

[RobWalt]

This PR currently includes some other general changes to Gizmos which could be factored out. They are:

• Generalizing circle drawing to ellipse drawing for gizmos
• Soon ™️ Arc3d drawing support for gizmos

The prior was pretty straight-forward. For the latter, here are some references how other game engines implement Arc3d, which I used for orientation:

• unity - https://docs.unity3d.com/ScriptReference/Handles.DrawWireArc.html
• unreal blueprints - https://docs.unrealengine.com/5.2/en-US/BlueprintAPI/GeometryScript/PolyPath/CreateArcPath3D/
• godot (doesn't seem to have built in arcs, here's a lib that implements it) - https://github.com/nyxkn/godot-draw3d/blob/main/addons/draw3d/draw_3d.gd#L212-L240

[RobWalt]

Some general notes for reviewers:

• I'll try to add an example to prove everything is working fine
• If you want I can split out the new general functionalities of Gizmos into a separate PR (Ellipses & Arcs)

@Jondolf Can you take a look over this and review it if you find some time?

[Jondolf]

I think the 3D arc should be in a separate PR since it's new functionality that isn't related to the primitives, which are the core focus of this PR. But the ellipse might be fine since it's needed for the primitives too.

I responded about the API in the issue and added a proposal for an alternative: #10571 (comment). I felt like it'd be too long and slightly off-topic to post here.

[atlv24]

please remove flake.lock, flake.nix, and template.nix

[RobWalt]

@rodolphito I will when this branch is ready. Atm I'm facing the problem described in #10571 (comment) which blocks this here from a merge anyways.

[RobWalt]

Resolved the last issue with this and also removed the nix files

[nothendev]

I'm wondering if Self::Output<'_> is 2018 idioms..

[RobWalt]

Just so I don't forget: This needs a rebase on this #11540 and some adjustments because of it.

I also noticed that some general Gizmo things changed. So this also needs a rebase on main first!

[Jondolf]

Looks pretty good overall, but I'm not a huge fan of having the position, rotation, and especially color configured via builders. In my opinion, the API should be:

gizmos.primitive_2d(primitive, translation, rotation, color).builder_stuff();

I can't think of many cases where I'd want to always draw something at the origin with gizmos, and primitives by themselves don't have any position or rotation associated with them. The color should also be given explicitly, as assuming a default color for gizmos is not a good idea in my opinion.

Using a builder pattern for the positioning and color is also inconsistent: all existing gizmos have color as the last argument(s), and they all have some kind of position as an argument.

The current names for the builders are also inconsistent: there's center, start_position, normal_position, translation, position... The user basically needs to guess what the name is for each primitive. I would unify them all to be just translation (as an argument to the gizmo method), as it simply translates the primitive relative to the origin.

One argument against this is that we might want to define e.g. line segments with one endpoint at the start position instead of having the center of the line segment be at the origin, but... do we? If you need that representation, you could just use gizmos.line_2d(...). All primitives are currently assumed to be centered at the origin, and I'm not sure if we want to break that assumption for gizmos.

The primitives.rs gizmo file is also getting really big now; I would split it into primitives2d and primitives3d modules, or a primitives module with dim2 and dim3 modules.

[Jondolf]

Ellipses should support rotation like other primitives. Imo every primitive (even the ones with a direction or normal) should be rotatable, as they don't have an inherent rotation associated with them

[Jondolf]

Another reason why having translation and rotation as arguments could be useful: the primitive gizmo method is in a trait, so you could make a generic system or method that renders the given primitive at some position. If the position and rotation are in the primitive-specific builders, a generic system like this is not possible (the gizmo is always at the origin).

Example use case: bevy_xpbd has a physics debug rendering pipeline. I could convert each collider to a primitive if possible, and call the primitive gizmo method for collider visualization. If I can't do this with generics, each primitive type must be handled individually. Same for the color, it must be an argument, otherwise every type needs special casing.

[Jondolf]

TLDR of what I'd personally change:

• Remove position, rotation, and color from all builders
• Change API to gizmos.primitive_2d(primitive, translation, rotation, color)
  • Every primitive is centered at the origin by default, and supports rotation
• Split primitives into 2D and 3D files

If you want to keep support for e.g. segments starting from a point instead of being centered at the origin, you could add that as a configuration option. But imo all primitives should be centered at the origin by default.

Sorry if this is a lot 😅 and feel free to disagree! This is just what I would find to be the most useful and consistent API

[RobWalt]

Makes sense! I implemented everything as requested. Should be more in line with the rest of the gizmo code. Another nice side effect of these changes is that the code is much "simpler". Less code = easier to maintain, so yay 🎉

The only thing that I didn't use from the review is the translation name. I used position instead to make it even more similar to the rest of the gizmo methods.

Thanks for the review 😁 👍🏼

[Jondolf]

I suggested translation because I don't consider e.g. lines and planes to have a specific "position"; they can simply be translated relative to the origin. This is probably nitpicking though, so position is fine too :)

I made a slightly stripped down version of the 3d_gizmos example:

2024-01-28.14-36-56.mp4

The primitives look good, but I see two things that I consider to be issues.

Firstly, I think the primitives (capsule, cylinder, cone, and conical frustum) should be upright instead of facing Z. I asked about this on the Discord, and heard that gizmos should typically face negative Z, but I think we agreed that it's reasonable for these primitives to be upright. The primitive meshes and colliders would most certainly be upright, so making gizmos differ from this is probably not a good idea.

It's also just intuitive in my opinion; if I draw a cone with no rotation, I'd expect it to be upright like a traffic cone, not pointing towards Z or negative Z. The names of the properties of the primitives also suggest this: e.g. cylinders and cones currently have a height property, not length.

Secondly, in my opinion, cones and conical frusta should not start at the base, but instead be centered on the origin like other primitives (i.e. the center of the cone is at the geometric center, half-way between the tip and base). Cylinders, conical frusta, and cones can be treated as different versions of the same shape: many engines only have a conical frustum (but call it a cylinder), and cylinders and cones are just a special case of this. They should all be positioned similarly, centered at the origin, like other primitives.

For reference, Godot has conical frusta (called cylinder) centered like this for both the mesh and collider, Parry has cone and cylinder colliders centered, and Blender also centers cylinders (but doesn't seem to have two radii for them).

Side note: The gizmo examples cycle through primitives with the Up/Down arrow keys, but the text overlays don't say this.

[RobWalt]

Good points 👍🏼 I'll tackle these asap.

The upright thing makes total sense and I already thought about if I should make that change. I used Z up mainly for my mental model while developing since I'm used to it from other applications using mainly 2D structures with an optional Z coordinate in specific cases.

As for the centering: That's also reasonable to further unify implicit assumptions, so cool.

I'll also add UI text telling the example users how to navigate the 3D gizmos.

[Jondolf]

Looking pretty good now! I have some final nits and fixes, mainly around lines and some comments.

You should probably also add the primitive gizmo traits to the bevy_gizmos prelude so that primitive_2d and primitive_3d work without having to explicitly import traits:

/// The `bevy_gizmos` prelude.
pub mod prelude {
    #[doc(hidden)]
    pub use crate::{
        aabb::{AabbGizmoConfigGroup, ShowAabbGizmo},
        config::{DefaultGizmoConfigGroup, GizmoConfig, GizmoConfigGroup, GizmoConfigStore},
        gizmos::Gizmos,
        // This
        primitives::{dim2::GizmoPrimitive2d, dim3::GizmoPrimitive3d},
        AppGizmoBuilder,
    };
}

[Jondolf]

FYI, Capsule was renamed to Capsule3d and a Capsule2d primitive was added: #11585

It should probably be added as a 2D gizmo too, it'd just be two semicircles and lines to connect them

[RobWalt]

FYI, Capsule was renamed to Capsule3d and a Capsule2d primitive was added: #11585

It should probably be added as a 2D gizmo too, it'd just be two semicircles and lines to connect them

Yeah I saw that. Solid addition btw! Really cool seeing the primitives evolve. They grow up so fast! 🥲

[alice-i-cecile]

I would really like docs for these public methods.

[RobWalt]

Good catch 🎯 They are exposed by the pub(crate) helper module but I guess I should've made them pub(crate) themselves as well. That's easier to read for the bevy devs which just jump there and are confused why they can't access it elsewhere.

I did some documentation there as well anyways

[alice-i-cecile]

A ton of code, but very straightforward. A nice demonstration of the utility of our new primitives.

I would like aggressive documentation, but it's not so bad that it can't be fixed in follow-up.

[alice-i-cecile]

Leaving this until Monday's merge train: if you have time to write docs this weekend, great! But I'm comfortable merging this either way.