Skip to content
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

Merged
merged 6 commits into from
Feb 1, 2021
Merged

3D OrthographicProjection improvements + new example #1361

merged 6 commits into from
Feb 1, 2021

Conversation

inodentry
Copy link
Contributor

Fixes and improvements for 3D orthographic views. New example to showcase it.

Leaving a "review", with contextual notes giving rationale for the specific changes.

@@ -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 {
Copy link
Contributor Author

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.

Copy link
Member

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,
Copy link
Contributor Author

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(),
Copy link
Contributor Author

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.

Copy link
Member

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,
Copy link
Contributor Author

@inodentry inodentry Jan 31, 2021

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,
Copy link
Member

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.

Copy link
Contributor Author

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::*;
Copy link
Member

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.


fn main() {
App::build()
.add_resource(Msaa { samples: 4 })
Copy link
Member

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

@inodentry
Copy link
Contributor Author

OK. Done. Fixed the example.

@cart are there any more changes needed? if not, just merge this.

@cart cart merged commit 7d065ee into bevyengine:master Feb 1, 2021
DJMcNab added a commit to DJMcNab/bevy that referenced this pull request Feb 8, 2021
cart pushed a commit that referenced this pull request Feb 9, 2021
rmsc pushed a commit to rmsc/bevy that referenced this pull request Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants