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

Add tonemapping switch to bloom 2d example #17789

Conversation

hukasu
Copy link
Contributor

@hukasu hukasu commented Feb 11, 2025

Objective

Allow switching through available Tonemapping algorithms on bloom_2d example to compare between them

Solution

Add a resource to bloom_2d that holds current tonemapping algorithm, a method to get the next one, and a check of key press to make the switch

Testing

Ran bloom_2d example with modified code

Showcase

https://github.com/user-attachments/assets/920b2d6a-b237-4b19-be9d-9b651b4dc913
Note: Sprite flashing is already described in #17763

examples/2d/bloom_2d.rs Outdated Show resolved Hide resolved
examples/2d/bloom_2d.rs Outdated Show resolved Hide resolved
@rparrett rparrett added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples labels Feb 11, 2025
@alice-i-cecile alice-i-cecile added C-Testing A change that impacts how we test Bevy or how users test their apps X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 11, 2025
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Nice, this seems useful beyond the debugging you were using it for. I played with this once a while back when I was trying to figure out if bloom would work for one of my games.

I don't want to nitpick this to death. I could follow up with the other suggestions later, but we should fix the outdated comment.

examples/2d/bloom_2d.rs Outdated Show resolved Hide resolved
examples/2d/bloom_2d.rs Outdated Show resolved Hide resolved
examples/2d/bloom_2d.rs Outdated Show resolved Hide resolved
@JMS55
Copy link
Contributor

JMS55 commented Feb 11, 2025

I'm not a huge fan of this, personally. The example should be for demonstrating bloom, not tonemapping.

@hukasu
Copy link
Contributor Author

hukasu commented Feb 11, 2025

I'm not a huge fan of this, personally. The example should be for demonstrating bloom, not tonemapping.

but tonemapping affects the appearance of bloom, wouldn't it be important for the user to see the difference?

@rparrett rparrett removed the X-Uncontroversial This work is generally agreed upon label Feb 11, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 11, 2025
@alice-i-cecile alice-i-cecile 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 Feb 11, 2025
Merged via the queue into bevyengine:main with commit fc98664 Feb 11, 2025
30 checks passed
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-Examples An addition or correction to our examples C-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants