-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
docs: Documentation and clean up of bevy_math. #3503
Conversation
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 is excellent, thank you! I left a few comments with my thoughts, let me know what you think.
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.
Is the FaceToward
trait really needed? It seems it's only duplicate functionality from https://docs.rs/glam/latest/glam/f32/struct.Mat4.html#method.look_at_rh. It's never used in Bevy code. There's also bitshifter/glam-rs#214, @alice-i-cecile do you have more information?
Reading the doc on Rect
, I find it very confusing. Looking through its usage in code:
- first thing I noticed, we actually have two different structs named
Rect
, the other one is defined inbevy_sprite
and has a much more logical definition
bevy/crates/bevy_sprite/src/rect.rs
Lines 8 to 13 in 1d0d8a3
pub struct Rect { /// The beginning point of the rect pub min: Vec2, /// The ending point of the rect pub max: Vec2, } - the
Rect
struct inbevy_math
is, as you noted, only used inbevy_ui
, and it doesn't actually define a rectangle, only what to do to each side of one (with of a border, width of a padding, ...)
I think the Rect
from bevy_math
should be moved to bevy_ui
(and renamed), and it could probably lose the type parameter as I think it's only used with Val
.
There is a pretty similar situation with the Size
struct in bevy_math
. It's only used as a generic Vec2
, and that's only needed in bevy_ui
as it's needed to have a Vec2
over Val
, which is not possible with the Vec2
from glam
. Most other places in code that need a size use directly a Vec2
.
I think the Size
struct should move to bevy_ui
and not be generic anymore. Other place that needs a size should use a Vec2
instead as it's way more powerful.
So... my recommendations to document bevy_math
is to remove all its modules, then it won't need that much doc 😄
I agree with what you said. I was also wondering what the point of the I'll start working on moving the geometry.rs to I will most likely remove the example which included |
I agree! IMO we should make sure we've exposed that, and then use that functionality instead, deleting the
I agree with this choice too. Perhaps
Agreed. This all looks excellent: although the scope is larger, it's nice to fix up some of these code quality issues while we're here. Can you update the PR title @KDecay once you're done so it's easier to track in the git log? Something like "Documented and cleaned up bevy_math" would be fine. |
The |
That could work yeah. We came up with a simple UiRect in the discord, but Padding could also work. I'll probably just stick to UiRect for now. We can discuss the final name after I did all the points mentioned and the name seems inaccurate.
Thanks for mentioning. The
Yes of course. I'll update the title and also the objective so that it's clear what happened here. |
What I didI removed every module inside of The @alice-i-cecile I decided to go with Removing
|
I really like your proposed change; let's do this here so it doesn't get decoupled. Thanks for the excellent work! |
What I didI removed the margins.rs from Migration Guide
|
Probably should suggest to use https://docs.rs/glam/latest/glam/f32/struct.Mat4.html#method.look_at_rh instead, as stated above. |
True, but this would only allow the users to use the I edited my last comment so that we have at least some sort of migration guide to provide for users. Thanks for pointing that out! |
Going to close this down and split it into multiple smaller PR's to make it easier to review and merge. |
# Objective - Closes #335. - Part of the splitting process of #3503. ## Solution - Remove the `margins.rs` file containing the `Margins` type. ## Reasons - It is unused inside of `bevy`. - The `Rect`/`UiRect` is identical to the `Margins` type and is also used for margins inside of `bevy` (rename of `Rect` happens in #4276) - Discussion in #3503. ## Changelog ### Removed - The `Margins` type got removed. ## Migration Guide - The `Margins` type got removed. To migrate you just have to change every occurrence of `Margins` to `UiRect`.
# Objective - Part of the splitting process of #3503. ## Solution - Remove the `face_toward.rs` file containing the `FaceToward` trait. ## Reasons - It is unused inside of `bevy`. - The method `Mat4::face_toward` of the trait is identical to `Mat4::look_at_rh` (see https://docs.rs/glam/latest/glam/f32/struct.Mat4.html#method.look_at_rh). - Discussion in #3503. ## Changelog ### Removed - The `FaceToward` trait got removed. ## Migration Guide - The `FaceToward` trait got removed. To migrate you just have to change every occurrence of `Mat4::face_toward` to `Mat4::look_at_rh`.
# Objective - Related #4276. - Part of the splitting process of #3503. ## Solution - Move `Size` to `bevy_ui`. ## Reasons - `Size` is only needed in `bevy_ui` (because it needs to use `Val` instead of `f32`), but it's also used as a worse `Vec2` replacement in other areas. - `Vec2` is more powerful than `Size` so it should be used whenever possible. - Discussion in #3503. ## Changelog ### Changed - The `Size` type got moved from `bevy_math` to `bevy_ui`. ## Migration Guide - The `Size` type got moved from `bevy::math` to `bevy::ui`. To migrate you just have to import `bevy::ui::Size` instead of `bevy::math::Math` or use the `bevy::prelude` instead. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Closes #335. - Related #4285. - Part of the splitting process of #3503. ## Solution - Move `Rect` to `bevy_ui` and rename it to `UiRect`. ## Reasons - `Rect` is only used in `bevy_ui` and therefore calling it `UiRect` makes the intent clearer. - We have two types that are called `Rect` currently and it's missleading (see `bevy_sprite::Rect` and #335). - Discussion in #3503. ## Changelog ### Changed - The `Rect` type got moved from `bevy_math` to `bevy_ui` and renamed to `UiRect`. ## Migration Guide - The `Rect` type got renamed to `UiRect`. To migrate you just have to change every occurrence of `Rect` to `UiRect`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Related bevyengine#4276. - Part of the splitting process of bevyengine#3503. ## Solution - Move `Size` to `bevy_ui`. ## Reasons - `Size` is only needed in `bevy_ui` (because it needs to use `Val` instead of `f32`), but it's also used as a worse `Vec2` replacement in other areas. - `Vec2` is more powerful than `Size` so it should be used whenever possible. - Discussion in bevyengine#3503. ## Changelog ### Changed - The `Size` type got moved from `bevy_math` to `bevy_ui`. ## Migration Guide - The `Size` type got moved from `bevy::math` to `bevy::ui`. To migrate you just have to import `bevy::ui::Size` instead of `bevy::math::Math` or use the `bevy::prelude` instead. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Closes bevyengine#335. - Related bevyengine#4285. - Part of the splitting process of bevyengine#3503. ## Solution - Move `Rect` to `bevy_ui` and rename it to `UiRect`. ## Reasons - `Rect` is only used in `bevy_ui` and therefore calling it `UiRect` makes the intent clearer. - We have two types that are called `Rect` currently and it's missleading (see `bevy_sprite::Rect` and bevyengine#335). - Discussion in bevyengine#3503. ## Changelog ### Changed - The `Rect` type got moved from `bevy_math` to `bevy_ui` and renamed to `UiRect`. ## Migration Guide - The `Rect` type got renamed to `UiRect`. To migrate you just have to change every occurrence of `Rect` to `UiRect`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Closes bevyengine#335. - Part of the splitting process of bevyengine#3503. ## Solution - Remove the `margins.rs` file containing the `Margins` type. ## Reasons - It is unused inside of `bevy`. - The `Rect`/`UiRect` is identical to the `Margins` type and is also used for margins inside of `bevy` (rename of `Rect` happens in bevyengine#4276) - Discussion in bevyengine#3503. ## Changelog ### Removed - The `Margins` type got removed. ## Migration Guide - The `Margins` type got removed. To migrate you just have to change every occurrence of `Margins` to `UiRect`.
# Objective - Part of the splitting process of bevyengine#3503. ## Solution - Remove the `face_toward.rs` file containing the `FaceToward` trait. ## Reasons - It is unused inside of `bevy`. - The method `Mat4::face_toward` of the trait is identical to `Mat4::look_at_rh` (see https://docs.rs/glam/latest/glam/f32/struct.Mat4.html#method.look_at_rh). - Discussion in bevyengine#3503. ## Changelog ### Removed - The `FaceToward` trait got removed. ## Migration Guide - The `FaceToward` trait got removed. To migrate you just have to change every occurrence of `Mat4::face_toward` to `Mat4::look_at_rh`.
# Objective - Migrate changes from #3503. ## Solution - Document `Size` and `UiRect`. - I also removed the type alias from the `size_ops` test since it's unnecessary. ## Follow Up After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in #3503. let me know if this is not needed or wanted anymore!
# Objective - Migrate changes from #3503. ## Solution - Change `Size<T>` and `UiRect<T>` to `Size` and `UiRect` using `Val`. - Implement `Sub`, `SubAssign`, `Mul`, `MulAssign`, `Div` and `DivAssign` for `Val`. - Update tests for `Size`. --- ## Changelog ### Changed - The generic `T` of `Size` and `UiRect` got removed and instead they both now always use `Val`. ## Migration Guide - The generic `T` of `Size` and `UiRect` got removed and instead they both now always use `Val`. If you used a `Size<f32>` consider replacing it with a `Vec2` which is way more powerful. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Migrate changes from bevyengine#3503. ## Solution - Document `Size` and `UiRect`. - I also removed the type alias from the `size_ops` test since it's unnecessary. ## Follow Up After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in bevyengine#3503. let me know if this is not needed or wanted anymore!
# Objective - Migrate changes from bevyengine#3503. ## Solution - Document `Size` and `UiRect`. - I also removed the type alias from the `size_ops` test since it's unnecessary. ## Follow Up After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in bevyengine#3503. let me know if this is not needed or wanted anymore!
# Objective - Migrate changes from bevyengine#3503. ## Solution - Change `Size<T>` and `UiRect<T>` to `Size` and `UiRect` using `Val`. - Implement `Sub`, `SubAssign`, `Mul`, `MulAssign`, `Div` and `DivAssign` for `Val`. - Update tests for `Size`. --- ## Changelog ### Changed - The generic `T` of `Size` and `UiRect` got removed and instead they both now always use `Val`. ## Migration Guide - The generic `T` of `Size` and `UiRect` got removed and instead they both now always use `Val`. If you used a `Size<f32>` consider replacing it with a `Vec2` which is way more powerful. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Migrate changes from bevyengine#3503. ## Solution - Document `Size` and `UiRect`. - I also removed the type alias from the `size_ops` test since it's unnecessary. ## Follow Up After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in bevyengine#3503. let me know if this is not needed or wanted anymore!
# Objective - Migrate changes from bevyengine#3503. ## Solution - Change `Size<T>` and `UiRect<T>` to `Size` and `UiRect` using `Val`. - Implement `Sub`, `SubAssign`, `Mul`, `MulAssign`, `Div` and `DivAssign` for `Val`. - Update tests for `Size`. --- ## Changelog ### Changed - The generic `T` of `Size` and `UiRect` got removed and instead they both now always use `Val`. ## Migration Guide - The generic `T` of `Size` and `UiRect` got removed and instead they both now always use `Val`. If you used a `Size<f32>` consider replacing it with a `Vec2` which is way more powerful. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Closes bevyengine#335. - Part of the splitting process of bevyengine#3503. ## Solution - Remove the `margins.rs` file containing the `Margins` type. ## Reasons - It is unused inside of `bevy`. - The `Rect`/`UiRect` is identical to the `Margins` type and is also used for margins inside of `bevy` (rename of `Rect` happens in bevyengine#4276) - Discussion in bevyengine#3503. ## Changelog ### Removed - The `Margins` type got removed. ## Migration Guide - The `Margins` type got removed. To migrate you just have to change every occurrence of `Margins` to `UiRect`.
# Objective - Part of the splitting process of bevyengine#3503. ## Solution - Remove the `face_toward.rs` file containing the `FaceToward` trait. ## Reasons - It is unused inside of `bevy`. - The method `Mat4::face_toward` of the trait is identical to `Mat4::look_at_rh` (see https://docs.rs/glam/latest/glam/f32/struct.Mat4.html#method.look_at_rh). - Discussion in bevyengine#3503. ## Changelog ### Removed - The `FaceToward` trait got removed. ## Migration Guide - The `FaceToward` trait got removed. To migrate you just have to change every occurrence of `Mat4::face_toward` to `Mat4::look_at_rh`.
# Objective - Related bevyengine#4276. - Part of the splitting process of bevyengine#3503. ## Solution - Move `Size` to `bevy_ui`. ## Reasons - `Size` is only needed in `bevy_ui` (because it needs to use `Val` instead of `f32`), but it's also used as a worse `Vec2` replacement in other areas. - `Vec2` is more powerful than `Size` so it should be used whenever possible. - Discussion in bevyengine#3503. ## Changelog ### Changed - The `Size` type got moved from `bevy_math` to `bevy_ui`. ## Migration Guide - The `Size` type got moved from `bevy::math` to `bevy::ui`. To migrate you just have to import `bevy::ui::Size` instead of `bevy::math::Math` or use the `bevy::prelude` instead. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
# Objective - Closes bevyengine#335. - Related bevyengine#4285. - Part of the splitting process of bevyengine#3503. ## Solution - Move `Rect` to `bevy_ui` and rename it to `UiRect`. ## Reasons - `Rect` is only used in `bevy_ui` and therefore calling it `UiRect` makes the intent clearer. - We have two types that are called `Rect` currently and it's missleading (see `bevy_sprite::Rect` and bevyengine#335). - Discussion in bevyengine#3503. ## Changelog ### Changed - The `Rect` type got moved from `bevy_math` to `bevy_ui` and renamed to `UiRect`. ## Migration Guide - The `Rect` type got renamed to `UiRect`. To migrate you just have to change every occurrence of `Rect` to `UiRect`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>
This PR is part of the issue #3492.
Closes #335.
Closes #3484.
Objective
FaceToward
trait, because it is unused.bevy_ui
and rename theRect
type toUiRect
to remove having two types with the same name (seebevy_sprite::Rect
).bevy_ui::Margins
.new(left: T, right: T, top: T, bottom: T)
function forbevy_ui::UiRect
.Size<T>
toSize<Val>
removing the generic, because Val should be extendable enough.UiRect<T>
toUiRect<Val>
removing the generic, because Val should be extendable enough.bevy_math
works with thedoc_markdown
lint.bevy_ui
works with thedoc_markdown
lint.Solution
bevy_ui
bevy_ui::Margins
.new(left: T, right: T, top: T, bottom: T)
function forbevy_ui::UiRect
.Size<T>
toSize<Val>
.UiRect<T>
toUiRect<Val>
.cargo clippy -p bevy_math --no-deps --all-targets --all-features -- -D warnings -A clippy::type_complexity -W clippy::doc_markdown
without errors.cargo clippy -p bevy_ui --no-deps --all-targets --all-features -- -D warnings -A clippy::type_complexity -W clippy::doc_markdown
without errors.