Tonemapper options #111
Tonemapper options #111
Conversation
Process imgui input before beginning imgui frame (ocornut/imgui#3575)
kabergstrom
force-pushed
the
tonemapping
branch
from
38be6e7 to
c0af53b
Compare
demo/shaders/glsl/bloom_combine.frag
Outdated
| layout (location = 0) in vec2 inUV; | ||
|
|
||
| layout (location = 0) out vec4 out_sdr; | ||
|
|
||
| // The code for ACESFitted was originally written by Stephen Hill (@self_shadow), who deserves all |
There was a problem hiding this comment.
Since this is a non-trivial amount of code, could we separate it to a separate .glsl? Also I think we should add a link to the original material
demo/shaders/glsl/bloom_combine.frag
Outdated
| void main() | ||
| { | ||
| vec4 color = texture(sampler2D(in_color, smp), inUV) + texture(sampler2D(in_blur, smp), inUV); | ||
|
|
||
| // tonemapping.. TODO: implement auto-exposure | ||
| vec3 mapped = color.rgb / (color.rgb + vec3(1.0)); | ||
| out_sdr = vec4(mapped, color.a); | ||
| if (config.tonemapper_type == 1) { |
There was a problem hiding this comment.
Is it possible to use constants here? Will make it more readable.
We should also put comments on the rust/glsl code saying "these indexes must be kept in sync with X"
Long term, it would be great if we could export glsl constants as rust code so that we don't have to define them more than once
There was a problem hiding this comment.
Do you think all of these modes are useful? (I really don't know much about tone mapping!)
There was a problem hiding this comment.
I'll make them constants.
Whether they're useful or not, I'm not sure. I bet all of them have been used in some released game. I mainly wanted to set up a testbed for showing the effects of various tonemapping operators.
| @@ -77,6 +78,17 @@ pub(super) fn bloom_combine_pass( | |||
| shaders::bloom_combine_frag::DescriptorSet0Args { | |||
| in_color: &sdr_image, | |||
| in_blur: &hdr_image, | |||
| config: &shaders::bloom_combine_frag::ConfigStd140 { | |||
| tonemapper_type: match render_options.tonemapper_type { | |||
| TonemapperType::None => 0, | |||
There was a problem hiding this comment.
Can we make this a c-style enum so that we don't need to do a match?
demo/src/lib.rs
Outdated
| @@ -80,6 +92,28 @@ impl RenderOptions { | |||
| .build(ui, &mut blur_pass_count); | |||
|
|
|||
| self.blur_pass_count = blur_pass_count as usize; | |||
| let tonemapper_names = [ | |||
There was a problem hiding this comment.
can these strings be a constant array that's right next to the enum? This will make it more obvious when the enum is changed to also update the array to be the correct order/same size. (We could add a to_string() function to the enum that indexes into that array).
Or make to_string() match like:
match value {
ToneMapperType::LogDerivative => "LogDerivative"
}Actually a to_string() match function by itself (instead of an array) seems like a good way to do it. Very readable and least error-prone
This adds two new tonemapper options, and two new visualization modes to bloom_combine.frag. The tonemapper can be selected in the RenderOptions menu.
Other fixes:
Fix issue with internal #![rustfmt::skip] attribute on nightly
Process imgui input before beginning imgui frame (ocornut/imgui#3575)