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 coordinate system trait and implementations for Vec3 and Vec2 #1674

Closed
wants to merge 1 commit into from

Conversation

bitshifter
Copy link
Contributor

This is related to #1663 which proposes changing local_y to local_up and so on. Internally these methods currently rotate Vec3::Y for local_y but if they are changed to local_up then they can now rotate Vec3::UP.

@alice-i-cecile alice-i-cecile added A-Core Common functionality for all bevy apps C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 17, 2021
@cart
Copy link
Member

cart commented Apr 12, 2021

I'm a bit torn on this one. I think right/left/up/down are friendlier/more natural names. But idk if the added "trait extension method" complexity is worth it. Its less discoverable and forces users to import it (either via prelude or manually). The cost of that versus learning that y is up, z is back, and x is right (and negating them for the other directions) feels pretty similar.

I briefly considered proposing "opt-in right hand and left hand coordinate system features for glam", which would define the appropriate constants when one or the other feature is enabled. But that probably doesn't solve the documentation / discovery problem. I'm still curious to hear your thoughts on that though :)

@bitshifter
Copy link
Contributor Author

I was experimenting with ways of doing this without adding them to glam basically. Ideally glam at it's core would be co-ordinate system agnostic, it's not quite there yet though. So I'm trying to come up with ways to have coordinate system specific functionality outside of core glam. I guess a features would be an alternative way, I have concerns about that approach, in that there might be a lot of different combinations so support, also there might be clashes if a project managed to have dependencies with different features enabled (not sure how realistic that is though).

Thinking about the math library we have at work and what is possibly more useful is having constants for the indices of different axes, e.g. SIDE = 0, UP = 1, FORWARD = 2. That won't help with directionality but it's useful for grabbing the correct axis out of a matrix. Those could be usize constants which don't need to be extension methods. That's kind of solving a different problem to these constants, but related. I do find it useful to be unambiguous about these things.

Other co-ordinate specific things that it would be good to move out of glam, or make more general are using Euler rotations and also look at matrix functions. So this PR is also exploring ways of defining those outside of glam, to suit the project that is using them.

@cart
Copy link
Member

cart commented Apr 13, 2021

Yeah I can see feature clashing being a problem. I guess at the end of the day users will probably need to import something to get these values. Maybe an extension trait is the right call.

In the short term I think I'll hold off on merging this as I'm still working out my opinions on it and I'm curious to see what others think.

@alice-i-cecile
Copy link
Member

I like this much better than the status quo. While from an end user perspective:

The cost of that versus learning that y is up, z is back, and x is right (and negating them for the other directions) feels pretty similar.

may be true, it's much more challenging to read code defined in terms of +/-z (especially with some padding zeros) than one that uses "up" and "down".

I would ultimately prefer a "typed coordinate systems" approach where these constants are defined within those types, and a common trait joining them all, but I suspect that that will be too heavy of an abstraction for most game programmers to swallow.

But that probably doesn't solve the documentation / discovery problem. I'm still curious to hear your thoughts on that though :)

IMO with an extended book and a chapter on transforms (plus use in the examples) I don't see this being a huge problem. They're also very discoverable through searching "up" on docs.rs, which goes a long way for the users that dive in.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
@alice-i-cecile alice-i-cecile added A-Transform Translations, rotations and scales S-Needs-Review and removed A-Core Common functionality for all bevy apps labels Sep 22, 2021
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

At this point you are going to need to rebase, but the code addition are nice and simple.

Copy link
Contributor

@inodentry inodentry left a comment

Choose a reason for hiding this comment

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

I like this and I'd like to vote in favor of merging it. Seems like a non-intrusive usability improvement.

@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 Sep 25, 2021
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR 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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 22, 2022
@cart
Copy link
Member

cart commented Oct 28, 2022

Closing as this is unlikely to be merged in its current state and I'm still not fully sold on the approach. Anyone is free to create a new issue if they want to open this conversation.

@cart cart closed this Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants