Drawing Primitives with Gizmos by JasmineLowen · Pull Request #11072 · bevyengine/bevy

JasmineLowen · 2023-12-23T00:07:07Z

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)

JasmineLowen · 2024-01-09T09:58:25Z

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

JasmineLowen · 2024-01-09T15:46:45Z

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 · 2024-01-09T21:55:05Z

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

JasmineLowen · 2024-01-22T11:44:11Z

@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.

JasmineLowen · 2024-01-25T19:53:50Z

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

nothendev · 2024-01-25T20:15:21Z

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

JasmineLowen · 2024-01-26T10:35:14Z

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 · 2024-01-27T16:47:07Z

crates/bevy_gizmos/src/primitives.rs

+/// Builder for configuring the drawing options of [`Ellipse`].
+pub struct Ellipse2dBuilder<'a, 'w, 's, T: GizmoConfigGroup> {
+    gizmos: &'a mut Gizmos<'w, 's, T>,
+
+    half_width: f32,  // Half-width of the ellipse
+    half_height: f32, // Half-height of the ellipse
+
+    center: Vec2, // Position of the center of the ellipse
+    color: Color, // Color of the ellipse
+}


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 · 2024-01-27T16:54:07Z

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 · 2024-01-27T17:07:34Z

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

# Objective - Implement an arc3d API for gizmos - Solves #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](#11072 (comment))

JasmineLowen · 2024-01-28T10:02:22Z

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 · 2024-01-28T13:12:05Z

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.

JasmineLowen · 2024-01-29T05:45:18Z

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,
    };
}

crates/bevy_gizmos/src/primitives/dim2.rs

crates/bevy_gizmos/src/primitives/dim3.rs

crates/bevy_gizmos/src/primitives/dim2.rs

crates/bevy_gizmos/src/primitives/dim3.rs

crates/bevy_gizmos/src/primitives/helpers.rs

Jondolf · 2024-01-29T20:50:48Z

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

JasmineLowen · 2024-01-30T10:24:21Z

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! 🥲

JasmineLowen · 2024-01-31T12:23:03Z

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

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

Fix code after rebasing on main. The capsule primitive was split for 2d and 3d cases. Authored-by: RobWalt <robwalter96@gmail.com>

This includes: - just drawing one line for line segements (2D/3D) - use minimum length for directions in 2D to prevent 1 pixel lines - now: 50 pixel - changing the infinit-ness of lines by adding big constant scale factors now: - 2D infinite length: 100_000 - 3D infinite length: 10_000 - refactor plane (2D/3D) to use other primitives for simplicity and less repetition (they now use a combination of Direction + Segement rendering) - add configurable options to Plane3d (number + detail + size of hinting axis) - adjust examples in 2D/3D cases - use consistent variable names - clean up docs

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

The examples should probably be cleaner than the previous state. After all people are learning with these so we should implement best practices and keep systems small and decoupled. Authored-by: RobWalt <robwalter96@gmail.com>

This commit implements a loose bunch of small polishing fixes: - use angles in radians instead of `Mat2` for rotation in 2D primitive case and ellipse case - use constants on `Direction2D` struct as much as possible - update key bindings in examples to be consistent - update user facing text in examples to reflect real key bindings - fix naming on configurating builder for `Plane3D` sorry should have added you earlier below 🙈 Reviewed-by: Joona Aalto <jondolf.dev@gmail.com> Authored-by: RobWalt <robwalter96@gmail.com>

Since a `half_size` `Vec2` was used on a lot of primitives lately I thought that we should be consistent here with ellipses as well. Authored-by: RobWalt <robwalter96@gmail.com>

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

alice-i-cecile · 2024-02-02T15:31:57Z

crates/bevy_gizmos/src/primitives/helpers.rs

+
+// helpers - affine transform
+
+pub fn rotate_then_translate_2d(angle: f32, position: Vec2) -> impl Fn(Vec2) -> Vec2 {


I would really like docs for these public methods.

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 · 2024-02-02T15:34:26Z

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.

Authored-by: Aviac <aviac@mailbox.org>

# 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))

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 bevyengine#10571 ## Solution - This implements the first design idea I had that covered everything that was mentioned in the Issue bevyengine#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) --- --------- Co-authored-by: nothendev <borodinov.ilya@gmail.com>

Nilirad added C-Feature A new feature, making something new possible A-Gizmos Visual editor and debug gizmos labels Dec 23, 2023

JasmineLowen marked this pull request as ready for review January 8, 2024 11:29

Jondolf self-requested a review January 9, 2024 19:57

Jondolf mentioned this pull request Jan 9, 2024

Gizmos for primitive shapes #10571

Closed

JasmineLowen changed the title ~~WIP: Feat/primitive gizmos~~ Feat/primitive gizmos Jan 9, 2024

Jondolf mentioned this pull request Jan 15, 2024

Tracking Issue: Primitive Shapes #10572

Open

41 tasks

atlv24 suggested changes Jan 21, 2024

View reviewed changes

This was referenced Jan 26, 2024

Let Gizmos support arcs in 3D #11536

Closed

Implement Arc3D for Gizmos #11540

Merged

JasmineLowen changed the title ~~Feat/primitive gizmos~~ Drawing Primitives with Gizmos Jan 26, 2024

Jondolf reviewed Jan 27, 2024

View reviewed changes

Jondolf suggested changes Jan 29, 2024

View reviewed changes

JasmineLowen added 15 commits February 1, 2024 07:43

fix(gizmos): switch mental model from Z-up to Y-up

3bac10d

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

fix(gizmos): center cone and conical frustrum rendering

12b4d2c

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

fix(gizmo-examples): add user facing explanations

85d245f

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

fix(cleanup): capsule3d changes

10f3df0

Fix code after rebasing on main. The capsule primitive was split for 2d and 3d cases. Authored-by: RobWalt <robwalter96@gmail.com>

feat(capsule2d): add gizmo impl for capusle 2d

cf383e4

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

fix(prelude): auto export new gizmo traits in prelude

a6dd10e

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

chore(exmaples): clean up examples

0fd2c46

The examples should probably be cleaner than the previous state. After all people are learning with these so we should implement best practices and keep systems small and decoupled. Authored-by: RobWalt <robwalter96@gmail.com>

fix(tests): fix doc tests + half_size consistency

15c68e1

Since a `half_size` `Vec2` was used on a lot of primitives lately I thought that we should be consistent here with ellipses as well. Authored-by: RobWalt <robwalter96@gmail.com>

chore(cleanup): rename and move systems around

edf5e4f

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

chore(fix-docs): doc links were broken

30c620a

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

chore(CI): fix error from bevy_internal CI lint

00aefe0

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

chore(typo): fixes via spell checking

7c64e9c

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

chore(examples): consistent user facing texts

d3df91f

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

alice-i-cecile reviewed Feb 2, 2024

View reviewed changes

alice-i-cecile approved these changes Feb 2, 2024

View reviewed changes

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 Feb 2, 2024

alice-i-cecile added this to the 0.13 milestone Feb 2, 2024

alice-i-cecile mentioned this pull request Feb 2, 2024

Add example for bounding volumes and intersection tests #11666

Merged

docs: increase amount of documentation

1531339

Authored-by: Aviac <aviac@mailbox.org>

alice-i-cecile added this pull request to the merge queue Feb 2, 2024

Merged via the queue into bevyengine:main with commit 041731b Feb 2, 2024

JasmineLowen deleted the feat/primitive-gizmos branch February 2, 2024 22:33

Jondolf mentioned this pull request Feb 3, 2024

Tracking issue: Amazing Gizmos #9400

Open

57 tasks

spectria-limina mentioned this pull request Feb 6, 2024

New circular primitives: Arc2d, CircularSector, CircularSegment #11748

Closed

3 tasks


		// helpers - affine transform

		pub fn rotate_then_translate_2d(angle: f32, position: Vec2) -> impl Fn(Vec2) -> Vec2 {

Uh oh!

Conversation

JasmineLowen commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Caveats

Changelog

Uh oh!

JasmineLowen commented Jan 9, 2024

Uh oh!

JasmineLowen commented Jan 9, 2024

Uh oh!

Jondolf commented Jan 9, 2024

Uh oh!

atlv24 left a comment

Choose a reason for hiding this comment

Uh oh!

JasmineLowen commented Jan 22, 2024

Uh oh!

JasmineLowen commented Jan 25, 2024

Uh oh!

nothendev commented Jan 25, 2024

Uh oh!

JasmineLowen commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jondolf left a comment

Choose a reason for hiding this comment

Uh oh!

Jondolf Jan 27, 2024

Choose a reason for hiding this comment

Uh oh!

Jondolf commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jondolf commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasmineLowen commented Jan 28, 2024

Uh oh!

Jondolf commented Jan 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasmineLowen commented Jan 29, 2024

Uh oh!

Jondolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jondolf commented Jan 29, 2024

Uh oh!

JasmineLowen commented Jan 30, 2024

Uh oh!

JasmineLowen commented Jan 31, 2024

Uh oh!

alice-i-cecile Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

JasmineLowen Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

JasmineLowen commented Dec 23, 2023 •

edited

Loading

JasmineLowen commented Jan 26, 2024 •

edited

Loading

Jondolf commented Jan 27, 2024 •

edited

Loading

Jondolf commented Jan 27, 2024 •

edited

Loading

Jondolf commented Jan 28, 2024 •

edited

Loading