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

Godot OpenXR, improved handling of state #83471

Closed
BastiaanOlij opened this issue Oct 17, 2023 · 3 comments · Fixed by #89734
Closed

Godot OpenXR, improved handling of state #83471

BastiaanOlij opened this issue Oct 17, 2023 · 3 comments · Fixed by #89734
Assignees
Milestone

Comments

@BastiaanOlij
Copy link
Contributor

Godot version

4.2.dev

System information

Any

Issue description

This issue is based on feedback after discussing various timing issues around OpenXR and clarification of when certain calls into the OpenXR runtime should take place.

Current Godot OpenXR loop

While I've color coded this with threading in mind, currently there are a number of issues to fix around this. The one that stands out is that xrWaitFrame provides us with the timing information on which we update our tracking data. In our Update XR Trackers step we're thus using predictedDisplayTime + predictedDisplayPeriod, however in a multi threading environment there is no guarantee that wait frame has already been processed before we start handling our next frame. This is part of what we need to fix.

The actual flow should be akin to:

New Godot OpenXR loop

Seems a simple enough change, xrWaitFrame is now called at the start from our main thread (this will need to be moved into our XR process, which is where our tracker update also happens), but we need to do a little more as some of our logic that runs in the render thread shares information with our main thread. That will need to be duplicated so we don't overwrite this data before hand. After this change we can purely rely on predictedDisplayTime. Obtaining new camera data will use updated tracking data to minimize latency, which was one of the misunderstandings in how this was designed to work.

Similarly, we keep track of the location of our XROrigin3D node in order for us to update the XR camera position correctly. This currently isn't implemented thread safe either.

The final change that is needed specifically effects Android. As part of our submit logic in the default rendering pipeline, we manage an internal swapchain for our normal game presentation. The contents of this swapchain is then output to screen. On Android presentation is fully handled by OpenXR and this introduces unnecessary overhead. We can skip this part if there is nothing to present (our internal blit list will be empty).

This last one also kind of applies to PCVR, but we currently don't have a way to prevent the default window from opening as this is a core part of the display server. That is a separate issue for another day.

Steps to reproduce

n/a

Minimal reproduction project

n/a

@BastiaanOlij
Copy link
Contributor Author

cc @lyuma @m4gr3d

@BastiaanOlij BastiaanOlij self-assigned this Oct 17, 2023
@BastiaanOlij
Copy link
Contributor Author

OK, getting this done and making sure XR works properly with rendering happening on a separate thread has now become a higher priority. Turns out issues on Quest 3 were related to Meta assuming rendering happens on a separate thread and there are timing issues when this is not the case...

@Kimau
Copy link
Contributor

Kimau commented Mar 4, 2024

Excited for this change 👍

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

Successfully merging a pull request may close this issue.

2 participants