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 Hable (Uncharted 2) and ACES filmic tone mapping #418

Merged
merged 2 commits into from Feb 27, 2017

Conversation

Projects
None yet
3 participants
@leMaik
Contributor

leMaik commented Feb 19, 2017

(I hit the enter key before writing a description, sorry!)

I know that there are plugins, but these two new tone mappings seem to be a useful addition to the core, as there already is the tonemapping operator 1.

Sometimes, the scene looks pretty grayish with gamma correction and even with the existing tone mapping operator

Gamma correction:
markus-100-gamma

Tone mapping op. 1:
markus-100-tonemap1

Hable's tonemapping looks really great, it preserves some colors better than the above modes:
markus-100-hable

And while we're at it, the ACES tone mapping curve aims to give the scenes a more filmic look, although it requires careful adjusting of the sunlight and exposure (my scene looks a little too blueish, but the contrast and color intensity is much better):
markus-100-aces
By the way, ACES is the default tonemap in Unreal Engine 4.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 19, 2017

Codecov Report

Merging #418 into master will decrease coverage by -0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #418      +/-   ##
============================================
- Coverage     11.86%   11.84%   -0.03%     
- Complexity        0       72      +72     
============================================
  Files           266      266              
  Lines         17595    17626      +31     
  Branches       2127     1890     -237     
============================================
  Hits           2087     2087              
- Misses        15502    15533      +31     
  Partials          6        6
Impacted Files Coverage Δ Complexity Δ
...src/java/se/llbit/chunky/renderer/Postprocess.java 0% <ø> (ø) 0 <ø> (ø)
...src/java/se/llbit/chunky/renderer/scene/Scene.java 0% <ø> (ø) 0 <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fe032b...50da860. Read the comment docs.

codecov-io commented Feb 19, 2017

Codecov Report

Merging #418 into master will decrease coverage by -0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #418      +/-   ##
============================================
- Coverage     11.86%   11.84%   -0.03%     
- Complexity        0       72      +72     
============================================
  Files           266      266              
  Lines         17595    17626      +31     
  Branches       2127     1890     -237     
============================================
  Hits           2087     2087              
- Misses        15502    15533      +31     
  Partials          6        6
Impacted Files Coverage Δ Complexity Δ
...src/java/se/llbit/chunky/renderer/Postprocess.java 0% <ø> (ø) 0 <ø> (ø)
...src/java/se/llbit/chunky/renderer/scene/Scene.java 0% <ø> (ø) 0 <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fe032b...50da860. Read the comment docs.

@llbit

I want to try running this before merging, but it looks good.

Show outdated Hide outdated chunky/src/java/se/llbit/chunky/renderer/Postprocess.java
@@ -36,6 +36,12 @@
@Override public String toString() {
return "Tonemap operator 1";
}
},
TONEMAP2 {
@Override public String toString() { return "ACES filmic tone mapping"; }

This comment has been minimized.

@llbit

llbit Feb 20, 2017

Owner

I prefer to not use this single-line style of method declaration. It's a personal preference, but it's better to keep everything consistent. See for example the other toString() methods in this file.

@llbit

llbit Feb 20, 2017

Owner

I prefer to not use this single-line style of method declaration. It's a personal preference, but it's better to keep everything consistent. See for example the other toString() methods in this file.

This comment has been minimized.

@leMaik

leMaik Feb 20, 2017

Contributor

Whoops... My IDE collapsed the other blocks so they looked like one-liners.
I (and my IDE) would prefer to also have the @Override in a separate line, and I've never really seen it on the same line as the method declaration, what do you think?

@leMaik

leMaik Feb 20, 2017

Contributor

Whoops... My IDE collapsed the other blocks so they looked like one-liners.
I (and my IDE) would prefer to also have the @Override in a separate line, and I've never really seen it on the same line as the method declaration, what do you think?

This comment has been minimized.

@llbit

llbit Feb 20, 2017

Owner

I have no preference about annotation placements - I usually have them on the same line if it fits.

@llbit

llbit Feb 20, 2017

Owner

I have no preference about annotation placements - I usually have them on the same line if it fits.

Show outdated Hide outdated chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java
float hD = 0.20f;
float hE = 0.02f;
float hF = 0.30f;
r *= 16; // manually adjust exposure to approx. match other post-processing modes

This comment has been minimized.

@llbit

llbit Feb 20, 2017

Owner

I looked at the linked blog, and they seem to use the same exposure adjustment constant. Did you try adjusting it?

Generally I prefer proper sentences in comments nowadays, i.e. start with capital letter and end with a period.

@llbit

llbit Feb 20, 2017

Owner

I looked at the linked blog, and they seem to use the same exposure adjustment constant. Did you try adjusting it?

Generally I prefer proper sentences in comments nowadays, i.e. start with capital letter and end with a period.

This comment has been minimized.

@leMaik

leMaik Feb 20, 2017

Contributor

Indeed, they use the same exposure adjustment. I should call it "hardcoded" instead of "manual" exposure adjustment. It's a little weird because they also use it for the tonemap1 operator, but you don't and it still looks fine, while Hable's tonemap looks far too dark without adjusting the exposure.

Regarding the sentence: Agree, will fix that.

@leMaik

leMaik Feb 20, 2017

Contributor

Indeed, they use the same exposure adjustment. I should call it "hardcoded" instead of "manual" exposure adjustment. It's a little weird because they also use it for the tonemap1 operator, but you don't and it still looks fine, while Hable's tonemap looks far too dark without adjusting the exposure.

Regarding the sentence: Agree, will fix that.

@leMaik leMaik changed the title from Add able (Uncharted 2) and ACES filmic tone mapping to Add Hable (Uncharted 2) and ACES filmic tone mapping Feb 20, 2017

@llbit

llbit approved these changes Feb 27, 2017

The ACES tonemapper seems a bit extreme, but I like the Hable one. I guess it depends on the situation, and it's nice to have more variety.

@llbit llbit merged commit 34af2e7 into llbit:master Feb 27, 2017

2 of 4 checks passed

codecov/patch 0% of diff hit (target 11.86%)
Details
codecov/project 11.84% (-0.03%) compared to 5fe032b
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

leMaik added a commit to leMaik/chunky that referenced this pull request Jul 24, 2017

Add new post-processors to the changelog
They were introduced a month after the 1.4.2 release by #418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment