Allow using custom Rect2i for rendering with OpenXR

[devloglogan]

This PR adds the ability to set a render_region Rect2i property in OpenXRAPI, which is passed along to the renderer's override properties. If a custom Rect2i is set, it will be applied to the rendered image rects. This is meant to be used with something like the XR_META_recommended_layer_resolution OpenXR extension.

The newly added OpenXRAPI functions set_render_region() and get_projection_layer() have been exposed to GDExtension to allow for the implementation of XR_META_recommended_layer_resolution in the vendor repo.

[dsnopek]

Thanks!

I tested this together with PR GodotVR/godot_openxr_vendors#226, and it appears to be working great! (I have notes about the performance gains I'm seeing, but that's more related to the META_recommended_layer_resolution extension, so I'll write about it on the other PR.)

The code looks good to me as well.

[m4gr3d]

Looks good!

Added some minor comments to address.

[m4gr3d]

Can be replaced by the following:

Rect2i new_render_region = (render_region != Rect2i()) ? render_region : Rect2i(Point2i(0, 0), render_state.main_swapchain_size);

In addition:

• The Rect2i documentation states that Rect2i() == false, so we can replace render_region != Rect2i() with render_region != false
• Point2i(0, 0) can be replaced by Vector2i.ZERO
• Do we expect Rect2i(Point2i(0, 0), render_state.main_swapchain_size) to change every frame?

[devloglogan]

I've made the suggested replacement, thanks! As to your other points, the Rect2i() == false and Vector2i.ZERO suggestions I believe would only work in editor?

Rect2i(Point2i(0, 0), render_state.main_swapchain_size) would typically remain the same, it would only be resized if a new swapchain was created (with a different size). Maybe on swapchain creation I could just store the size in a variable rather than creating this every frame.

[devloglogan]

@Mickeon / @AThousandShips thanks so much for the review! I believe I've applied all your suggested changes. :)

[Mickeon]

Docs a-okay now, assuming accuracy.

[BastiaanOlij]

Looks really cool as far as functionality goes, it'll be interesting to see if we can create our own logic for altering the render region even outside of this extension. XrSwapchainSubImage.imageRect is available in the core.
Similarly I wonder if making the render region core viewport functionality instead of an override makes sense. The use cases here extend beyond XR, being able to dynamically effect the render resolution without recreating all buffers could be useful.

Couple of things that stand out that need to be discussed further:

1. Dealing with the pesky render_target_size_multiplier feature we added to OpenXR, we talked about this I think in the XR meeting already, but if this is set to anything other than its default 1.0, especially if its set smaller, it could cause havoc when combined with this.
2. The implementation of render_region on the OpenXR side, is not thread safe. When rendering happens on a separate thread you've created a possibility for the render region to be set for a new frame, while a frame is being rendered, with the possibility that render result and render region don't match. You have to take the same approach as a number of other properties have, where you have a copy in render_state that is set through calling call_on_render_thread. See set_render_target_size_multiplier and set_render_state_multiplier as an example.
3. I'm not sure I like us exposing the projection layer. Here too there is a high risk of thread issues and it will be easy to break things. Especially when future work will require modifications that will clash with what is done in an extension. We spoke about this before and that we probably don't have any other choice, but I'd like to know more of the reasoning behind this and it would be good to have this properly documented. If we can find a cleaner way to expose the required functionality that would have my preference, but without knowing more I can't think of good suggestions.
4. What is the impact on other features? Does the VRS map need to be calculated based on the render_region, how are build in effects effected especially when this functionality is adopted on PCVR with the Forward+ renderer. Etc.

[devloglogan]

Thanks for the review @BastiaanOlij!

1. render_target_size_multiplier is taken into account on the creation of the swapchain, and the resolution recommendation from the OpenXR extension will always be less than or equal to the inital size of the swapchain. I've tested modifying this multiplier and it seems to work fine with the OpenXR extension. Do you have concerns about setting render_region outside of the use of this extension? Perhaps we could add a check so that the region would have to be in the bounds of the swapchain size? From the spec:

If the XrRecommendedLayerResolutionGetInfoMETA::layer attribute of the info argument of the function contains valid swapchain handles in all fields where required, the runtime must return a resolution recommendation which is less than or equal to the size of that swapchain, so that the application may render into an existing swapchain or swapchains without reallocation.

2. Understood, I believe in my most recent changes render_region should now be thread safe. :)

3. The OpenXR XrRecommendedLayerResolutionGetInfoMETA struct needs the projection layer to be set as the layer property in order for the extension to return a resolution of lesser or equal value via the xrGetRecommendedLayerResolutionMETA() function. I believe the only alternative would be introducing vendor specific structs or functions to this PR, which I know we don't want. So exposing the projection layer is probably the only way we'll be able to make this OpenXR extension work.

4. Don't have a good response to this right now, but I'll look into it and get back to you!

[devloglogan]

@BastiaanOlij the VRS map did indeed need to be calculated based on render_region. I've made it do so in my latest push, tested both on standalone and PCVR. I'm not aware of any other built-in effects that are affected by this change, but if you have any more concerns let me know and I can test them out!

[clayjohn]

I think we need to clean up the API here a bit. This feature is implemented with 3 assumptions that I think are all incorrect:

1. We are always using an overridden render target (alternatively stated "we are always using XR")
2. The render target is always the same size as the render buffers
3. We are always using dynamic scaling

Building on Bastiaan's comments above. This API needs to consider dynamic scaling on non-XR devices and expose an API that can be accessed from rather than building in an API that can only be used from XR

[clayjohn]

We should clean this up to consider the case where there is no render target override. Most of the time when this code runs, it will be in a non-XR environment, so we shouldn't add something that assumes we are in XR and using dynamic scaling.

[devloglogan]

I've updated it so that render_region is no longer a part of the override/xr stuff, it's now just a property of the RenderTarget struct.

[clayjohn]

Discussed in the rendering meeting. We discussed that the render region should be stored in the RenderData struct.

You can store it here: https://github.com/godotengine/godot/blob/fafc07335bdecacd96b548c4119fbe1f47ee5866/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp#L1221C31-L1221C105

Then, throughout rendering we can apply the region consistently by accessing render data

Also noticed that the transparent pass is missing the render region in the mobile renderer, so it needs to be used there.

[devloglogan]

@clayjohn thanks for sharing the suggested changes! I've applied them in my latest push. :)

[BastiaanOlij]

Looks good to me.

My only cringe is still the get_projection_layer being exposed, but looking at how it is used for xrGetRecommendedLayerResolutionMETA, it seems its only needed for info, not to actually change or update the data (which is my main concern because that can cause breaking logic being introduced).

Maybe we should define the function as const XrCompositionLayerProjection *OpenXRAPI::get_projection_layer() const.
Getters should always be const if possible, but also returning a const pointer would communicate clearly that the contents should not be altered.
XrRecommendedLayerResolutionGetInfoMETA takes a const pointer to begin with so it shouldn't pose a problem.

[devloglogan]

Maybe we should define the function as const XrCompositionLayerProjection *OpenXRAPI::get_projection_layer() const. Getters should always be const if possible, but also returning a const pointer would communicate clearly that the contents should not be altered. XrRecommendedLayerResolutionGetInfoMETA takes a const pointer to begin with so it shouldn't pose a problem.

Makes sense to me! I've updated the function signature.

[BastiaanOlij]

Great work!

[akien-mga]

Thanks!