-
-
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
3D OrthographicProjection improvements + new example #1361
Conversation
@@ -17,7 +18,8 @@ pub struct Camera { | |||
pub depth_calculation: DepthCalculation, | |||
} | |||
|
|||
#[derive(Debug)] | |||
#[derive(Debug, Clone, Copy, Reflect, Serialize, Deserialize)] | |||
#[reflect_value(Serialize, Deserialize)] | |||
pub enum DepthCalculation { |
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 is now a field in OrthographicProjection
, so I had to add some extra derives, similar to the ScalingMode
and WindowOrigin
enums.
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.
Maybe a comment that Distance
is generically applicable and ZDifference
is an optimisation for an orthographic projection looking towards negative -z
(i.e. its forward vector is +z
because #1153, although don't include that in the comment)?
@@ -156,6 +157,7 @@ impl Default for OrthographicProjection { | |||
window_origin: WindowOrigin::Center, | |||
scaling_mode: ScalingMode::WindowSize, | |||
scale: 1.0, | |||
depth_calculation: DepthCalculation::Distance, |
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.
Distance
makes sense as the default, because it always works everywhere. The 2D camera bundles will override it with ZDifference
.
@@ -231,7 +231,7 @@ pub fn visible_entities_system( | |||
// smaller distances are sorted to lower indices by using the distance from the camera | |||
FloatOrd(match camera.depth_calculation { | |||
DepthCalculation::ZDifference => camera_position.z - position.z, | |||
DepthCalculation::Distance => (camera_position - position).length(), | |||
DepthCalculation::Distance => (camera_position - position).length_squared(), |
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 square root operation isn't necessary.
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.
(Because we only use this value for an ordering)
@@ -106,7 +110,11 @@ impl OrthographicCameraBundle { | |||
name: Some(base::camera::CAMERA_3D.to_string()), | |||
..Default::default() | |||
}, | |||
orthographic_projection: Default::default(), | |||
orthographic_projection: OrthographicProjection { | |||
scaling_mode: ScalingMode::FixedVertical, |
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 3D applications, ScalingMode::FixedVertical
makes more sense as the default, because it behaves similarly to the perspective projection (does not reveal extra content as you resize the window). This is the correct and expected behavior for 3D.
Edit:
With FixedVertical
, the scale
field behaves more intuitively. scale = 1.0
shows the scene at the default "zoom level", and it can be adjusted to "zoom in/out".
If ScalingMode::Window
is used (as was before), the scene appears tiny (as 1 "3d unit" = 1 pixel !), so the scale would have to be set to something like 0.01
to actually reasonably show the scene.
orthographic_projection: Default::default(), | ||
orthographic_projection: OrthographicProjection { | ||
scaling_mode: ScalingMode::FixedVertical, | ||
depth_calculation: DepthCalculation::Distance, |
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 should already be the default.
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 know. I added it explicitly, just to ensure that upon any possible future changes, it does not accidentally break. I don't think there is any harm in explicitly overriding it with the correct value. It just seems safer (and more sensible) to me that the camera bundle constructors should explicitly ensure that any relevant parameters are set to the correct values, rather than implicitly relying on the default just happening to be the right thing.
@@ -0,0 +1,62 @@ | |||
use bevy::prelude::*; |
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.
cargo.toml needs to be updated to include this example.
examples/3d/orthographic.rs
Outdated
|
||
fn main() { | ||
App::build() | ||
.add_resource(Msaa { samples: 4 }) |
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 will need to be updated to be insert_resource
as we just merged that
OK. Done. Fixed the example. @cart are there any more changes needed? if not, just merge this. |
Fixes and improvements for 3D orthographic views. New example to showcase it.
Leaving a "review", with contextual notes giving rationale for the specific changes.