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 Grid gizmos #11988

Merged
merged 14 commits into from
Feb 28, 2024
Merged

Add Grid gizmos #11988

merged 14 commits into from
Feb 28, 2024

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented Feb 20, 2024

Objective

Solution

  • Added gizmos.grid(...) and gizmos.grid_2d(...)
  • The grids may be configured using .outer_edges(...) to specify whether to draw the outer border/edges of the grid and .skew(...)to specify the skew of the grid along the x or y directions.

Changelog

  • Added a grid module to bevy_gizmos containing gizmos.grid(...) and gizmos.grid_2d(...) as well as assorted items.
  • Updated the 2d_gizmos and 3d_gizmos examples to use grids.

Additional

The 2D and 3D examples now look like this:
Screenshot 2024-02-20 at 15 09 40
Screenshot 2024-02-20 at 15 10 07

@TrialDragon TrialDragon added C-Feature A new feature, making something new possible A-Gizmos Visual editor and debug gizmos labels Feb 20, 2024
Copy link
Contributor

@afonsolage afonsolage left a comment

Choose a reason for hiding this comment

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

I ran the examples and they are fine and I'm not a math guy, but the implementation seems fine too, my only point is about grid_2d since to me both grids are 2d, since a grid_3d would means, at least to me, a "cubic grid" not a "square grid".

IMO we should have only grid and showcase how to use them in 2d_gizmos and 3d_gizmos.

@nicopap nicopap mentioned this pull request Feb 20, 2024
57 tasks
@lynn-lumen
Copy link
Contributor Author

I get the confusion, but this follows the naming conventions for other gizmos like .rect() and .rect_2d. The basic idea is to provide a gizmo to 2D users who don't want to engage with quaternions just for the sake of drawing gizmos. I have not seen a 'cubic grid' (gizmo) used anywhere so far, but if there is interest I will add it.

@afonsolage
Copy link
Contributor

Sorry if I wasn't clear, but my point is I don't think we need a gizmos_2d just gizmos is enough.

@lynn-lumen
Copy link
Contributor Author

Yes, this would still provide all the functionality, but isn't approachable for new users. Imagine someone building a 2D game: they have only dealt with Vec2 positions and f32 rotations. If they want to draw a gizmo, they should continue to be fine with that and not be forced to learn about quaternions just for gizmos.

@pablo-lua
Copy link
Contributor

pablo-lua commented Feb 20, 2024

In reality, would be good to have a cubic grid IMO. And one could find good to be able to change the space between X lines and Y lines. (In the cubic grid example, we would have Z lines)

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.

Can we maybe change the color of the grid lines in the example? It was a bit hard to me to see at first. Maybe we can use white?

@lynn-lumen
Copy link
Contributor Author

And one could find good to be able to change the space between X lines and Y lines.

I'm not sure I understand this . You can have different spacings for x and y lines already. Simply pass something like Vec2::new(2., 1.) as the spacing argument. Am I missing something?

@pablo-lua
Copy link
Contributor

And one could find good to be able to change the space between X lines and Y lines.

I'm not sure I understand this . You can have different spacings for x and y lines already. Simply pass something like Vec2::new(2., 1.) as the spacing argument. Am I missing something?

Not really, I'm saying thats good, sorry, wasn't clear.
The only thing that I think deserves to be there is the cubic grid in reality, as Users can find good use in it for debug purposes

@afonsolage
Copy link
Contributor

And one could find good to be able to change the space between X lines and Y lines.

I'm not sure I understand this . You can have different spacings for x and y lines already. Simply pass something like Vec2::new(2., 1.) as the spacing argument. Am I missing something?

Not really, I'm saying thats good, sorry, wasn't clear.
The only thing that I think deserves to be there is the cubic grid in reality, as Users can find good use in it for debug purposes

But as follow-up PR, right? Because I think a cubic-grid is out of scope of this PR, since this can be controversial, since the implementation can be trick IMO.

@lynn-lumen
Copy link
Contributor Author

Can we maybe change the color of the grid lines in the example? It was a bit hard to me to see at first. Maybe we can use white?

White was a bit overwhelming, but it should be better now.

@pablo-lua
Copy link
Contributor

But as follow-up PR, right? Because I think a cubic-grid is out of scope of this PR, since this can be controversial, since the implementation can be trick IMO.

Yea, we can track this in #9400

@lynn-lumen
Copy link
Contributor Author

But as follow-up PR, right?

I think I could add this here, if that is ok. It would be just adding another axis to the existing implementation and something like gizmos.cubic_grid(...)

@pablo-lua
Copy link
Contributor

I think I could add this here, if that is ok. It would be just adding another axis to the existing implementation and something like gizmos.cubic_grid(...)

If we implement in the same way that is now, I think that's okay, but might be good keep this as uncontroversial as possible and do this in a follow-up? Not sure.
About the cubic_grid part, wouldn't be better call it "grid_3d" or only "grid" to keep the convention?

@lynn-lumen
Copy link
Contributor Author

lynn-lumen commented Feb 20, 2024

If we implement in the same way that is now, I think that's okay, but might be good keep this as uncontroversial as possible and do this in a follow-up? Not sure. About the cubic_grid part, wouldn't be better call it "grid_3d" or only "grid" to keep the convention?

Ok, fair enough. As for the name, we already have .grid which we could just extend to include a third axis or we could do something like .lattice? I don't think grid_3d would be a good idea though, as it suggests (at least to me) that it is the same shape as .grid although it is different/more general.

@pablo-lua
Copy link
Contributor

Keeping .grid(...) to do that function is better IMO.

@lynn-lumen
Copy link
Contributor Author

Keeping .grid(...) to do that function is better IMO.

Ok, so we will add that functionality to .grid in a future PR, right?

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.

The math behind this looks fine to me, and nothing to say about the grid itself. All good

Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

New to gizmos, so seeing Drop used for finalizing the builder was interesting

Looks good

@alice-i-cecile 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 27, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

The Drop pattern is a little too clever for my taste, and doesn't appear to be used elsewhere in gizmos. Can we find a simpler solution?

@MiniaczQ
Copy link
Contributor

The Drop pattern is a little too clever for my taste, and doesn't appear to be used elsewhere in gizmos. Can we find a simpler solution?

oh, I thought it was a standard for gizmos so I didn't question it
In that case agreed

@pablo-lua
Copy link
Contributor

I'm a little confused, isn't the drop pattern used in multiple gizmos builders? Or is there something different here? For starters, arc builders use this

@lynn-lumen
Copy link
Contributor Author

isn't the drop pattern used in multiple gizmos builders?

Yes, the Drop pattern is used for all primitives, ellipses and arcs.

@alice-i-cecile
Copy link
Member

Ah my apologies, I did a search for impl Drop, which didn't reveal anything because of the intervening generics.

@alice-i-cecile alice-i-cecile self-requested a review February 27, 2024 23:32
@alice-i-cecile
Copy link
Member

@solis-lumine-vorago can you rebase (the examples got moved) and I'll give this a final pass before merging?

auto-merge was automatically disabled February 28, 2024 00:02

Head branch was pushed to by a user without write access

@lynn-lumen
Copy link
Contributor Author

@solis-lumine-vorago can you rebase (the examples got moved) and I'll give this a final pass before merging?

Yeah sure, it appears that Color was renamed but it should be all good now.

@lynn-lumen
Copy link
Contributor Author

@alice-i-cecile Could you enable Auto-merge again, please :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 28, 2024
@lynn-lumen
Copy link
Contributor Author

thx :D

Merged via the queue into bevyengine:main with commit 35cec8b Feb 28, 2024
25 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.

6 participants