Skip to content

openxr_data: fire VREvent_Quit event when session state changes to STOPPING#230

Open
ImSapphire wants to merge 2 commits into
Supreeeme:mainfrom
ImSapphire:quit-on-session-loss
Open

openxr_data: fire VREvent_Quit event when session state changes to STOPPING#230
ImSapphire wants to merge 2 commits into
Supreeeme:mainfrom
ImSapphire:quit-on-session-loss

Conversation

@ImSapphire

@ImSapphire ImSapphire commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

Tested in VRChat via closing with WiVRn's quit button.

Comment thread src/system.rs
Comment thread src/openxr_data.rs Outdated
@ImSapphire
ImSapphire requested a review from Supreeeme November 27, 2025 07:06
@ImSapphire
ImSapphire force-pushed the quit-on-session-loss branch 2 times, most recently from 1537d95 to 273ad97 Compare November 27, 2025 08:16
@Supreeeme

Copy link
Copy Markdown
Owner

Sorry for the delay. Looks good, but a test for this would be cool.

@ImSapphire
ImSapphire force-pushed the quit-on-session-loss branch from 273ad97 to 06a5c92 Compare December 3, 2025 07:20
@ImSapphire

ImSapphire commented Dec 3, 2025

Copy link
Copy Markdown
Contributor Author

Sorry for the delay. Looks good, but a test for this would be cool.

I'm not exactly sure how to write unit tests, and I'm working on some other things right now, would it be fine to merge without a test?

@Supreeeme

Copy link
Copy Markdown
Owner
  1. I realized this is actually session loss, not instance loss. The commit message should probably be reworded.
  2. I was thinking about this and came to a realization: is telling the application on session loss really the right thing here? To me VREvent_Quit seems more like a way to tell an application to gracefully close, but session loss seems closer to an error condition, and if we have applications gracefully exit it won't be obvious something erroneous happened. I would think session state being STOPPING or EXITING would be more appropriate.

@ImSapphire

Copy link
Copy Markdown
Contributor Author

@Supreeeme Would emitting VREvent_Quit event on change to STOPPING state be enough? I thought there would be more involved in that case, though I guess nothing in OpenXR mandates that the app stop immediately

@ImSapphire
ImSapphire force-pushed the quit-on-session-loss branch from 06a5c92 to 40b491b Compare December 8, 2025 08:12
@ImSapphire ImSapphire changed the title openxr_data: fire VREvent_Quit event when instance loss is pending openxr_data: fire VREvent_Quit event when session state changes to STOPPING Dec 8, 2025
@Supreeeme

Copy link
Copy Markdown
Owner

Based on the spec, I think the correct thing to do would be:

  1. Send VREvent_Quit on STOPPING
  2. Call end_session (xrEndSession) when the application calls AcknowledgeQuit_Exiting

@ImSapphire

Copy link
Copy Markdown
Contributor Author

iirc in SteamVR, AcknowledgeQuit_Exiting just extends SteamVR's kill timer to give the app time to safely shutdown, if we stop the session there we'd probably crash because the game would keep calling functions like WaitGetPoses, Get*ActionData etc. Not sure what the right thing to do there is

@ImSapphire

Copy link
Copy Markdown
Contributor Author

Some apps also simply don't call AcknowledgeQuit_Exiting (VRChat, Resonite from what I've tested so far)

@ImSapphire

Copy link
Copy Markdown
Contributor Author

Added a test

@ImSapphire

Copy link
Copy Markdown
Contributor Author

@Supreeeme is there anything I could do to get this merged?

@ImSapphire
ImSapphire force-pushed the quit-on-session-loss branch from 0d8d322 to 3cd1cfe Compare May 17, 2026 23:01
Comment thread src/openxr_data.rs
state = Some(event.state());
info!("OpenXR session state changed: {:?}", event.state());
if let Some(system) = self.system.get().filter(|_| {
(cfg!(test) || session_data.temp_vulkan.is_none())

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why is this unconditional for tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When running the test, the session is using a temp vulkan instance, that seemed like the easiest way to get it working to me

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't love adding test specific logic to actual code. I think it would be better to have some kind of use_real_session helper method in the test that handles doing this.

@ImSapphire
ImSapphire force-pushed the quit-on-session-loss branch from 3cd1cfe to 3a4e123 Compare June 1, 2026 01:40
@ImSapphire
ImSapphire force-pushed the quit-on-session-loss branch from 3a4e123 to 4265b0d Compare June 5, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants