Remove OrthographicProjection.scale (adopted)#15075
Remove OrthographicProjection.scale (adopted)#15075alice-i-cecile merged 3 commits intobevyengine:mainfrom
Conversation
|
Just workin' through the CI failures now... |
| 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 |
|
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? |
| /// 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.
|
@richchurcher let me know when merge conflicts are resolved and I'll get this in for you :) |
# 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 <[email protected]> Co-authored-by: Jan Hohenheim <[email protected]>
4370a27 to
0c3fedd
Compare
|
@alice-i-cecile I think we should be good now! |
…)" This reverts commit f326705.
# 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 <[email protected]>
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