bevy
bevy copied to clipboard
bevyengine
Add integer equivalents for `Rect`
What problem does this solve or what need does it fill?
Currently there is no IRect or URect, #7867 is an example of where these would be useful.
What solution would you like?
Create IVec2 and UVec2 equivalents of the following Rect struct and the corresponding methods and tests.
https://github.com/bevyengine/bevy/blob/38a08d9b18c881f0320aff955dc66d4955c63681/crates/bevy_math/src/rect.rs#L14-L19
Not 100% sure on how this should be implemented, glam uses tera templates for their types but that seems way over complicated for this case. The obvious way is to just duplicate the rect.rs file and find and replace all uses of f32 and Vec2 but then you end up with a lot of code duplication. Though I can't think of a better option.
Some thoughts on this would be great.
I'd just use macro_rules and have create_rect_type!(Rect, Vec2), create_rect_type!(URect, UVec2), etc.
On a separate note, I want to potentially SIMD accelerate the type(s) since it's perfectly sized for SSE instructions (~16 bytes). Definitely not immediately necessary, but could be done in a followup PR.
I'll have a go at this, SIMD is out of my scope, so I'll leave that for a future PR.
@james7132 I don't know if macro_rules is the best idea, it will break the doctest examples when writing floats or ints, and it will also make the code more complex and inconsistent with glam.
Examples
let r = Rect::from_center_half_size(Vec2::ZERO, Vec2::ONE); // w=2 h=2
assert!(r.min.abs_diff_eq(Vec2::splat(-1.), 1e-5));
assert!(r.max.abs_diff_eq(Vec2::splat(1.), 1e-5));
I don't know if macro_rules is the best idea, it will break the doctest examples when writing floats or ints...
I went down the macro_rules route and have now hit this issue, do you have any ideas for a better solution? Currently just duplicating the current Rect and doing some find and replace seems like the best route.
@LiamGallagher737 Sorry, I also don't know which solution is better either, I think duplicating the code is the easiest, but maybe using generics or even tera templates could work too. Maybe someone with more experience can help you
I went down the macro_rules route and have now hit this issue, do you have any ideas for a better solution? Currently just duplicating the current Rect and doing some find and replace seems like the best route.
Not a big fan of it, but we could macro out individual functions instead of the entire impl block, and still retain the docs/doctests. Though that also seems like a general loss in readability. Might just want to abandon the macro rules and accept hand-writing it. Unfortunate, but probably needed.
I proposed the macro_rules since this is how glam approaches it's primitive-based variants, though it's pretty apparent we don't have the same level of code duplication glam does here.
In the 0.21 glam stopped being macro-centric, and instead uses code generation: https://github.com/bitshifter/glam-rs/blob/main/CHANGELOG.md#0210---2022-06-22
Not sure if we should add that since it might complicate the surrounding infrastructure for validation/builds. This level of code duplication is probably something we'll just need to stomach.
I'd just use
macro_rulesand havecreate_rect_type!(Rect, Vec2),create_rect_type!(URect, UVec2), etc.On a separate note, I want to potentially SIMD accelerate the type(s) since it's perfectly sized for SSE instructions (~16 bytes). Definitely not immediately necessary, but could be done in a followup PR.
Once this issue is merged, I can start on SIMD.
@LiamGallagher737 Sorry, I also don't know which solution is better either, I think duplicating the code is the easiest, but maybe using generics or even tera templates could work too. Maybe someone with more experience can help you
I support the generics idea, would involve some refactoring but could be more scalable long term
@LiamGallagher737 Sorry, I also don't know which solution is better either, I think duplicating the code is the easiest, but maybe using generics or even tera templates could work too. Maybe someone with more experience can help you
I support the generics idea, would involve some refactoring but could be more scalable long term
After experimenting with this I've run into a couple issues
The primary one is the Vec2 dependency doesn't seem to support generics
Refactoring generics into it requires implementing the From trait to convert whatever type is passed in to f32 for Vec2. This could lead to breaking changes for existing uses of Rect.
Duplicate code for the IRect type may be the quickest way to getting this implemented, but maintaining duplicate functionality for both types long term may be a pain.
With all of that being said, I am a Rust and Bevy newbie so there may be something I am missing or doing incorrectly. However, there doesn't seem to be an urgent need for IRect to be implemented and not having it wouldn't cause anything to break or force other things to change. Due to this lack of urgency, I think that exploring options and possibilities for making Rect generic may be worth the deeper time investment to save us from maintaining duplicate code in the future and making existing code more abstract.
One idea is to remove the Vec2 dependency and create our own Bevy Vec2 type which does support generics. This could lead to more work in the short term, but more control and flexibility in the long term. Again, being a newbie I'm not sure what the core maintainers have envisioned for this long term, but just a thought.
Another thought, and also genuinely curious, is given these constraints or possibility of duplicate code, what is the actual need for IRect to begin with?
@LiamGallagher737 Sorry, I also don't know which solution is better either, I think duplicating the code is the easiest, but maybe using generics or even tera templates could work too. Maybe someone with more experience can help you
I support the generics idea, would involve some refactoring but could be more scalable long term
After experimenting with this I've run into a couple issues
The primary one is the Vec2 dependency doesn't seem to support generics
Refactoring generics into it requires implementing the From trait to convert whatever type is passed in to f32 for Vec2. This could lead to breaking changes for existing uses of Rect.
Duplicate code for the IRect type may be the quickest way to getting this implemented, but maintaining duplicate functionality for both types long term may be a pain.
With all of that being said, I am a Rust and Bevy newbie so there may be something I am missing or doing incorrectly. However, there doesn't seem to be an urgent need for IRect to be implemented and not having it wouldn't cause anything to break or force other things to change. Due to this lack of urgency, I think that exploring options and possibilities for making Rect generic may be worth the deeper time investment to save us from maintaining duplicate code in the future and making existing code more abstract.
One idea is to remove the Vec2 dependency and create our own Bevy Vec2 type which does support generics. This could lead to more work in the short term, but more control and flexibility in the long term. Again, being a newbie I'm not sure what the core maintainers have envisioned for this long term, but just a thought.
Another thought, and also genuinely curious, is given these constraints or possibility of duplicate code, what is the actual need for IRect to begin with?
Is there an option, in your opinion, to use a tera template?
IMO, as a fellow Bevy newbie (not so much for Rust), a lot of Bevy's data types come from glam, and unless there's a need to support more architectures, I don't think the juice is worth the squeeze on writing our own data types in house. Someone correct me if I'm off base.
I would agree on if it's actually worth it to support an in house architecture for vec2 solely for an int based rect, unless there's something about that which would align w the long term goals of Bevy, just a spitball more than anything
Don't know enough about Tera templates to give meaningful advice on that one though
RE: in house vec2 - I'll leave that call for the maintainers of Bevy to decide, as they know more than I do what their long term goals are aligned with.
If you want a hand with Tera templates, I can set aside some time to work with you on that or I can handle those myself if we want to go that route (which, for code duplication, IMO, makes sense).
RE: in house vec2 - I'll leave that call for the maintainers of Bevy to decide, as they know more than I do what their long term goals are aligned with.
If you want a hand with Tera templates, I can set aside some time to work with you on that or I can handle those myself if we want to go that route (which, for code duplication, IMO, makes sense).
If there are any existing uses in Bevy or just general examples to reference that you know of that would be great. I can read through the docs and tinker around in the meantime.
I think that any sort of generalized/abstract approach would be great considering this issue is for IRect and https://github.com/bevyengine/bevy/pull/7867 mentions possibly adding a UVect as well, which would just be additional code duplication
Actually, I found them by looking at Glam RS. Here's an example tera template. So long as everything is pretty much exact duplicates, I think this is the right way to go.
Maybe a glam-rect crate maintained separately from Bevy could work, that way the Bevy repo can stay free from any templating complexity.
IMO we should just manually reproduce the code in bevy_math for now. I'd rather not introduce another dependency if we can avoid it.
Sounds good to me. @LiamGallagher737 if you still want to helm that, I'll worry about SIMD.
Sounds good to me. @LiamGallagher737 if you still want to helm that, I'll worry about SIMD.
It's ready to merge in #7984