Skip to content
This repository was archived by the owner on Jun 14, 2024. It is now read-only.

Conversation

@chrisfromwork
Copy link
Contributor

@chrisfromwork chrisfromwork commented Jan 2, 2020

Please view #308 before this pull request.

This review includes the following changes:

  1. The preprocessor directive required to enable qr code detection is no longer needed. The QR Code binary is now included by default in the SpectatorView.Unity project.
  2. A script is added to the repo that begins to enable nuget package generation for the spectator view source.
  3. A SpectatorView.ExternalDependencies asmdef is added that references the QR Code binary.

Note: this change requires the following change in the MSBuildForUnity repo: microsoft/MSBuildForUnity#94

Breaking Change Details

Notes:

With this change, folks no longer have to set a preprocessor directive include the QR Code Plugin binary. This means that codes will always need the QR Code Plugin dll for things to compile. Therefore, we are now distributing the QR Code Plugin with the main spectator view codebase and folks will need to delete any MixedReality-QRCodePlugin symbolic links from existing projects.

Migration Instructions:

  1. Open your project's assets folder in windows file explorer.
  2. Delete the symbolic link to MixedReality-QRCodePlugin.

assetBundleVariant:
script: {fileID: 11500000, guid: 0e4dca1acba66fd478b3854f09a32ec7, type: 3}
buildEngine: 0
profiles: []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these new meta files exist? Seems like something that might be missing from the .gitignore since I don't see the corresponding files, just the meta files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the call is on this. I'll ask andrei today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we want to include these files, there may be something wrong if we aren't including csprojs .I will follow up in a different review.

Copy link
Contributor

@matthejo matthejo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@chrisfromwork chrisfromwork merged commit 5faf097 into microsoft:master Jan 8, 2020
@chrisfromwork chrisfromwork added the breaking change Pull request contains a breaking change and requires additional information. label Jan 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking change Pull request contains a breaking change and requires additional information.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants