bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Drawing Primitives with Gizmos

Open RobWalt opened this issue 1 year ago • 11 comments
trafficstars

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:

  • [x] docs for all the new structs and traits
  • [x] implement Default and derive other useful traits for the new structs
  • [x] Take a look at the notes again (Do this after a first round of reviews)
  • [x] 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 #10571

Solution

  • This implements the first design idea I had that covered everything that was mentioned in the Issue https://github.com/bevyengine/bevy/issues/10571#issuecomment-1863646197

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 avatar Dec 23 '23 00:12 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 avatar Jan 09 '24 09:01 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?

RobWalt avatar Jan 09 '24 15:01 RobWalt

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: https://github.com/bevyengine/bevy/issues/10571#issuecomment-1883854471. I felt like it'd be too long and slightly off-topic to post here.

Jondolf avatar Jan 09 '24 21:01 Jondolf

@rodolphito I will when this branch is ready. Atm I'm facing the problem described in https://github.com/bevyengine/bevy/issues/10571#issuecomment-1893429450 which blocks this here from a merge anyways.

RobWalt avatar Jan 22 '24 11:01 RobWalt

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

RobWalt avatar Jan 25 '24 19:01 RobWalt

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

nothendev avatar Jan 25 '24 20:01 nothendev

Just so I don't forget: This needs a rebase on this https://github.com/bevyengine/bevy/pull/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!

RobWalt avatar Jan 26 '24 10:01 RobWalt

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 avatar Jan 27 '24 16:01 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

Jondolf avatar Jan 27 '24 17:01 Jondolf

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 😁 👍🏼

RobWalt avatar Jan 28 '24 10:01 RobWalt

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:

https://github.com/bevyengine/bevy/assets/57632562/6942b2ad-98c6-4769-bb52-23d0948ef274

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.

Jondolf avatar Jan 28 '24 13:01 Jondolf

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.

RobWalt avatar Jan 29 '24 05:01 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

Jondolf avatar Jan 29 '24 20:01 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

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

RobWalt avatar Jan 30 '24 10:01 RobWalt

@Jondolf Examples are updated now and you can take a look at most primitives (except for the polygon ones)

RobWalt avatar Jan 31 '24 12:01 RobWalt

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.

alice-i-cecile avatar Feb 02 '24 15:02 alice-i-cecile