Fix using MRTK in Unity editors on non-Windows platforms (part 1) #4088
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| } | ||
|
|
||
| #if UNITY_STANDALONE_WIN || UNITY_WSA || UNITY_EDITOR_WIN | ||
| /// <inheritdoc /> |
There was a problem hiding this comment.
I would scope these tighter, inside the function; at least to avoid two function declarations, as when the signature changes we will hit this issue again if the second is forgotten to be updated.
There was a problem hiding this comment.
That was how I had it originally, to match the SpeechInputProvider, but I couldn't think of another way to resolve this warning without just defining a non-async version in the other function declaration:
There was a problem hiding this comment.
Ah, I suppose I could define a no-op await or something.
There was a problem hiding this comment.
Updated to tighten the scope.
Assets/MixedRealityToolkit.Providers/WindowsVoiceInput/WindowsSpeechInputProvider.cs
Outdated
Show resolved
Hide resolved
|
In general, I like to scope the #if to as tight as possible, this way it eliminates issues when we change an API of another class that this class uses. Although in this case the majority of devs are on Windows, and we shouldn't have the issue, still a good practice. |
|
I fixed this in my fork by making a base class for the dictation providers. Then made the interface methods virtual and overrode them in the platform specific implementations. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
By the way, was this verified directly on mac hardware or using that win->osx compiling extension thing? |
|
This one will only repro directly on mac hardware. |
4 participants
Overview
Reported on Slack: opening the MRTK caused issues on non-Windows platforms (in this case, Mac).
This PR fixes most of them, but I'll need to get my hands on a Mac to repro the glTF-specific ones.
The fix: I moved the methods required by the interfaces outside of the Windows-platform
#ifs, where possible, or created a second version for non-Windows platforms where not possible.Report: https://holodevelopers.slack.com/archives/C2H4HT858/p1556141818234600