Expose access to 3D color buffer through ViewportTexture #90811

clayjohn · 2024-04-17T17:07:28Z

Proper fix for: #54122
Supersedes: #61667 and #70970

This PR does a few things:

Replaces the "use_xr" property of viewports with the enum "ViewportMode"
Allows viewports to be tagged as used for "2D and 3D", "3D", or "XR"
When tagged as "3D" 2D rendering is not performed and ViewportTextures will display the raw 3D buffer (before post processing/tonemapping/EOTF) which is in linear space and has the full dynamic range

Open Questions

When using "3D" mode, should writing to the swapchain be disabled (i.e. the texture can only be used by ViewportTexture) or should tonemapping happen directly into the swapchain (i.e. 2D is simply skipped)? My feeling is that "3D" mode should imply
Right now alpha is cleared to 0 for the 3D buffers. This is because it is used for SSS. For VR passthrough, we most likely need to start always using alpha for actual alpha and put SSS in a separate texture. Should we make that change before this one as it will impact all reads from the color buffer?
How will this interact with exposing the depth/normal buffers through ViewportTexture? Perhaps this functionality is better exposed through the ViewportTexture anyway.

Alternative approach

Building on question 3 above, we could change this PR to instead expose a similar set of options in the ViewportTexture itself: "2D and 3D", 3D, Depth, Normal-Roughness. Then users can use the ViewportTexture with whatever buffer they want.

In the Viewport API, we already have options to disable 2D if we want. So we could leave the questions of disabling 2D/3D in the Viewport and the question of what texture to expose to the ViewportTexture. To me, exposing these through ViewportTexture seems a bit cleaner and it doesn't carry implications for what rendering/processing is done. Further, users could also access multiple buffers from the same Viewport which I think is very important.

Example

In the below scene, the mesh has a red emission with a strength of 10. As expected, when dividing by 10 in "2D and 3D" mode, the color ends up quite dark as the color range is clipped to 0-1. When using the "3D" mode, the color is not clipped, so dividing by 10 restores it to the expected color
"2D and 3D" mode (same as current master)

New "3D" mode

This allows users to keep their textures in linear color space and use the pre-tonemapped version

RedMser · 2024-04-17T17:34:59Z

Should the checkbox "Disable 3D" be merged into the viewport mode enum as well? I assume there's no benefit to using 3D mode + Disable 3D checkbox in combination, for example. Or hide the property / show a configuration warning in such a case.

clayjohn · 2024-04-17T23:06:18Z

Should the checkbox "Disable 3D" be merged into the viewport mode enum as well? I assume there's no benefit to using 3D mode + Disable 3D checkbox in combination, for example. Or hide the property / show a configuration warning in such a case.

That's a good question. We also have a disable_2d option that is currently only exposed through the RenderingServer API. We could add a "2D" option to the enum and then deprecate Disable 3D.

In part because of how this would interact with disable_2d and disable_3d I am strongly starting to lean towards the "alternative approach" I discussed above. I think it might make sense to split the functionality in two:

What texture to display should be controlled by the ViewportTexture
What rendering tasks are done to do should be controlled by the Viewport (this is already done by disable_3d, disable_2d, and use_xr)

BastiaanOlij · 2024-04-18T01:34:51Z

scene/main/viewport.cpp

+
+void Viewport::set_use_xr(bool p_use_xr) {
+	ERR_MAIN_THREAD_GUARD;
+	set_viewport_mode(p_use_xr ? VIEWPORT_MODE_XR : VIEWPORT_MODE_2D_AND_3D);


I think one change you need to do here is that if p_use_xr = false, and viewport_mode != VIEWPORT_MODE_XR, that you skip this. There is a high risk here that viewport_mode is set to the required mode, but set_use_xr is still called afterwards (loading properties for instance), and we end up unsetting the mode we want.

BastiaanOlij · 2024-04-18T01:36:03Z

scene/main/viewport.cpp

@@ -4752,7 +4776,8 @@ 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::BOOL, "use_xr"), "set_use_xr", "is_using_xr");


I think we need to keep use_xr as a property but marked as deprecated for a few versions. We're going to be dealing with a lot of toolkits that need to work both with current and with older versions of Godot that use this property to enable XR.

I'm fine with just leaving it exposed for backwards compatibility. I commented it out just for testing, I will uncomment it before removing this PR from draft (if we decide to move forward with these changes)

BastiaanOlij · 2024-04-18T01:51:00Z

servers/rendering/renderer_viewport.cpp

@@ -1078,7 +1082,11 @@ RID RendererViewport::viewport_get_texture(RID p_viewport) const {
 	const Viewport *viewport = viewport_owner.get_or_null(p_viewport);
 	ERR_FAIL_NULL_V(viewport, RID());

-	return RSG::texture_storage->render_target_get_texture(viewport->render_target);
+	if (viewport->viewport_mode == RS::VIEWPORT_MODE_2D_AND_3D || viewport->render_buffers.is_null()) {


So I'm guessing this should return our render target also when viewport_mode == RS::VIEWPORT_MODE_XR && view_count = 1

I meant to ask you about that. I have no idea what the expected return value should be when using the XR mode.

From this comment it sounds like:

If view_count is 1 return the render_target

if view_count is greater then one then return something else?

BastiaanOlij · 2024-04-18T01:53:03Z

servers/rendering/renderer_viewport.cpp

@@ -265,7 +265,7 @@ void RendererViewport::_draw_viewport(Viewport *p_viewport) {

 	/* Camera should always be BEFORE any other 3D */

-	bool can_draw_2d = !p_viewport->disable_2d && p_viewport->view_count == 1; // Stereo rendering does not support 2D, no depth data
+	bool can_draw_2d = !p_viewport->disable_2d && p_viewport->view_count == 1 && p_viewport->viewport_mode == RS::VIEWPORT_MODE_2D_AND_3D; // Stereo rendering does not support 2D, no depth data


This should also be true if viewport_mode == VIEWPORT_MODE_XR && view_count == 1

You probably want to change this to:

bool can_draw_2d = !p_viewport->disable_2d && ((p_viewport->view_count == 1 && p_viewport->viewport_mode == RS::VIEWPORT_MODE_XR) || p_viewport->viewport_mode == RS::VIEWPORT_MODE_2D_AND_3D);

Though this may not be future proof, right now we only allow stereo rendering for XR, but hopefully in the future we'll allow stereo rendering outside of an XR viewport as well, so maybe

bool can_draw_2d = !p_viewport->disable_2d && p_viewport->view_count == 1 && (p_viewport->viewport_mode == RS::VIEWPORT_MODE_XR || p_viewport->viewport_mode == RS::VIEWPORT_MODE_2D_AND_3D);

is safer..

The point is that stereo rendering is what disables 2d, not the fact that it is an XR viewport.

That makes sense. So maybe we can just do p_viewport->viewport_mode != RS::VIEWPORT_MODE_3D instead?

This condition is part of the reason I am wary about this PR. We already have logic to disable 2D when "disable_2d" is set and when using XR with a greater than 1 view count. So it doesn't seem all that beneficial to add a new setting which controls the same thing.

Expose access to 3D color buffer through ViewportTexture

b71ec1c

This allows users to keep their textures in linear color space and use the pre-tonemapped version

clayjohn added enhancement topic:rendering topic:2d topic:3d labels Apr 17, 2024

clayjohn added this to the 4.x milestone Apr 17, 2024

BastiaanOlij reviewed Apr 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose access to 3D color buffer through ViewportTexture #90811

Expose access to 3D color buffer through ViewportTexture #90811

clayjohn commented Apr 17, 2024

RedMser commented Apr 17, 2024

clayjohn commented Apr 17, 2024

BastiaanOlij Apr 18, 2024

BastiaanOlij Apr 18, 2024

clayjohn Apr 18, 2024

BastiaanOlij Apr 18, 2024

clayjohn Apr 18, 2024

BastiaanOlij Apr 18, 2024

BastiaanOlij Apr 18, 2024 •

edited

Loading

clayjohn Apr 18, 2024

clayjohn Apr 18, 2024

Expose access to 3D color buffer through ViewportTexture #90811

Are you sure you want to change the base?

Expose access to 3D color buffer through ViewportTexture #90811

Conversation

clayjohn commented Apr 17, 2024

Open Questions

Alternative approach

Example

RedMser commented Apr 17, 2024

clayjohn commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BastiaanOlij Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BastiaanOlij Apr 18, 2024 •

edited

Loading