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: Add runtime selection dropdown #85117

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Nov 20, 2023

This PR re-introduces the OpenXR runtime selection dropdown from Godot 3 IF OpenXR is enabled:

image

This selection only applies to the runtime selected when Godot is run in PCVR mode (e.g. when Run Project / F5 is used).

In editor settings there is a section where the user can add runtimes by specifying the locations of the OpenXR runtime configuration json, Godot will only add items to the dropdown if the files are found at startup (e.g. restart is required). A number of default locations is included with this PR:

image

Note, the selection made by the user is not saved. OpenXR clearly states that applications should use the system selected OpenXR runtime at all times. The dropdown is offered to allow quick deviation from that default. E.g. The users has SteamVR set as their default OpenXR runtime, but quickly wants to test if their game works on the Meta XR Runtime (Quest over Link). It's a temporary selection.

@BastiaanOlij
Copy link
Contributor Author

@m4gr3d this would work well in conjunction with the Meta XR Simulator. All that needs to be done is add the path to the simulators json:
image

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.

Added a few cleanup comments, otherwise it looks good!

@BastiaanOlij BastiaanOlij force-pushed the openxr_runtime_select branch 2 times, most recently from 348cef7 to 9fd8b88 Compare November 20, 2023 22:08
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

modules/openxr/editor/openxr_select_runtime.cpp Outdated Show resolved Hide resolved
modules/openxr/editor/openxr_select_runtime.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

Is this the best place to put it in? How often do the users need to change this and how visible it needs to be? For example, the reason the renderer selector is so visible is in large part for our benefit, so it's clear from any given screenshot which renderer is used, even though it's not expected that users would need to change this often.

How much of an issue it is for this particular option?


Also it should at least use the consistent style with the renderer selector, if we decide to keep it as is.

@BastiaanOlij
Copy link
Contributor Author

Is this the best place to put it in? How often do the users need to change this and how visible it needs to be? For example, the reason the renderer selector is so visible is in large part for our benefit, so it's clear from any given screenshot which renderer is used, even though it's not expected that users would need to change this often.

How much of an issue it is for this particular option?

Remember that it is ONLY visible if you enable the OpenXR feature in Godot, and for people who are working on an OpenXR project, it's an important setting that should be easily and quickly changeable before running a project.

@BastiaanOlij
Copy link
Contributor Author

Also it should at least use the consistent style with the renderer selector, if we decide to keep it as is.

I could use some help with that, trying to copy the same code that drives the renderer selector I got crashes. It looks like creating this from an editor plugin is having some issues.

@YuriSizov
Copy link
Contributor

Remember that it is ONLY visible if you enable the OpenXR feature in Godot

Well I don't want to compromise on user experience just because it only affects a few users ;) It feels weirdly tacked on right now, and it takes a lot of space. So I would like to understand how important it is and how often it's going to be used rather than being told that it's important.

I could use some help with that

Sure, I'll lend you a hand once we start the 4.3 dev cycle. Poke me if I don't remember on my own!

@decacis
Copy link
Contributor

decacis commented Nov 22, 2023

Works as expected! In my opinion, is ok having it as a dropdown to quickly test between runtimes, as sometimes there are (minor) differences when using SteamVR vs Oculus. Or in the case of the Meta XR Simulator, being able to switch between it and normal VR with two clicks sounds great!

@BastiaanOlij
Copy link
Contributor Author

Well I don't want to compromise on user experience just because it only affects a few users ;)

Agreed, it is something that does come up regularly, people want to be able to switch quickly between XR runtimes when testing, especially with the new Meta XR simulator, and it did work well like this in Godot 3.

But we're already getting a lot of problems because of the double run button, and that the renderer selection only applies to desktop. This XR runtime selection also only applies to desktop. It's probably our no 1 user error where we waste a lot of time providing support on the XR discord.

Ideally we should only have one play button, and one dropdown for selecting in what configuration we're running, and a place where we can configure these configurations setting whether we are running locally or remotely (android), which export template to use, and override for project settings, override for command line parameters, etc (Lyuma had some wishes here because they run between XR and non-XR modes though a command line parameter).
Problem is that this would require a serious rewrite of how Godot organises running a game, so doing it properly is Godot 5 territory imho.

Just for perspective here. The most common use cases for a Godot XR developer are that they are regularly switching between the following scenarios:

  • Testing their game on desktop on SteamVR for PCVR games.
  • Once DX12 is fully possible, WMR becomes a viable choice too.
  • Testing their game on desktop on the Oculus runtime over link (faster than deploying to Android).
  • Testing their game native on Quest/Pico (full android deploy).
  • A new variant I can't fully talk about yet until this comes out of NDA, but it will also require the XR runtime selection.

Developers who are purely developing towards Quest will generally speaking only use the compatibility renderer and switch between link (for fast testing) and deploying on Quest (for accurate testing).

Developers who are targeting both PCVR and stand alone will often switch between Vulkan and compatibility renderers and thus often report confusion over Godot running the wrong renderer when deploying to Quest. It is also these users who will often switch between different runtimes as they are testing how their game runs on different hardware.

@BastiaanOlij
Copy link
Contributor Author

Sure, I'll lend you a hand once we start the 4.3 dev cycle. Poke me if I don't remember on my own!

Indeed, this is all 4.3 dev cycle stuff so no rush, just working my way through my backlog. Thanks for the offer, will gladly make use of it :)

@YuriSizov YuriSizov force-pushed the openxr_runtime_select branch from 0375e98 to afd1dcf Compare January 18, 2024 14:59
@BastiaanOlij BastiaanOlij requested review from a team as code owners January 18, 2024 14:59
@YuriSizov
Copy link
Contributor

Well, I still don't really like the UX of this, but I can't offer anything right now. So in the interest of moving things forward I just fixed the styling and force-pushed for you (also rebased for good measure).

@YuriSizov YuriSizov merged commit de95a3e into godotengine:master Jan 18, 2024
16 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@BastiaanOlij BastiaanOlij deleted the openxr_runtime_select branch February 27, 2024 23:44
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.

6 participants