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

Add OpenXR support to the core #56394

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Jan 1, 2022

This is a port of the OpenXR plugin to core, the main reason for this is the way OpenXR ties into Vulkan, with OpenXR wanting to take ownership of creating the Vulkan instance and managing the creation of swap chains for use in XR.
This requires OpenXR to be started and initialized at startup, for this reason new project settings have been added to ensure OpenXR is loaded. More settings will be added soon to further configure this. There is also a placeholder setting for running OpenXR in the editor, this for future plans to enable XR functionality within the editor.

The main OpenXR logic is implemented in the drivers folder through a new OpenXR device. This handles the OpenXR implementation itself. Just like our original plugin the driver is implemented with extensions that implement specific parts of the implementation. There is for instance a Vulkan extension that implements the Vulkan bits of the solution. In due time we can create an OpenGL extension if the OpenGL renderer is used.

The Vulkan driver has been modified to use the Vulkan extension, when available, to set things up.

There is also an OpenXRInterface implementation in modules that implements the XRServer components.

I think this PR is ready for review and start doing the cleanup to get this to be merged. There are a few things left to do but I think these can have their own follow up PRs

  • currently I'm doing a copy into the swapchain buffer instead of rendering directly into this. The main problem I have here is one of timing, OpenXR needs to have its session created and setup before we can create our swapchains. This is something that happens way too late as this will happen in the background, often waiting for the XR runtime to startup and switch over. It may require finishing Implement external texture support for XR plugins that require render… #51179 and using our current approach to introduce the OpenXR swapchain just for the ARVR viewport. That said, OpenXR requires swapchains to be used for 2D UI viewports when using layers so there is more to do here.
  • Action map and controller logic is finished but we need to add a UI to edit the actionmap
  • Still issues with OpenXR wanting us to render in linear color space but linear color space on RGBA8 gives banding.
  • Should work on Windows and Linux but only tested SteamVR on Windows, but I've disabled Android and we can do this in a follow up PR. Note that currently we are assuming that we can use the Khronos loader on Android instead of using Metas proprietary loader for Quest.
  • Port over features like hand tracking, pass through, composition layers, etc.

A demo project to test this work with can be found here: https://github.com/BastiaanOlij/godot4_openxr_demo

@BastiaanOlij BastiaanOlij self-assigned this Jan 1, 2022
@Calinou Calinou added this to the 4.0 milestone Jan 1, 2022
@akien-mga akien-mga changed the title Open xr core4 Add OpenXR support to the core Jan 5, 2022
@akien-mga akien-mga self-requested a review January 5, 2022 16:47
@BastiaanOlij BastiaanOlij force-pushed the OpenXR_Core4 branch 2 times, most recently from 7d60d1c to 1e9fba6 Compare January 6, 2022 06:24
@BastiaanOlij
Copy link
Contributor Author

This is starting to work, it now is based on #51179, while there are some issues still around updating the framebuffers.

Two major problems at the moment:

  • the timing is WAY off, it has a lot of problems doing proper predicting
  • keep_linear has disappeared, now this might have to do with Vulkan being able to cater for this better, but OpenXR is expecting our data in linear color space, not converted to sRGB.

I have a number of things to polish up on 51179 first, but we're getting somewhere with this port.

@m4gr3d
Copy link
Contributor

m4gr3d commented Jan 7, 2022

@BastiaanOlij It may be worth splitting the thirdpary/openxr/... additions into a separate PR. I'm guessing it's mostly a dump, so no reviews should be needed for that section?
It'll make reviewing the actual integration easier.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jan 7, 2022

@BastiaanOlij It may be worth splitting the thirdpary/openxr/... additions into a separate PR. I'm guessing it's mostly a dump, so no reviews should be needed for that section? It'll make reviewing the actual integration easier.

It's in a separate commit, so if you just look at the last commit only, those are the real changes.

First commit is a copy of (an older version of) 51179
Second commit is the openxr thirdparty library
Third commit is the real work.

edit no longer using 51179

@m4gr3d
Copy link
Contributor

m4gr3d commented Jan 7, 2022

Thanks for the clarification!

@BastiaanOlij
Copy link
Contributor Author

I've redone the last bit of this so it no longer used 51179 but does a copy of Godots results into the textures provided by OpenXR. This is slightly slower but it doesn't require us to do really ugly stuff. Once I improve 51179 we'll revisit that.

I'm still having the timing issues, I may try and move part of process closer to the moment we render to see if that solves it, but its very weird for sure.

Also I think Steam now doesn't like us providing the data in sRGB when its expecting linear color space so things are too bright. Still looking into that.

@BastiaanOlij BastiaanOlij force-pushed the OpenXR_Core4 branch 6 times, most recently from e4f7823 to 391e820 Compare January 14, 2022 07:54
@fire
Copy link
Member

fire commented Jan 14, 2022

Noticed that changing vulkan vsync to mailbox moves the fps from 60fps to 90fps (HMD). Hope that helps.

@BastiaanOlij
Copy link
Contributor Author

Noticed that changing vulkan vsync to mailbox moves the fps from 60fps to 90fps (HMD). Hope that helps.

Yeah we need to find out how to turn vsync off all together. OpenXR will set the pace.

@fire
Copy link
Member

fire commented Jan 15, 2022

Sent you the branch for tracker velocity including the head.

I think the bug is the engine tick is not the tracker (head) tick from the openxr runtime.

@BastiaanOlij BastiaanOlij force-pushed the OpenXR_Core4 branch 3 times, most recently from 8a53ce1 to a3a34e8 Compare January 16, 2022 04:18
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Feb 12, 2022

@reduz @m4gr3d , after talking this over with Reduz last night I spend today moving everything into modules and changing the way things are mixed in to other parts of the system.

I'm not done yet because I want to look into merging openxr_device into openxr_interface, I'm not sure about this just yet.
It's already worth having a look at how things are structured now even before this merge so feel free to give feedback.

Also note that I moved the flag that enables the XR shaders into the XR area of the project settings, you must turn this on or your project won't run. Defo need to improve the error handling here.

Btw @akien-mga , any idea how to make the CI happy about
./drivers/vulkan/vulkan_hooks.h:40:7: error: 'VulkanHooks' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor,2]
This error is given on platforms that do not support OpenXR because the only implementation of this template class is in there.
I guess I could give each function a default implementation? edit did this for now....

@BastiaanOlij BastiaanOlij force-pushed the OpenXR_Core4 branch 5 times, most recently from fd28c94 to 4994196 Compare February 14, 2022 11:11
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Feb 15, 2022

After trying out a few things I've decided to keep the split between the 'OpenXRInterface' class and the OpenXRAPI (was driver) class which is basically a wrapper over the OpenXR API.

Things got really messy trying to combine it especially around the action map and controller logic. This nicely splits it in logic that faces OpenXR and logic that focuses on the Godot side.

@BastiaanOlij
Copy link
Contributor Author

@SaracenOne made a small change making the Vulkan version check a warning for now. Godot checks with the Vulkan driver what version of Vulkan is supported on the host system, we select the highest v1.x.x version of Vulkan that is supported by the host (currently usually 1.2.0 on desktop).

We then have an extra check with the OpenXR runtime as OpenXR will also tell us which version of Vulkan the runtime wishes to use. The weird thing here is that Oculus reports it only supports Vulkan 1.0.0.

We believe we can safely ignore this, 1.2.0 is backwards compatible with 1.0.0 so anything the OpenXR runtime wants to use should work, but we don't want to limit ourselves in using newer Vulkan features (if available) just because the OpenXR runtime doesn't want to use those.

So far this seems to work fine.

@BastiaanOlij BastiaanOlij force-pushed the OpenXR_Core4 branch 3 times, most recently from e329e98 to f470fcd Compare February 19, 2022 03:14
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great!

Added some minor feedback.

modules/openxr/action_map/openxr_action.cpp Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_action_set.cpp Outdated Show resolved Hide resolved
modules/openxr/openxr_api.cpp Outdated Show resolved Hide resolved
wrapper->on_state_ready();
}

// TODO emit signal
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to be added in a follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wanted to get the base line working first, there are a bunch of things we're doing in the plugin that either need to still be ported but are waiting on something (like Meta adopting the Khronos loader) and things that need to change.

But seeing this PR already covers a whole bunch of things, plus that this will allow us to divide some of the work

@akien-mga
Copy link
Member

Getting this warning when compiling on Linux:

In file included from thirdparty/openxr/src/loader/loader_platform.hpp:17,
                 from thirdparty/openxr/src/loader/api_layer_interface.hpp:18,
                 from thirdparty/openxr/src/loader/api_layer_interface.cpp:10:
thirdparty/openxr/src/common/platform_utils.hpp: In function 'char* detail::ImplGetSecureEnv(const char*)':
thirdparty/openxr/src/common/platform_utils.hpp:60:55: note: '#pragma message: Warning:  Falling back to non-secure getenv for environmentallookups!  Consider updating to a different libc.'
   60 |     "lookups!  Consider updating to a different libc.")
      |                                                       ^

We should either add a common_config.h with the proper definition for all platforms, or ensure we set them all in the SCsub (it's done for Windows but likely something else is needed for other platforms).

@akien-mga
Copy link
Member

Added a commit to remove all the stuff we don't need and don't care about:

 126 files changed, 23 insertions(+), 22185 deletions(-)

Should be squashed with the first commit eventually so that we don't add files to remove them and make the repository history needlessly bigger. But I kept it separate for now for review. (And I'm not done reviewing the rest of the changes.)

@akien-mga
Copy link
Member

akien-mga commented Feb 23, 2022

Getting this warning when compiling on Linux:

In file included from thirdparty/openxr/src/loader/loader_platform.hpp:17,
                 from thirdparty/openxr/src/loader/api_layer_interface.hpp:18,
                 from thirdparty/openxr/src/loader/api_layer_interface.cpp:10:
thirdparty/openxr/src/common/platform_utils.hpp: In function 'char* detail::ImplGetSecureEnv(const char*)':
thirdparty/openxr/src/common/platform_utils.hpp:60:55: note: '#pragma message: Warning:  Falling back to non-secure getenv for environmentallookups!  Consider updating to a different libc.'
   60 |     "lookups!  Consider updating to a different libc.")
      |                                                       ^

We should either add a common_config.h with the proper definition for all platforms, or ensure we set them all in the SCsub (it's done for Windows but likely something else is needed for other platforms).

Fixed, HAVE_SECURE_GETENV was set for Windows when it's supposed to be set for non Windows (I also excluded UWP to be sure).

Edit: Restricted it to Linux for now as we only compiled for Linux and Windows right now. I'm not sure if it's available on Android (might need recent enough API level), and it might not be available on macOS and iOS.

Will be compiled and used in the next commit.

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

The thirdparty library integration is good now 👍
I removed everything we don't need currently, and fixed the bug with not defining HAVE_SECURE_GETENV on Unix (#56394 (comment)).

Didn't review the Vulkan context changes and OpenXR integration code, but @reduz did, so it should be good to go.

@akien-mga akien-mga merged commit 1f3916e into godotengine:master Feb 23, 2022
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the OpenXR_Core4 branch June 23, 2022 06:39
@sldev14
Copy link

sldev14 commented Jul 11, 2022

What can cause this error? :

E 0:00:00:0168 create_instance: OpenXR: OpenXR Runtime does not support OpenGL extension!
   <C++ error> Method/function failed. return: false
   <C++ source code>modules/openxr/openxr_api.cpp:234 @ create_instance()

Samsung HMD Odyssey Plus.

-> FIX :
Using the (openxr-explorer utility) , I changed the bootloader from WMR to SteamVR. And everything worked!

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.

8 participants