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
6k stars 2.12k forks source link

Sharing sample -> User joins session but never joins room #292

Closed Holo-Krzysztof closed 7 years ago

Holo-Krzysztof commented 7 years ago

I've noticed a strange bug which seems like a race condition: sometimes, when looking at the debug output of the Sharing sample you get a "User joining session" debug print but after that it seems to be stuck forever in the AnchorStore_Initialized state because the sharing service never gets marked as ready. This happens because SharingSessionTracker.SendJoinEvent() is called before ImportExportAnchorManager gets to subscribe to this event.

The problem went away after moving SharingSessionTracker.Instance.SessionJoined += Instance_SessionJoined; from SharingManagerConnected() to Start() , so I'd propose that as a solution. Can anyone else reproduce this issue and give me some more insight?

NeerajW commented 7 years ago

I can believe this. Were you in Debug mode? This was a relatively recent change by @alexdrenea inside SharingStage. Seems reasonable to move SharingSessionTracker.Instance.SessionJoined += Instance_SessionJoined; to Start().

@alexdrenea any thoughts on this?

alexdrenea commented 7 years ago

I checked the code as well and I agree that the event should be hooked up in Start, there's no good reason for it to be in the SharingManagerConnected() method.

I can't validate it on a real device at the moment though (due to lack of availability 😞).

NeerajW commented 7 years ago

@Holo-Krzysztof would you like to make the change?

Holo-Krzysztof commented 7 years ago

I'm not entirely sure yet, because after that change I've got a NullReferenceException in InitRoomApi() once, but that could be due to other changes in the project. I'll investigate that and if there are no issues, I'll make a pull request.

On Di. Okt. 18 19:54:43 2016 GMT+0200, Neeraj Wadhwa wrote:

@Holo-Krzysztof would you like to make the change?

You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Microsoft/HoloToolkit-Unity/issues/292#issuecomment-25458708

Holo-Krzysztof commented 7 years ago

Btw, it was in release mode, @NeerajW.

Holo-Krzysztof commented 7 years ago

Alright, I discovered another bug related to my change: Sometimes SharingManagerConnected never gets called and thus roomManager is null, leading to exceptions in InitRoomApi().

Let's look at a sample execution:

I'm currently thinking of a good way to solve this and will edit this comment accordingly. Any thoughts on this, @NeerajW @alexdrenea?

EDIT: I think I've solved it, but don't have time right now to submit my changes. You'll get a pull request next week.

StephenHodgson commented 7 years ago

Yeah I'm no fan of Invoke().

Especially this one:

private void Instance_SessionJoined(object sender, SharingSessionTracker.SessionJoinedEventArgs e)
{
    // We don't need to get this event anymore.
    SharingSessionTracker.Instance.SessionJoined -= Instance_SessionJoined;
     // We still wait to wait a few seconds for everything to settle.
    Invoke("MarkSharingServiceReady", 5);
}

Another issue is that testing in the Editor is difficult because ImportExportAnchorManager only puts you in a room if WorldAnchorStore.GetAsync(AnchorStoreReady); completes successfully (Which it never does because we're not actually on device).