Share tonemapper code between the Forward+ and Mobile renderers.#114149
Share tonemapper code between the Forward+ and Mobile renderers.#114149DarioSamo wants to merge 1 commit into
Conversation
| return textureLod(source_sampler, color, 0.0).rgb; | ||
| } | ||
|
|
||
| vec3 apply_bcs(vec3 color, vec3 bcs, sampler2D source_sampler_lut_1d, sampler3D source_sampler_lut_3d, bool use_color_correction, bool use_color_correction_lut_1d, bool convert_to_srgb) { |
There was a problem hiding this comment.
| vec3 apply_bcs(vec3 color, vec3 bcs, sampler2D source_sampler_lut_1d, sampler3D source_sampler_lut_3d, bool use_color_correction, bool use_color_correction_lut_1d, bool convert_to_srgb) { | |
| // color must be linear-encoded. The returned value will be encoded with the | |
| // nonlinear sRGB transfer function if convert_to_sRGB is true. | |
| vec3 apply_bcs_and_srgb(vec3 color, vec3 bcs, sampler2D source_sampler_lut_1d, sampler3D source_sampler_lut_3d, bool use_color_correction, bool use_color_correction_lut_1d, bool convert_to_srgb) { |
There was a problem hiding this comment.
Thanks for doing this! It's something I knew would be really beneficial, so I'm glad to see you've done the work.
I think the name and commit message of this PR should instead be "Use specialization constants in Forward+ tonemap shader." instead. I think most people would see the part about sharing code as only a simple side effect of this.
I've made a number of comments about minor code cleanup stuff. The most important suggestions that I believe must be applied are the one I previously made and the comments in tonemapper_inc.glsl. The rest are minor and if you disagree with the value of any of those changes, you're welcome to ignore them.
Regarding the code, I don't see any other issues. Probably good to get another set of eyes and some more testing, but I've finished my review of this PR now.
| #ifndef SUBPASS | ||
| #include "fxaa_inc.glsl" | ||
| #endif // !SUBPASS | ||
|
|
||
| #include "../tonemapper_inc.glsl" |
There was a problem hiding this comment.
Using the same ordering as tonemap.glsl makes it easier to diff these two files and keep them in sync.
| #ifndef SUBPASS | |
| #include "fxaa_inc.glsl" | |
| #endif // !SUBPASS | |
| #include "../tonemapper_inc.glsl" | |
| #include "../tonemapper_inc.glsl" | |
| #ifndef SUBPASS | |
| #include "fxaa_inc.glsl" | |
| #endif // !SUBPASS |
There was a problem hiding this comment.
With four uint pads and four float near the end, it should be possible to rearrange these to not need any padding. This might no longer be the case when HDR output is merged, but when reviewing this PR in isolation, I figure it's worth mentioning.
| uint flags; | ||
| uint pad1; | ||
|
|
||
| vec2 pixel_size; |
There was a problem hiding this comment.
Not related to this PR, but while we're reworking this... Using the same name as tonemap_mobile.glsl makes it easier to diff these two files and keep them in sync.
| vec2 pixel_size; | |
| vec2 src_pixel_size; |
This is a minor suggestion that requires touching a bunch of other places where this name is used. It's fine if you'd prefer not to apply this suggestion.
| color.rgb = apply_bcs(color.rgb, params.bcs, source_color_correction_1d, source_color_correction_3d, use_color_correction, use_color_correction_lut_1d, convert_to_srgb); | ||
| } else if (convert_to_srgb) { | ||
| color.rgb = linear_to_srgb(color.rgb); // Regular linear -> SRGB conversion. | ||
| // Regular linear -> SRGB conversion. |
There was a problem hiding this comment.
Minor suggestion not related to this PR, but this comment is not needed and doesn't exist in tonemap.glsl.
| // Regular linear -> SRGB conversion. |
| @@ -0,0 +1,270 @@ | |||
| // Tonemapping routines are used by both the single pass tonemapper and the Forward shader when opting to do tonemapping before blending. | |||
There was a problem hiding this comment.
Is this comment specific to the older PR? I don't think it makes sense in this PR.
| // When using color correction and convert_to_srgb is false, there | ||
| // is no need to convert back to linear because the color correction | ||
| // texture sampling does this for us. | ||
| if (use_color_correction_lut_1d) { | ||
| color.rgb = apply_color_correction_1d(color.rgb, source_sampler_lut_1d); | ||
| } else { | ||
| color.rgb = apply_color_correction_3d(color.rgb, source_sampler_lut_3d); | ||
| } |
There was a problem hiding this comment.
This comment was originally placed after colour correction is applied because it is referring to the state of things after colour correction is applied and, importantly, has nothing to do with the state before colour correction is applied. To make this more clear, I have adjusted the wording to say "has already done this for us." in the following suggestion that moves this comment back to its original placement:
| // When using color correction and convert_to_srgb is false, there | |
| // is no need to convert back to linear because the color correction | |
| // texture sampling does this for us. | |
| if (use_color_correction_lut_1d) { | |
| color.rgb = apply_color_correction_1d(color.rgb, source_sampler_lut_1d); | |
| } else { | |
| color.rgb = apply_color_correction_3d(color.rgb, source_sampler_lut_3d); | |
| } | |
| if (use_color_correction_lut_1d) { | |
| color.rgb = apply_color_correction_1d(color.rgb, source_sampler_lut_1d); | |
| } else { | |
| color.rgb = apply_color_correction_3d(color.rgb, source_sampler_lut_3d); | |
| } | |
| // When using color correction and convert_to_srgb is false, there | |
| // is no need to convert back to linear because the color correction | |
| // texture sampling has already done this for us. |
Splitting off #113853.
While developing that PR, it was deemed to be useful to move the implementation of the tone mappers between both renderers to a shared file. However, since the linked PR is still a bit up for debate as it introduces a small error, it'd be best to separate the effort that was done for it into its own PR.
This PR also introduces specialization constants for Forward+'s tone mapper, which should help reduce the size of the final pipeline significantly.