Implement XrContext for ARKit#833
Conversation
chrisfromwork
requested review from
Alex-MSFT,
bghgary,
rgerd,
ryantrem,
syntheticmagus and
tymlipari
| PUBLIC "include") | ||
|
|
||
| target_link_to_dependencies(napi | ||
| target_link_libraries(napi |
There was a problem hiding this comment.
Was this change intentional?
There was a problem hiding this comment.
yes depending on cmake version target_link_to_dependencies doesn't exist as a valid cmake command. I'm not sure why we've been using it here.
There was a problem hiding this comment.
actually looks like this was a custom command. Do we have more context around why we've been using this custom command:
There was a problem hiding this comment.
I believe this was originally introduced, among other reasons, to copy DLLs for V8 on Windows. Does that still work with this change?
There was a problem hiding this comment.
I may be naive/wrong here, but it doesn't look like we are setting ON_LINKED_AS_DEPENDENCY_CMAKE_FILES for this library, so I don't think using the custom target_link_to_dependencies is necessary here?
There was a problem hiding this comment.
if folks want this reverted, I can look into updating our consuming projects to locate this target_link_to_dependencies function. That going said, if it doesn't serve any additional purpose it seems worth converting back to target_link_libraries so that external consumers of napi jsi don't have to include these additional cmake function definitions
There was a problem hiding this comment.
given this could potentially have issues on UWP but I've been primarily testing on iOS, I'm just going to refer this change.
There was a problem hiding this comment.
We will be using add_on_linked_as_dependency_cmake_file for napi-jsi in order to pull in v8jsi dll's (see #832). I also thought CMakeLists globally enforced a minimum version across files, so I'm not sure why iOS specifically would have issues consuming the custom function, unless there's a file with a version setter that needs to get revved.
| PUBLIC "include") | ||
|
|
||
| target_link_to_dependencies(napi | ||
| target_link_libraries(napi |
There was a problem hiding this comment.
I believe this was originally introduced, among other reasons, to copy DLLs for V8 on Windows. Does that still work with this change?
|
|
||
| struct System::Impl { | ||
| public: | ||
| std::shared_ptr<XrContextARKit> XrContext{std::make_shared<XrContextARKit>()}; |
There was a problem hiding this comment.
Is this ownership actually conceptually shared? If not, could this be a simpler type?
There was a problem hiding this comment.
I don't think it will be possible to hand a shared pointer across dll/lib boundaries. That going said, from a legibility standpoint id prefer this to stay a pointer compared to a memory reference to a struct field. We could change this to a unique_ptr if that's preferred over shared_ptr?
There was a problem hiding this comment.
talked offline, opted to switch to unique_ptr. this should now be fixed.
4 participants
This review contains the following changes:
Note: these changes are needed for iOS XR service scenarios such as Azure Spatial Anchors support.