[Merged by Bors] - Improve OrthographicCamera consistency and usability
#6201
Conversation
alice-i-cecile
added
A-Rendering
|
Ping @bzm3r for review. |
|
@alice-i-cecile shouldn't this be tagged with "breaking change"? |
alice-i-cecile
added
the
M-Needs-Migration-Guide
|
Yep, thank you! |
| /// Match the window size. | ||
| /// The argument is the number of pixels that equals one world unit. | ||
| WindowSize(f32), | ||
| /// Use the minimal possible projection size while keeping the aspect ratio. |
There was a problem hiding this comment.
Can you explain what is meant by "projection size"? Is this the same as the frustum's cross-section?
There was a problem hiding this comment.
Yes. I think using "projection" as the plane that the frustum expands from makes it a bit simpler (rather than having to specify cross sectional area every time). I find it fairly intuitive but if you have any suggestions that's more than welcome.
There was a problem hiding this comment.
@xgbwei We can use projection, but then we should define it first. The issue is can mean multiple things in different contexts. (Also, what is intuitive to one reader might not be intuitive to another; that depends on the experiences the reader might have had.)
I think we should have a sentence that defines projection, and connects it to the idea of cross-sectional area, and then we can use projection. I will try and provide a suggestion.
There was a problem hiding this comment.
I'm wondering if we need to take into account the near and far clipping planes as part of the "shape" of the projection. Clipping planes are mostly there for technical reasons, they aren't inherently part of orthographic projection.
There was a problem hiding this comment.
That being said, they should be taken into account when describing the view frustum, since frustum culling culls objects outside of the near and far clipping planes.
Oh, I didn't notice that. That's probably because I made the default for |
|
I’d like to take a look at this if I get time. |
| Center, | ||
| BottomLeft, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Reflect, FromReflect, Serialize, Deserialize)] |
There was a problem hiding this comment.
| #[derive(Debug, Clone, Reflect, FromReflect, Serialize, Deserialize)] | |
| /// Specifies how the size of a projection is calculated. The projection is the cross-sectional area | |
| /// of the view frustum which intersects with the window. | |
| #[derive(Debug, Clone, Reflect, FromReflect, Serialize, Deserialize)] |
|
I'm trying to add a separate "config" struct for initializing, but it's a bit of a mess (and might be unnecessarily complicated). Maybe using a builder or just a simple constructor would work better? I'm not sure if there's enough fields to warrant a builder, but there's too many fields to fit into constructor parameters comfortably. Are there any methods that would make configurating the fields ergonomic but also allow defaults? |
|
Unfortunately no; you can't use struct update syntax unless the fields are pub. I think a builder pattern or an elaborate constructor is the way to go. |
| /// projection. When the projection scales, the position of the camera doesn't change, and since `viewport_origin` | ||
| /// specifies the point on the viewport where the camera sits, this point will always remain at the same position | ||
| /// (relative to the viewport size; if `viewport_origin` is (0.3, 0.6), objects at (0.3, 0.6) (normalized) on the | ||
| /// viewport will always remain at (0.3, 0.6) (normalized). |
There was a problem hiding this comment.
I've tried to make this as specific as possible. Looking for ways to make it simpler.
There was a problem hiding this comment.
Thanks for the change! I salute the documenting part which is much welcome
I sort of agree with this. Do you have any suggestions for alternatives? Every other method I've looked at has had some other downside. I could also just leave the fields public. |
Because this is the recommended and promoted pattern in the rest of the codebase, yes I would just leave those as public fields for now. We can discuss whether this is the right pattern or not (I doubt we can convince anyone of a change at this point though), but I wouldn't make this object inconsistent unless there's an overwhelming benefit to the builder pattern, which I don't think we showed here. |
That's fair. Clearly documenting that l/r/b/t should be for reading only should be fine for now. |
Co-authored-by: sark <[email protected]>
Co-authored-by: Jerome Humbert <[email protected]>
|
Did I do that right? |
|
Rebase looks almost correct: you have an empty |
|
bors r+ |
# Objective - Terminology used in field names and docs aren't accurate - `window_origin` doesn't have any effect when `scaling_mode` is `ScalingMode::None` - `left`, `right`, `bottom`, and `top` are set automatically unless `scaling_mode` is `None`. Fields that only sometimes give feedback are confusing. - `ScalingMode::WindowSize` has no arguments, which is inconsistent with other `ScalingMode`s. 1 pixel = 1 world unit is also typically way too wide. - `OrthographicProjection` feels generally less streamlined than its `PerspectiveProjection` counterpart - Fixes #5818 - Fixes #6190 ## Solution - Improve consistency in `OrthographicProjection`'s public fields (they should either always give feedback or never give feedback). - Improve consistency in `ScalingMode`'s arguments - General usability improvements - Improve accuracy of terminology: - "Window" should refer to the physical window on the desktop - "Viewport" should refer to the component in the window that images are drawn on (typically all of it) - "View frustum" should refer to the volume captured by the projection --- ## Changelog ### Added - Added argument to `ScalingMode::WindowSize` that specifies the number of pixels that equals one world unit. - Added documentation for fields and enums ### Changed - Renamed `window_origin` to `viewport_origin`, which now: - Affects all `ScalingMode`s - Takes a fraction of the viewport's width and height instead of an enum - Removed `WindowOrigin` enum as it's obsolete - Renamed `ScalingMode::None` to `ScalingMode::Fixed`, which now: - Takes arguments to specify the projection size - Replaced `left`, `right`, `bottom`, and `top` fields with a single `area: Rect` - `scale` is now applied before updating `area`. Reading from it will take `scale` into account. - Documentation changes to make terminology more accurate and consistent ## Migration Guide - Change `window_origin` to `viewport_origin`; replace `WindowOrigin::Center` with `Vec2::new(0.5, 0.5)` and `WindowOrigin::BottomLeft` with `Vec2::new(0.0, 0.0)` - For shadow projections and such, replace `left`, `right`, `bottom`, and `top` with `area: Rect::new(left, bottom, right, top)` - For camera projections, remove l/r/b/t values from `OrthographicProjection` instantiations, as they no longer have any effect in any `ScalingMode` - Change `ScalingMode::None` to `ScalingMode::Fixed` - Replace manual changes of l/r/b/t with: - Arguments in `ScalingMode::Fixed` to specify size - `viewport_origin` to specify offset - Change `ScalingMode::WindowSize` to `ScalingMode::WindowSize(1.0)`
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
OrthographicCamera consistency and usabilityOrthographicCamera consistency and usability
# Objective - Terminology used in field names and docs aren't accurate - `window_origin` doesn't have any effect when `scaling_mode` is `ScalingMode::None` - `left`, `right`, `bottom`, and `top` are set automatically unless `scaling_mode` is `None`. Fields that only sometimes give feedback are confusing. - `ScalingMode::WindowSize` has no arguments, which is inconsistent with other `ScalingMode`s. 1 pixel = 1 world unit is also typically way too wide. - `OrthographicProjection` feels generally less streamlined than its `PerspectiveProjection` counterpart - Fixes bevyengine#5818 - Fixes bevyengine#6190 ## Solution - Improve consistency in `OrthographicProjection`'s public fields (they should either always give feedback or never give feedback). - Improve consistency in `ScalingMode`'s arguments - General usability improvements - Improve accuracy of terminology: - "Window" should refer to the physical window on the desktop - "Viewport" should refer to the component in the window that images are drawn on (typically all of it) - "View frustum" should refer to the volume captured by the projection --- ## Changelog ### Added - Added argument to `ScalingMode::WindowSize` that specifies the number of pixels that equals one world unit. - Added documentation for fields and enums ### Changed - Renamed `window_origin` to `viewport_origin`, which now: - Affects all `ScalingMode`s - Takes a fraction of the viewport's width and height instead of an enum - Removed `WindowOrigin` enum as it's obsolete - Renamed `ScalingMode::None` to `ScalingMode::Fixed`, which now: - Takes arguments to specify the projection size - Replaced `left`, `right`, `bottom`, and `top` fields with a single `area: Rect` - `scale` is now applied before updating `area`. Reading from it will take `scale` into account. - Documentation changes to make terminology more accurate and consistent ## Migration Guide - Change `window_origin` to `viewport_origin`; replace `WindowOrigin::Center` with `Vec2::new(0.5, 0.5)` and `WindowOrigin::BottomLeft` with `Vec2::new(0.0, 0.0)` - For shadow projections and such, replace `left`, `right`, `bottom`, and `top` with `area: Rect::new(left, bottom, right, top)` - For camera projections, remove l/r/b/t values from `OrthographicProjection` instantiations, as they no longer have any effect in any `ScalingMode` - Change `ScalingMode::None` to `ScalingMode::Fixed` - Replace manual changes of l/r/b/t with: - Arguments in `ScalingMode::Fixed` to specify size - `viewport_origin` to specify offset - Change `ScalingMode::WindowSize` to `ScalingMode::WindowSize(1.0)`
# Objective - Terminology used in field names and docs aren't accurate - `window_origin` doesn't have any effect when `scaling_mode` is `ScalingMode::None` - `left`, `right`, `bottom`, and `top` are set automatically unless `scaling_mode` is `None`. Fields that only sometimes give feedback are confusing. - `ScalingMode::WindowSize` has no arguments, which is inconsistent with other `ScalingMode`s. 1 pixel = 1 world unit is also typically way too wide. - `OrthographicProjection` feels generally less streamlined than its `PerspectiveProjection` counterpart - Fixes bevyengine#5818 - Fixes bevyengine#6190 ## Solution - Improve consistency in `OrthographicProjection`'s public fields (they should either always give feedback or never give feedback). - Improve consistency in `ScalingMode`'s arguments - General usability improvements - Improve accuracy of terminology: - "Window" should refer to the physical window on the desktop - "Viewport" should refer to the component in the window that images are drawn on (typically all of it) - "View frustum" should refer to the volume captured by the projection --- ## Changelog ### Added - Added argument to `ScalingMode::WindowSize` that specifies the number of pixels that equals one world unit. - Added documentation for fields and enums ### Changed - Renamed `window_origin` to `viewport_origin`, which now: - Affects all `ScalingMode`s - Takes a fraction of the viewport's width and height instead of an enum - Removed `WindowOrigin` enum as it's obsolete - Renamed `ScalingMode::None` to `ScalingMode::Fixed`, which now: - Takes arguments to specify the projection size - Replaced `left`, `right`, `bottom`, and `top` fields with a single `area: Rect` - `scale` is now applied before updating `area`. Reading from it will take `scale` into account. - Documentation changes to make terminology more accurate and consistent ## Migration Guide - Change `window_origin` to `viewport_origin`; replace `WindowOrigin::Center` with `Vec2::new(0.5, 0.5)` and `WindowOrigin::BottomLeft` with `Vec2::new(0.0, 0.0)` - For shadow projections and such, replace `left`, `right`, `bottom`, and `top` with `area: Rect::new(left, bottom, right, top)` - For camera projections, remove l/r/b/t values from `OrthographicProjection` instantiations, as they no longer have any effect in any `ScalingMode` - Change `ScalingMode::None` to `ScalingMode::Fixed` - Replace manual changes of l/r/b/t with: - Arguments in `ScalingMode::Fixed` to specify size - `viewport_origin` to specify offset - Change `ScalingMode::WindowSize` to `ScalingMode::WindowSize(1.0)`
# Objective - Terminology used in field names and docs aren't accurate - `window_origin` doesn't have any effect when `scaling_mode` is `ScalingMode::None` - `left`, `right`, `bottom`, and `top` are set automatically unless `scaling_mode` is `None`. Fields that only sometimes give feedback are confusing. - `ScalingMode::WindowSize` has no arguments, which is inconsistent with other `ScalingMode`s. 1 pixel = 1 world unit is also typically way too wide. - `OrthographicProjection` feels generally less streamlined than its `PerspectiveProjection` counterpart - Fixes bevyengine#5818 - Fixes bevyengine#6190 ## Solution - Improve consistency in `OrthographicProjection`'s public fields (they should either always give feedback or never give feedback). - Improve consistency in `ScalingMode`'s arguments - General usability improvements - Improve accuracy of terminology: - "Window" should refer to the physical window on the desktop - "Viewport" should refer to the component in the window that images are drawn on (typically all of it) - "View frustum" should refer to the volume captured by the projection --- ## Changelog ### Added - Added argument to `ScalingMode::WindowSize` that specifies the number of pixels that equals one world unit. - Added documentation for fields and enums ### Changed - Renamed `window_origin` to `viewport_origin`, which now: - Affects all `ScalingMode`s - Takes a fraction of the viewport's width and height instead of an enum - Removed `WindowOrigin` enum as it's obsolete - Renamed `ScalingMode::None` to `ScalingMode::Fixed`, which now: - Takes arguments to specify the projection size - Replaced `left`, `right`, `bottom`, and `top` fields with a single `area: Rect` - `scale` is now applied before updating `area`. Reading from it will take `scale` into account. - Documentation changes to make terminology more accurate and consistent ## Migration Guide - Change `window_origin` to `viewport_origin`; replace `WindowOrigin::Center` with `Vec2::new(0.5, 0.5)` and `WindowOrigin::BottomLeft` with `Vec2::new(0.0, 0.0)` - For shadow projections and such, replace `left`, `right`, `bottom`, and `top` with `area: Rect::new(left, bottom, right, top)` - For camera projections, remove l/r/b/t values from `OrthographicProjection` instantiations, as they no longer have any effect in any `ScalingMode` - Change `ScalingMode::None` to `ScalingMode::Fixed` - Replace manual changes of l/r/b/t with: - Arguments in `ScalingMode::Fixed` to specify size - `viewport_origin` to specify offset - Change `ScalingMode::WindowSize` to `ScalingMode::WindowSize(1.0)`
Objective
window_origindoesn't have any effect whenscaling_modeisScalingMode::Noneleft,right,bottom, andtopare set automatically unlessscaling_modeisNone. Fields that only sometimes give feedback are confusing.ScalingMode::WindowSizehas no arguments, which is inconsistent with otherScalingModes. 1 pixel = 1 world unit is also typically way too wide.OrthographicProjectionfeels generally less streamlined than itsPerspectiveProjectioncounterpartOrthographicProjection#5818OrthographicProjection's public fields are misleading #6190Solution
OrthographicProjection's public fields (they should either always give feedback or never give feedback).ScalingMode's argumentsChangelog
Added
ScalingMode::WindowSizethat specifies the number of pixels that equals one world unit.Changed
window_origintoviewport_origin, which now:ScalingModesWindowOriginenum as it's obsoleteScalingMode::NonetoScalingMode::Fixed, which now:left,right,bottom, andtopfields with a singlearea: Rectscaleis now applied before updatingarea. Reading from it will takescaleinto account.Migration Guide
window_origintoviewport_origin; replaceWindowOrigin::CenterwithVec2::new(0.5, 0.5)andWindowOrigin::BottomLeftwithVec2::new(0.0, 0.0)left,right,bottom, andtopwitharea: Rect::new(left, bottom, right, top)OrthographicProjectioninstantiations, as they no longer have any effect in anyScalingModeScalingMode::NonetoScalingMode::FixedScalingMode::Fixedto specify sizeviewport_originto specify offsetScalingMode::WindowSizetoScalingMode::WindowSize(1.0)