chore: update glam to 0.33.2#24814
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨ |
ca70f38 to
4402b22
Compare
| pub use compass::{CompassOctant, CompassQuadrant}; | ||
| pub use direction::*; | ||
| pub use float_ord::*; | ||
| pub use glam::camera; |
There was a problem hiding this comment.
Hi there, I'm the author of glam. One thing I was thinking with this change was that engines like bevy only need to use the view and proj methods that are relevant to their project (e.g. bevy is right handed and uses directx style projection matrices). Similarly they don't need to re-export methods that aren't relevant to users of the engine. The way I was thinking this could be used in bevy would be to do (replacing pub use glam::* at the bottom of the file):
pub use glam::prelude::*; // exports all glam types but not the camera module
pub use glam::camera::rh::proj::directx as proj; // just the projection methods that bevy might use - this could be even more targeted to only the methods bevy currently uses.Obviously it's up to bevy to decide what's best for their project and users, I thought I'd give some of the rational and intent for these changes in glam.
There was a problem hiding this comment.
Thank you for the suggestions, I've just applied them. I'm open to any further feedback to improve this PR.
| glam = { version = "0.32.0", default-features = false, features = [ | ||
| glam = { version = "0.33.2", default-features = false, features = [ | ||
| "serde", | ||
| "all-types", |
There was a problem hiding this comment.
The breaking change in 0.33 is that you can opt out of compiling all types in glam. By default f32 types are always compiled but everything else is optional. On my machine just f32 takes ~0.81s to compile and all-types takes ~4.19s. So there is decent build time saving if you don't need all of the other types.
In bevy there are a couple of considerations; bevy_reflect exposes all of the glam types except for size-types. Also bevy re-exports all glam types so potentially removing them might break user code that depends on them (which raises a question if bevy-math should enable all-types so that they're re-exported). If bevy was interested in disabling types that aren't actively used you might want to consider adding feature flags so user who want them to enable them. That said users can use glam directly if they want these but I assume most bevy users use glam types via bevy_math. For that reason it's not something that you might want to necessarily change for this PR but I thought I'd mention it as something that may want to be considered at some stage.
If you did want to minimise the types compiled by bevy this could be tightened in bevy_reflect to float-types and integer-types here instead of all-types (effectively dropping size-types as bevy doesn't use these anywhere).
There was a problem hiding this comment.
Thanks, I've added your suggestion. Anyway, I think this PR is already a breaking change and won't be in Bevy before 0.20. The compile time improvement is definitely welcome!
cdb32a2 to
d84a289
Compare
|
I just rebased to include this fix: #24943, which fixes the merge queue CI. |
Objective
glamto 0.33.2 to simplify upgrading crates that depend onbevy,glam, andglamxSolution
hexasphere; use ofglam's newfloat-typesandinteger-typesfeaturesTesting
cargo check, as my PC doesn't have enough RAM to run the other tests