-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Feature/glam cross constants #21561
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
base: main
Are you sure you want to change the base?
Feature/glam cross constants #21561
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
reason = "compatibility with the unstable rust definition" | ||
)] | ||
/// Approximation of 1/sqrt(3) needed for the diagonals in 3D space | ||
const FRAC_1_SQRT_3: f32 = 0.577350269189625764509148780501957456_f32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit weird, since an f32 can't hold more than about 7 digits of precision, but I don't know if there's maybe another reason to want an extra-precise constant definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these excessively long definitions are copied from the rust std::f32::consts, but I'm unsure why they would be there in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i propose to keep this definition for now for the sake of consistency with the rust stdlib, so that when the FRAC_1_SQRT_3 gets stabilized we can just switch over to that and be certain that there's no weird behavior from unexpected changes
crates/bevy_math/src/direction.rs
Outdated
)] | ||
/// Approximation of 1/sqrt(3) needed for the diagonals in 3D space | ||
const FRAC_1_SQRT_3: f32 = 0.577350269189625764509148780501957456_f32; | ||
/// The diagonals between the cardinal directions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation here is a little confusing, from the comment I understood that this is a diagonal between two cardinal directions, like a diagonal between X and Y, but it's diagonals between triplets of cardinal directions. So it's directions pointing towards the vertices of a cube centered at the origin / the main diagonals of such a cube.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
changed the documentation to reflect that these are directions to vertices
the directions between 2 lines are represented in the ALL_EDGES group
crates/bevy_math/src/direction.rs
Outdated
Self::SOUTH_EAST, | ||
Self::SOUTH_WEST, | ||
]; | ||
/// All neighboring directions on a grid. A combination of [`Self::CARDINALS`] and [`Self::DIAGONALS`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Neighborhoods aren't a standardised concept, so I think it would be good to specify it's in a 3x3 neighboorhood. Similarly for the 3d case specify a 3x3x3 neighboorhood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
annotated the ALL_NEIGHBORDS collections with a "on a cube/square grid in a 3x3(x3) neighborhood"
crates/bevy_math/src/direction.rs
Outdated
)), | ||
]; | ||
/// All neighboring directions on a grid. A combination of [`Self::CARDINALS`] and [`Self::DIAGONALS`] | ||
pub const ALL_NEIGHBORS: [Self; 14] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't all neighbors in a 3x3x3 range, it's the directly adjacent ones coresponding to 6 faces of a cube + the ones corresponding to the 8 vertices of a cube, but then there's missing 12 neighbors which correspond to the edges of a cube (which are closer than the corner ones, so it imho doesn't make sense to not include them). Overall there should be 26 of them (Since 3x3x3 = 27 and we remove the center).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, thank you! missed that. will add later
I'm guessing we would want a definition for them as well, at least for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
added the ALL_EDGES collection
Thank you for working on this! |
fix typo Co-authored-by: Joona Aalto <[email protected]>
Objective
Fixes #21333
Solution
This PR introduces several new constants for Dir2 and Dir3,
providing convenient shorthands for popular directions on a grid, such as:
Testing
Changes were not tested via autotests, but I did check my math to make sure I didn't mess up.
You can look at the changed files and make sure that the constants contain the directions they are supposed to
Showcase
For example. if you have a grid and wish to get all neighboring cells on it,
you don't have to define the directions yourself and can do the following