OpenXR: Only run xrSyncActions when application has focus #107619

BastiaanOlij · 2025-06-17T07:53:18Z

This is a small fix to ensure xrSyncActions is only run when our session has the focused state. We were running this as soon as the session was created and silently receiving errors.

I've also added a method to provide access to the current session state in case extensions want to access the information for similar reasons.

Fixes #107612

dsnopek

Thanks!

Even aside from the bug fix, this is a great idea. I have definitely connected to signals on OpenXRInterface just to track the session state, and being able to directly access the state directly would make that much easier

modules/openxr/doc_classes/OpenXRInterface.xml

dsnopek · 2025-06-17T12:35:16Z

modules/openxr/openxr_interface.h

@@ -216,6 +216,21 @@ class OpenXRInterface : public XRInterface {
 	void on_refresh_rate_changes(float p_new_rate);
 	void tracker_profile_changed(RID p_tracker, RID p_interaction_profile);

+	/** Session */
+	enum OpenXrSessionState { // Should mirror XrSessionState


We've used OpenXR with a capital R pretty much everywhere, so I think we should here as well:

Suggested change

enum OpenXrSessionState { // Should mirror XrSessionState

enum OpenXRSessionState { // Should mirror XrSessionState

Doesn't this give problems with pascal casing, become open_x_r_session_state? Thats why so far we've done alot of enums with OpenXr instead of OpenXR.

Could be wrong though.

Hm, do you have an example of an enum we've done that's OpenXr with lower-case r?

When I do case-sensitive search, I only find that in C# stuff (I'm assuming because it does some conversion to prevent consecutive upper-case letters)

Mostly the new PRs, render models and spatial entities. Same reason, I ran into pascal casing issues with it.

Also the enum in OpenXR itself is XrSessionState, with a small r.

I'm happy to go for the large R if we really want to :)

In this case, maybe we should just remove the "OpenXR*" prefix entirely?

This is an enum inside of OpenXRInterface, so, we could just call it SessionState? It would be addressed as OpenXRInterface.SessionState - we don't need to say "OpenXR" twice.

However, I do feel pretty strongly that we shouldn't start naming types as OpenXr with a lower-case r, because it'll be super weird to have some types as OpenXR* (which is everything currently merged), and some as OpenXr*. Autocomplete will help, but I think this will create confusion.

Seperate from this PR we should probably do a bit of a stock take here.

The argumention I'm using here and in spatial entities is to set these enums apart because they are mirrors from OpenXR enums, as opposed to enums we've created ourselves.

But it's indeed likely this hasn't been consistently applied.

I'm happy to change this (and spatial entities enums) to have a capital R, I'm not going to die on the hill just for the pascal case issue. C# devs will just have to deal with that :P

Now that this has been merged, I've created PR #107710 to continue this discussion

I agree the prefix is redundant and it can be removed.

Doesn't this give problems with pascal casing, become open_x_r_session_state? Thats why so far we've done alot of enums with OpenXr instead of OpenXR.

Where do you see this being converted to snake_case in this way?

Where do you see this being converted to snake_case in this way?

This was something that we had issues with in the past, so maybe its just something that stuck with me.

modules/openxr/openxr_interface.cpp

BastiaanOlij · 2025-06-17T13:54:12Z

Changed this to draft pending further discussion with other OpenXR experts to determine if our conclusions are correct or whether we should react differently to XR_SESSION_NOT_FOCUSED

modules/openxr/doc_classes/OpenXRInterface.xml

BastiaanOlij · 2025-06-18T02:07:13Z

Ok, so there is a bit more to this than we initially thought. See #107612 (comment)

I think there are a few good changes in this that we should keep, but the original fix is unnecesairy. I'll commit an update shortly.

…date.

BastiaanOlij · 2025-06-18T07:38:44Z

Changed this over as discussed in the issue after a long discussion in the OpenXR WG. Our original conclusions weren't correct but did find a few tweaks we should do.

In the end this mostly introduces the ability to obtain the session state, which is useful. Right now in many projects I'm tracking the state by reacting on the signals and I gather many others do too.

AThousandShips · 2025-06-18T09:04:54Z

modules/openxr/doc_classes/OpenXRInterface.xml

@@ -216,7 +222,7 @@
 		</signal>
 		<signal name="session_focussed">
 			<description>
-				Informs our OpenXR session now has focus.
+				Informs our OpenXR session now has focus, e.g. output is sent to our HMD and we're receiving XR input.


Suggested change

Informs our OpenXR session now has focus, e.g. output is sent to our HMD and we're receiving XR input.

Informs our OpenXR session now has focus, for example output is sent to our HMD and we're receiving XR input.

AThousandShips · 2025-06-18T09:05:09Z

modules/openxr/doc_classes/OpenXRInterface.xml

 		<signal name="session_visible">
 			<description>
-				Informs our OpenXR session is now visible (output is being sent to the HMD).
+				Informs our OpenXR session is now visible, e.g. output is being sent to the HMD but we don't receive XR input.


Suggested change

Informs our OpenXR session is now visible, e.g. output is being sent to the HMD but we don't receive XR input.

Informs our OpenXR session is now visible, for example output is being sent to the HMD but we don't receive XR input.

AThousandShips · 2025-06-18T09:05:46Z

modules/openxr/doc_classes/OpenXRInterface.xml

+		</constant>
+		<constant name="OPENXR_SESSION_STATE_VISIBLE" value="4" enum="OpenXrSessionState">
+			The application has synched its frame loop with the runtime and we're rendering output to the user, however we receive no user input. [signal session_visible] is emitted when we change to this state.
+			[b]Note:[/b] This is the current state just before we get the focused state, whenever the user opens a system menu, switches to another application or takes off their headset.


Suggested change

[b]Note:[/b] This is the current state just before we get the focused state, whenever the user opens a system menu, switches to another application or takes off their headset.

[b]Note:[/b] This is the current state just before we get the focused state, whenever the user opens a system menu, switches to another application, or takes off their headset.

Please use Oxford comma.

Kimau · 2025-06-18T09:46:27Z

On surface looks great but you need to check this with some different runtimes, might have improved but espcially with PCVR and different streaming runtimes we had a nightmare time with the engine I maintained at Adobe. Partly it was because the application was a hybrid app between desktop and VR so users don/doffed the headset more than is typical but these state transitations can get horribly locked if your not careful.

Also be aware the spec doesn't guarantee you transition through all states. So you can jump from Sync -> Focused skipping visible for example. Also the shutdowns often aren't clean.

I'm not updating the engine for the current title I'm working on and I doubt this would affect it but for hybrid XR apps this stuff is critical and a minefield.

BastiaanOlij · 2025-06-18T10:17:08Z

@Kimau indeed, in hindsight we undid the main timing change because after discussing at length, it seemed incorrect and the behaviour issue is likely an XR runtime issue that we can work around in the extension itself until it's fixed. So I don't think anything is left in this that will change the flow.

With adding the extra signal and documentation around the different states, hopefully it clears up what the expected behaviour is. So far I think we've got things working in a way that runtimes that skip the synchronisation and/or visible states and go straight to focussed will work just fine.

The main issue, as was mentioned in the issue and has been reported a number of times over time, is one that is actually intentional. E.g, if we're in the visible state (either headset off, user has entered a system menu, or task switched to a different application), we no longer receive XR input and positional updates of controllers.

This has been an oft cause of frustration with users as OpenVR doesn't have(/had) this behaviour but it is one enforced by OpenXR and for good reasons as getting input in these situations can adversely effect the user experience at best, and potentially create an avenue for social hacking at worst..

Repiteo · 2025-06-18T23:30:58Z

Thanks!

BastiaanOlij self-assigned this Jun 17, 2025

BastiaanOlij requested a review from a team as a code owner June 17, 2025 07:53

BastiaanOlij added the bug label Jun 17, 2025

BastiaanOlij requested a review from a team as a code owner June 17, 2025 07:53

BastiaanOlij added the topic:xr label Jun 17, 2025

AThousandShips added this to the 4.5 milestone Jun 17, 2025

dsnopek reviewed Jun 17, 2025

View reviewed changes

BastiaanOlij marked this pull request as draft June 17, 2025 13:53

m4gr3d reviewed Jun 17, 2025

View reviewed changes

modules/openxr/doc_classes/OpenXRInterface.xml Outdated Show resolved Hide resolved

m4gr3d approved these changes Jun 17, 2025

View reviewed changes

BastiaanOlij force-pushed the openxr_fix_xrSyncAction_timing branch from bc8e860 to 529548c Compare June 18, 2025 07:36

OpenXR: Add access to session state and change interaction profile up…

df06aa8

…date.

BastiaanOlij force-pushed the openxr_fix_xrSyncAction_timing branch from 529548c to df06aa8 Compare June 18, 2025 07:37

BastiaanOlij marked this pull request as ready for review June 18, 2025 07:37

AThousandShips approved these changes Jun 18, 2025

View reviewed changes

dsnopek mentioned this pull request Jun 18, 2025

OpenXR: Add support for spatial entities extension #107391

Open

Repiteo merged commit c1a746a into godotengine:master Jun 18, 2025
20 checks passed

BastiaanOlij deleted the openxr_fix_xrSyncAction_timing branch June 19, 2025 05:28

dsnopek mentioned this pull request Jun 19, 2025

Rename OpenXRInterface.OpenXrSessionState to OpenXRInterface.SessionState #107710

Merged

	enum OpenXrSessionState { // Should mirror XrSessionState
	enum OpenXRSessionState { // Should mirror XrSessionState

	Informs our OpenXR session now has focus, e.g. output is sent to our HMD and we're receiving XR input.
	Informs our OpenXR session now has focus, for example output is sent to our HMD and we're receiving XR input.

	Informs our OpenXR session is now visible, e.g. output is being sent to the HMD but we don't receive XR input.
	Informs our OpenXR session is now visible, for example output is being sent to the HMD but we don't receive XR input.

	[b]Note:[/b] This is the current state just before we get the focused state, whenever the user opens a system menu, switches to another application or takes off their headset.
	[b]Note:[/b] This is the current state just before we get the focused state, whenever the user opens a system menu, switches to another application, or takes off their headset.

Uh oh!

OpenXR: Only run xrSyncActions when application has focus #107619

OpenXR: Only run xrSyncActions when application has focus #107619

Uh oh!

Conversation

BastiaanOlij commented Jun 17, 2025

Uh oh!

dsnopek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BastiaanOlij commented Jun 17, 2025

Uh oh!

Uh oh!

BastiaanOlij commented Jun 18, 2025

Uh oh!

BastiaanOlij commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kimau commented Jun 18, 2025

Uh oh!

BastiaanOlij commented Jun 18, 2025

Uh oh!

Uh oh!

Repiteo commented Jun 18, 2025

Uh oh!

Uh oh!