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

Add axes_2d gizmo. #12334

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

lambertsbennett
Copy link

Objective

This PR addresses #12222 (Fixes #12222). Simple addition to add a 2D axes gizmo.

Solution

  • Add a new method axes_2d which takes a transform and a case length and then draws two arrows in the XY plane.

The only thing I'm not sure about here is taking a 3D transform as an argument. It says in the transform comments that for 2D the z-axis is used for ordering, so I figured I'd keep it that way?


Changelog

  • Add method axes_2d.
  • Update arrow_2d to also calculate the tip length depending on arrow length as in arrow.
  • Add axes_2d to examples 2d_gizmos.

Copy link
Contributor

github-actions bot commented Mar 6, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@lambertsbennett
Copy link
Author

lambertsbennett commented Mar 6, 2024

I see that the build/CI are failing because the compiler says that From<Vec3> is not implemented for Vec2. This is puzzling to me because I did implement it as:

impl From<Vec3> for Vec2 {
    #[inline]
    fn from(v: Vec3) -> Self {
        (v.x, v.y)
    }
}

I am relatively new to rust though, so perhaps this isn't correct? Any pointers here would be great! The strange thing is that the example works just fine (2d_gizmos). It's just when compiling the crate that there is an error.

@notmd
Copy link
Contributor

notmd commented Mar 6, 2024

You can't implement a trait for a type if both of them are not in your local module. See https://doc.rust-lang.org/reference/items/implementations.html#orphan-rules
With this change, it's work for me

diff --git a/crates/bevy_gizmos/src/arrows.rs b/crates/bevy_gizmos/src/arrows.rs
index 899af3374..e9dad936c 100644
--- a/crates/bevy_gizmos/src/arrows.rs
+++ b/crates/bevy_gizmos/src/arrows.rs
@@ -199,7 +199,7 @@ impl<'w, 's, T: GizmoConfigGroup> Gizmos<'w, 's, T> {
         let end_x = transform.transform_point(base_length * Vec3::X);
         let end_y = transform.transform_point(base_length * Vec3::Y);

-        self.arrow_2d(start.into(), end_x.into(), RED);
-        self.arrow_2d(start.into(), end_y.into(), GREEN);
+        self.arrow_2d(start.truncate(), end_x.truncate(), RED);
+        self.arrow_2d(start.truncate(), end_y.truncate(), GREEN);
     }
 }

bevy_sprite = { path = "../bevy_sprite", version = "0.14.0-dev", optional = true }
bevy_pbr = { path = "../bevy_pbr", version = "0.14.0-dev", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

you should avoid changing this

@@ -87,6 +87,8 @@ fn draw_example_collection(
Vec2::from_angle(sin / -10. + PI / 2.) * 50.,
YELLOW,
);

gizmos.axes_2d(Transform::from_translation(Vec3::new(sin, 0.0, 0.0)), 25.0)
Copy link
Member

Choose a reason for hiding this comment

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

this example is already quite crowded. this will also put it on the grid gizmo already present, making it hard to read

Comment on lines 202 to 203
self.arrow_2d(start.into(), end_x.into(), RED);
self.arrow_2d(start.into(), end_y.into(), GREEN);
Copy link
Member

@mockersf mockersf Mar 6, 2024

Choose a reason for hiding this comment

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

Suggested change
self.arrow_2d(start.into(), end_x.into(), RED);
self.arrow_2d(start.into(), end_y.into(), GREEN);
self.arrow_2d(start.xy(), end_x.xy(), RED);
self.arrow_2d(start.xy(), end_y.xy(), GREEN);

and you'll need to import bevy_math::Vec3Swizzles

Comment on lines 128 to 135
let length = (end - start).length();
ArrowBuilder {
gizmos: self,
start: start.extend(0.),
end: end.extend(0.),
color: color.into(),
tip_length: length / 10.,
}
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this?

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to be consistent with the 3d arrow drawing method, but I realize now that was not a smart change.

@lambertsbennett
Copy link
Author

Ok thanks for the feedback @notmd and @mockersf - I reverted a bunch of things and now it seems to be in a better state.

@Jondolf Jondolf added C-Enhancement A new feature A-Gizmos Visual editor and debug gizmos labels Mar 6, 2024
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

Good change, thank you. Can you put this feature in the axes example? I think we can place it in the plane on the example? We can make the bevy logo bounce around in the floor I believe.

@@ -163,3 +163,36 @@ impl<'w, 's, T: GizmoConfigGroup> Gizmos<'w, 's, T> {
self.arrow(start, end_z, BLUE);
}
}

impl<'w, 's, T: GizmoConfigGroup> Gizmos<'w, 's, T> {
Copy link
Contributor

@pablo-lua pablo-lua Mar 6, 2024

Choose a reason for hiding this comment

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

I think we should adress both methods in the same impl if possible and move this to the impl above this.

@lambertsbennett
Copy link
Author

@pablo-lua I moved it into the same impl block as you suggested and added a 2D axis to the axes example. My first try at that caused there to be axes flying around in space and so I pinned them to the plane. Maybe this is not exactly illustrative of the capability of the 2D axes to transform though.

@pablo-lua
Copy link
Contributor

Fair, I was thinking in placing this as a Sprite in reality, like, spawning a Sprite of some kind and just use the transform in it to move it around and so. We can address this in another PR though, so I think we should revert it for now and do this as a follow up.

@lambertsbennett
Copy link
Author

@pablo-lua ok reverted the example, but otherwise I hope its all good!

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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

axes_2d gizmo
6 participants