Skip to content
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

Openxr KHR metal enable #98872

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BastiaanOlij
Copy link
Contributor

This PR adds support for the metal backend to our OpenXR module.

Depends on: #98803

This can be tested using Metas XR simulator on Mac: https://github.com/Oculus-VR/homebrew-repo/blob/main/meta-xr-simulator.md

Issues still to be resolved:

  • Preview window doesn't open.
  • Meta XR Simulator may load outdated version of MoltenVK.
  • Textures are not supplied correctly yet.

@AThousandShips AThousandShips added this to the 4.x milestone Nov 6, 2024
@dsnopek
Copy link
Contributor

dsnopek commented Nov 6, 2024

I know this is a work-in-progress and not finished, but I wanted to share my experience in testing it so far!

When I first tried using the Meta XR Simulator with this PR on my M2 Mac Mini, it would crash with this stacktrace:

image

So, then I tried:

  1. Updating to the latest XR Simulator stable (69.0.0) - still crashed
  2. Updating system-installed MoltenVK to the latest (1.3.296.0) - still crashed
  3. Updating to the latest XR Simulator beta (71.0.0-beta.2) - works!

I'm not sure if the MoltenVK update was actually required.

However, I don't actually see anything rendered in either the Godot window, or the XR Simulator window (like I do when using Vulkan).

Anyway, I'm looking forward to testing this again as it progresses!

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Nov 7, 2024

I know this is a work-in-progress and not finished, but I wanted to share my experience in testing it so far!

When I first tried using the Meta XR Simulator with this PR on my M2 Mac Mini, it would crash with this stacktrace:

image

So, then I tried:

  1. Updating to the latest XR Simulator stable (69.0.0) - still crashed
  2. Updating system-installed MoltenVK to the latest (1.3.296.0) - still crashed
  3. Updating to the latest XR Simulator beta (71.0.0-beta.2) - works!

I'm not sure if the MoltenVK update was actually required.

However, I don't actually see anything rendered in either the Godot window, or the XR Simulator window (like I do when using Vulkan).

Anyway, I'm looking forward to testing this again as it progresses!

That's pretty much what I experienced as well, I couldn't get it to work with 69, it did stop crashing with 71 beta (which has MoltenVK linked in statically if I understand correctly).

I am still at a loss on why we're not getting output however, I think we need @stuartcarnie input here.

@BastiaanOlij
Copy link
Contributor Author

@stuartcarnie @dsnopek I think I figured out what we're missing and why this is not working. I finally managed to get Xcode playing ball (thanks @clayjohn !!).

OpenXR requires us to create our swapchain using an sRGB texture format. We've gone down this road before with Vulkan and OpenGL but it applies the same on Metal. To be exact, the Meta XR simulator doesn't even offer the option of using a non sRGB texture format.

Off course, our renderer uses a normal non sRGB texture format because we do our own sRGB conversion in a shader (which btw on Metal/Apple is a problem because Apple uses a different display gamma compared to what we use, thats been a constant bane for graphics artists and webdesigners for decades :P ).

Our rendering pipeline is thus being constructed using MTLPixelFormatRGBA8Unorm as the pixel format, while our texture has been created using MTLPixelFormatRGBA8Unorm_sRGB and our pipeline fails to create.

I did try to see if I could go down the same route as we do with Vulkan (behind the scenes) and create a shared texture with the format MTLPixelFormatRGBA8Unorm and submitting that to the rendering engine. But to no avail. I'm going to do a bit more testing with this because I've found Xcode being untrustworthy in actually realising that files have changed an a fresh build is needed (or for some other reason it sometimes starts my previous build) :P

@BastiaanOlij BastiaanOlij force-pushed the openxr_khr_metal_enable branch 3 times, most recently from b1c0315 to fb1ca10 Compare November 25, 2024 10:44
@dsnopek
Copy link
Contributor

dsnopek commented Nov 25, 2024

OpenXR requires us to create our swapchain using an sRGB texture format. We've gone down this road before with Vulkan and OpenGL but it applies the same on Metal. To be exact, the Meta XR simulator doesn't even offer the option of using a non sRGB texture format.

On OpenGL, we're able to create the swapchain with an sRGB format, but then disable the automatic conversion to sRGB (via glDisable(GL_FRAMEBUFFER_SRGB)), so that we can still do the sRGB conversion ourselves.

Is there a way to do that in Vulkan or Metal too?

@BastiaanOlij
Copy link
Contributor Author

On OpenGL, we're able to create the swapchain with an sRGB format, but then disable the automatic conversion to sRGB (via glDisable(GL_FRAMEBUFFER_SRGB)), so that we can still do the sRGB conversion ourselves.

Is there a way to do that in Vulkan or Metal too?

In Vulkan you do this by creating a texture view for the non-sRGB format and attaching that to the framebuffer, you do that by calling texture_create_shared. If I recall correctly this supposedly happens internally as well and why the texture meta data specified both normal and sRGB formats can be used.

I tried forcing that here as well but to no avail though talking to @stuartcarnie I got the impression the same applies to Metal.
Hopefully its just a typo/bug somewhere and he'll have time to have a look at what we're doing soon.

@dsnopek
Copy link
Contributor

dsnopek commented Dec 2, 2024

The latest version of this PR (which includes PR #99905) is working for me on my Mac Mini, with Meta's XR Simulator 71.0.0-beta.2! Rendering works, and I can move the camera and hands around, and see it update in the XR Simulator window

@BastiaanOlij BastiaanOlij marked this pull request as ready for review December 3, 2024 00:49
@BastiaanOlij BastiaanOlij requested review from a team as code owners December 3, 2024 00:49
@BastiaanOlij
Copy link
Contributor Author

I'm pretty convinced the issue I'm having is localised to my Mac. likely an old version of the Vulkan SDK still kicking around (the XR simulator is still using MoltenVK).

Comment on lines -1178 to +1182
color_swapchain_format = usable_swapchain_formats[0]; // just use the first one and hope for the best...
print_line("Couldn't find usable color swap chain format, using", get_swapchain_format_name(color_swapchain_format), "instead.");
color_swapchain_format = supported_swapchain_formats[0]; // just use the first one and hope for the best...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this switch from usable_swapchain_format[0] to supported_swapchain_format[0]? This would seem to mean we could select a swapchain format the OpenXR runtime supports, but which we don't think should be used for the color swapchain. I think with this we could end up trying to use a depth format for the color swapchain, or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats actually a bug fix. If none of the swapchain formats we want is supported, we were chosing the first swapchain format we want (which we know is not supported), so it would fail to create the swapchain (because it's not supported).

So I changed it that if none of the swapchain formats we want is supported, we choose the first supported swapchain format, and hope for the best.

Honestly, there is a part of me that feels we should just fail in this situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants