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

Update glam to 0.12.0 #1249

Merged
merged 1 commit into from
Jan 17, 2021
Merged

Update glam to 0.12.0 #1249

merged 1 commit into from
Jan 17, 2021

Conversation

bitshifter
Copy link
Contributor

Added

  • Added f64 primitive type support
    • vectors: DVec2, DVec3 and DVec4
    • square matrices: DMat2, DMat3 and DMat4
    • a quaternion type: DQuat
  • Added i32 primitive type support
    • vectors: IVec2, IVec3 and IVec4
  • Added u32 primitive type support
    • vectors: UVec2, UVec3 and UVec4
  • Added bool primitive type support
    • vectors: BVec2, BVec3 and BVec4

Changed

  • Vec2Mask, Vec3Mask and Vec4Mask have been replaced by BVec2, BVec3,
    BVec3A, BVec4 and BVec4A. These types are used by some vector methods
    and are not typically referenced directly. This is a breaking change.

Removed

  • build.rs has been removed.

@mockersf
Copy link
Member

would it make sense to have bevy_reflect re-export glam, and bevy_math use it from there instead of both declaring it as a dependency?

@bitshifter
Copy link
Contributor Author

I don't know, glam appears to be optional in bevy_reflect, but perhaps it is intended to be built standalone and would always have glam enabled if built as part of bevy.

@bitshifter
Copy link
Contributor Author

It did catch me out that two places required to be updated, so perhaps it would be a good thing to do.

@mockersf
Copy link
Member

mockersf commented Jan 16, 2021

oh right, I missed that glam is optional in bevy_reflect for standalone. still as bevy_math always enable the feature, it should work to re-export it

@cart
Copy link
Member

cart commented Jan 16, 2021

Awesome! I'm looking forward to int-ifying a number of apis.

And yeah bevy_reflect is intended to be usable standalone without bevy (or glam), which is why its optional. We could pull glam deps from bevy_math instead of glam directly, but that felt dirtier because it meant consumers of bevy_reflect would need to consume bevy_math when they really only wanted glam.

@mockersf
Copy link
Member

as bevy_math depends on bevy_reflect, it would have to be bevy_reflect as the original source of glam, so it shouldn't impact other consumers of bevy_reflect

@cart
Copy link
Member

cart commented Jan 16, 2021

Thats a valid point. But it still seems a bit odd from a "self documenting" standpoint. I'm not sure the loss of clarity is worth the removal of duplicate instances. From a cargo/rustc perspective it shouldn't matter right?

Alternatively glam could add optional bevy_reflect impls :)

@cart
Copy link
Member

cart commented Jan 16, 2021

Although then we'd need to make sure the versions stay in sync, which is harder to do across projects.

@mockersf
Copy link
Member

mockersf commented Jan 16, 2021

From a cargo/rustc perspective it shouldn't matter right?

none at all, it's just error prone when updating, but an error that will be caught very fast by CI, although with not the best rust error message in my opinion

@Moxinilian Moxinilian added A-Core Common functionality for all bevy apps C-Dependencies A change to the crates that Bevy depends on labels Jan 16, 2021
@cart
Copy link
Member

cart commented Jan 17, 2021

I think for now I'm inclined to merge this as-is. Feel free to open up an issue for merging deps if you want 😄

@cart
Copy link
Member

cart commented Jan 17, 2021

(but im still not personally sold in the idea)

@cart cart merged commit e7dab0c into bevyengine:master Jan 17, 2021
rparrett pushed a commit to rparrett/bevy that referenced this pull request Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants