-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Rename ToneMapper to PostProcessor and add a 3D mode for post process viewport #70970
Conversation
e8de91a
to
8dc6dba
Compare
If anyone has time, it may be worth checking whether it's possible to save viewport data as OpenEXR with MovieWriter could then have OpenEXR output support (within a refactored MovieWriterPNGWAV), so that you can write HDR videos using Godot. I suppose doing this also require adding a project setting to change the root viewport's framebuffer mode, so that it can be done without having to use a script to set the property on the root viewport. |
I think for the most part I agree with this PR, I'm on the fence however on adding I do think it belongs there but it would be a change that should be relegated to 4.1 |
I agree, I would really love to see this change merged ASAP, but we are too close to release to merge something so large and potentially risky. Let's plan on evaluating shortly after we release stable |
d14f169
to
4c9c123
Compare
Just rebased to handle the latest change on luminance properly. |
6f7194b
to
c231dbe
Compare
That could work, do we have a way to mark properties as deprecated? That would be pretty neat.
This should ensure that even with saving both Then in a couple of builds we can retire |
And on that same line of thought, should we also retire |
@BastiaanOlij I already did the change for |
rt->color_format = GL_RGBA; | ||
rt->color_type = rt->is_transparent ? GL_UNSIGNED_BYTE : GL_UNSIGNED_INT_2_10_10_10_REV; | ||
rt->image_format = Image::FORMAT_RGBA8; | ||
if (rt->viewport_mode == RS::VIEWPORT_MODE_3D && config->framebuffer_float_supported) { |
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.
rt->color_internal_format = rt->is_transparent ? GL_RGBA16F : GL_RGB16F;
I think this is wrong, we should never be using GL_RGB16F on mobile devices.
Actually @clayjohn thats an interesting question, are you using GL_RGB16F
for rendering in OpenGL? Seeing we're targeting low end I don't think we should. We should always stick to GL_RGB10_A2
and do the same luminance multiplier workaround we've done on the mobile Vulkan renderer. Or if we do want to support GL_RGB16F
on desktop, make this is project setting of the OpenGL renderer.
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 think the intent was to handle that with framebuffer_float_supported
but the implementation may be problematic in the case you descrided.
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.
Yeah lets see what Clay has to say about it. It's definitely not something that is switched on transparency and the needed changes to run on GL_RGB10_A2
may be far more complex than what we're looking into right now.
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.
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.
The GL Compatibility renderer should always render to a 32bpp sRGB render target. The GL Compatibility assumes that rendering always takes place in sRGB space, for older mobile devices this is a very important optimization. In order to keep results consistent between devices, we stick with the limitations of the lowest common denominator. That way users can develop on desktop and expect that the results look the same on mobile
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 thought that checking for support of float framebuffer was done to force the sRGB on mobile with this limitation. My main usecase of the GL Compatibility is for Web so i did not test any mobile build.
@@ -3889,7 +3888,7 @@ void Viewport::_bind_methods() { | |||
ClassDB::bind_method(D_METHOD("get_vrs_texture"), &Viewport::get_vrs_texture); | |||
|
|||
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "disable_3d"), "set_disable_3d", "is_3d_disabled"); | |||
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "use_xr"), "set_use_xr", "is_using_xr"); | |||
ADD_PROPERTY(PropertyInfo(Variant::INT, "viewport_mode", PROPERTY_HINT_ENUM, "2D_AND_3D,3D,XR"), "set_viewport_mode", "get_viewport_mode"); |
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.
Here is where I mean we shouldn't remove ADD_PROPERTY(PropertyInfo(Variant::BOOL, "use_xr"), "set_use_xr", "is_using_xr");
We should keep it but have set_use_xr
and is_using_xr
be based on the value of viewport_mode
.
This will ensure that existing XR applications will keep working and not suddenly break.
In the long run we can remove it.
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.
You don't need to add a proxy property, you can handle this in _set()
and _get()
directly.
#ifndef DISABLE_DEPRECATED
bool Viewport::_set(const StringName &p_name, const Variant &p_value) {
if (p_name == "use_xr") {
set_viewport_mode(VIEWPORT_MODE_XR);
return true;
}
return false;
}
bool Viewport::_get(const StringName &p_name, Variant &r_ret) const {
if (p_name == "use_xr") {
r_ret = get_viewport_mode() == VIEWPORT_MODE_XR;
return true;
}
return false;
}
#endif
This preserves support for existing serialized use_xr
properties (not for using the old property name or methods from code however, but this is IMO OK at the beta stage).
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.
@JonqsGames or did you mean you already changed this but it's not yet pushed in?
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.
c231dbe958b52247afa151d006f552d8247f6913 was supposed to reenable the setter, i choose to not expose the property to enforce usage of the viewport mode after conversion.
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.
@akien-mga ah ok, didn't know that, yeah that's perfect.
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.
The compatibility methods suggested by Akien need to be added so this doesn't break backwards compatibility
rt->image_format = rt->is_transparent ? Image::FORMAT_RGBA8 : Image::FORMAT_RGB8; | ||
if (rt->viewport_mode == RS::VIEWPORT_MODE_3D) { | ||
// For 3D only viewport keep precision | ||
rt->color_format = RD::DATA_FORMAT_R16G16B16A16_SFLOAT; |
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.
So here the same thing, we should not hardcode RD::DATA_FORMAT_R16G16B16A16_SFLOAT
here. The mobile renderer does not use an RGBA16F buffer, those don't work well at all. We use RD::DATA_FORMAT_A2B10G10R10_UNORM_PACK32
buffers here with all color values divided by 2. That's going to be interesting for people who don't realize they have to multiply by 2 when accessing it (this is the luminance multiplier you see in various shaders).
You can call RendererSceneRenderRD::get_singleton()->_render_buffers_get_color_format()
to get the format you need to use here.
You can call RendererSceneRenderRD::get_singleton()->_render_buffers_get_luminance_multiplier()
to get the luminance multiplier if we want to write something that detects we're using a viewport like this.
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.
Thanks for the precision, i'm indeed a bit confused by how luminance is applied. I'll try to use the mentionned method to be more reliable.
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.
Basically the problem on mobile GPUs is that they suck when you're using a 64bit value to store color data in. 32bit formats are optimised and often have hardware compression to minimize bandwidth while loading/saving data from tile memory.
Hence we use RGB10_A2 but since this does not store a floating point value, but is rather a 10bit uint value that is normalized to 0.0 - 1.0
, we divide the color by 2 to extend this range to '0.0 - 2.0` and have some measure of higher dynamic range. We've still got more precision that when using 8bits so it prevents color banding unless you have very dark scenes that you're making brighter by exposure.
The 2.0 is a fixed ceiling so in very bright scenes you just loose the upper band.
Anyway, that is why we do it, the result is that all colors need to be multiplied by 2 when read from the texture to regain their original value.
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.
Note also that if we set transparent on, we do default back to RGBA8
as we're assuming tonemapping and sRGB conversion has been applied by now seeing RGB10_A2
only has 2 bits of transparency.
Using RGB10_A2
here really only makes sense if we're not applying post processing. That is the only scenario where using RGB8
would cause banding as it lacks the precision to store linear color data so I must admit I'm a little confused why we're even applying the check.
395fa48
to
ba03fc0
Compare
servers/rendering_server.cpp
Outdated
@@ -2183,6 +2183,7 @@ void RenderingServer::_bind_methods() { | |||
/* VIEWPORT */ | |||
|
|||
ClassDB::bind_method(D_METHOD("viewport_create"), &RenderingServer::viewport_create); | |||
ClassDB::bind_method(D_METHOD("viewport_set_use_xr", "viewport", "use_xr"), &RenderingServer::viewport_set_use_xr); |
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.
#70970 (comment)
This fixed the issue in Linux CI, the setter is exposed only at RenderingServer
level. I think we can remove the break_compatibility tag.
ba03fc0
to
3392520
Compare
I've been trying out this PR as part of porting my project from 3.5.2 and although the images it produces are shown as being in 16 bit linear floating point, the actual color data within the images seems to be limited. After Speaking to @JonqsGames I've put together two test projects, one for 3.5.2 and one based on this PR. I've tried to mirror the setup as much as possible between both of them. I've been clicking around for a few hours but It is very possible that I've configured the new viewports incorrectly, my apologies if so. Either way, hopefully this is help with testing! |
This is indeed what I expected. This pull request aims to provide an option to resolve this. |
@DigitallyTailored Thanks for the data, there is indeed an issue of banding on your test project. For the viewport setting it to |
Nice, I've tested the update locally as well and we have lots more color in there now, matching the output of Godot 3 I believe. The 2D output looks perfect: I wonder though, is it correct that we need to edit the 3D environment lighting to get brighter output/expected(?) for the 3D test? Just wanted to add, I'm trying this out with my project now where I also use the alpha value and it's also outputting as expected. |
Using full white ambient light was a way to have data that can be compared with the 2D output. (full white multiply value by one, before change it was the background color, a gray, that lowered a bit the output) |
I'm not sure if |
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.
My apologies for the slow review.
This PR is a great step in the right direction, but it still needs some work to be ready. Most of these issues have already been flagged by Bastiaan earlier, but I will summarize here:
- This PR inappropriately changes framebuffer formats in places it shouldnt. Setting the viewport mode shouldn't alter the framebuffer being used. The point of this mode is to expose the already existing HDR viewport from 3D rendering.
- The rename from tone_mapper to post_processor creates a lot of git noise that makes it hard to review the actual changes made by this PR. It is also misleading as the bulk of the post_process happens before the tonemapping shader is run. I would revert the rename.
- This PR doesn't seem to actually change which texture is returned by the Viewport. Instead the 3D buffer is still rendered into the 2D render target and that is what is exposed to the user. As a reminder, the suggestion from reduz was that Viewport mode 3D only would avoid using the 2D render target altogether. When accessing the texture of the Viewport, the 3D render buffer should be returned instead of the 2D render target. That way we don't need to make any changes to the 2D render target and users can still access the HDR texture used by 3D rendering.
rt->color_format = GL_RGBA; | ||
rt->color_type = rt->is_transparent ? GL_UNSIGNED_BYTE : GL_UNSIGNED_INT_2_10_10_10_REV; | ||
rt->image_format = Image::FORMAT_RGBA8; | ||
if (rt->viewport_mode == RS::VIEWPORT_MODE_3D && config->framebuffer_float_supported) { |
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.
The GL Compatibility renderer should always render to a 32bpp sRGB render target. The GL Compatibility assumes that rendering always takes place in sRGB space, for older mobile devices this is a very important optimization. In order to keep results consistent between devices, we stick with the limitations of the lowest common denominator. That way users can develop on desktop and expect that the results look the same on mobile
if (scene_data.use_linearize) { | ||
frag_color.rgb = linear_to_srgb(frag_color.rgb); | ||
} |
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.
The Gl Compatibility renderer renders in sRGB space, the linear->srgb conversion should never be disabled
@@ -3889,7 +3888,7 @@ void Viewport::_bind_methods() { | |||
ClassDB::bind_method(D_METHOD("get_vrs_texture"), &Viewport::get_vrs_texture); | |||
|
|||
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "disable_3d"), "set_disable_3d", "is_3d_disabled"); | |||
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "use_xr"), "set_use_xr", "is_using_xr"); | |||
ADD_PROPERTY(PropertyInfo(Variant::INT, "viewport_mode", PROPERTY_HINT_ENUM, "2D_AND_3D,3D,XR"), "set_viewport_mode", "get_viewport_mode"); |
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.
The compatibility methods suggested by Akien need to be added so this doesn't break backwards compatibility
/* tone_mapper.cpp */ | ||
/* post_processor.cpp */ |
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 would prefer not to rename the tone_mapper. While the new name is more accurate, it creates a lot of noise in the git history for no tangible benefit
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.
The main reason is that it took me a while to understand that the tonemapper was doing most of the work before serving the image. There is so much more than tonemapping in the GLSL shader but i can revert it. (I may restart from scratch again because reverting that will be a pain)
RENDER_TIMESTAMP("Tonemap"); | ||
RD::get_singleton()->draw_command_begin_label("Tonemap"); | ||
RENDER_TIMESTAMP("PostProcess"); | ||
RD::get_singleton()->draw_command_begin_label("PostProcess"); |
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.
These changes should be reverted. The "Tonemap" pass only captures the very final stage of the post process pipeline. Renaming it to PostProcess will be misleading as most of the cost of post processing comes from the effects like DoF and Glow
|
||
RD::get_singleton()->draw_command_end_label(); | ||
} | ||
|
||
if (fsr && can_use_effects && rb->get_scaling_3d_mode() == RS::VIEWPORT_SCALING_3D_MODE_FSR) { | ||
RD::get_singleton()->draw_command_begin_label("FSR 1.0 Upscale"); | ||
|
||
printf("FSR ?"); |
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.
FSR upscaling should still take place when using viewport mode 3D only. As users will expect a full resolution buffer
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.
IMO render resolution is expected in this case.
If I understand it correctly, this PR returns the HDR buffer if and only if |
HDR displays can generally display 10 bpc or even 12 bpc (via hardware dithering), so it should indeed be possible for the final texture to be greater than 8 bpc even in 2D-only projects. |
That is a different issue entirely. This PR provides access to the 3D buffer for 3D rendering. We have discussed HDR in 2D, but a separate PR will be needed because it is unrelated to this |
Assuming that another PR will be done in the future, wouldn't the interface be strange in that case? // This is 3D project
viewport->set_viewport_mode(VIEWPORT_MODE_3D);
/* no further configuration is required, we have HDR buffer now */ // This is 2D project
viewport->set_viewport_mode(VIEWPORT_MODE_2D_AND_3D);
viewport->enable_hdr_for_2d(); // ????? |
I don't see what is so strange about that. Right now 2D and 3D are rendered to seperate buffers. 3D renders to an HDR buffer while 2D does not. |
Alright. rt->color_format = RD::DATA_FORMAT_R16G16B16A16_SFLOAT @clayjohn Is there any technical reason to hard-code this rather than allowing users to specify the buffer format directly? If we could avoid mapping I'm asking this question because I'm trying to learn the internals of Godot for potential contributions. |
That's a good question, and it comes up a lot. The fact of the matter is that the 3D renderers are designed around a particular texture format as the particular texture format needs to specified in advance at various places in the rendering pipeline. For example, a compute shader that operates on an The problem in this PR is that instead of exposing the HDR 3D render buffer. It changes the format of the 2D render target and then blits the 3D buffer into the 2D buffer. But it still exposes the 2D buffer to the user, even if the user has requested 3D only. This PR should expose the 3D buffer directly so that users can create 3D effects without touching the 2D rendering pipeline and while keeping their data in HDR. A future PR should also be created to allow switching to an HDR buffer for 2D rendering. |
I'll look more into it later but i think this will start from scratch again.
I think part of this will raise new issue, i don't know when i'll have time to think of the changes. For people in need of a quick solution this PR does the trick for my current projects to create post process effect (the advantage over full screen quad is that it handles transparent materials properly) |
Closed as per the above discussion, as this will need a new PR. Thanks for contributing! |
Fixes #54122
This is a rework of #61667
Now using an enum.
ViewportMode
:2D_AND_3D
Default, render the scene and serve it as it served to screen3D
Serve the 3d render buffer in float (if possible) and without tonemapping and unlinearizedXR
Replace theuse_xr
flagFor cleaner implementation i renamed
Tonemapper
toPostProcessor
and add the abitlity to disable linearize and tonemap step from the shader.bugsquad edit: Fixes #70970