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

Windows.* namespace conflict #4168

Closed radicalad closed 5 years ago

radicalad commented 5 years ago

Describe the bug

The addition of the Microsoft.MixedReality.Toolkit.Windows.* namespace has created a namespace conflict with Windows.*. Typically the work around is to append UnityEngine or System, but in the case of Windows, it is the root of the namespace, and will always assume you're referencing Microsoft.MixedReality.Toolkit.Windows.*.

The only way around the issue is to make a short hand reference to the namespace: using winPerception = Windows.Perception.Spatial. Its clunky and awkward.

To reproduce

Inside of the Microsoft.MixedReality.Toolkit.* namespace try referencing Windows.*.

Screenshots

image

image

Your Setup (please complete the following information)

Target Platform (please complete the following information)

keveleigh commented 5 years ago

Are you able to work around this by just referencing the class name directly, after adding the using? We use references declared in the Windows namespace in some scripts, like https://github.com/Microsoft/MixedRealityToolkit-Unity/blob/mrtk_release/Assets/MixedRealityToolkit.Providers/WindowsMixedReality/WindowsMixedRealityArticulatedHand.cs#L232 without doing anything specific.

radicalad commented 5 years ago

Nope, it works in that file because the namespace is scoped past the conflict. Notice in my first screenshot I have a using Windows.Storage.

I know this is less of an issue for people consuming MRTK. I just wanted to point it out because I spent half the day yesterday thinking my assemblies were screwed up or I was missing a reference.

julenka commented 5 years ago

@keveleigh is this something that @andreiborodin is owning, since he worked on namespaces? I'm unclear on who would take this bug on.

andreiborodin commented 5 years ago

Yeah, @julenka, this should be assigned to me most likely.

chrisfromwork commented 5 years ago

It's a bit unclear whether this breaking change will get accepted, but Andrei is actively looking into it.

wiwei commented 5 years ago

Note that it's possible to workaround like this as well:

https://github.com/microsoft/MixedRealityToolkit-Unity/blob/9ab0e9b69afe45f7856b5731436ae427c3fe7cc4/Assets/MixedRealityToolkit/Definitions/InputSystem/SpeechCommands.cs#L49

It might be possible to do a using with this at the top. Nevertheless, it's still annoying because it breaks the intellisense flow.

I ran into this a while back and raised the issue and there wasn't a lot of traction at that time. Given the scope of changes here I don't know if it would make sense to take yet another breaking change if it doesn't affect consumers...

wiwei commented 5 years ago

If someone is willing to do the work to write a migration script for customers then I think it's far less concerning, but otherwise it just seems like the tradeoff isn't worth it.

radicalad commented 5 years ago

but otherwise it just seems like the tradeoff isn't worth it.

I agree - mostly I wanted to point this out. I could not for the life of me figure out what was wrong, I thought there was a broken assembly or reference something.

julenka commented 5 years ago

So should we close this or leave it open? Not clear to me the next action / next step so I would vote to close it.

wiwei commented 5 years ago

Okay, closing this out. If there are strong enough opinions in the other camp and they are willing to write a migration script, feel free to reopen.

Or, if there is other data (i.e. customers will be affected), feel free to reopen with additional data because that could change our decision