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.

Sign up for GitHub

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

Jump to bottom

OrthographicProjection's public fields are misleading #6190

Closed
xgbwei opened this issue Oct 6, 2022 · 5 comments
Closed

OrthographicProjection's public fields are misleading #6190

xgbwei opened this issue Oct 6, 2022 · 5 comments
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@xgbwei
Copy link
Contributor

xgbwei commented Oct 6, 2022 •
edited
Loading

What problem does this solve or what need does it fill?

While working on #6177, I noticed some problems with OrthographicProjection's public fields:

  • left, right, bottom, and top, which are used to set the borders of the projection, are public, even though they'll be set automatically if scaling_mode is anything other than ScalingMode::None.

  • window_origin affects the origin of the scaling, which means it doesn't have any effect if scaling_mode is ScalingMode::None.

What solution would you like?

  • left, right, bottom, and top should be private with public getters. Maybe they can be set through ScalingMode::None's fields; this might make it a bit difficult to update though.

  • window_origin should be renamed to scaling_origin or something similar.

What alternative(s) have you considered?

Replace scale with separate width and height values, which will scale the entire projection whether scaling_mode is enabled or not. Then make window_origin (scaling_origin) more flexible with a fractional value of the width and height instead of just WindowOrigin::BottomLeft and WindowOrigin::Center. This will improve usability overall.
@xgbwei xgbwei added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Oct 6, 2022
@xgbwei
Copy link
Contributor Author

xgbwei commented Oct 6, 2022

Just looking for some input here. I can merge this into #6177 (or create a new PR? whatever the conventions is) if it's deemed an issue.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Oct 7, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Oct 7, 2022 •
edited
Loading

I'm on board with all of these changes. I think a second PR is better; smaller PRs are much easier to review and merge.

Your call on which solution you prefer; both seem reasonable to me. The latter is a bit nicer, but less consistent with other camera types.

@bzm3r
Copy link
Contributor

bzm3r commented Oct 7, 2022

@xgbwei Thanks so much for looking into this ❤️

Working with OrthographicProjection has been a major pain point, so I appreciate it is getting looked at.

About this solution:

Replace scale with separate width and height values, which will scale the entire projection whether scaling_mode is enabled or not. Then make window_origin (scaling_origin) more flexible with a fractional value of the width and height instead of just WindowOrigin::BottomLeft and WindowOrigin::Center. This will improve usability overall.

If you make it scale the entire projection regardless of scaling_mode, then won't this clash with scaling_mode's purpose: https://docs.rs/bevy/latest/bevy/render/camera/enum.ScalingMode.html?

@xgbwei
Copy link
Contributor Author

xgbwei commented Oct 7, 2022 •
edited
Loading

If you make it scale the entire projection regardless of scaling_mode, then won't this clash with scaling_mode's purpose: https://docs.rs/bevy/latest/bevy/render/camera/enum.ScalingMode.html?

The width and height values replace scale and the left, right, bottom and top values when applied with ScalingMode::None, but for every scaling mode. That is, they don't affect the size of the viewport and instead only scales the view frustum (the image will stretch if width and height aren't equal).

@bzm3r
Copy link
Contributor

bzm3r commented Oct 7, 2022

If you make it scale the entire projection regardless of scaling_mode, then won't this clash with scaling_mode's purpose: https://docs.rs/bevy/latest/bevy/render/camera/enum.ScalingMode.html?

The width and height values replace scale and the left, right, bottom and top values when applied with ScalingMode::None, but for every scaling mode. That is, they don't affect the size of the viewport and instead only scales the view frustum (the image will stretch if width and height aren't equal).

Oh, I see! That's important to document then. The view frustrum really provides an "image" which is then stretched into the window.

@xgbwei xgbwei mentioned this issue Oct 8, 2022
Closed
@bors bors bot closed this as completed in 09cb590 Feb 8, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this issue Feb 11, 2023
@xgbwei @myreprise1
Improve OrthographicCamera consistency and usability (bevyengine#6201)
5974da6 
# 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)`
myreprise1 pushed a commit to myreprise1/bevy that referenced this issue Feb 11, 2023
@xgbwei @myreprise1
Improve OrthographicCamera consistency and usability (bevyengine#6201)
3601c13 
# 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)`
myreprise1 pushed a commit to myreprise1/bevy that referenced this issue Feb 15, 2023
@xgbwei @myreprise1
Improve OrthographicCamera consistency and usability (bevyengine#6201)
38b359c 
# 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)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Improve OrthographicCamera consistency and usability xgbwei/bevy
3 participants
@alice-i-cecile @bzm3r @xgbwei