bevy
bevy copied to clipboard
Drawing Primitives with Gizmos
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
Defaultand 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_mathin 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)dimpls forDirection(2/3)dto 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
Primitive2dforDirection2dand implPrimitive3dforDirection3d - implement trait to draw primitives with specialized details with gizmos
GizmoPrimitive2dfor all the 2D primitivesGizmoPrimitive3dfor 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)
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
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
Gizmosinto a separate PR (Ellipses & Arcs)
@Jondolf Can you take a look over this and review it if you find some time?
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.
@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.
Resolved the last issue with this and also removed the nix files
I'm wondering if Self::Output<'_> is 2018 idioms..
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!
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.
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
primitivesinto 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
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 😁 👍🏼
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.
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.
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
FYI,
Capsulewas renamed toCapsule3dand aCapsule2dprimitive was added: #11585It 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! 🥲
@Jondolf Examples are updated now and you can take a look at most primitives (except for the polygon ones)
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.