OpenXR: Cleanup swapchain logic (was Fix render target multiplier)

[BastiaanOlij]

This PR cleans up and, to a minor extent, refactors the swapchain logic in the OpenXR module. This will make it easier to create additional logic in the future where additional swapchains need to be created. @dsnopek already partially improved this for composition layers, I just took it a step further as I'll be needing this for some of the more advanced view configurations as well.

In a nutshell the following changes are applied:

• OpenXRAPI::OpenXRSwapChainInfo is now a class with the create/free/acquire and release methods as members
• A queue mechanism for freeing swapchains has been added to give the rendering logic proper time to untangle itself from the swapchain before it is actually removed. While OpenXR has a similar internal mechanism for preventing early freeing of resources, that did not prevent issues on our end.
• Obtaining the supported swapchain formats is now a separate method that is executed only once early on in the initialisation process.
• The logic for creating the main swapchains for rendering HMD output is now identified as such and handled in pre_render so that we can swap out the swapchain with a new one if a new swapchain is required.

These changes started in order to fix changing the "render target multiplier" not updating the actual swapchain. This is now working correctly and the result is testable in the Vulkan renderer.

Currently in OpenGL there is still an issue where the swapchain is correctly updated, but the XR runtime is still treating it as if its at it's original size. I'm still investigating this issue.

[BastiaanOlij]

@clayjohn with this I'm running into the same problem Patrick was with TiltFive. Because we have no dependency management and our textures are used by the render target, they are marked as such, and in use in various places, I can't delete the textures and the process of creating new swapchains fails.

One options would be to remember the old swap chains somewhere and delete them next frame, but I've already been thinking if we shouldn't improve this part in OpenGL anyway.

Worth a chat.

[BastiaanOlij]

@Malcolmnixon @dsnopek something I've been wondering about is to introduce an overriding render size and deprecating our target multiplier. It's very hard to control the desired resolution with the multiplier, just having a property that overrides the size to what the user desires and only use the recommended target size if the overridden size is set to (0,0) might make more sense.

Should be a separate PR but...

[dsnopek]

something I've been wondering about is to introduce an overriding render size and deprecating our target multiplier.

That makes sense to me. Render size is expressed in integers (although, we strangely use Size2 rather than Size2i for it in many parts of the API - it's eventually cast to integers before it's actually used, though), and using a multiplier to get specific integer values isn't the most ergonomic.

[BastiaanOlij]

Render size is expressed in integers (although, we strangely use Size2 rather than Size2i for it in many parts of the API

This is because most of this was designed in Godot 3 days when everything was GDNative and Size2i either didn't exist yet, or wasn't exposed.

We should probably clean that up some day

[BastiaanOlij]

Rebased this on top of @dsnopek 's changes and refactored a bit so code is a bit nicer organised, and is prepared for some other changes that are coming.

I also added a delete queue for the swapchains so when new swapchains are created, they are actually cleaned up when we render the next frame, this specifically because it gives OpenGL a chance to change the frame buffer bindings to the new swapchain images and release the old. While this fixes the early release issue there is still an issue in OpenGL I need to look at.

[m4gr3d]

@Malcolmnixon @dsnopek something I've been wondering about is to introduce an overriding render size and deprecating our target multiplier. It's very hard to control the desired resolution with the multiplier, just having a property that overrides the size to what the user desires and only use the recommended target size if the overridden size is set to (0,0) might make more sense.

Should be a separate PR but...

@BastiaanOlij the issue with that approach is that each platform provides different recommended target size, so if we were to override the render size directly, we'd need to provide an option to define it per platform.
Whereas the multiplier allows to specify a value that's related to the platform's recommended size..

Also, we should keep this in mind: https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_META_recommended_layer_resolution

[BastiaanOlij]

@m4gr3d indeed, though users still have access to the recommended target size if we go down this path and can do a calculation themselves. This problem even exists with the multiplier because the ideal render size is not a factor of the recommended target size, it will be different for each and every platform. Whether we use a factor or a fixed size, it really is only usable when focusing on a single platform.

To be honest, there are many days I regret caving in and adding the render size multiplier in the first place.

There is a really really really good reason why the XR runtime was designed with supplying a recommended target size, especially seeing that OpenVR, and I believe VrAPI, both had provisions for adjusting this size based on performance metrics during execution of the game, a feature that still has not been added back into OpenXR but likely someday will. Users overriding this size will end up with really unpredictable behaviour.

As for the recommended layer size extension, we'll need to add that into the vendors plugin. Luckily here things work pretty much as per normal, whatever size we come up with, we set that as the size of the viewport used for a composition layer. It is then within handling of the viewport in the OpenXR module that we create the correct swapchain and tell the rendering engine to overrule the rendering target. This is a much better approach than that which we decided upon many years ago for the stereo rendering where the viewport size is ignored and overruled by the primary XR interface.
I've long wanted to change this and actually move this logic into the viewport logic, having the size handled on the viewport and then flowing through.

While we're musing about the future, and there are remarks for this in the code already, at some point I also want to change the overrule mechanism. Right now we still allocate the internal Godot buffers which sit there unused. The rendering engine really needs to be enhanced to know how to work with swapchains on the viewport level when required and trigger allocating these. The design of OpenXR has made this extremely clunky though.

All in all, this is all future music and not something I'm planning to take on board with this particular PR. This PR focusses on streamlining the existing logic and ensuring that it's both stable, has the foundation to do more with it and in doing so, solves the issue with the current multiplier implementation.

[dsnopek]

The code looks good to me! It's definitely a nice clean-up - great work! :-)

I tested on the Quest 3, with both the Mobile and OpenGL renderers and everything seems to be working fine. This includes changing XRInterface.render_target_size_multiplier which is working great on Vulkan, but also surprisingly, with OpenGL:

Currently in OpenGL there is still an issue where the swapchain is correctly updated, but the XR runtime is still treating it as if its at it's original size. I'm still investigating this issue.

This PR actually seems to have fixed it for me with OpenGL on the Quest 3!

I made a test project which swaps the XRInterface.render_target_size_multiplier through a set of values when I press a button, and I can see the effects of the render resolution change, without the weird "warping" that I see when trying the same project on Godot 4.2.1.

Which platform are you testing on where you're still seeing issues?

[akien-mga]

Thanks!

[BastiaanOlij]

Which platform are you testing on where you're still seeing issues?

I tested this mainly on SteamVR so maybe it's an issue specific to Steam?