-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
OrthographicProjection scaling mode + camera bundle refactoring #400
Conversation
Could you make this an external crate instead? It wouldn't need to be fancy |
Yeah an external crate would be easier to prove out an approach. Non-pixel-sized rendering is absolutely something we need in bevy, theres just many ways to do it (ex: render to texture, fit to screen, add letterboxing) |
@DJMcNab thanks for digging out this PR! i had kinda forgotten about it. sure, i'll move this to a separate crate, to make it usable for people who want it (assuming @cart isn't interested in merging it (it is a pretty simple thing that IMO should be built into bevy), or i'll rebase/update this PR instead)
that sounds very heavyweight for something that can be easily solved with different values on the orthographic projection, like shown in this PR ofc nothing is stopping people from rendering to texture, if they have other benefits from that (like postprocessing effects) i think a simple/normalized orthographic projection like this PR proposes should be merged into bevy regardless
yes, this could be a useful addition, to limit the aspect ratio otherwise, games need to take into consideration that some players might try to run them on ultrawide monitors or other funky aspect ratios, which might lead to undesirable effects but that is separate from this issue; control over the aspect ratio (letterboxing) should be useful regardless of whether your projection is pixel-sized or normalized
really? i don't like to plainly dismiss conversations, but i don't think this is that controversial; this is the simplest way to do it and i can't see why other approaches would be preferred (or rather, why the possibility for other approaches would mean that bevy shouldn't offer this). i think this normalized projection should be offered by bevy regardless although, perhaps it would be useful to add an option to choose which axis to fix it to (whether X or Y should be mapped to -1.0 .. 1.0), because games intended to be played in portrait mode (like mobile games) might want to choose to keep the horizonal axis fixed, rather than the vertical. it would also be trivial to make the |
BTW, can i somehow change the branch in my fork that this PR is based on, without making a new PR from the new branch? I shouldn't have made the changes in |
You can't change the head branch of a PR, unfortunately One approach would be to |
The current But the space for other kinds of orthographic projections is big enough that we don't want to support them all in-tree at this early stage moment (even just the suggestions of fixed-x, fixed-y, letterboxing). That is, if someone wants to use an orthographic projection in their game, then a good enough solution would be to just apply a That is, this mechanism is probably a bit too niché to be in-engine at the moment, as orthographic games are relatively rare. Although if we did find that the external crate was seeing widespread adoption we'd certainly reconsider. (Usage of we here is more in a general sense, since I can't speak officially) |
Correct me if I am wrong, but letterboxing cannot be done with projection/camera parameters alone, right? It needs to be done by adjusting the viewport (in opengl terms, idk if modern apis have that), or something else in the render pipeline, correct? |
I updated the PR with some new commits to implement these extra features. It seems like a very simple thing to attach to this implementation.
I think this already covers basically all uses for a scaled orthographic projection. It's not that big, as you can see it can very easily be covered by a single projection type with a few configurable parameters. These 3 projection types as they are now (perspective, pixel-orthographic, scaled-orthographic) cover basically all reasonable use cases. I know many people want a scaled orthographic projection, for both 2D and for CAD-like 3D views. (i've seen the demand for this many times in discord chats) It really doesn't add much to support this. I think making it an external crate would just be awkward for everyone.
Again, isn't that a separate feature / irrelevant to this PR? |
Yeah, I don't think letterboxing is a projection issue as such. |
Also added a commit for a new general orthographic camera bundle, using this new projection. IDK if the same "far" trick as found in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been convinced that this functionality is simple enough to not be worth maintaining an external crate for, and useful enough to have built-in.
(I never whether to approve or comment in these cases, because there are still changes I would like to be made, but I also want to give you the happy green tick)
crates/bevy_render/src/entity.rs
Outdated
/// | ||
/// Use this for CAD-like 3D views or non-pixel-oriented 2D games. | ||
#[derive(Bundle)] | ||
pub struct CameraOrthoBundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say Orthographic3dCameraBundle
is probably a nicer name, although it is less consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very long name, and the camera is useful for 2D games, too. Actually, I'd advise anyone who is making a 2D game, but not going for a pixel-perfect artstyle, to consider if they would be better served by this new projection. It would automatically take care of scaling and window resizing / different resolutions, without having to mess with the camera.
I'd prefer if the name doesn't include either "2D" or "3D".
crates/bevy_render/src/entity.rs
Outdated
|
||
impl Default for CameraOrthoBundle { | ||
fn default() -> Self { | ||
// FIXME does this same far trick from `Camera2DBundle` make any sense here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that the correct answer here is no, it doesn't make sense.
I think a sane default would be located at (100, 100, 100)
, looking at (0,0,0)
, and scaled to 50x
, i.e. include more of the scene.
But I think that something similar would also apply to perspective transforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought so too. Whatever the 3D perspective camera does is probably right for this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I will just make it initialize to default (like the perspective projection). That seems like it would be less surprising to users than hardcoding some arbitrary values.
crates/bevy_render/src/entity.rs
Outdated
/// | ||
/// Uses a normalized orthographic projection. | ||
/// | ||
/// Use this for CAD-like 3D views or non-pixel-oriented 2D games. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good example of a game which uses this kind of perspective is Into the Breach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, 2.5d
is normally the term used to describe this style of projection for 3d scenes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's a good example. A game with a pixel-art aesthetic like that is probably better done with a pixel-sized 2D orthographic projection (like the default in bevy), with 2D tiles/sprites with suitable positioning and draw order. Otherwise, good luck getting your pixel art to align and look pretty.
A better example is something like School Tycoon:
This game actually uses 3D models with a freely-rotating overhead camera, but with an orthographic projection to give that "isometric" look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose captain toad treasure tracker is another example, which I'm fairly certain actually is 3d
self.near, | ||
self.far, | ||
), | ||
(WindowOrigin::BottomLeft, BaseAxis::Vertical) => Mat4::orthographic_rh( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sorting by WindowOrigin
makes more sense - it makes the parallels between horizontal and vertical more obvious.
impl Default for ScaledOrthographicProjection { | ||
fn default() -> Self { | ||
ScaledOrthographicProjection { | ||
scale: 1.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this scale field shouldn't be here, and instead should be handled by the scale on the Transform
of self
.
Obviously that would also scale far
, which isn't ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly disagree. They serve different purposes. I think it is important to have this field, to be able to set the "span" of the projection.
The camera's transform is typically used to perform "zoom in/out" effects in the game.
The "scale" of the projection is if you need to normalize the sizes of things (say, depending on what units you work in, particularly for a 2D game).
Say, for example, you are making a 2D game and you want it to scale based on screen resolution, to always fit the same content on screen.
You might want the projection to go -600.0 to +600.0 (1200 total height), because you designed your assets for a nominal 1200-vertical-pixels screen size. You shouldn't have to complicate your camera math to account for that. The camera scale should stay at 1.0 unless you are doing some "zoom in/out" effects.
This is the same argument I've been giving before w.r.t your suggestion of "just use the window-pixels 2d projection bevy already provides for 2D games and fiddle with the camera scale to make your content fit the screen" ... it's an ugly workaround and not ergonomic.
It really doesn't cost anything to have this field, and I think it opens up some likely-useful extra flexibility for users. Why potentially cripple/limit the functionality, just to avoid that extra field? IMO it makes sense to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that if we did have scale only as the field on transform, then that would be the only thing defining the size (of the fixed axis) of the in-world screen/size of the in-world viewing pane
Whereas at the moment you have to multiply it by this scale field to know that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the projection scale is at 1.0 by default, so what you said applies, unless the user explicitly goes to mess with the projection. I think it is valuable for the users to have the option of doing that.
Which means we shouldn't remove the scale field.
It's not going to surprise or confuse anyone; it's only relevant to those users who know they want to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that also makes sense.
I always forget to run |
fn default() -> Self { | ||
CameraOrthoBundle { | ||
camera: Camera { | ||
name: Some(base::camera::CAMERA_3D.to_string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the new camera use the CAMERA_3D
or CAMERA_2D
name? What purpose do these serve / what difference would it make?
I'll admit that these aren't identical solves and that I was thinking too much about a specific application of "non-pixel sized scales" in my last comment. The "render to texture" / letterboxing / scale-to-fit approach I mentioned doesn't handle "dynamic" orthographic scaling or projection scaling where the "output" image should not be scaled. There are legitimate use cases for that (which have been discussed above). That set of use cases is common enough that its worth supporting in-tree and I agree that (other than surface level api stuff) the implementation is not particularly controversial. I'll do another pass and start thinking about the problem in general. Off the top of my head: Before creating a new bundle, I think its worth discussing alternatives. The "most interesting" one i can think of is:
|
The downside to the approach above is adding cameras from the editor. I expect Bundles to have a nice "autocomplete" ui for codeless scene composition (think godot "nodes"). Separate bundles for each context would mean that you could easily search for a CameraUiBundle and add it to a scene. If we decouple bundles from 2d/3d contexts, it adds an extra step where you need to manually select the camera name in the editor. |
@cart Ok, so now that we are discussing the camera bundles, I want to bring up some points:
I think that is actually a good thing, both for the editor and for the code API ...
... we should also add a constructor that lets the user specify a custom camera name. Here is the motivation: As it is, only trivial games with a single camera for main rendering can use the existing bundles. There are many use cases for having multiple cameras for more advanced rendering / render passes. For example, a friend of mine is currently working on a FPS game, and he will want to have a separate camera and render pass for rendering the gun/hands in front of the player in the first person view, or for the view inside of the scope of a weapon. Right now, Bevy's bundles are useless for these secondary cameras, because there is no way to specify a custom name. It is necessary to either spawn the camera without using a bundle (just as a tuple of components), or roll your own bundle. This can be error-prone. If there was a general Of course, the default 3D/2D camera should have its own special constructor. This fits nicely here, as we are discussing decoupling 2D/3D from the type of camera anyway.
I like this.
I don't like this. I think we should keep it, just for clarity and ergonomics. It is nice for users to be able to just easily spawn their "ui camera" without knowing what it really is. It should just be renamed to
I hadn't considered this. Now that I think about it, I like it, because it would make the two types of orthographic projection more similar and unify their feature-set. Then, the choice of projection for the user becomes simply:
Both of those would offer scaling if the user wants to adjust the size. That said -- some more bikeshedding: I don't like the current names: (actually, if i were to go by my intuition, I'd call the old projection "scaled" (because it scales with the window) and the new one the normal, because it is fixed and behaves similarly to the perspective projection) Also, the simple name I think we should come up with new names for both of the projections, to clearly indicate the difference. Some suggestions for names:
(the words could also be swapped: I don't particularly like any of these names that much, let me know what you think or if you have better suggestions. |
I don't think its clear cut for the editor. I think the "constructor approach" is almost certainly better for the in-code api. And as long as the bundle defaults are as expected (ex: Perspective defaults to 3d pipelines, Ortho defaults to 2d pipelines) we're probably good on the editor side. But imo Camera2d and Camera3d (with configurable projections) is really nice (and visually self-documenting) in the editor. It felt very natural to me in godot. Of course consistency is important so we shouldn't do one thing in code and another thing in the editor. I'm cool with continuing this discussion here, but we can also table it if you'd prefer to just extend the Orthographic projection with a scale property in this pr.
I agree
This is false: .spawn(Camera3dBundle {
camera: Camera {
name: Some("custom name".to_string()),
..Default::default()
},
..Default::default()
});
How is the UI camera case any different from the 2d/3d case? They seem identical to me. If OrthographicCamera::2d() and OrthographicCamera::3d() is the "right" way to distinguish between the two, imo that should also be true for ui. However on the topic of UI camera ergonomics, ideally we just don't require the UI camera (and fall back internally). The percentage of people who need to move the "ui camera" is almost zero.
When I said "add "scale" directly to OrthographicProjection", I meant that we would have a single consolidated OrthographicProjection. Window width and height would be the base "scale == 1". This is how both blender and godot handle it, and I think thats the best behavior for something named OrthographicProjection. This satisfies the common use case of providing a "zoomable camera". The "corner case" of ignoring pixel size entirely could be handled by any of these:
|
Also, it looks like cameras don't actually update when Camera is mutated (only when the window is resized). We'll need to fix that if we add a scale field. (we should fix it anyway 😄 ) |
We seem to disagree about whether this is a "corner case". I don't think it is. A projection that fits the same content on screen regardless of window resizing seems to me like a common thing to want. That's why I set out to make this PR (and the OP got so many hearts :D ). Of course, "ignoring pixel size" is totally the wrong thing to do for UI. That's why we have the existing orthographic projection. But for the game itself, it is a very sensible thing to want.
Then we are back to where we started. That is, having to do specialized trickery with the camera / projection / transform to account for the window size. Rather than having it simply work out of the box (by choosing the appropriate projection type). |
UI is kinda "special", conceptually. It's not really part of the game world, but a separate thing on top. When you make your game, you build your game world, and you build your UI, they are separate parts of the game design and implementation. I guess to me it feels special, so my intuition says it should have an independent API, rather than reusing the one for the game camera, but that's subjective. I can totally understand the opposite argument. I don't want to continue this discussion about the UI Bundle. If you think we should remove it and unify it with the orthographic bundle, I'm fine with that. |
@cart I figured out how to make this even better. I think you will like the new changes. I made some updates based on the things we talked about: 1. Camera Bundle changes:Rename camera bundles to
2. Unify the orthographic projections!I had the epiphany that the scaling behavior could be made selectable with an enum field, rather than having 2 separate projection types! I like it this way a lot more, and I think you will, too. 😄 Now there is a single orthographic projection type, with a scaling mode field, and a 3. UI cameraI did not remove the bundle, because I wasn't sure what to do about the crate boundaries. I renamed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
#[derive(Debug, Clone, Reflect, Serialize, Deserialize)] | ||
#[reflect_value(Serialize, Deserialize)] | ||
pub enum ScalingMode { | ||
// Match the window size. 1 world unit = 1 pixel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason these aren't doc comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, that was an accident. i'll fix it
// Keep vertical axis constant; resize horizontal with aspect ratio. | ||
FixedVertical, | ||
// Keep horizontal axis constant; resize vertical with aspect ratio. | ||
FixedHorizontal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well have ScalingMode::None
too, if we're here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean this: that nothing should happen on window resize (let the image stretch), just letting the user set the left
/right
/top
/bottom
fields manually.
OK, added this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly that. No point limiting the functionality.
self.right, | ||
self.bottom, | ||
self.top, | ||
self.left * self.scale, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 I really like this simplification
FIXME: does the same "far" trick from Camera2DBundle make any sense here?
left: -1.0, | ||
right: 1.0, | ||
bottom: -1.0, | ||
top: 1.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the defaults here, to allow for this:
let projection = ОrthographicProjection {
scaling_mode: ScalingMode::None,
..Default::default()
};
@cart is there anything more to do here? or is this PR ready for merging? |
Yup this is much better! I dig all of the changes. My only nit is that I would prefer things like |
@cart I wanted to call them However, Rust identifiers cannot start with a digit. 😆 I got a compiler error. Hence the current names. |
|
So if there is no other criticism, can we merge this? |
Sounds like a plan! |
If I was previously using in 0.4:
is the equivalent upgrade in 0.5 simply:
Specifically, does that result in the same transforms and scale? edit: Answer -> YES And be sure to update shaders to CameraViewProj. Both of these would probably be handy in the migration guide. |
It is a common use case for 2D games to have an orthographic projection that scales the world to the window (so the same content is always displayed, regardless of window size).
One logical/sensible way to define such a projection is to make Y span
-1.0
..1.0
(if0.0
is at the center of the screen) or0.0
..1.0
(if0.0
is at the bottom left). To not stretch the image, X is then scaled to the aspect ratio.This results in the same amount of content displayed vertically, regardless of aspect ratio, but a variable amount of content horizontally.
There are other possible ways to do it, but this seems like a simple and sane approach that should suit most people.
Other things we need to discuss:
Naming? Now that there are two built-in orthographic projection types, they need to be named clearly. Perhaps the old
OrthographicProjection
should be renamed to make it clear that it is pixel-sized? I would guess that both usecases would be roughly equally commonly desired, so one should not feel more difficult to use than the other.What should we do with the
Camera2dComponents
bundle? It should be made easy for the user to select either projection.