GodotVR / godot_openxr_vendors

Godot 4 wrapper for OpenXR vendors loaders and extensions
MIT License
92 stars 19 forks source link

Add Pico Loader #13

Closed rsjtdrjgfuzkfg closed 1 year ago

rsjtdrjgfuzkfg commented 1 year ago

This PR contains two changes, which build upon each other:

Fixes #8

See also the main Godot PR for partial Pico 4 support: https://github.com/godotengine/godot/pull/68023

Historical note: Earlier versions of this PR did include a mock implementation of the two classes used by the plugin instead of a Maven dependency and did not include the loader binary due to licensing concerns, which since have been resolved.

BastiaanOlij commented 1 year ago

On the subject of GodotLib, agreed this needs to be shared amongst loaders, not 100% sure if having a stub makes sense, indeed the file is not compiled in, we use the one that is supplied during export. I did change it in #12 to download the latest version as part of CI.

I'm going to see if we can get a contact at PICO somehow and find out what their legal stance is on including the loader binary. It seems silly they don't allow that. We have to resolve this somehow even if this means downloading the binary in the CI script to embed them into the deliverables or no normal user will ever be able to use PICO devices (not up to us to make this possible I guess)

Other than that, great work so far, feels like we're getting pretty close :)

BastiaanOlij commented 1 year ago

On a side note, don't forget to update CHANGES.md and add your name to CONTRIBUTORS.md :)

rsjtdrjgfuzkfg commented 1 year ago

@BastiaanOlij I can switch the godot-libs project to a binary version if you want? Or are you fine with this PR introducing the mock?

At least with the current scope of this repository I don't see an advantage in keeping the binary around, while the disadvantage is as added complexity to download and/or occasionally update an additional blob.

Imho the cleanest long-term solution without a mock would require godot's lib (or an official mock, given that it always is a compile-time dependency) to be published to some maven-compatible repository, then resolve it through gradle just like the other dependencies. I assume that would also benefit more complex android plugins.

BastiaanOlij commented 1 year ago

@rsjtdrjgfuzkfg I kinda want @m4gr3d input here, I don't mind the mockup but I'm just worried that it may become a limitation.

I believe @m4gr3d publishes the lib on Maven but he may only do so for Godot 3.

rsjtdrjgfuzkfg commented 1 year ago

@BastiaanOlij as discussed via mail, I updated the PR to use the artefact on Maven and to include the Pico binary, which was clarified to be redistributable by Pico.

BastiaanOlij commented 1 year ago

Looks good @rsjtdrjgfuzkfg , could you squash the two commits and then I'll merge it