Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing remote mapping for IL2CPP support #948

Conversation

keveleigh
Copy link
Contributor

@keveleigh keveleigh commented Sep 8, 2017

See #265

This also fixes a bug where spatial mapping file saving didn't properly get data from the remote mapping source.

Copy link

@angelaHillier angelaHillier left a comment

Choose a reason for hiding this comment

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

Worked for my project. Thanks!

Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

Should this be ported to the master branch as well?

@keveleigh
Copy link
Contributor Author

keveleigh commented Sep 12, 2017

@StephenHodgson Yep, it should! I put it in this branch first, since porting efforts to Windows Mixed Reality are more likely to come from IL2CPP, as opposed to HoloLens apps. It's definitely a fix that needs to go everywhere though.

Edit: Actually, the remote mapping fix is really a HoloLens specific fix too.

@StephenHodgson
Copy link
Contributor

Good to hear. Please be sure to keep the issue open until it's merged into the master branch.

@StephenHodgson
Copy link
Contributor

In fact I wouldn't mind seeing a new PR for this along side this one.

@keveleigh
Copy link
Contributor Author

Will do! I believe GitHub does that by default, auto-closing the issue when the PR actually gets merged into master.

@StephenHodgson
Copy link
Contributor

StephenHodgson commented Sep 12, 2017

Just have to remove the first line of your initial comment to prevent that.

I'd change it to See: #265 instead of Fixes.

@keveleigh
Copy link
Contributor Author

I think this is the preferred way to do it, since GitHub doesn't close the issue until the PR is merged into the default branch.

Of course, if we're merging a separate PR into master, the contents of this one doesn't really matter haha.

@StephenHodgson
Copy link
Contributor

StephenHodgson commented Sep 12, 2017

That's what I'm saying. I would like to close the issue with the other PR because it's on the master branch.

@keveleigh
Copy link
Contributor Author

keveleigh commented Sep 13, 2017

It's possible I'm misunderstanding, since merging this into the dev branch would not have closed the issue automatically, but I changed the wording anyway.

@StephenHodgson
Copy link
Contributor

Ah I see. Thanks for clarifying

@keveleigh
Copy link
Contributor Author

PR into master opened: #965

@StephenHodgson StephenHodgson merged commit 52ce945 into microsoft:Dev_Unity_2017.2.0 Sep 13, 2017
@keveleigh keveleigh deleted the FixingRemoteMappingForIL2CPP branch September 13, 2017 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants