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

Remove OrthographicProjection.scale #11022

Closed
wants to merge 1 commit into from
Closed

Remove OrthographicProjection.scale #11022

wants to merge 1 commit into from

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Dec 19, 2023 •
edited
Loading

While trying to set up orthographic projection, I spent some time figuring out what these parameters do. Examples like this:

bevy/examples/3d/orthographic.rs

Lines 20 to 24 in 05b0026

projection: OrthographicProjection {
scale: 3.0,
scaling_mode: ScalingMode::FixedVertical(2.0),
..default()
}

do not help much.

Apparently OrthographicProjection.scale and ScalingMode.xxx are redundant: one is multiplied by the other. So having to learn about them is overhead.

Not sure why .scale exists. Is it useful?

I have three alternatives:

  • Remove .scale field (implemented by this PR)
  • Remove some fields of ScalingMode, e.g. remove parameter from ScalingMode::FixedVertical, but keep ScalingMode::Fixed

bevy/crates/bevy_render/src/camera/projection.rs

Lines 210 to 215 in 05b0026

/// Keep the projection's height constant; width will be adjusted to match aspect ratio.
/// The argument is the desired height of the projection in world units.
FixedVertical(f32),
/// Keep the projection's width constant; height will be adjusted to match aspect ratio.
/// The argument is the desired width of the projection in world units.
FixedHorizontal(f32),

Three modified examples look the same after this PR.

@stepancheg stepancheg mentioned this pull request Dec 19, 2023
Merged
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 19, 2023
@Davier
Copy link
Contributor

Davier commented Dec 19, 2023

Not sure why .scale exists. Is it useful?

I usually set ScalingMode at startup and never touch it again. Then, while the app is running, I use .scale to easily implement zoom. It's definitely possible to do without .scale but less convenient.

@doonv
Copy link
Contributor

doonv commented Dec 19, 2023

It's definitely possible to do without .scale but less convenient.

This could easily be fixed by implementing Mul<f32> and MulAssign<f32> for ScalingMode. That would make changing the zoom as easy as doing it with .scale.

@stepancheg stepancheg mentioned this pull request Dec 19, 2023
Merged
@alice-i-cecile
Copy link
Member

If this is merged, we should merge #11030 with it.

@DanielJin21
Copy link
Contributor

I believe this may lead to an ergonomics loss for users who want to implement zoom via OrthographicProjection . If you wish to add a "reset zoom" function. You now need to keep a copy of the original ScalingMode as that information is no longer available as soon as you change the zoom level. Happy to be corrected if I'm wrong on this.

@stepancheg
Copy link
Contributor Author

@DanielJin21 there's an issue with redundant parameters and API complexity. I proposed three answers. Which would you prefer?

@DanielJin21
Copy link
Contributor

@DanielJin21 there's an issue with redundant parameters and API complexity. I proposed three answers. Which would you prefer?

I would personally prefer option 3 (improve documentation) the most. Even better, if they can be accompanied by an explanation of zoom being a possible use case for scale. I understand where you are coming from with the examples being confusing. Perhaps it can be changed so that while scale remains as a field, it is kept as 1.0 in most examples where changing the ScalingMode is sufficient to produce the same effect, and only use it if we are demonstrating its utility in implementing zoom.

@alice-i-cecile
Copy link
Member

I prefer this PR's solution: the values in ScalingMode are a better fit for the problem domain, and we shouldn't have redundant representations.

github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2024
@stepancheg
Explain OrthographicProjection.scale (#11023)
38ef170 
Alternative to #11022.

(Also remove "in world units", it is probably a mistake.)
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2024
@stepancheg
Mul<f32> for ScalingMode (#11030)
8d9a0a8 
Complement to #11022: if
`OrthographicProjection.scale` is removed, this can be used instead.

CC @doonv @Davier
tychedelia
tychedelia approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen users run into confusion around scale vs ScalingMode before, and it makes sense to me to direct them towards the better representation, even if it comes with a bit more complexity.

@tychedelia tychedelia added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 16, 2024
@tychedelia tychedelia added this to the 0.15 milestone Aug 16, 2024
@tychedelia
Copy link
Contributor

PR has merge conflicts.

@tychedelia tychedelia mentioned this pull request Aug 16, 2024
Closed
@alice-i-cecile alice-i-cecile added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 16, 2024
This was referenced Sep 5, 2024
Closed
Merged
@richchurcher
Copy link
Contributor

I've adopted this PR over at #15075.

github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
@richchurcher @stepancheg
Remove OrthographicProjection.scale (adopted) (#15075)
f326705 
# Objective

Hello! I am adopting #11022 to resolve conflicts with `main`. tldr: this
removes `scale` in favour of `scaling_mode`. Please see the original PR
for explanation/discussion.

Also relates to #2580.

## Migration Guide

Replace all uses of `scale` with `scaling_mode`, keeping in mind that
`scale` is (was) a multiplier. For example, replace
```rust
    scale: 2.0,
    scaling_mode: ScalingMode::FixedHorizontal(4.0),

```
with
```rust
    scaling_mode: ScalingMode::FixedHorizontal(8.0),
```

---------

Co-authored-by: Stepan Koltsov <stepan.koltsov@gmail.com>
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 17, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 17, 2024
@alice-i-cecile @MiniaczQ
Improve API for scaling orthographic cameras (#15969)
2bd3282 
# Objective

Fixes #15791.

As raised in #11022, scaling orthographic cameras is confusing! In Bevy
0.14, there were multiple completely redundant ways to do this, and no
clear guidance on which to use.

As a result, #15075 removed the `scale` field from
`OrthographicProjection` completely, solving the redundancy issue.

However, this resulted in an unintuitive API and a painful migration, as
discussed in #15791. Users simply want to change a single parameter to
zoom, rather than deal with the irrelevant details of how the camera is
being scaled.

## Solution

This PR reverts #15075, and takes an alternate, more nuanced approach to
the redundancy problem. `ScalingMode::WindowSize` was by far the biggest
offender. This was the default variant, and stored a float that was
*fully* redundant to setting `scale`.

All of the other variants contained meaningful semantic information and
had an intuitive scale. I could have made these unitless, storing an
aspect ratio, but this would have been a worse API and resulted in a
pointlessly painful migration.

In the course of this work I've also:

- improved the documentation to explain that you should just set `scale`
to zoom cameras
- swapped to named fields for all of the variants in `ScalingMode` for
more clarity about the parameter meanings
- substantially improved the `projection_zoom` example
- removed the footgunny `Mul` and `Div` impls for `ScalingMode`,
especially since these no longer have the intended effect on
`ScalingMode::WindowSize`.
- removed a rounding step because this is now redundant 🎉 

## Testing

I've tested these changes as part of my work in the `projection_zoom`
example, and things seem to work fine.

## Migration Guide

`ScalingMode` has been refactored for clarity, especially on how to zoom
orthographic cameras and their projections:

- `ScalingMode::WindowSize` no longer stores a float, and acts as if its
value was 1. Divide your camera's scale by any previous value to achieve
identical results.
- `ScalingMode::FixedVertical` and `FixedHorizontal` now use named
fields.

---------

Co-authored-by: MiniaczQ <xnetroidpl@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tychedelia tychedelia tychedelia approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
Rendering
Status: Done
Milestone
0.15
Development

Successfully merging this pull request may close these issues.
7 participants
@stepancheg @Davier @doonv @alice-i-cecile @DanielJin21 @tychedelia @richchurcher