dolphin
dolphin copied to clipboard
dolphin-emu
OpenXR - Basic integration for Meta Quest
I am going to delete my DolphinXR fork. To not lose the OpenXR integration, I try again to get it merged (the first attempt was https://github.com/dolphin-emu/dolphin/pull/11723).
It uses https://github.com/amwatson/2DVrHybrid integration and is compatible only with Oculus/Meta Quest headsets.
I would appreciate smooth review. Last time you were requesting changes for one month and in the end you told me you do not want to merge it. This is a minimal implementation to make it work. Anything could be done in a follow-up PR.
The experience is out of the box quite bad. To get a smooth experience for most of the games:
- Enable AudioStretch
- Enable ImmediateXFBEnable
- Enable VISkip
- Enable OverclockEnable
- Set Overclock to 0.8
To improve the colors:
- Enable CorrectGamma
- Set GameGamma to 2.0
TODOs
- [x] Hybird (2D/VR) app support
- [x] Meta SDK license issues
- [x] Refactor structs (C -> C++)
- [ ] Move classes into other paths/namespaces
- [ ] PCVR support
- [ ] Stereoscopy
I would appreciate smooth review. Last time you were requesting changes for one month and in the end you told me you do not want to merge it.
First I need to address this comment. This is not how open source development works. No one is assigned these PRs, so devs may join late into the review process. No one may look at a PR for some time and then you might get an influx of devs looking at it a month or two later. Even if someone is looking at a PR, it's not always possible to spot everything in an initial review (especially for large PRs).
The overall process may take months for any number of reasons. This is what you saw on your last PR and you didn't want to continue because of that. And that's understandable. We are happy to take another look at this and we hope we can get it into a merge-able state but just wanted to let you know that the process hasn't changed and I didn't want you to have different expectations.
I will see if I can look over the code later in more depth this week but I discussed somewhat with other devs and we still largely have the same concerns as before. At a bare minimum, we consider these blockers:
- There are input/graphics changes in
Common. That is not the place for those changes. Graphics related code that is backend agnostic goes intoVideoCommon, backend specific code belongs in the particular backend target that it relates to (ex: OpenGL). Similarly, input details should go under inputs. - There is a binary file in the externals. We generally build our external libraries directly inline with the source. We aren't comfortable with including the binary directly in the code base.
@iwubcode I know how opensource works. Adding a new platform or hardware support is always problematic. Most of the opensource projects are in cases like this more supportive.
To address your comments:
There are input/graphics changes in Common. That is not the place for those changes. Graphics related code that is backend agnostic goes into VideoCommon, backend specific code belongs in the particular backend target that it relates to (ex: OpenGL). Similarly, input details should go under inputs.
If you are talking about Common/VR then it is a common topic with OpenXR. The OpenXR is connection of render/input/tracking, splitting it into different modules usually leads after some revisions into input delays and lags. For great experience it is better to keep the code in the same module.
There is a binary file in the externals. We generally build our external libraries directly inline with the source. We aren't comfortable with including the binary directly in the code base.
This could be done but it might be quite a big chance. I am willing to do it in a follow-up PR. This project is using the runtime loading of OpenXR: https://github.com/amwatson/CitraVR (it could be used as a reference).
@CrossVR Thank you that you self-assigned yourself to this. Do you have Quest headset?
There is a binary file in the externals. We generally build our external libraries directly inline with the source. We aren't comfortable with including the binary directly in the code base.
Building the loader from source for Meta Quest is currently not possible because it uses a custom OpenXR loader from their SDK. Unfortunately this is another reason we can't include the library, because the license on it explicitly prohibits it:
You may not (or allow those acting on your behalf to): 1.2.8 use or redistribute the SDK or any portion thereof in any manner that would cause the SDK (or any portion thereof) or MPT to become subject to the terms of any open source license or other restrictions.
Thank you that you self-assigned yourself to this. Do you have Quest headset?
I have a Quest 1 in storage, but I think it requires a Meta account now which I'm not interested in registering for. I'm planning to either use the Meta XR Simulator or hook up my Oculus Rift, though either of these options probably requires the PR to have PC support. Having support for the simulator in particular would be a great aid for review and maintenance because any developer can run it without needing access to the hardware.
Building the loader from source for Meta Quest is currently not possible because it uses a custom OpenXR loader from their SDK. Unfortunately this is another reason we can't include the library, because the license on it explicitly prohibits it.
I will check how much work is needed to make it work without the library in externals.
I have a Quest 1 in storage, but I think it requires a Meta account now which I'm not interested in registering for. I'm planning to either use the Meta XR Simulator or hook up my Oculus Rift, though either of these options probably requires the PR to have PC support. Having support for the simulator in particular would be a great aid for review and maintenance because any developer can run it without needing access to the hardware.
The simulator is for PCVR only. I am not going to add support for that because it would make the PR even bigger and harder to merge.
The simulator is for PCVR only. I am not going to add support for that because it would make the PR even bigger and harder to merge.
Lines of code alone do not determine how hard it is to get a PR merged. When maintainers look at a PR they think in terms of: "Am I going to be able to maintain this code if the original author suddenly disappears?". So features that make the code easier to test and maintain make a PR more likely to get merged.
I believe this is also one of the reasons other efforts have failed in the past. The first developer to attempt OpenXR support didn't even have access to a headset, nevermind the reviewers of such a PR (#8380). Let's give everyone a chance to contribute to this effort by removing that barrier from the start.
@CrossVR hey, nice to meet you, btw! Been following along in the Dolphin Discord a bit.
Building the loader from source for Meta Quest is currently not possible because it uses a custom OpenXR loader from their SDK. Unfortunately this is another reason we can't include the library, because the license on it explicitly prohibits it.
I'm excited to say it is now possible. If you want, @lvonasek can use my solution from the CitraVR project for using a custom OpenXR loader on Quest 3.
The non-proprietary Khronos version of the OpenXR loader is under the Apache license, requires 0 opaque binaries and has 0 dependencies on the Meta Quest SDK. It should be GPLv3-compatible.
Hope it helps.
The non-proprietary Khronos version of the OpenXR loader is under the Apache license, requires 0 opaque binaries and has 0 dependencies on the Meta Quest SDK. It should be GPLv3-compatible.
Definitely aware of the standard loader, but I didn't know that Meta Quest compatibility was ready that's great news. Yeah this PR should use the standard loader in that case.
@CrossVR hey, nice to meet you, btw!
Likewise!
@lvonasek happy to add a PR to your branch/otherwise help, if you want
I have it already working but it will break compiling on Windows and I have no idea how to fix that. I'll try to push it today.
Anyway I would appreciate any help on the PR. The requirement to support PCVR is something I cannot fulfill myself.
I got rid of the binary from Meta SDK. Kudos to @amwatson for her work on this. It requires two submodules:
- OpenXR-SDK-Source - patched SDK to work on Meta Quest OS V62 (the current one)
- OpenXR-SDK - should satisfy compiler on Windows (and maybe other platforms)
@CrossVR, Could we move this out of my fork into a branch in this repo? If people contribute in my fork then it won't go through the tests and the PR might theoretically get worse. Don't get me wrong, I am happy for all contributions
I will be off for at least a week (I am close to burnout).
I had a small exchange with OpenXR maintainer here: https://github.com/KhronosGroup/OpenXR-SDK-Source/pull/454
Once the next OpenXR SDK is released (expected this month), I could switch to the official submodule and not using the forked OpenXR-SDK. However I will still need that someone adds KhronosGroup/OpenXR-SDK as a trusted host on the build machine(s).
@CrossVR, Could we move this out of my fork into a branch in this repo? If people contribute in my fork then it won't go through the tests and the PR might theoretically get worse. Don't get me wrong, I am happy for all contributions
If we put it in a branch in the main repo we lose this PR as a place of discussion and review. I don't think there's anything wrong with contributing to a fork and then merging it through a PR to the main repo when it's ready.
As far as CI and tests are concerned, you will still get those tests when a PR is merged on your fork just not before. I don't think we need that many PRs to get it in a merge-able state where that becomes a bottleneck.
I will be off for at least a week (I am close to burnout).
No problem, don't overwork yourself especially not for an open-source project. Take care of your health first. 👍
No problem, don't overwork yourself especially not for an open-source project. Take care of your health first. 👍
Thank you @CrossVR. As I am not touching the code these days, will you try to make it working on PCVR?
Another possible contribution which requires better understanding of the render pipeline is the stereoscopy. Currently the right eye is always black. Adding stereoscopy should be a few lines of code change but I didn't manage to make it work.
Thank you @CrossVR. As I am not touching the code these days, will you try to make it working on PCVR?
I am planning to work on that yeah, but let's not get too focused on PCVR. The more important task is to refine the code quality, this comment is a good start for that: https://github.com/dolphin-emu/dolphin/pull/12564#discussion_r1480533225
Another possible contribution which requires better understanding of the render pipeline is the stereoscopy. Currently the right eye is always black. Adding stereoscopy should be a few lines of code change but I didn't manage to make it work.
I can help with that, I wrote the current stereoscopy implementation in Dolphin. When enabling stereoscopy the EFB becomes a layered framebuffer with two layers. These two layers can then be submitted for the left and right eye buffers.
Currently it looks like you're taking the framebuffer and then blitting it into a single layer swapchain, this would only copy the first layer. However it would be more efficient (especially on a mobile chipset) to use the swapchain for the framebuffer directly to avoid blitting. This would not only fix stereoscopy, it would also eliminate most of the OpenGL code and provide an easier path to supporting more backends.
Only issue would be how to draw the cursor, since you wouldn't want to draw that into the framebuffer. However you can leverage the compositor for this by using a quad layer as the cursor. This would require some trigonometry to correctly position onto a curved layer though.
It would be great to get rid of the blitting (and the VRDisplay class).
I do not see the problem with the cursor. I have the cursor support in the code but it is unused in Dolphin.
Edit: VRDisplay class will likely have to stay due to XrSwapchain.
Quick update, I need a few more days to finish up some other stuff for Dolphin that's taking a bit longer, then I'll completely shift my focus to this PR.
@CrossVR, I refactored the structs as required. However I have no idea how @iwubcode's request should be done.
Currently, there is a clear dependency structure where "DolphinVR" is basically an API to VR (all other classes aren't used other than through DolphinVR class):
In Meta OpenXR SDK examples is all the code in one huge class (which is hard to read and might be hard to maintain if too many features are added). I use this structure:
- DolphinVR is integrated with the emulator and holds all instances to other VR modules
- OpenXRLoader takes care of loading OpenXR implementation
- VRBase calls OpenXRLoader, initializes VR and holds XrSession and XrInstance
- VRInput requires VRBase, handles controller mapping, tracking and updating
- VRRenderer requires VRBase, handles head tracking, rendering and app events
- VRFramebuffer is the only class which calls OpenGL to maintain XrSwapchain framebuffers
I added TODO list into the PR header to keep track of the progress. Feel free to pick anything, I am not touching this PR code until 26.2.
@CrossVR feel free to remove multiview support if it makes the framebuffer refactor easier.
Give me know, if you need any help or support from me.
@CrossVR do you have a time estimation when you look into this?
Due to lawsuit Nintendo vs Yuzu, I want to delete my fork ASAP.
@lvonasek I was away for a week, I was hoping I could work on it before I went away but ran out of time. Now that I'm back I have plenty of time, sorry for the delay.
Due to lawsuit Nintendo vs Yuzu, I want to delete my fork ASAP.
Even with your fork deleted you will still be listed as a contributor on the main repo if this PR is merged. If the lawsuit has made you reconsider whether you want to be a contributor it would be better to close the PR now.
Even with your fork deleted you will still be listed as a contributor on the main repo if this PR is merged. If the lawsuit has made you reconsider whether you want to be a contributor it would be better to close the PR now.
Maybe I am too paranoid but I have problems to be on the contributor list. @CrossVR, would you be willing to open a new PR and lead this? (of course with me supporting you)
I did some changes to satisfy @iwubcode´s requirement. Feel free to revert it as you want.
Maybe I am too paranoid but I have problems to be on the contributor list. @CrossVR, would you be willing to open a new PR and lead this? (of course with me supporting you)
If you don't want to be a contributor, then it's best if I start from scratch and do a new PR based on my own work. I'd focus on PCVR first, but of course I'd welcome advice on how to add Meta Quest support to it.