Skip to content

Split OrthographicProjection::default into 2d & 3d (Adopted)#15073

Merged
alice-i-cecile merged 7 commits intobevyengine:mainfrom
Azorlogh:orthographic_projection-defaults
Sep 9, 2024
Merged

Split OrthographicProjection::default into 2d & 3d (Adopted)#15073
alice-i-cecile merged 7 commits intobevyengine:mainfrom
Azorlogh:orthographic_projection-defaults

Conversation

@Azorlogh
Copy link
Copy Markdown
Contributor

@Azorlogh Azorlogh commented Sep 7, 2024

Adopted PR from dmlary, all credit to them! #9915

Original description:

Objective

The default value for near in OrthographicProjection should be different for 2d & 3d.

For 2d using near = -1000 allows bevy users to build up scenes using background z = 0, and foreground elements z > 0 similar to css. However in 3d near = -1000 results in objects behind the camera being rendered. Using near = 0 works for 3d, but forces 2d users to assign z <= 0 for rendered elements, putting the background at some arbitrary negative value.

There is no common value for near that doesn't result in a footgun or usability issue for either 2d or 3d, so they should have separate values.

There was discussion about other options in the discord 0, but splitting default() into default_2d() and default_3d() seemed like the lowest cost approach.

Related/past work #9138, #9214, #9310, #9537 (thanks to @Selene-Amanita for the list)

Solution

This commit splits OrthographicProjection::default into default_2d and default_3d.

Migration Guide

  • In initialization of OrthographicProjection, change ..default() to ..OrthographicProjection::default_2d() or ..OrthographicProjection::default_3d()

Example:

--- a/examples/3d/orthographic.rs
+++ b/examples/3d/orthographic.rs
@@ -20,7 +20,7 @@ fn setup(
         projection: OrthographicProjection {
             scale: 3.0,
             scaling_mode: ScalingMode::FixedVertical(2.0),
-            ..default()
+            ..OrthographicProjection::default_3d()
         }
         .into(),
         transform: Transform::from_xyz(5.0, 5.0, 5.0).looking_at(Vec3::ZERO, Vec3::Y),

dmlary and others added 2 commits September 7, 2024 01:18
@dmlary @Azorlogh
Split OrthographicProjection::default into 2d & 3d
dcc4e15 
The default value for `near` in `OrthographicProjection` should be
different for 2d & 3d.

For 2d using `near = -1000` allows bevy users to build up scenes using
background `z = 0`, and foreground elements `z > 0` similar to css.
However in 3d `near = -1000` results in objects behind the camera being
rendered.

Using `near = 0` works for 3d, but forces 2d users to assign `z <= 0`
for rendered elements, putting the background at some arbitrary negative
value.

There was discussion about other options in the discord [0], but this
seemed like the lowest cost approach.

This commit splits `OrthographicProjection::default` into `default_2d`
and `default_3d`.

[0]: https://discord.com/channels/691052431525675048/1154114310042292325
@Azorlogh
fixes
8823372
@Azorlogh Azorlogh mentioned this pull request Sep 7, 2024
Closed
@IQuick143 IQuick143 added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 7, 2024
janhohenheim
janhohenheim suggested changes Sep 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me :) I would like to not add API without documentation, otherwise this looks good.

crates/bevy_core_pipeline/src/core_2d/camera_2d.rs
@@ -41,11 +41,7 @@ pub struct Camera2dBundle {

impl Default for Camera2dBundle {

This comment was marked as resolved.

Sign in to view

crates/bevy_render/src/camera/projection.rs
}

impl OrthographicProjection {
pub fn default_2d() -> Self {

This comment was marked as resolved.

Sign in to view

crates/bevy_render/src/camera/projection.rs
}
}

pub fn default_3d() -> Self {

This comment was marked as resolved.

Sign in to view

@Azorlogh
Copy link
Copy Markdown
Contributor Author

Azorlogh commented Sep 7, 2024

@janhohenheim Thanks for the review! I removed the outdated comment & added docs to default_2d & default_3d. Let me know if they're clear enough

janhohenheim
janhohenheim approved these changes Sep 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks! Left some minor documentation suggestions, but nothing blocking :)

Azorlogh and others added 3 commits September 7, 2024 20:42
@Azorlogh @janhohenheim
Update crates/bevy_render/src/camera/projection.rs
e265fe4 
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@Azorlogh @janhohenheim
Update crates/bevy_render/src/camera/projection.rs
855f9b9 
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@Azorlogh @janhohenheim
Update crates/bevy_render/src/camera/projection.rs
c946c95 
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
IQuick143
IQuick143 approved these changes Sep 8, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense for Default to default to default_3d (like FromWorld does)?

@IQuick143 IQuick143 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 8, 2024
@Azorlogh
Copy link
Copy Markdown
Contributor Author

Azorlogh commented Sep 8, 2024

Would it make sense for Default to default to default_3d (like FromWorld does)?

It would defeat the point, since beginners would keep writing

OrthographicProjection {
    scaling_mode: ScalongMode::FixedVertical(2.0),
    ..default()
}

on their 2d cameras, ending up with a black screen.
With this PR they're forced to make the choice

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2024
Merged via the queue into bevyengine:main with commit 82e416d Sep 9, 2024
This was referenced Sep 9, 2024
Merged
Closed
@Azorlogh Azorlogh mentioned this pull request Sep 9, 2024
Closed
@ThierryBerger ThierryBerger mentioned this pull request Sep 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@janhohenheim janhohenheim janhohenheim approved these changes

@IQuick143 IQuick143 IQuick143 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Azorlogh @janhohenheim @IQuick143 @alice-i-cecile @dmlary