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

[Merged by Bors] - Add the possibility to create custom 2d orthographic cameras #4048

Closed
wants to merge 1 commit into from

Conversation

nsarlin
Copy link
Contributor

@nsarlin nsarlin commented Feb 26, 2022

Objective

  • The OrthographicCameraBundle constructor for 2d cameras uses a hardcoded value for Z position and scale of the camera. It could be useful to be able to customize these values.

Solution

  • Add a new constructor custom_2d that takes far (Z position) and scale as parameters. The default constructor new_2d uses this constructor with far = 1000.0 and scale = 1.0.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 26, 2022
Self::custom_2d(1000.0, 1.0)
}

/// Create an orthographic projection camera with a custom Z position and scale.
Copy link
Member

Choose a reason for hiding this comment

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

Might want to add some documentation indicating that the camera will be placed at a z-position of far - 0.1 looking toward the world origin (like in the documentation for new_2d()).

@jakobhellermann
Copy link
Contributor

Since you can already express this with

let bundle = OrthographicCameraBundle {
  orthographic_projection: OrphographicProjection {
    far: 1000.0,
    scale: 1.0,
    depth_calculation: DepthCalculation::ZDifference,
    ..Default::default()
  },
 ..OrthographicCameraBundle::new_2d(),
};

I wonder whether having a custom contructor is worth it. Usually in bevy the recommended way to configure bundles is by overriding some fields and extending ..Default::default() or ..OrthographicCameraBundle::new_2d(), like there is no PbrBundle::new(mesh, material).

When you have a custom contructor you need to decide between having a very long new_with_far_scale_depth_calculation_scaling_mode(_: f32, _: f32, _. DepthCalculation, _: ScalingMode) or having a constructor for the common case, but then when someone wants to change the scaling mode they need to use the other pattern anyways.

@nsarlin
Copy link
Contributor Author

nsarlin commented Feb 26, 2022

Since you can already express this with

let bundle = OrthographicCameraBundle {
  orthographic_projection: OrphographicProjection {
    far: 1000.0,
    scale: 1.0,
    depth_calculation: DepthCalculation::ZDifference,
    ..Default::default()
  },
 ..OrthographicCameraBundle::new_2d(),
};

I don't think this would work in that case because other components of the bundle would still be set with far = 1000.0. This argument is used in different places so I think it can be valuable to have a way to set it just once.

@jakobhellermann
Copy link
Contributor

Oh right, the value is used both in the Camera and OrthographicProjection. In that case I totally see the value.

I don't like the name custom_2d though because it gives no intuition about what the values mean. Maybe with_far_scale would be an option, although it sounds like it sets the "far scale". with_far_plane_and_scale?

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Feb 26, 2022
}

/// Create an orthographic projection camera with a custom Z position and scale.
pub fn custom_2d(far: f32, scale: f32) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

The other option for clarity here is that we could use custom CameraPlane::Far(f32) and CameraScale(f32) types for these.

A longer name is probably better, but I thought it was worth bringing up

@MrGVSV
Copy link
Member

MrGVSV commented Feb 26, 2022

Oh right, the value is used both in the Camera and OrthographicProjection. In that case I totally see the value.

I don't like the name custom_2d though because it gives no intuition about what the values mean. Maybe with_far_scale would be an option, although it sounds like it sets the "far scale". with_far_plane_and_scale?

Alternatively, we could just have it set the far plane since that's the tricky one. Users could then just mutate the scale after construction via camera.orthographic_projection.scale if they need to.

@nsarlin
Copy link
Contributor Author

nsarlin commented Feb 27, 2022

Alternatively, we could just have it set the far plane since that's the tricky one. Users could then just mutate the scale after construction via camera.orthographic_projection.scale if they need to.

That's a good point. Maybe with the name 2d_with_far new_2d_with_far to make it clear that it's a 2d camera ?

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 28, 2022
@mockersf
Copy link
Member

mockersf commented Mar 8, 2022

I'm not completely a fan of the fn new_2d_with_far(far: f32) -> Self method...
What would you think of having instead a fn with_far(mut self, far: f32) -> Self that would apply the given far to an existing OrthographicCameraBundle? So you would do OrthographicCameraBundle::new_2d().with_far(5000.0).
Not suggesting the change, it has drawbacks too like needing to duplicate some code.

@superdump
Copy link
Contributor

I'm not completely a fan of the fn new_2d_with_far(far: f32) -> Self method... What would you think of having instead a fn with_far(mut self, far: f32) -> Self that would apply the given far to an existing OrthographicCameraBundle? So you would do OrthographicCameraBundle::new_2d().with_far(5000.0). Not suggesting the change, it has drawbacks too like needing to duplicate some code.

I prefer the current approach. Otherwise I feel like for API consistency we should have ::new().as_2d().with_far(5000.0) but then for construction efficiency I'd probably want it to be a builder not building at every step.

@alice-i-cecile
Copy link
Member

but then for construction efficiency I'd probably want it to be a builder not building at every step.

This is probably a silly concern here; 99% of games will have exactly one camera built ever ;)

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Apr 25, 2022
# Objective

- The `OrthographicCameraBundle` constructor for 2d cameras uses a hardcoded value for Z position and scale of the camera. It could be useful to be able to customize these values.

## Solution

- Add a new constructor `custom_2d` that takes `far` (Z position) and `scale` as parameters. The default constructor `new_2d` uses this constructor with `far = 1000.0` and `scale = 1.0`.
@bors bors bot changed the title Add the possibility to create custom 2d orthographic cameras [Merged by Bors] - Add the possibility to create custom 2d orthographic cameras Apr 25, 2022
@bors bors bot closed this Apr 25, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
…ine#4048)

# Objective

- The `OrthographicCameraBundle` constructor for 2d cameras uses a hardcoded value for Z position and scale of the camera. It could be useful to be able to customize these values.

## Solution

- Add a new constructor `custom_2d` that takes `far` (Z position) and `scale` as parameters. The default constructor `new_2d` uses this constructor with `far = 1000.0` and `scale = 1.0`.
@nsarlin nsarlin deleted the custom_ortho_cam branch July 26, 2022 19:19
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…ine#4048)

# Objective

- The `OrthographicCameraBundle` constructor for 2d cameras uses a hardcoded value for Z position and scale of the camera. It could be useful to be able to customize these values.

## Solution

- Add a new constructor `custom_2d` that takes `far` (Z position) and `scale` as parameters. The default constructor `new_2d` uses this constructor with `far = 1000.0` and `scale = 1.0`.
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 simple quality-of-life change that makes Bevy easier to use 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.

None yet

6 participants