-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add sub_camera_view
, enabling sheared projection
#15537
Add sub_camera_view
, enabling sheared projection
#15537
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
examples/3d/camera_sub_view.rs
Outdated
@@ -0,0 +1,267 @@ | |||
//! Renders multiple cameras with different sub view efffects. |
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.
This needs a less jargon-y explanation: what's a "sub view", and why might a user care about 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.
I added mentions of some possible use cases, however the screen shake use case is not covered by my example. Is it fine to leave it in?
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.
Yep, that's fine. We should add a dedicated "screen_shake" example at some point, but that's totally fine to leave for follow-up.
I like the feature and the code is good, but I think we need to do a better job explaining why users might care :) |
@@ -66,6 +66,31 @@ impl Default for Viewport { | |||
} | |||
} | |||
|
|||
/// Settings to define a camera sub view. | |||
/// | |||
/// When added to a camera, only the sub-section of the image defined by `size` |
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.
/// When added to a camera, only the sub-section of the image defined by `size` | |
/// When [`Camera::sub_camera_view`] is `Some`, only the sub-section of the image defined by `size` |
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.
Much clearer, thanks!
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.
Seems like the code isn't taking into account scaling factor.
Also, we can bikeshed this more later, but I don't like "sub view" naming. I'm not sure what the prior art is here, but I would minimally prefer something spelled out like "ViewSubset" or even just the offset terminology used in the linked api by three.js.
examples/3d/camera_sub_view.rs
Outdated
sub_camera_view: Some(SubCameraView { | ||
// Set the sub view camera to a fifth of the full view and | ||
// move it in another system | ||
full_size: uvec2(500, 500), |
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.
Will let @alice-i-cecile make a determination, but we tend not to use these function constructors anywhere in engine code (although I personally find them quite nice!)
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, let's keep it consistent and use the non-function forms.
#[derive(Debug, Clone, Copy, Reflect, PartialEq, Eq)] | ||
pub struct SubCameraView { | ||
/// Size of the entire camera view | ||
pub full_size: UVec2, |
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'm a bit confused about the purpose of this field, shouldn't this be computed from the camera/viewport itself rather than manually specified?
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.
It could theoretically be calculated from the viewport itself, however I think this would make the API more confusing for users. Take the example of a multi-monitor setup for example.
┌───┬───┐
│ A │ B │
├───┼───┤
│ C │ D │
└───┴───┘
If each monitor is 1920x1080 and we decide to use a 1920x1080 viewport for each monitor, then the whole should be 3840x2160.
Now to determine the sub view values for monitor/viewport A, the sub view has to have half the dimensions of the "full image":
- If we calculated the full view from the size of our viewport, the sub view would have to be 960x540 to correctly take up a quarter of the physical viewport that is 1920x1080
- If we have the additional
full_size
field, the users don't have to think about the actual size of their physical viewport but can consider the size of the "full image" instead. And can then just use the value of the physical viewport of A for their sub view.
As I write this out I'm starting to realize that this should also work with full_size
being 32x18 and size
being 16x9.
I will change the example to make this more apparent, and adjust the doc comments to make it clearer that the ratio is what is relevant.
I will also allow for offset to be a regular Vec2
to allow for smaller steps when size
and full_size
is made up of small values.
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.
Alright, that makes sense to me.
/// whole image) is projected to the cameras viewport. | ||
#[derive(Debug, Clone, Copy, Reflect, PartialEq, Eq)] | ||
pub struct SubCameraView { | ||
/// Size of the entire camera view |
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 add "in pixels".
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.
The sub view is defined by the ratio between full_size
, size
and offset
, so in this case "in pixels" would be misleading. That was however not sufficiently documented, so I have added further detail.
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.
API looks pretty good. Looking forward to testing with it.
I'm not quite sure what you did to get this effect. I tried If you want, I can expand the example to include automatic scaling of the viewports.
I would not like to use "ViewSubset", since subset implies a countable number of possible views and that the current view is one or more of them. I'd be happy to use the offset naming analogous to three.js though :) |
40d0499
to
f422172
Compare
Thanks for the careful documentation and patient responses to reviewers :) I'm happy with the state of this now: merging. |
@alice-i-cecile This is still broken on macOS and will minimally need a quick follow up for that. |
# Objective - This PR fixes bevyengine#12488 ## Solution - This PR adds a new property to `Camera` that emulates the functionality of the [setViewOffset()](https://threejs.org/docs/#api/en/cameras/PerspectiveCamera.setViewOffset) API in three.js. - When set, the perspective and orthographic projections will restrict the visible area of the camera to a part of the view frustum defined by `offset` and `size`. ## Testing - In the new `camera_sub_view` example, a fixed, moving and control sub view is created for both perspective and orthographic projection - Run the example with `cargo run --example camera_sub_view` - The code can be tested by adding a `SubCameraView` to a camera --- ## Showcase ![image](https://github.com/user-attachments/assets/75ac45fc-d75d-4664-8ef6-ff7865297c25) - Left Half: Perspective Projection - Right Half: Orthographic Projection - Small boxes in order: - Sub view of the left half of the full image - Sub view moving from the top left to the bottom right of the full image - Sub view of the full image (acting as a control) - Large box: No sub view <details> <summary>Shortened camera setup of `camera_sub_view` example</summary> ```rust // Main perspective Camera commands.spawn(Camera3dBundle { transform, ..default() }); // Perspective camera left half commands.spawn(Camera3dBundle { camera: Camera { sub_camera_view: Some(SubCameraView { // Set the sub view camera to the left half of the full image full_size: uvec2(500, 500), offset: ivec2(0, 0), size: uvec2(250, 500), }), order: 1, ..default() }, transform, ..default() }); // Perspective camera moving commands.spawn(( Camera3dBundle { camera: Camera { sub_camera_view: Some(SubCameraView { // Set the sub view camera to a fifth of the full view and // move it in another system full_size: uvec2(500, 500), offset: ivec2(0, 0), size: uvec2(100, 100), }), order: 2, ..default() }, transform, ..default() }, MovingCameraMarker, )); // Perspective camera control commands.spawn(Camera3dBundle { camera: Camera { sub_camera_view: Some(SubCameraView { // Set the sub view to the full image, to ensure that it matches // the projection without sub view full_size: uvec2(450, 450), offset: ivec2(0, 0), size: uvec2(450, 450), }), order: 3, ..default() }, transform, ..default() }); // Main orthographic camera commands.spawn(Camera3dBundle { projection: OrthographicProjection { ... } .into(), camera: Camera { order: 4, ..default() }, transform, ..default() }); // Orthographic camera left half commands.spawn(Camera3dBundle { projection: OrthographicProjection { ... } .into(), camera: Camera { sub_camera_view: Some(SubCameraView { // Set the sub view camera to the left half of the full image full_size: uvec2(500, 500), offset: ivec2(0, 0), size: uvec2(250, 500), }), order: 5, ..default() }, transform, ..default() }); // Orthographic camera moving commands.spawn(( Camera3dBundle { projection: OrthographicProjection { ... } .into(), camera: Camera { sub_camera_view: Some(SubCameraView { // Set the sub view camera to a fifth of the full view and // move it in another system full_size: uvec2(500, 500), offset: ivec2(0, 0), size: uvec2(100, 100), }), order: 6, ..default() }, transform, ..default() }, MovingCameraMarker, )); // Orthographic camera control commands.spawn(Camera3dBundle { projection: OrthographicProjection { ... } .into(), camera: Camera { sub_camera_view: Some(SubCameraView { // Set the sub view to the full image, to ensure that it matches // the projection without sub view full_size: uvec2(450, 450), offset: ivec2(0, 0), size: uvec2(450, 450), }), order: 7, ..default() }, transform, ..default() }); ``` </details>
Objective
Solution
Camera
that emulates the functionality of the setViewOffset() API in three.js.offset
andsize
.Testing
camera_sub_view
example, a fixed, moving and control sub view is created for both perspective and orthographic projectioncargo run --example camera_sub_view
SubCameraView
to a cameraShowcase
Shortened camera setup of `camera_sub_view` example