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

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

Merged

Conversation

Azorlogh
Copy link
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
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
@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-Needs-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
Copy link
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.

@@ -41,11 +41,7 @@ pub struct Camera2dBundle {

impl Default for Camera2dBundle {

This comment was marked as resolved.

}

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

This comment was marked as resolved.

}
}

pub fn default_3d() -> Self {

This comment was marked as resolved.

crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved
@Azorlogh
Copy link
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

Copy link
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 :)

crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved
Copy link
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
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-Needs-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
Development

Successfully merging this pull request may close these issues.

5 participants