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

Remove redundant math in tonemapping. #9669

Merged
merged 1 commit into from
Sep 2, 2023
Merged

Remove redundant math in tonemapping. #9669

merged 1 commit into from
Sep 2, 2023

Conversation

DGriffin91
Copy link
Contributor

@DGriffin91 DGriffin91 commented Sep 2, 2023 •
edited
Loading

Objective

  • Tony McMapface has some math that cancels out.

Solution

  • Remove it.

@JMS55 JMS55 added the A-Rendering Drawing game state to the screen label Sep 2, 2023
@JMS55 JMS55 added this to the 0.12 milestone Sep 2, 2023
alice-i-cecile
alice-i-cecile approved these changes Sep 2, 2023
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

The math checks out.

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change labels Sep 2, 2023
@JMS55
Copy link
Contributor

JMS55 commented Sep 2, 2023

I'm not good enough at math to see this easily 😅, @DGriffin91 do you mind writing out the derivation for me?

@DGriffin91
Copy link
Contributor Author

DGriffin91 commented Sep 2, 2023 •
edited
Loading

I'm not good enough at math to see this easily 😅, @DGriffin91 do you mind writing out the derivation for me?

This impl doesn't have the extra math: https://github.com/h3r2tic/tony-mc-mapface/blob/main/shader/tony_mc_mapface.hlsl
And I tested the output and it was the same either way.

@alice-i-cecile alice-i-cecile 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 Sep 2, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 2, 2023
Merged via the queue into bevyengine:main with commit e88b6c8 Sep 2, 2023
3 checks passed
@DGriffin91 DGriffin91 deleted the simplify_tony branch September 3, 2023 08:29
@rparrett
Copy link
Contributor

rparrett commented Sep 4, 2023 •
edited
Loading

I have a feeling that the new version is more correct and the difference is too subtle for me to really make out, but the 3d examples are different before/after this PR. (testing on macos)

After:
lighting

Before:
lighting

Output from image-compare's "hybrid comparison"
lighting

edit: lighting seems more interesting.

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@DGriffin91
Remove redundant math in tonemapping. (bevyengine#9669)
2d6d9b3 
# Objective

- Tony McMapface has some math that cancels out.

## Solution

- Remove it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JMS55 JMS55 JMS55 approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@IceSentry IceSentry Awaiting requested review from IceSentry

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times 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
Milestone
0.12
Development

Successfully merging this pull request may close these issues.
4 participants
@DGriffin91 @JMS55 @rparrett @alice-i-cecile