Skip to content

Split OrthographicProjection::default into 2d & 3d#9915

Closed
dmlary wants to merge 1 commit intobevyengine:mainfrom
dmlary:orthographic_projection-defaults
Closed

Split OrthographicProjection::default into 2d & 3d#9915
dmlary wants to merge 1 commit intobevyengine:mainfrom
dmlary:orthographic_projection-defaults

Conversation

@dmlary
Copy link
Copy Markdown
Contributor

@dmlary dmlary commented Sep 24, 2023

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.


Changelog

  • OrthographicProjection::default() removed
  • OrthographicProjection::default_3d() added
  • OrthographicProjection::default_2d() added
  • impl FromWorld for OrthographicProjection added; calls default_3d()
    • necessary for #[derive(Reflect)] when impl Default removed

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),

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

dmlary commented Sep 24, 2023

I'll be honest, I'm not in love with default_2d and default_3d. It solves the problem, but feels inelegant and like an awful pattern if duplicated.

I'm opening this PR mostly for feedback and to continue the discussion about better options.

@Selene-Amanita Selene-Amanita self-requested a review September 24, 2023 19:24
@Selene-Amanita Selene-Amanita added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Needs-SME This type of work requires an SME to approve it. labels Sep 24, 2023
@mgi388
Copy link
Copy Markdown
Contributor

mgi388 commented Oct 3, 2023

Would it make sense to match how things like Camera2dBundle and friends are named and create Orthographic2dProjection and Orthographic3dProjection? Or maybe as a suffix OrthographicProjection2d and OrthographicProjection3d. Then the default() doesn’t have the awkward _Xd suffix.

@dmlary
Copy link
Copy Markdown
Contributor Author

dmlary commented Oct 3, 2023

Would it make sense to match how things like Camera2dBundle and friends are named and create Orthographic2dProjection and Orthographic3dProjection? Or maybe as a suffix OrthographicProjection2d and OrthographicProjection3d. Then the default() doesn’t have the awkward _Xd suffix.

This was mentioned in discord. The net result of this is duplicating the entirety of the class, for different defaults. That feels wrong for code maintenance, despite providing a cleaner API.

There’s a trait-based approach where all the code is implemented in trait default implementations, but that felt like a lot of added complexity to allow for one default value to be different.

That said, if maintainers are ok with either option, we can definitely go that route.

@Selene-Amanita
Copy link
Copy Markdown
Member

Another example of why it could be useful: https://discord.com/channels/691052431525675048/1167754343638904912/1167754343638904912

@juh9870
Copy link
Copy Markdown

juh9870 commented Nov 28, 2023

I would absolutely love to have projection settings be completely separate for 2d, with clipping planes removed completely. They just introduce complexity to 2d games, without providing any use cases that can't be solved by just changing visibility. Z-order of a sprite should not make it disappear just because of some arbitrary value on the 2d camera. Current system feels very Unity-like, where 2d is just an afterthought of a 3d system.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Needs-SME This type of work requires an SME to approve it. labels Aug 16, 2024
@Azorlogh
Copy link
Copy Markdown
Contributor

Azorlogh commented Sep 5, 2024

This keeps coming up as a big footgun for beginners. Examples:
https://discord.com/channels/691052431525675048/1281174361973329961
https://discord.com/channels/691052431525675048/1272111908874358846
https://discord.com/channels/691052431525675048/1034547455032832021/1271313018822721651
https://discord.com/channels/691052431525675048/1275165498786447402 (this one didn't report a problem but we can see they didn't set the near plane and they're in 2d, so they likely ran into it later)

It's a frustrating problem for beginners because it's easy to fall into this trap when simply trying to adjust scaling, and they end up thinking there's something wrong with scaling instead of it being related to near planes.
I'm in favor of merging this just to have any solution, because until then people will keep running into this.
If we decide there is another solution later, it will be a very mechanical migration anyway.

@dmlary
Copy link
Copy Markdown
Contributor Author

dmlary commented Sep 6, 2024

This PR looks like it’s seeing movement, but I’m tied up and won’t be able to resolve the merge conflicts, and test. If someone wants to adopt it, please do so.

1 similar comment
@dmlary
Copy link
Copy Markdown
Contributor Author

dmlary commented Sep 6, 2024

This PR looks like it’s seeing movement, but I’m tied up and won’t be able to resolve the merge conflicts, and test. If someone wants to adopt it, please do so.

@Azorlogh
Copy link
Copy Markdown
Contributor

Azorlogh commented Sep 7, 2024

This PR looks like it’s seeing movement, but I’m tied up and won’t be able to resolve the merge conflicts, and test. If someone wants to adopt it, please do so.

Done 👌 #15073

@alice-i-cecile
Copy link
Copy Markdown
Member

Adopted and merged :D

github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 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](https://discord.com/channels/691052431525675048/1154114310042292325),
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:
```diff
--- 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),
```

---------

Co-authored-by: David M. Lary <dmlary@gmail.com>
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
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 X-Contentious There are nontrivial implications that should be thought through

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants