Allow customization of OpenXR application name, version, required layers. #197
Conversation
jmgao
force-pushed
the
openxr
branch
2 times, most recently
from
b02852f to
18bda61
Compare
There was a problem hiding this comment.
This looks fantastic as always @jmgao 👍🏻
Have left a couple of notes in the PR!
examples/crab-saber/Cargo.toml
Outdated
| @@ -19,7 +19,7 @@ rand = "0.8.0" | |||
| approx = "0.5" | |||
|
|
|||
| [target.'cfg(target_os = "android")'.dependencies] | |||
| ndk-glue = "=0.6.0" | |||
| ndk-glue = "0.6" | |||
There was a problem hiding this comment.
There's historical reasons for this, unfortunately.
We MUST keep our ndk-glue in lockstep with the rest of the ndk-glue ecosystem. Several upstream packages, such as openxr and cpal will break if the ndk-glue version gets out of sync.
I'd prefer to leave this as is for now, and approach this by switching to ndk-context as documented here:
There was a problem hiding this comment.
Doesn't cargo's semver resolution solve this? My understanding is that compatible versions of libraries (i.e. 0.6.x) are required to resolve to one specific version. The problem that this is intended to fix is that openxr no longer depends on ndk-glue, but depends on ndk-context instead, but an application using hotham can't force the version of ndk-glue to 0.6.2, which is the version that starts using ndk-context.
There was a problem hiding this comment.
re: semver resolution, no. I thought the same thing but was unpleasantly surprised when things blew up my face on a fresh build when something in the ndk-glue ecosystem had been updated. To be clear, we could still get bitten by other dependencies but this is still a start.
re: openxr and ndk-context can you expand on that a bit further?
There was a problem hiding this comment.
When we update to the next version of openxr (or in my case, if you use [patches.crates-io] to move to a top of tree version to get access to XR_FB_display_refresh_rate), things will break, because openxr is removed its dependency on ndk-glue and added one on ndk-context instead.
In version 0.6.2, ndk-glue moves its global state into ndk-context and properly initializes ndk-context, so if a sufficiently new version of ndk-glue is used for initialization, crates that depend on ndk-context will also work. However, we're forcing the version to exactly 0.6.0, so the result is that hotham applications will have use the old ndk-glue that doesn't initialize ndk-context, and a new version of openxr will use an uninitialized ndk-context and immediately crash.
(BTW, we're not in lockstep with the rest of the ndk-glue ecosystem: cpal uses 0.6, and openxr used 0.6, and effectively will be >0.6.2)
There was a problem hiding this comment.
Completely agree @jmgao - moving to ndk-context is the way to go. I'd just like to approach that in a separate PR. 😄
There was a problem hiding this comment.
Please remove the changes to ndk-glue for this PR - we'll handle that separately in #201
Make it so that applications can configure a bunch of OpenXR stuff by adding an EngineBuilder and XrContextBuilder to plumb things down to.
While we're at it, fix the android build, and make the ndk/ndk-glue versioning less restrictive so that top of tree openxr works.