Remove OrthographicProjection.scale (adopted) #15075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Just workin' through the CI failures now... |
bas-ie
commented
Sep 7, 2024
| @@ -176,6 +177,6 @@ fn fit_canvas( | |||
| let h_scale = event.width / RES_WIDTH as f32; | |||
| let v_scale = event.height / RES_HEIGHT as f32; | |||
| let mut projection = projections.single_mut(); | |||
| projection.scale = 1. / h_scale.min(v_scale).round(); | |||
| projection.scaling_mode = ScalingMode::WindowSize(h_scale.min(v_scale).round()); | |||
There was a problem hiding this comment.
Note: this appears to produce the same behaviour as the existing example, though if there's a simpler method I haven't seen that's great too!
|
...and now I know |
bas-ie
changed the title
IQuick143
added
A-Rendering
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
MrGVSV
reviewed
Sep 8, 2024
Comment on lines
-376
to
-379
| /// Note: scaling can be set by [`scaling_mode`](Self::scaling_mode) as well. | ||
| /// This parameter scales on top of that. |
There was a problem hiding this comment.
This makes it pretty easy to scale multiple variants of ScalingMode since you can just modify a single f32. Are we at all concerned about hurting ergonomics?
There was a problem hiding this comment.
Not particularly; projects will only need to handle this once, and the majority of projects will have a single scaling mode.
There was a problem hiding this comment.
I've also run into a few people now who miss the existence of ScalingMode because of scale, leading them to ask questions. In this way, I think it's hurting them by not just making them think up front about how the camera should be configured.
alice-i-cecile
approved these changes
Sep 8, 2024
tychedelia
approved these changes
Sep 8, 2024
tychedelia
added
S-Ready-For-Final-Review
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
|
@richchurcher let me know when merge conflicts are resolved and I'll get this in for you :) |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 9, 2024
# Objective Add examples for zooming (and orbiting) orthographic and perspective cameras. I'm pretty green with 3D, so please treat with suspicion! I note that if/when #15075 is merged, `.scale` will go away so this example uses `.scaling_mode`. Closes #2580 --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
bas-ie
force-pushed
the
orth-proj-scale
branch
from
4370a27 to
0c3fedd
Compare
|
@alice-i-cecile I think we should be good now! |
alice-i-cecile
added a commit
to alice-i-cecile/bevy
that referenced
this pull request
Oct 17, 2024
…)" This reverts commit f326705.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 17, 2024
# 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Hello! I am adopting #11022 to resolve conflicts with
main. tldr: this removesscalein favour ofscaling_mode. Please see the original PR for explanation/discussion.Also relates to #2580.
Migration Guide
Replace all uses of
scalewithscaling_mode, keeping in mind thatscaleis (was) a multiplier. For example, replacewith