Make the RemoveController API public so that applications can work around stale sources when the app is paused

[matthejo]

Overview

When Unity is paused and then resumed, the InteractionManager gets in a bad state, and some InteractionSources never have their SourceLost event raised even if the sources are no longer relevant. This change is making the RemoveController API public so that applications like Remote Assist can work around this problem while waiting for a proper fix from Unity.

Here's the additional code we plan to add to our application as part of this workaround:

public class StaleInteractionSourceMonitor : MonoBehaviour
{
    private static readonly TimeSpan staleSourceWaitTime = TimeSpan.FromMilliseconds(500);
    private CancellationTokenSource cancelInteractionSourceCleanup;

    private void OnApplicationPause(bool pause)
    {
        this.cancelInteractionSourceCleanup?.Cancel();

        if (!pause)
        {
            this.cancelInteractionSourceCleanup = new CancellationTokenSource();
            CheckForStaleSourcesAsync(this.cancelInteractionSourceCleanup.Token);
        }
    }

    private static HashSet<InteractionSource> GetCurrentSources()
    {
        var interactionSources = new HashSet<InteractionSource>();
        foreach (var interactionSourceState in InteractionManager.GetCurrentReading())
        {
            interactionSources.Add(interactionSourceState.source);
        }

        return interactionSources;
    }

    private async void CheckForStaleSourcesAsync(CancellationToken cancellationToken)
    {
        HashSet<InteractionSource> potentiallyStaleSources = GetCurrentSources();
        Action<InteractionSourceLostEventArgs> sourceLost = e =>
        {
            potentiallyStaleSources.Remove(e.state.source);
        };

        InteractionManager.InteractionSourceLost += sourceLost;
        await Task.Delay(staleSourceWaitTime, cancellationToken);
        InteractionManager.InteractionSourceLost -= sourceLost;

        if (cancellationToken.IsCancellationRequested)
        {
            return;
        }

        HashSet<InteractionSource> stillRelevantSources = GetCurrentSources();

        var deviceManager = CoreServices.GetInputSystemDataProvider<WindowsMixedRealityDeviceManager>();
        if (deviceManager != null)
        {
            foreach (InteractionSource staleSource in potentiallyStaleSources)
            {
                if (!stillRelevantSources.Contains(staleSource))
                {
                    Debug.Log($"Removing a stale source {staleSource.id}");
                    deviceManager.RemoveController(staleSource);
                }
                else
                {
                    Debug.Log("$Potentially stale source was still relevant, keeping it");
                }
            }
        }
        else
        {
            Debug.LogError("Failed to get the WindowsMixedRealityDeviceManager");
        }
    }
}

[keveleigh]

I'm not sure about this approach. Making a method public, especially one that was never intended to be called outside the class' internal state management, feels like a step in the wrong direction.

[matthejo]

@keveleigh, what approach do you suggest? I think right now the three options are:

1. This PR, with Remote Assist using the code I've posted in the PR description to call this
2. Nothing at all changes in MRTK, and Remote Assist continues to call this API through reflection to work around the issue
3. The StaleInteractionSourceMonitor gets added to the MRTK directly, and still needs to call some API on the WindowsMixedRealityDeviceManager to trigger this state change (and also needs to be created on a GameObject in order to receive OnApplicationPause notification from Unity)

[wiwei]

I was just typing up stuff but then got pulled into a bunch of meetings!

Following up here: I tend to agree with @keveleigh 's initial thoughts, which is flipping from private->public on a function that we long term really want to be private (because it reasons over internal and really not public state) is going to put ourselves in a one-way type street where we'd be making a breaking change to go back.

Have we done stuff like this in the past? Probably but it hasn't felt great.

I think that there's a middle ground here where we instead add a NEW public function (i.e.

[Obsolete("This function exists to workaround a bug in Unity and will be removed in an upcoming release. For more details, see https://github.com/microsoft/MixedRealityToolkit-Unity/pull/8101")]
public void RemoveControllerForSuspendWorkaround(InteractionSource interactionSource)

And just make that function call RemoveController.

This is super pedantic, but this makes it clear that this functionality will be short lived and has a clear marker (obsolete) for it to not be used aside from a specific temporary use case.

I think that in the chance that we get feedback in the future that removing that broke someone we still can say that it wasn't intended to be used permanently and would also give us the opportunity to have a conversation then to see what they were trying to do

[wiwei]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).