microsoft / MixedRealityToolkit-Unity

This repository is for the legacy Mixed Reality Toolkit (MRTK) v2. For the latest version of the MRTK please visit https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity
https://aka.ms/mrtkdocs
MIT License
6.01k stars 2.12k forks source link

Spectator View (Preview) README needs updating with new instructions #3169

Closed BenWoodford closed 5 years ago

BenWoodford commented 5 years ago

The instructions direct you to the discontinued ARKit Plugin on the Asset Store for one.

Plus, I feel like we really should bundle the required DLLs with MRTK? It's like that for pretty much everything else in MRTK after all. It's a lot of extra steps for DLLs that are going to usually look identical from one machine to the next.

EDIT: Also the Unity ARKit plugin is provided with the Preview release package

StephenHodgson commented 5 years ago

I think there was some question about the legality of redistribution of such dlls

BenWoodford commented 5 years ago

Another suggestion: Recommend a known-to-be-working vcpkg commit. 56ea0dca4098754cee6dba7766c9e38d1fe40165 seemed to work for me (at least it resolved the issue with tiff I was having and downgrades opencv to 3.4.1)

I would suggest perhaps vcpkg is not a very good system for this, as the only way to actually get specific versions is to pull a specific commit that happens to have older versions. I'm gonna grab all the license links so they can be reviewed for redistribution.

BenWoodford commented 5 years ago

License for: opencv_aruco341.dll, opencv_calib3d341.dll, opencv_core341.dll, opencv_features2d341.dll, opencv_flann341.dll, opencv_imgproc341.dll

License for: zlib1.dll

Can't imagine any reason that the SpectatorViewPlugin, which has no libraries with it, would not be redistributable.

BenWoodford commented 5 years ago

Seems the main objection to including them was here: https://github.com/Microsoft/MixedRealityToolkit-Unity/pull/1884#discussion_r177601177

@davidkline-ms why...? Policy or not it makes using the plugin considerably more difficult, especially as using specific versions through vcpkg is pretty damn unintuitive and using master seems to be prone to breaking and/or version updates that mean you have to roll back to a commit or modify the included library files due to OpenCV having versioned DLLs and Libs.

ras0219-msft commented 5 years ago

it resolved the issue with tiff I was having

@BenWoodford What issue were you having with tiff? Have you opened a bug on vcpkg?

@StephenHodgson if you wanted to add instructions to recommend 56ea0dca4098754cee6dba7766c9e38d1fe40165, you could do

>git clone -n https://github.com/microsoft/vcpkg
>cd vcpkg
>git checkout 56ea0dca4098754cee6dba7766c9e38d1fe40165
BenWoodford commented 5 years ago

it resolved the issue with tiff I was having

@BenWoodford What issue were you having with tiff? Have you opened a bug on vcpkg?

@StephenHodgson if you wanted to add instructions to recommend 56ea0dca4098754cee6dba7766c9e38d1fe40165, you could do

>git clone -n https://github.com/microsoft/vcpkg
>cd vcpkg
>git checkout 56ea0dca4098754cee6dba7766c9e38d1fe40165

I have updated the issue in question here: https://github.com/Microsoft/vcpkg/issues/4745

Tiff doesn't build on the lastest master, which consequently means OpenCV fails. Not that it matters massively for this as it's the wrong version of OpenCV for the plugin anyway.

Yoyozilla commented 5 years ago

@chrisfromwork is this part of the doc update you are doing for spectator view?

chrisfromwork commented 5 years ago

So, i have updated the clone instructions to also try master branch if the last known good commit fails. I agree with Ben that we should figure out a better mechanism for distributing the plugin dll's so that developers don't have to obtain opencv/compile native code. Right now its seeming like we may be able to use Nuget packages for this. But it is still TBD.

chrisfromwork commented 5 years ago

I'm closing this issue for now based on a lack of activity. If you are still blocked by not being able to obtain spectator view native plugins, please reopen this issue.