Partial support for Pico 4 #68023
Partial support for Pico 4 #68023
Conversation
|
I'd love input form the community to overcome the limitations in this PR, towards full support for the Pico 4. I update this comment with new findings as I make them. If desired, we can move this to a different issue instead of discussing on the PR. Either way, I hope for an expert to support me or take over (@BastiaanOlij ?). I'm stuck, so I'm looking forward to input on what else to try or how to approach the problem in a new direction. :) The main blocker is that adding shaded or otherwise textured meshes with mobile renderer caused failed pipeline creation: adding a simple mesh without explicit material causes vkCreateGraphicsPipelines to fail with error -13 ( Current best guess: something is enabled in the mobile renderer whenever The rest of this post gives some information on what I tried so far or that I considered relevant to the question. Feel free to ask for clarifications / more details when I missed something! Vulkan Validation Layer Vulkan validation layers give quite a few violations, but I believe the issues identified before the pipeline creation either are not relevant or fixable, but the fix did not help:
This warning seems to be caused by the app starting in non-VR mode (until a script sets the viewport's This warning also occurs when starting godot on a scene that does render properly.
Switching to This warning also occurs when starting godot on a scene that does render properly.
Can be fixed by passing the correct formats, but that messes up colors (as explained in the comment in the source code, we're intentionally violating spec there) and does not fix the actual rendering issues. Maybe it could also be fixed by setting This warning also occurs when starting godot on a scene that does render properly.
We pass unitialized memory as This warning also occurs when starting godot on a scene that does render properly.
This warning appears when switching from the non-VR to the VR swapchain. I initially believed it is likely this is caused by the timing of the switch, but this warning is triggered again later on, possibly for each frame. This warning also occurs when starting godot on a scene that does render properly. Similar warnings will then continue throughout the run of the application, likely every frame.
This warning appears due to us passing a VK_STRUCTURE_TYPE_RENDER_PASS_MULTIVIEW_CREATE_INFO; a comment suggested it is no longer needed, I did remove it and that fixed the warning without changing anything else (as far as I can see, at least). This warning also occurs when starting godot on a scene that does render properly.
Adding This warning also occurs when starting godot on a scene that does render properly.
Passing the This warning also occurs when starting godot on a scene that does render properly. Debugging / Experimentation Besides looking at validation layers, I also did some debugging on possible causes, both with a debugger and by experimenting, but that did not yield tangible results. From what I tested, any mesh with a shader using The forward_plus renderer on the other hand does kind of work in VR. With this PR applied, the scene renders all elements but I get a lot of vulkan validation layer issues, the performance is terrible and the display and tracking of the Pico 4 occasionally glitches. I did not spend a lot of time debugging the forward_plus renderer, though. In scenes that do render properly, there are still constant validation errors (VUID-vkQueueSubmit-fence-00064, UID-vkQueueSubmit-fence-00063, VUID-VkPresentInfoKHR-pImageIndices-01296), not sure if these are normal or also related to a Pico-specific issue. Performing the changes in this PR in Godot 3.5 with the add-on mostly works as expected. The demo scene renders properly. The issues thus seem to be Vulkan-related. The only issue in 3.5 is that the controllers are stuck on the simple_controller interaction profile, which seems fixable though I did not instantly see how (the same issue likely also exists in 4.0, but I did not get that far ;) ). Metadata that might help Vulkan and OpenXR information / Metadata what the Pico 4 looks like from Godot for reference (from --verbose startup): |
|
Hey @rsjtdrjgfuzkfg , The current version of Godot will attempt to load the loader externally, have a look at: https://github.com/GodotVR/godot_openxr_loaders @kisg @konczg , I was surprised that we no longer have I am worried about all the errors you're getting in the log, Godot certainly will be slowed down a lot if its spamming the log on each frame. Sounds like the PICO has very incapable Vulkan drivers. That said, @clayjohn and I have been hunting situations where GPUs that only support Vulkan 1.0 and non of the required extensions aren't being properly detected and therefor having Godot wrongfully try to do things that cant be done. I would have expected PICO to have pretty up to date Vulkan support though.. |
|
@BastiaanOlij thanks for your response! Yes, the loader can be distributed as android plugin, placing it in libs as explained in the first post is just to speed up testing and not a permanent solution. It is not necessary to make changes beyond this PR in core godot, I tried to keep everything similar to how Meta/Oculus is handled at the moment (metadata in core, loader from an asset). If it helps I can also provide you with a PR against GodotVR/godot_openxr_loaders for everything but the .so file? I'd not be comfortable to include the .so file myself for legal concerns around the loader's license not permitting redistribution outside of concrete applications. That said, we might get a clear official permission from Pico to redistribute the loader as part of the XR asset and its repository if we ask them nicely? I can ask, but somebody publicly associated with godot might have better chances.
To have something to compare against: did you run the current main branch on a Quest with validation layers? How much / which messages do you get there? I don't have a Quest to test. Quite a few of the messages seems to me to be obvious bugs in godot (like passing uninitialised memory, that can't be right, can it?). I plan to provide a PR for those once I get things to render, but as I'm not very experienced with Vulkan yet I want to take things one step at a time. |
|
You're joking that PICO does not allow redistribution of their loader? Whats wrong with these companies? Do they not want their devices to be used or something? That doesn't just effect us, that will make it harder for Unity and Unreal to distribute builds that support the PICO as well. It's a ****ing loader, there is no IP embedded in it, geez.... /end rant A few things in this PR do need to move into the loader and I'm waiting on @kisg and/or @konczg to react on the Android changes, we always had that bit in the Godot 3 plugin so I'm not sure why they haven't been reproduced here. It's possible with all their changes it already happens but through another mechanism. I don't remember seeing validation errors on my side but I have to admit most of my testing has been on desktop, once I'm through my current workload I will do some testing on my end as well. As for what stands out:
Ok, I think this is because we're still initializing Godot as a normal application, we initially allocate a buffer at the size of the display, which likely is a 4320x2160 panel, but it seems larger then their GPU allows (weird, very very weird). I guess we could check for min and max and clamp it, @clayjohn what do you think?
Not sure where that is coming from, I'm guessing when we create the subviews to access the individual layers of the stereo image. But that is code that is used all over the place in Godot so not sure why it suddenly doesn't like the setup.
That's really weird that we run into this now but good to know. For the most part Godot runs
This one is worrisome but also easy to fix if it is just uninitialized memory. I've noticed that the compilers used for Android don't clear memory the same way desktop compilers do, I've run into this before with other commands. edit having a search through the code, every place
I have a feeling this may just be a flow on effect of the failed
This one is weird because from the log the multiview extension is available. That said, things did change with the introduction of
That does make sense, indeed suggest to add that bit.
Indeed should be fixed by adding the bit. All in all I think the error caused by uninitialized memory is the one that is most important. |
|
Can you give #68102 a try? This I think will deal with the most pressing validation errors. |
|
Hi, we have national holidays today and tomorrow, that is why we have been silent. :) We will back on the 2nd November. |
|
@BastiaanOlij thanks for the review and #68102! I posted test results regarding the validation errors and a note regarding the device capabilities there. Similar to my more crude workarounds, the failed pipeline issue persists but most warnings were fixed. There is only one remaining warning that could point to the main issue (VUID-vkQueueSubmit-fence-00063) and one for each frame while rendering works (VUID-VkPresentInfoKHR-pImageIndices-01296).
Not intentionally, but IANAL, so maybe I just misunderstand their terms. The official unity / unreal plugins seem to only be available from their website, under the same license. Regarding this PR: I'll split things up as requested in the review. |
|
@BastiaanOlij well, it was too late for the PR against the loader repo :D From my POV the second commit on this PR is now obsolete, and replaced by GodotVR/godot_openxr_vendors#13 Edit: I also posted a comment with remaining validation layer issues in #68102, maybe seeing things in the exact order and context helps to identify remaining issues. |
I have checked and neither |
|
We have reviewed the addition of the Pico Loader to |
Yeah I think I'm mixing this up with the code we had that performed the android init even before the openxr instance is created. |
| jobject activity_object = env->NewGlobalRef(static_cast<OS_Android *>(OS::get_singleton())->get_godot_java()->get_activity()); | ||
|
|
||
| instance_create_info_next = memalloc(sizeof(XrInstanceCreateInfoAndroidKHR)); | ||
| *reinterpret_cast<XrInstanceCreateInfoAndroidKHR *>(instance_create_info_next) = { |
There was a problem hiding this comment.
Note that Godot has build in memory functions/macros that should be used.
That said, look at the other extensions where we add structures, the pattern we're following is to make this a member variable of the class.
There was a problem hiding this comment.
I switched to a static instance in the cpp to match the pattern you suggested (the vulkan extension uses a static class member, which has pretty much the same properties), but can't say I love it as it ties a static instance to all instances of the class. That said, it should be reasonably safe and work, as there is at most one instance. I guess some uglyness can't be avoided, just like the leaked global reference to the activity – which I copied, so now there are two of those ;)
Note that it would not be possible to place the instance into the class without major changes, as the struct is defined within openxr_platform.h, whose content varies depending on the #defines that were set up before the first include in each compile unit. As long as different extensions use different parts of openxr_platform.h, use of that header from other headers should be avoided.
I'd propose to also change the vulkan extension to match whatever pattern we decide on for the android extension, as the vulkan extension includes the header in the header already and thus might cause issues down the road. Alternatively we could also move all relevant #defines out of the extensions to make including openxr_platform.h safe from headers, though that likely comes with its own problems.
There was a problem hiding this comment.
The extensions are instanced as singletons so the variable itself can be a member variable of the class, it doesn't need to be static. Same for your JAVA instance variable.
The extension instances are also cleaned up when the OpenXR singleton is destroyed though I suspect there might be some timing issues there so that should allow us to do any cleanup needed.
Including the needed platform headers in order to properly include OpenXR has been a pain in my behind because it screws up other parts of the system. Especially on windows once windows.h is included it causes a lot of pain.
But it seems we can't internalize this enough, I wish there was a solid way to ensure only the .cpp files of the extensions were required to include the needed parts of OpenXR.
Defo welcome for suggestions but lets not let your PR hang on that improvement.
There was a problem hiding this comment.
it doesn't need to be static.
My comment on a static member was regarding the vulkan extension I looked at for reference, that one uses a static member for its next chain entry.
Same for your JAVA instance variable.
I can add cleanup for the jobject, but would prefer to treat both instances the same (so the cleanup should also apply to the instance passed via xrInitializeLoaderKHR in OpenXRAndroidExtension::on_before_instance_created). My assumption was, however, that the missing cleanup was intentional, as the standard does not clearly state whether OpenXR assumes ownership or creates its own reference.
Defo welcome for suggestions
Imho a void pointer in the class and memalloc memfree are a bit nicer, but I can live without that. But I think you did not want me to use those two, and have other godot memory macros to use? Either way, I'm also fine merging as-is.
I wish there was a solid way to ensure only the .cpp files of the extensions were required to include the needed parts of OpenXR
We could go the pimpl (header does not contain any implementation details) or pure abstract / interface route (header does not contain the class at all, but only a factory method). The latter is probably nicer. But both are not in the scope of this PR imho.
This commit adds support for the OpenXR extension XR_KHR_android_create_instance, which seems to be required on Pico devices.
|
A short update: behind the scenes @BastiaanOlij and myself were discussing with engineers from Pico, which permitted us to identify the rendering issue as bug or incompatibility within the current vulkan driver on the headset. Engineering builds of Pico OS supposedly already work with this PR. In addition, they provided us with the correct mapping for the Pico's controllers and cleared up our licensing concerns, so we're on a good track towards full Pico 4 support. \o/ |
There was a problem hiding this comment.
After talking with @rsjtdrjgfuzkfg , we can likely merge this PR though it mostly contains generic improvements to the Android support in OpenXR.
For PICO specific there are:
- the changes to the loader (which have now been merged)
- added PICO controller definitions, but this is waiting on both an addition to the OpenXR headers and finishing Various fixes for OpenXR action map meta data and editing #68528, we'll create a separate PR for this
- a driver upgrade on the PICO itself
|
Thanks! |
I attempted to port Godot 4 to the Pico 4, but so far did not succeed to get the whole engine up and running. This PR adds support for the necessary OpenXR extension and injects necessary metadata to permit the engine to start in VR mode.
With this PR, only extremely simple scenes will render properly (with the mobile renderer). I tested this with a scene with the usual XR nodes (origin, camera, controllers) and a world environment with sky shader and unshaded, untextured geometry, on a Pico 4 with Pico OS 5.1. As far as I see, the remaining issues are likely to reside either on the Vulkan side or in the OpenXR-Vulkan interactions.
I hope this PR helps people with more Vulkan / OpenXR / godot core experience to either complete the port or point me in the right direction so I can get further. I'll post a comment on this PR shortly with information about the issues I identified so far and some information about what I tried already. That said, it might be reasonable to merge it as-is and in the meantime and continue the discussion for full Pico 4 support elsewhere, as the two commits in this PR are self-contained.
Similar to Meta/Oculus Quest, Pico devices support OpenXR through a proprietary loader included in their OpenXR SDK download. To test this code in a crude way, it is sufficient to copy the 64-bit libopenxr_loader.so file to platform/android/java/app/libs/dev/arm64-v8a and rebuild the Android export template.