Support interacting with ARKit XR system in external native modules#260
Support interacting with ARKit XR system in external native modules#260chrisfromwork merged 8 commits intoBabylonJS:masterfrom
Conversation
chrisfromwork
requested review from
Alex-MSFT,
bghgary,
rgerd,
ryantrem,
syntheticmagus and
tymlipari
| #if __has_include("jsi/jsi.h") | ||
| namespace Babylon::Plugins::NativeXr | ||
| { | ||
| bool TryGetNativeAnchor(facebook::jsi::Runtime& jsiRuntime, facebook::jsi::Value& jsAnchor, ARAnchor*& nativeAnchor) |
There was a problem hiding this comment.
I think I prefer to use reference to pointer, but I feel like the pattern in OpenXR/Vulkan/D3D is usually to do pointer to pointer. Idk which is actually better, just wanted to point it out
There was a problem hiding this comment.
Also, without looking too hard I think the first two references could be const all the way down
There was a problem hiding this comment.
I agree on switching things that aren't getting set to constants. I think that a reference to a pointer is more legible than a double pointer personally.
There was a problem hiding this comment.
I am going to leave things as they are for now. jsiRuntime.global() isn't marked as a const operation so we can get away with handing around a const jsiRuntime here.
| bool TryGetNativeAnchor(facebook::jsi::Runtime& jsiRuntime, facebook::jsi::Value& jsAnchor, ARAnchor*& nativeAnchor) | ||
| { | ||
| nativeAnchor = nullptr; | ||
| uintptr_t nativeAnchorPtr{reinterpret_cast<uintptr_t>(nullptr)}; |
There was a problem hiding this comment.
going to leave as uintptr_t to stay consistent with rest of file/avoid editing the rest of the file.
|
|
||
| #if __has_include("IXrContextARKit.h") | ||
| #include "IXrContextARKit.h" | ||
| #if __has_include("jsi/jsi.h") |
There was a problem hiding this comment.
This feels like something that should be handled at the CMake level (like we do with AppRuntime* etc) instead of doing compile-time if's
There was a problem hiding this comment.
these files can be referenced by anyone independent of whether they use cmake
2 participants
This review contains the following changes:
Testing changes: