Skip to content

Add axes_2d gizmo. #12334

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

Merged
merged 7 commits into from
Jun 4, 2024
Merged

Add axes_2d gizmo. #12334

merged 7 commits into from
Jun 4, 2024

Conversation

lambertsbennett
Copy link
Contributor

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
Contributor 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
Contributor Author

@lambertsbennett lambertsbennett Mar 6, 2024

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
Contributor 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-Feature A new feature, making something new possible 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
Contributor 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
Contributor Author

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

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 4, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 4, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 4, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 4, 2024
Merged via the queue into bevyengine:main with commit 37cc8ea Jun 4, 2024
2 checks passed
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

axes_2d gizmo
7 participants