ultraleap
Move OpenXR to main Tracking package
Summary
This pull request:
- Moves the OpenXR functionality from the separate OpenXR package to the main
Trackingpackage as a separateasmdef. - Uses conditional inclusion to only include that
asmdefif the Unity OpenXR plugin package is installed as part of the project. - Moves the OpenXR examples into the main Examples package.
This means that OpenXR functionality for tracking is available if the project is using OpenXR, but is otherwise hidden.
Contributor Tasks
- [ ] Pair review with a member of the QA team.
- [ ] Add any release testing considerations to the MR for the next release.
- [x] Check any relevant CHANGELOG files have been updated.
- [ ] Ensure documentation requirements are met e.g., public API is commented.
- [x] Consider any licensing/other legal implications for this MR e.g. notices required by any new libraries.
- [x] Add any relevant labels such as
breakingto this MR. - [x] If this MR closes a Jira issue, make sure the fix version on the JIRA issue is set to the correct one.
Reviewer Tasks
- [x] Code reviewed.
- [x] Non-code assets e.g. Unity assets/scenes reviewed.
- [ ] Documentation has been reviewed. Includes checking documentation requirements are met and not missing e.g., public API is commented.
- [ ] Checked and agree with release testing considerations added to MR for the next release.
- [x] Check non-OpenXR projects compile with no errors.
- [x] Check functionality is usable in OpenXR projects with no errors.
Closes JIRA Issue
Tested this locally, it works a charm! (this is both as a UPM and .unitypackage :) )
I did notice that when the Unity OpenXR package is not included, our OpenXR Example can still be added but then has missing scripts (as you'd expect)
perhaps we should add an extra line to the comment on the OpenXRProvider explaining that it requires the Unity Package?
I did wonder if something like the OpenXR examples does start to make the case for the separate example sets and you select what you install. You could put the fact it required the OpenXR plugin as part of the description for that, but then you'd recreate the issue where you can't reference content in the main example scenes.
The comment would be easy to update though, in the absence of a better solution?
I did wonder if something like the OpenXR examples does start to make the case for the separate example sets and you select what you install. You could put the fact it required the OpenXR plugin as part of the description for that, but then you'd recreate the issue where you can't reference content in the main example scenes.
I think if we kept it behind a separate example Sample we would be reducing its visibility?
The current setup feels fine to me, but that would change if the example had its own scripts (it would cause errors unless we used the OpenXR #ifdefs) or extra content
I think if we kept it behind a separate example Sample we would be reducing its visibility?
The current setup feels fine to me, but that would change if the example had its own scripts (it would cause errors unless we used the OpenXR #ifdefs) or extra content
Thinking more about this, I feel like this might be somewhat of moot point when we move towards a single service provider, as there will no longer be separate examples at that point. I think with that in mind, adapting the comment for now would probably be the best solution for now.
Happy with the state of this, just want to make sure it's looked at by QA before approving :)
What have people looked at here from a test PoV?
@MattGrayUL did you run through the tests you'd mentioned yesterday when you pulled this?
So to test it, we would need to: Import Tracking package into a project that doesn't have OpenXR Unity package installed Make a build of any old scene Add OpenXR unity package ensure UL tracking is ticked in OpenXR plugin management Run a scene that isn't the OpenXR example scene Run the OpenXR example scene Test for tracking issues, so hands interlocking etc Build the OpenXR example scene and test that too
It sounded quite extensive so wasn't expecting you to have covered all of this when you looked at it. I'm currently drawing out some further considerations for testing this against different runtimes and headsets. As this won't go in until 6.1.0 now maybe it's worth performing some of the headset and runtime tests at this point(pre merge into develop) also.
@MattGrayUL did you run through the tests you'd mentioned yesterday when you pulled this?
@Greg-UL I did everything except the builds when I did my dev testing 🙂
@MattGrayUL I still maintain building an OpenXR app from this would be a good shout but I also think there's some extensive testing that's been done here. Please proceed
Ah @rblenkinsopp, did you say about keeping the old package.json around so people don't get errors, even though the package itself would be empty?