Skip to content

Split Camera.hdr out into a new component #18873

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Apr 18, 2025

Objective

  • Simplify Camera initialization
  • allow effects to require HDR

Solution

  • Split out Camera.hdr into a marker Hdr component

Testing

  • ran bloom_3d example

Showcase

// before
commands.spawn((
  Camera3d
  Camera {
    hdr: true
    ..Default::default()
  }
))

// after
commands.spawn((Camera3d, Hdr));

// other rendering components can require that the camera enables hdr!
// currently implemented for Bloom, AutoExposure, and Atmosphere.
#[require(Hdr)]
pub struct Bloom;

@ecoskey ecoskey 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 labels Apr 18, 2025
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@JMS55
Copy link
Contributor

JMS55 commented Apr 19, 2025

I'm very much in favor of this change, but:
A) I think we should rename the component to something better
B) We should call out explicitly that this is internal render targets only, and not for display output, which is what people usually think of as HDR
C) We should really make this the default behavior

Copy link
Contributor

@Henauxg Henauxg left a comment

Choose a reason for hiding this comment

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

About the rename, now that it's a type and not a field, it could be something like HighDynamicRange instead of Hdr.
I'm not very aware about the "internal render targets only" details though, so maybe it needs an additional information in the name to carry that.

@Henauxg
Copy link
Contributor

Henauxg commented Apr 20, 2025

The blend_mode and transmission examples also seem like they need to be updated.

@IceSentry
Copy link
Contributor

I'm very much in favor of this change, but:
A) I think we should rename the component to something better
B) We should call out explicitly that this is internal render targets only, and not for display output, which is what people usually think of as HDR
C) We should really make this the default behavior

I don't think A or C should happen in this PR. I'd prefer to keep this PR without too many behaviour changes. We can have a discussion about renaming it later.

For B, I think the current docs already do that?

@IceSentry
Copy link
Contributor

I agree with the general direction of this PR. I haven't reviewed the changes yet though.

@hukasu
Copy link
Contributor

hukasu commented Apr 23, 2025

I'm not very aware about the "internal render targets only" details though

from what i understand from being on the rendering channel for far too long is that each camera has a texture that they render to, and later it gets transferred to the swapchain texture, the camera's texture is Hdr but the swapchain's texture is not

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
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants