Enable post process with full precision using SubViewport

[JonqsGames]

Fixes #54122

I was facing #54122 when trying to port a post effect i did in 3.4.4
This quick fix make the subviewport keep the DATA_FORMAT_R16G16B16A16_SFLOAT precision and disable tonemapping and linearization in the tonemapper shader.

The current implementation i did looks really bad but i'll try to clean it later.

• Introduce a keep_linear_3d checkbox in SubViewport parameters
• Pass the keep_linear_3d through SubViewports property to the render target
• Documentation (buffer format, disabled tonemapping...)

Edit:
I made a quick test project for this feature :
TestShaderPostProcess.zip
Each cube is using the same shader that has a float uniform that is put into the color value.
Cube value of main scene are (-1.0, 0.0, 0.5, 1.0, 50.0).
Post process is simply putting a random color based on retrieved color value.
Base result (both keep_linear and force_high_precision disabled)

Result with both flag enabled (Vulkan):

[clayjohn]

A few things to keep in mind as your work to clean this up:

1. Tonemapping should not be disabled for SubViewports. SubViewports should have an option to keep 3D in linear space (in 3.x this option was called keep_3D_linear and was exposed as a checkbox on Viewports)
2. The setting should not be exposed as a parameter to render_scene it should be a property of SubViewports (internally a property of RenderingTarget).
3. We should keep the default behaviour the same. Right now we keep 2D operations in sRGB space with a RGBA8 buffer to improve performance and memory usage. Users shouldnt pay the extra cost of a higher precision format unless it is actually necessary.

[JonqsGames]

Made a first pass of refactor on that. Remove the whole is_sub_vp behavior and added a keep_linear attribute instead.
Also refactored the way color format is choosen for the RenderTarget, made the format available using setters and remove the hard coded part that was forcing the format.
I think the format could be simplified to one value (the actual buffer format) and all other format (srgb, image_format) could be chose based on it, this may be out of scope for this PR.

[lyuma]

At some point, it might be good to make tonemapping entirely optional for the linear case...my assumption is you will pass this linear texture into another viewport at some point. if it runs tonemapping twice, that might look wrong. Furthermore, it would mean we can save a full-screen render pass on these linear targets which would improve performance.

There's a lot of plumbing here. My biggest concern is how these color_format and color_format_srgb are passed around everywhere and pollute the API, especially when they are often the same. It seems like much of the code is written under the assumption that those two formats are different. in the case of SFLOAT, from what I can tell, _update_render_target is still creating the alias for "color_format_srgb" despite it being the same format.

[ I'm talking about this code:

              tex->rd_texture = RD::get_singleton()->texture_create_shared(view, rt->color);
                if (rt->color_format_srgb != RD::DATA_FORMAT_MAX) {
                        view.format_override = rt->color_format_srgb;
                        tex->rd_texture_srgb = RD::get_singleton()->texture_create_shared(view, rt->color);
                }

]
since color_format_srgb is not RD::DATA_FORMAT_MAX in the linear case. (and if it were, that might cause other bugs)

perhaps it would be better to move the linear flag here, and keep the current code otherwise. that way we wouldn't need those color format functions and all the duplicate code that picks both formats.

Overall, a lot of this change is plumbing. I'm not a rendering team lead, but the main problem I see with it is from making the color format code more confusing.

(there's also a question about do we really need another 128 bits of push constants for a single boolean? Well, there are already many other booleans being passed in this way, so I guess it's just an optimization job for another day.

[JonqsGames]

@lyuma For Tonemapping in this case i left the implementation as is. Linear is not doing anything.
Regarding the color_format, the change introduce here are needed because the user now has control over the buffer format in the editor (By the keep_linear checkbox). In my opinion user should have a total control on the RenderTarget buffer format for viewports but i think this belong to another PR and it needs to be thinked globaly. As @BastiaanOlij was saying, at the moment the whole viewport implementation asssume that it will be rendered in screen context so there is some shortcuts taken.
Exposing the color_format as any parameter in RenderTarget seems more logical to me and it also make the shortcuts more visible for the future refactoring.
I agree on the extra padding but i don't think it could be done another way. The tonemap.glsl shader is doing a lot more than tonemapping and several parameter could be factorize to different set of uniform and padding could be avoided but i think this belong to another PR.

[clayjohn]

Great work! You are getting much closer to a working implementation.

Now is the time for us to solidify exactly what the API should look like as I can see you and I have differing ideas.

My thinking was to just expose two booleans keep_3d_linear (2D is always sRGB), and force_high_precision.

keep_3d_linear would be assigned to the render_target and would disable linear->sRGB in the tonemapper. You have this nearly implemented now and it should be fine.

For force_high_precision my idea was that it would also get assigned to the render_target and then during allocation, the rendertarget would select between two formats based on what force_high_precision is set to.

In my opinion, the above is about as simple as we can make this feature (it also worked similarly in 3.x).

However, I can see you are going in a slightly different direction, one which would provide the user with a little more flexibility by allowing them to override the default precision. I like this idea as well, and it aligns with our goal of giving users more control in 4.0.

Accordingly, if you want to expose the ability to set the color format directly I would do it as follows:

1. Still have keep_3d_linear (this is needed in any case)
2. do not create force_high_precision instead create an override_color_format variable in Viewport which would call render_target_set_override_color_format (the same as your current render_target_set_color_format). Then in update_render_target you check and see if override_color_format is used, if it is, then you set color_format, color_format_srgb, and image_format based on override_color_format, if not, you just use the default.
3. render_target would of course need a boolean use_override_color_format which is set to true if render_target_set_override_color_format is called with an argument that is not RD::DATA_FORMAT_R8G8B8A8_UNORM

[JonqsGames]

Thanks for the detailed feedbacks. Both review converge for the color_format and i agree. I'll change that behavior to use keep_precision if it's fine for now to have two checkbox for that post process usecase.
I chose to merge both behavior under the keep_linear because i thought the raw color doesn't make sense unless the buffer use full precision. Maybe i'm lacking some understanding on how the data is converted to RenderTarget expected color format.
To stay in scope of this PR, i'll limit my change to keep_precision toggle switching between the two scenario of color format. The idea of exposing can be done later if we want to give more control to the user.

[JonqsGames]

Did a second pass on that, removed the whole exposing color_format and added the force_high_precision in both viewport and render target. I also fixed the reported issues (the way keep linear is passed to shader, unrelated changes ...).
I'm facing an issue with my project on shader compilation. I'll double check everything to confirm that it's not related to this PR changes but i doubt it.
Edit: all good the issue was related to the way i use global shader variable.

[clayjohn]

Looks mostly good! I left a few comments on style changes and documentation changes. But it is very close to being done.

I think before merging, we should also have the settings working in the GLES3 renderer. It will work the exact same as in the RD renderer, so it should not be much more work. You can look at the GLES3 renderer in 3.x for the naming of the color format.

[JonqsGames]

I did the requested changes. The only thing left is the behavior in edior for GLES3. I think this is due to other issue not related to this PR.

[lyuma]

I might be inclined to agree that this needn't wait for GLES3. My guess is that there is not any work needed for GLES3. Note that GLES3 is a low-end renderer (much like GLES2 was in Godot 3.x), so it likely uses gamma rendering in non-HDR
(in Godot shader terms, I would expect OUTPUT_IS_SRGB to be true in GLES3 when using Godot 4.x).

It might be good to have an explicit half- or full-precision float render target mode for GLES3 (useful for calculations / simulation viewports), but that should be proposed and then added as a separate PR if there is demand, I think.

[JonqsGames]

Actually the feature works in GLES3 with the same behavior than in Vulkan. I just noticed that the behavior in editor is not the same. When changing the flags, the viewport does not update properly sometimes. I also had the case where the keep_linear flag was applied to all viewport/windows in the editor. But when reloading the scene or running it everything behave as expected.

[clayjohn]

My apologies, I missed a few things on my last review. After this change it should be good to go!

[clayjohn]

I'm happy with this implementation. Would be nice for @BastiaanOlij and @reduz to weigh in before merging as this is a big change.

[BastiaanOlij]

As for making it work as we did in Godot 3 this seems sound, but I've never really found we're on the right track with this to begin with. Possibly something to discuss in more detail in a render meeting when we have more time.

The core issue here are the assumptions that:
A) we're always combining 2D and 3D
B) we're always outputting to screen
C) we're always outputting color data

None of these assumptions are true and how we render our content differs greatly. Even 2D output can be valid input as a texture used in 3D where colors are not sRGB or where we even don't want RGBA8 as our texture format. Think of rendering noise or height map data.

Now in Vulkan we do have more control over how we treat color data both when writing out and when reading but I think we need far more control over configuring the data type of our (sub)viewport and what sort of rendering we're doing.

This comes back down to my issue with having RenderTarget and RenderBuffers as 2D and 3D constructs within the render server and believing this is the wrong approach. Yes if a viewport is used for final output and we're doing both 3D and 2D rendering, we need to render to a high precision 3D buffer first, then blit and tonemap into our SRGBA8 buffer, then render 2D, as we do now. But we have other variations possible too especially when we're producing an intermediate result.

Anyway, enough ranting :)

[akien-mga]

As a side note, the commits will need to be squashed before merging (see PR workflow).

[reduz]

I don't think this is the right way to tackle this problem to be honest.

• After tonemap, whathever it happens can' t be considered post processing.
• Post processing needs access to data before the tonemap, including depth, normal, etc. buffers, and it should be able to allocate textures/buffers to do this in a flexible way.
• I think we need a way to have viewports in HDR, but setting them to linear is not the way and a hack. Instead, what needs to be done is add proper HDR support for 2D.

I think this is stuff that we need to discuss properly and that will most likely be done for 4.1, the time to add this sort of features has run out for 4.0.

[JonqsGames]

Totally agree on the fact that this a hack. I still think that it's better than nothing. The fact that this hack was (kind of) available in 3.x version may be an issue for some people.
Giving global control over the buffer formats and default processing steps for viewports would be better but require a lot of changes and a global plan to keep things coherents.
On the state of viewport now, it's handy to have them but it can be misleading for new user to understand the many restriction around them. In my opinion along with some documentation this hack can unlock some usage on post processing that will help plan the next steps.

[clayjohn]

@JonqsGames reduz and I discussed his view further in the rendering channel in Rocketchat. The main problems with the current approach (that you and I decided on) is that it conflicts with eventually adding proper HDR support for HDR monitors. For that, we need to add a viewport option that tonemaps 3D into the proper color space and then renders 2D in the proper color space (most likely linear with a tonemapping operator at the end). This current design would end up having to be removed leading to breaking changes.

Instead what reduz suggests (and I now agree) is the best approach is to expose the 3D rendering buffer through the viewport API so users can select whether they want their final texture to be the 2D render target, or the 3D render buffer. This has the added benefit that the entire 2D rendering pipeline can be skipped if the user just wants the 3D render buffer (the tonemap shader won't even run).

We already do something similar for XR with the use_xr property. Reduz suggests that we expand that into an enum:

enum ViewportMode {
    2D_AND_3D,
    3D,
    XR
};

stale

[clayjohn]

Superseded by #70970