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.

Sign up for GitHub

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

Jump to bottom

Extend the default 2d render range to -1000..1000 #9214

Closed
wants to merge 1 commit into from
Closed

Extend the default 2d render range to -1000..1000 #9214

wants to merge 1 commit into from

Conversation

opstic
Copy link
Contributor

@opstic opstic commented Jul 20, 2023 •
edited
Loading

Objective

Solution

  • Extend the default far value of Camera2dBundle so that 2d entities with z values within the range of -1000 to 1000 gets rendered (default far value is now 2000).
  • Also the new_with_far function of Camera2dBundle now means the far value will be split in two for both the positive and negative ranges (ex: new_with_far(1000.) => -500..500, new_with_far(2000.) => -1000..1000).

@jnhyatt
Copy link
Contributor

jnhyatt commented Jul 20, 2023 •
edited
Loading

new_with_far(1000.) => -500..500, new_with_far(2000.) => -1000..1000).

These seem potentially confusing to me. I'd expect calling this function that the camera would have the far plane I specified, and the near plane isn't even mentioned. What are your thoughts on renaming this function? Maybe new_with_range? Or something else?

@opstic
Copy link
Contributor Author

opstic commented Jul 20, 2023

@jnhyatt By the far splitting in two I really mean the camera will be moved forward by half of the far value (instead of the entire far value), so that half of the far will be in negative space and the other half will be in the positive space.
The far value in OrthographicProjection will still be set to the user specified far.

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jul 20, 2023 •
edited
Loading

I didn't see you submitted a PR sorry ><'

I commented something here: #9138 (comment) waiting for Cart or Alice's reply.

(Edit: Alice put a 👍 reaction)

@jnhyatt
Copy link
Contributor

jnhyatt commented Jul 20, 2023 •
edited
Loading

I think it makes the most sense to use a negative near plane rather than moving the camera backwards. It seems like this change would make it too easy to accidentally make half your sprites disappear with something innocuous-looking:

// Re-center camera
transform.translation = Vec3::ZERO;

In this case, all your sprites with Z<0 would disappear and the reason would not be entirely obvious.

@opstic
Copy link
Contributor Author

opstic commented Jul 20, 2023

@Selene-Amanita How do you think the API should look like?
The way @jnhyatt suggested by doing new_with_range with a range or to split the value passed to new_with_far between the far and near planes?

The new_with_range one is a change in API and I don't think a patch release should have that...

But also, if the behavior of new_with_far results in it splitting the value between the far and near planes, the function name won't be accurate anymore (since it's also using it for near) and there might be people confused seeing the far value of OrthographicProjection isn't the one that they passed in...

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jul 20, 2023 •
edited
Loading

You could split it in two PRs:

  • a first one to be merged in 0.11.1 that would leave a new_with_far, this method would set transform to Vec3::ZERO and set far to the value provided, and in default() you use it, store it in a mutable variable then modify the variable to set the near after that, then you return the variable.
  • a second one to be merged in 0.12 with new_with_range, or maybe new_with_transform_near_far, idk

@jnhyatt
Copy link
Contributor

jnhyatt commented Jul 21, 2023

That seems like a good path forward to me

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 21, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11.1 milestone Jul 21, 2023
@opstic opstic closed this Jul 30, 2023
@opstic opstic deleted the render-with-negative-z branch July 30, 2023 16:11
@opstic opstic restored the render-with-negative-z branch July 30, 2023 16:11
@opstic opstic deleted the render-with-negative-z branch July 30, 2023 16:11
@mockersf mockersf removed this from the 0.11.1 milestone Jul 30, 2023
@opstic
Copy link
Contributor Author

opstic commented Jul 30, 2023 •
edited
Loading

Closed in favor of #9310.

@Selene-Amanita Selene-Amanita mentioned this pull request Jul 30, 2023
Merged
@Selene-Amanita Selene-Amanita mentioned this pull request Aug 22, 2023
Merged
@dmlary dmlary mentioned this pull request Sep 24, 2023
Closed
@Azorlogh Azorlogh mentioned this pull request Sep 7, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
@Azorlogh @dmlary @janhohenheim
Split OrthographicProjection::default into 2d & 3d (Adopted) (#15073)
82e416d 
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 <[email protected]>
Co-authored-by: Jan Hohenheim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sprites not being rendered with translation.z < 0.0
5 participants
@opstic @jnhyatt @Selene-Amanita @alice-i-cecile @mockersf