-
Notifications
You must be signed in to change notification settings - Fork 61
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
Implement adaptive exposure #1559
base: master
Are you sure you want to change the base?
Conversation
I'll see if I can add a non-compute path later. The compute one can also easily give histogram information for Unvanquished/Unvanquished#221. |
b1ef5f8
to
b7277fe
Compare
I've put some of these changes into #1563 to make this pr lighter. |
Hmm, I might've broken something with the more recent push, the changes were pretty mild when I was testing it. |
b7277fe
to
01ab8df
Compare
Rebased + fixed some issues. That should hopefully fix the drastic exposure changes as seen on the above screenshot too. |
01ab8df
to
d7b2a06
Compare
LGTM |
d7b2a06
to
90becba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my laptop's Nvidia card, taking a screenshot goes from no noticeable delay to a 2-second delay when adaptive exposure is enabled.
Any plans to smooth out the exposure changes over time, like how the eye adapts to light maybe? For now the exposure variation seems too large and rapid to be really usable; all world surfaces are flickering when you move.
Doors feel kind of buggy in general. There is often a bright flash when the door is opening. But maybe that's just the same thing as the preceding paragraph.
#if defined(HAVE_ARB_explicit_uniform_location) && defined(HAVE_ARB_shader_atomic_counters) | ||
if( u_TonemapAdaptiveExposure ) { | ||
const float l = GetAverageLuminance( luminanceU ) - 8; | ||
color.rgb *= clamp( 0.18f / exp2( l * 0.8f + 0.1f ), 0.0f, 2.0f ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the lower limit shouldn't be allowed to go 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't, the lowest it can go is ~0.0016.
I miscalculated the max value for luminosity sum, so it does overflow sometimes. Will add a fix later. |
Also the instant exposure adjustments mess up other effects that are supposed to cause rapid changes in brightness. The flickering of the firelight in |
Hmm, it might be best to improve the screenshotting in some way. I don't think there's much that can be done with this from the adaptive exposure side.
Yeah, I plan to add some exponential smoothing there.
It might be due to the overflow too.
Yeah, smoothing it over time should make it better, maybe some value tweaking too. |
c5cb774
to
fb62668
Compare
41564ef
to
f251a5e
Compare
That looks nice but yes the feature should remain disabled by default until some delay / average on some time is implemented, because of the flicking when moving. |
f251a5e
to
d1f2c9c
Compare
GLUniform4f( shader, "u_TonemapParms2", true ) { | ||
} | ||
|
||
void SetUniform_TonemapParms2( vec4_t tonemapParms2 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a more readable interface. Arguments with named float(s) instead of vec4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps so. I'd just made it as an "extension" to u_TonemapParms
that was already there.
gl_luminanceReductionShader->SetUniform_ViewWidth( width ); | ||
gl_luminanceReductionShader->SetUniform_ViewHeight( height ); | ||
vec4_t parms { log2f( r_toneMappingHDRMax.Get() ) }; | ||
parms[1] = UINT32_MAX / ( width * height * ( uint32_t( parms[0] ) + 8 ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is UINT32_MAX used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the maximum value that can be held in an atomic_uint
, so it ensures that there will be no overflow.
71adf84
to
8e25a56
Compare
Add a compute shader that will compute the geometric mean of scene luminance, then map it to an exposure curve in the `cameraEffects` shader. This is controlled by `r_tonemapAdaptiveExposure`.
682037b
to
86e7478
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get a GLSL compile error if I do set r_arb_shader_atomic_counter_ops 0
.
#if defined(SUBGROUP_ATOMIC) | ||
const float luminanceSum = subgroupInclusiveAdd( luminance ); | ||
|
||
if( subgroupElect() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed spaces and tabs here
Requires #1550 and #1558
Implements #29
This builds up on #1550 by adding automatic exposure based on scene brightness.
When
r_tonemapAdaptiveExposure
is enabled, a compute shader gets the geometric mean of luminance before tonemapping. Then in the tonemapper this value is mapped to a curve with coefficients chosen to try to avoid sudden changes and too high/too low exposure values, and multiplied with the HDR colour.An example of this in practice: https://users.unvanquished.net/~reaper/adaptiveLighting/adaptiveLightingGlobal.mp4
Also moved the material stuff from
common_cp
tomaterial_cp
. This wasn't an issue prior because only the material system used compute shaders, but now this would result in shaders failing to compile ifr_materialSystem
is disabled (and using macros would result in unnecessary shader re-compilations).