Implementing override functionality for XR #65227

BastiaanOlij · 2022-09-02T03:42:46Z

This PR adds a bit of cleanup to to the render target API and adds 3 overrides to it: render_target_set_override_color, render_target_set_override_depth and render_target_set_override_velocity.

When setting an override, output is directed to these textures instead of our build in textures (it's not smart enough yet to lazy create these and skip if an override is present, may add that later).

The use case here is to prevent overhead copying data from the build in buffers to the output buffers for XR applications. Especially with XR these are large buffers and on mobile hardware like the Quest copying 6 large textures (3 textures x 2 eyes) has a measurable detrimental effect on performance.
For non XR this opens the door to render fullscreen viewports directly into our swapchain, important for Android, though some research needs to be done to verify this works fine for 2D rendering. I think it does, Reduz has some doubts but this is a matter of checking for situations where this is not possible.

Also important to note, in our use cases we're always cycling through images in swap chains as the images are passed to the XR compositor while we've started rendering the next frame. Hence each frame the overrides are updated in a round robin fashion. This is now possible because of earlier work where we're caching framebuffers and we'll end up cycling through 3 sets of framebuffers as we are rendering.

Note:

Color buffer override is always used with OpenXR
Depth buffer override is used if depth layer or spacewarp is available
Velocity override is used for spacewarp only (atm).
This PR implements depth layer support in OpenXR but Spacewarp we'll implement later, this just builds the foundation.

Known issues:

The texture object currently created for each viewport to allow viewport textures still points to the build in texture to which we no longer render when overrides are used (will deal with this in a separate PR in due time)
This PR has minor compatibility breakage mainly as we're retiring the old overrule function. The only thing effected by this is our Godot 4 OpenVR plugin though that is being superseded by our build in OpenXR functionality anyway.

Todos (these will be done in new PRs after this is merged):

if MSAA is used, we do not have a depth and velocity texture resolve in the mobile renderer yet
velocity is currently only enabled for TAA, it also needs to be enabled when the velocity texture override is specified
velocity is not yet implemented in the mobile renderer and needs to be added and enabled if a velocity texture override is specified

BastiaanOlij · 2022-09-02T03:43:31Z

@kisg you might want to try this out together with your Oculus Quest changes, this should have a measurable performance improvement on the Quest.

BastiaanOlij · 2022-09-02T03:45:10Z

@clayjohn note that I have removed the old implementation from the GLES3 driver. It currently was a copy of the Godot 3 implementation which no longer applies. We'll have to see in due time what we're going to do with GLES3, it will likely only have an impact for webXR and for webXR we may not require the overrule logic at all because webXR supplies framebuffers we can't use, and thus we're stuck with a copy anyway.

edit @dsnopek in due time you, me and clay should have a discussion of what we should do with webXR and getting it to work again.

dsnopek · 2022-09-04T00:05:09Z

We'll have to see in due time what we're going to do with GLES3, it will likely only have an impact for webXR and for webXR we may not require the overrule logic at all because webXR supplies framebuffers we can't use, and thus we're stuck with a copy anyway.

edit @dsnopek in due time you, me and clay should have a discussion of what we should do with webXR and getting it to work again.

@BastiaanOlij Yes, we should chat about this! But here's a preview of my thoughts on this:

I mentioned this on another issue/PR somewhere, but using the new-ish WebXR Layers spec we can get WebXR to give us two 2D texture arrays (one for the color buffer and another for the depth buffer).

The WebXR Layers spec is supported in Chrome and on the Oculus Browser, but there's also a polyfill that can be used for browsers that don't support it, which will create its own 2D texture arrays and just copy from them to the framebuffer from WebXR, meaning it should be safe to rely on this spec.

So, I think the key to WebXR is (1) having multiview support in the opengl3 driver and (2) using the new WebXR Layers spec rather than "original method" which just gives us that framebuffer.

BastiaanOlij · 2022-09-04T23:40:36Z

We'll have to see in due time what we're going to do with GLES3, it will likely only have an impact for webXR and for webXR we may not require the overrule logic at all because webXR supplies framebuffers we can't use, and thus we're stuck with a copy anyway.
edit @dsnopek in due time you, me and clay should have a discussion of what we should do with webXR and getting it to work again.

@BastiaanOlij Yes, we should chat about this! But here's a preview of my thoughts on this:

I mentioned this on another issue/PR somewhere, but using the new-ish WebXR Layers spec we can get WebXR to give us two 2D texture arrays (one for the color buffer and another for the depth buffer).

The WebXR Layers spec is supported in Chrome and on the Oculus Browser, but there's also a polyfill that can be used for browsers that don't support it, which will create its own 2D texture arrays and just copy from them to the framebuffer from WebXR, meaning it should be safe to rely on this spec.

So, I think the key to WebXR is (1) having multiview support in the opengl3 driver and (2) using the new WebXR Layers spec rather than "original method" which just gives us that framebuffer.

That sounds very promising, that approach is much more in line with what we're doing.

servers/rendering/renderer_rd/storage_rd/texture_storage.h

clayjohn

Changes to GLES3 look great. I have a comment on how the override slices are implementation, but overall on the rendering side this looks great to me.

clayjohn

Looks good to me! I know reduz wanted to give this a quick look as well

reduz · 2022-09-26T08:03:36Z

servers/rendering/dummy/storage/texture_storage.h

@@ -151,14 +151,17 @@ class TextureStorage : public RendererTextureStorage {
 	virtual RID render_target_create() override { return RID(); }
 	virtual void render_target_free(RID p_rid) override {}
 	virtual void render_target_set_position(RID p_render_target, int p_x, int p_y) override {}


I am starting to wonder whether we need this API, is this used anywhere really?

render_target_create is called when a new viewport is registered with the rendering server to start creating the render target textures. render_target_free is called when the viewport is destroyed. So those are needed.

render_target_set_position is also still called from the viewport logic but I think it's only used in GLES, could be wrong, @clayjohn do you remember how this is used?

Ah I remember now, that's right, I completed the API for render target adding missing functions for each member for when we eventually expose this in GDExtension so the change itself has nothing to do with the override.

Github is being a bit weird with positioning these comments, so we're talking about render_target_set/get_position and render_target_set/get_size ? Yeah I have no idea if they are actually still used or if they may only be applicable to GLES. But they are settings on our viewport so it's a question if it is enough to record them there or if they indeed need to be recorded on the render target.

I think it is likely better to keep this if we want to do the rendering directly to window optimization in OpenGL3 (or in the modern renderers). We need a position and size if we render directly to screen.

reduz · 2022-09-26T08:12:23Z

servers/xr/xr_interface_extension.h

@@ -102,13 +102,19 @@ class XRInterfaceExtension : public XRInterface {
 	virtual Transform3D get_transform_for_view(uint32_t p_view, const Transform3D &p_cam_transform) override;
 	virtual Projection get_projection_for_view(uint32_t p_view, double p_aspect, double p_z_near, double p_z_far) override;
 	virtual RID get_vrs_texture() override;


If this works with OpenGL, does it make sense that it returns a RID ? Should it not return instead a Variant? As OpenGL may need to return a GLInt.

No I think these should be RIDs, we're returning the texture object in the rendering server. Even in rendering device I encapsulate the vulkan texture objects we get from OpenXR in a texture object so we can store size and formatting and expose the texture to the rest of the engine.

As far as I understand this is a RenderingServer Texture not a RenderingDevice texture. So it should be an RID and not a GLint.

akien-mga · 2022-10-04T09:25:30Z

Needs rebase.

BastiaanOlij · 2022-10-05T00:34:42Z

drivers/gles3/storage/texture_storage.cpp

@@ -1439,105 +1428,7 @@ RID TextureStorage::render_target_get_texture(RID p_render_target) {
 	RenderTarget *rt = render_target_owner.get_or_null(p_render_target);
 	ERR_FAIL_COND_V(!rt, RID());

-	if (rt->external.fbo == 0) {


@dsnopek just a heads up though I think we already talked about this, note that I'm removing the old external texture logic as it has become defunct but am not implementing the new feature for GLES3, only for Vulkan.

We'll eventually need to implement the new approach for GLES3 as well, something best done after your multiview changes are completed (or as part thereof).

Yep, that sounds good!

It probably makes sense to add it after the multiview PR (as opposed to being a part of it) because I'll need to make some pretty big changes to WebXR (switching to using the WebXR Layers API) in order to really test that it works, and there's already lots to do in order to get the multiview PR done.

…y for XR

akien-mga · 2022-10-05T07:01:02Z

Thanks!

BastiaanOlij added enhancement topic:rendering topic:xr breaks compat topic:3d labels Sep 2, 2022

BastiaanOlij added this to the 4.0 milestone Sep 2, 2022

BastiaanOlij self-assigned this Sep 2, 2022

BastiaanOlij force-pushed the complete_render_target_api branch from 109c681 to 839803c Compare September 2, 2022 03:50

BastiaanOlij mentioned this pull request Sep 2, 2022

Implement external texture support for XR plugins that require render… #51179

Closed

BastiaanOlij force-pushed the complete_render_target_api branch from 839803c to 9fc8c0b Compare September 2, 2022 04:07

dsnopek mentioned this pull request Sep 4, 2022

[opengl] Add multiview to the opengl3 driver #65334

Merged

BastiaanOlij force-pushed the complete_render_target_api branch 6 times, most recently from 2529cf4 to fa8f6a2 Compare September 6, 2022 00:45

BastiaanOlij marked this pull request as ready for review September 6, 2022 00:49

BastiaanOlij requested review from a team as code owners September 6, 2022 00:49

clayjohn reviewed Sep 7, 2022

View reviewed changes

servers/rendering/renderer_rd/storage_rd/texture_storage.h Show resolved Hide resolved

clayjohn reviewed Sep 7, 2022

View reviewed changes

BastiaanOlij force-pushed the complete_render_target_api branch from fa8f6a2 to 5526fde Compare September 8, 2022 10:43

clayjohn approved these changes Sep 9, 2022

View reviewed changes

BastiaanOlij force-pushed the complete_render_target_api branch from 5526fde to 90e6c9b Compare September 15, 2022 12:44

clayjohn mentioned this pull request Sep 15, 2022

ExternalTexture missing in Godot 4 #65778

Open

BastiaanOlij force-pushed the complete_render_target_api branch from 90e6c9b to 5f999fc Compare September 22, 2022 01:54

reduz reviewed Sep 26, 2022

View reviewed changes

BastiaanOlij force-pushed the complete_render_target_api branch 2 times, most recently from db005cb to 15dbb0a Compare September 27, 2022 10:18

BastiaanOlij force-pushed the complete_render_target_api branch from 15dbb0a to 6127bfd Compare October 5, 2022 00:27

BastiaanOlij commented Oct 5, 2022

View reviewed changes

Adding getters to RenderTarget and implementing override functionalit…

c765697

…y for XR

BastiaanOlij force-pushed the complete_render_target_api branch from 6127bfd to c765697 Compare October 5, 2022 00:38

akien-mga merged commit 58a1121 into godotengine:master Oct 5, 2022

BastiaanOlij deleted the complete_render_target_api branch October 5, 2022 23:31

dsnopek mentioned this pull request Nov 3, 2022

Collapse three seperate texture storage methods into render_target_set_override() #68212

Merged

Cyberrebell mentioned this pull request Nov 3, 2022

Fix several render issues found while debugging XR #68102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing override functionality for XR #65227

Implementing override functionality for XR #65227

BastiaanOlij commented Sep 2, 2022 •

edited

Loading

BastiaanOlij commented Sep 2, 2022

BastiaanOlij commented Sep 2, 2022 •

edited

Loading

dsnopek commented Sep 4, 2022 •

edited

Loading

BastiaanOlij commented Sep 4, 2022

clayjohn left a comment

clayjohn left a comment

reduz Sep 26, 2022

BastiaanOlij Sep 26, 2022

BastiaanOlij Sep 26, 2022

clayjohn Sep 27, 2022

reduz Sep 26, 2022

BastiaanOlij Sep 26, 2022 •

edited

Loading

clayjohn Sep 27, 2022

akien-mga commented Oct 4, 2022

BastiaanOlij Oct 5, 2022

dsnopek Oct 5, 2022

akien-mga commented Oct 5, 2022

Implementing override functionality for XR #65227

Implementing override functionality for XR #65227

Conversation

BastiaanOlij commented Sep 2, 2022 • edited Loading

BastiaanOlij commented Sep 2, 2022

BastiaanOlij commented Sep 2, 2022 • edited Loading

dsnopek commented Sep 4, 2022 • edited Loading

BastiaanOlij commented Sep 4, 2022

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn left a 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

Choose a reason for hiding this comment

BastiaanOlij Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Oct 5, 2022

BastiaanOlij commented Sep 2, 2022 •

edited

Loading

BastiaanOlij commented Sep 2, 2022 •

edited

Loading

dsnopek commented Sep 4, 2022 •

edited

Loading

BastiaanOlij Sep 26, 2022 •

edited

Loading