Sharing fixes #306

Holo-Krzysztof · 2016-10-24T11:19:48Z

This references issue #292

The major fixes here are subscribing to NetworkConnection events so that we know when we're connected to the server instead of using Invoke with a certain time (which failed when init took more than one second) and using Singleton<T> for SharingStage, as the previous implementation was broken and an unnecessary duplication of efforts.

My previous testing showed the Sharing sample scene to work now, which wasn't the case before; I would appreciate if some other people tested and verified this to be true.

of a race condition that led to the user never getting into a room.

call to Start.

…t an event from NetworkConnection instead of calling Invoke with a timer.

msftclas · 2016-10-24T11:19:51Z

Hi @Holo-Krzysztof, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

NeerajW

Please sign the CLA :)

NeerajW · 2016-10-24T16:33:55Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

-
-        // We will register for session joined to indicate when the sharing service
-        // is ready for us to make room related requests.
-        SharingSessionTracker.Instance.SessionJoined += Instance_SessionJoined;


Should this be removing the event handler in OnDestroy?

The GitHub view is misleading. This removed line was in the SharingManagerConnected method and moved to Awake in this pull request.

Edit: Unless you're asking if this event handler should be -= in OnDestroy, then I agree haha

lol, yeah this actually is in the SharingManagerConneced method.

I am asking for it to be -= in the OnDestroy :)

Interesting that it gets removed after we raise the Instance_SessionJoined event.

We also never unsubscribe to SharingManagerConnected on line 128 either.

We should probably be unsubscribing to any/all of these events whenever we call OnDestroy.

I've added a statement to unregister that handler in OnDestroy.

NeerajW · 2016-10-24T16:34:09Z

Assets/HoloToolkit/Sharing/Scripts/SharingStage.cs

 using UnityEngine;

 namespace HoloToolkit.Sharing
 {
-    public class SharingStage : MonoBehaviour
+    public class SharingStage : Singleton<SharingStage>


NeerajW · 2016-10-24T16:34:57Z

Assets/HoloToolkit/Sharing/Scripts/SharingStage.cs

            //delay sending notification so everything is initialized properly
-            Invoke("SendConnectedNotification", 1);
+            //Invoke("SendConnectedNotification", 1);


Remove these lines then?

keveleigh · 2016-10-26T21:01:11Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

@@ -116,18 +116,19 @@ public bool AnchorEstablished
    /// </summary>
    RoomManagerAdapter roomManagerCallbacks;

-    void Start()
+    void Awake()


Changing this to Awake can cause issues accessing a Singleton Instance. There's no guarantee that the Singleton object will be initialized until every object's Awake is called.

Aren't scene objects loaded before the first Awake call is made?
If yes, Singleton already accounts for that by calling FindObjectOfType if the instance doesn't exist.
Otherwise I could change it to Start, but I'd need to verify that everything still works.

Keep in mind that the Singleton pattern is being updated due to the new input module #277

Unfortunately, we found that FindObjectOfType will fail if Awake() hasn't been called yet. Since the execution order can vary, this was occasionally seen in BasicCursor.cs and raised in #246.

In addition, @HodgsonSDAS brings up a good point. The new pattern has Instance set in Awake(), so there's no guarantee it will be set in any other Awake().

msftclas · 2016-11-02T10:48:31Z

@Holo-Krzysztof, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

…ion.

StephenHodgson

Just one little nit. But while we're editing the ImportExportAnchorManager class anyway, could we make all the methods properties and fields explicitly private?

StephenHodgson · 2016-11-02T16:23:42Z

Assets/HoloToolkit/Sharing/Scripts/SharingStage.cs

            {
                Connect();
            }
        }

        protected void OnDestroy()
        {
-            Instance = null;



Nit: Extra line

StephenHodgson · 2016-11-02T16:26:14Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

@@ -116,18 +116,19 @@ public bool AnchorEstablished
    /// </summary>
    RoomManagerAdapter roomManagerCallbacks;

-    void Start()
+    void Awake()


Keep in mind that the Singleton pattern is being updated due to the new input module #277

Holo-Krzysztof · 2016-11-03T11:54:12Z

I've moved the singleton access calls into start, WorldAnchorStore is fine in Awake, I guess.

StephenHodgson · 2016-11-03T13:08:00Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

@@ -500,7 +505,7 @@ void Export()
    /// Called by the WorldAnchorTransferBatch as anchor data is available.
    /// </summary>
    /// <param name="data"></param>
-    public void WriteBuffer(byte[] data)


These should probably stay public, if they were already public.
Sorry, I just meant the things that weren't explicit.

StephenHodgson · 2016-11-03T13:08:23Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

@@ -509,7 +514,7 @@ public void WriteBuffer(byte[] data)
    /// Called by the WorldAnchorTransferBatch when anchor exporting is complete.
    /// </summary>
    /// <param name="status"></param>
-    public void ExportComplete(SerializationCompletionReason status)


Should stay public

StephenHodgson · 2016-11-03T13:08:57Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

    {
        get
        {
            return currentState.ToString();
        }
    }

-    public bool AnchorEstablished
+    private bool AnchorEstablished


should stay public

StephenHodgson · 2016-11-03T13:09:03Z

Assets/HoloToolkit/Sharing/Tests/ImportExportAnchorManager.cs

@@ -41,15 +41,15 @@ enum ImportExportState

    ImportExportState currentState = ImportExportState.Start;

-    public string StateName
+    private string StateName


should stay public

StephenHodgson · 2016-11-03T13:23:21Z

Also, enum ImportExportState could be private, as well as ImportExportState currentState.

Unless someone whats to know the currentState, then it might be better as a property with a private setter.

Holo-Krzysztof added 5 commits October 24, 2016 13:11

Moved SessionJoined event handler registration to Start() because

03faa43

of a race condition that led to the user never getting into a room.

Moved ImportExportAnchorManager init to Awake and SharingStage's Connect

5a6da82

call to Start.

Fixed broken singleton implementation in SharingStage.

cec0c75

Changed SharingStage.SharingManagerConnected to be called after we ge…

447efbd

…t an event from NetworkConnection instead of calling Invoke with a timer.

Removed debug prints.

b4615ac

msftclas added the cla-required label Oct 24, 2016

NeerajW reviewed Oct 24, 2016

View reviewed changes

NeerajW added the bug label Oct 24, 2016

keveleigh reviewed Oct 26, 2016

View reviewed changes

msftclas added cla-signed and removed cla-required labels Nov 2, 2016

Holo-Krzysztof added 2 commits November 2, 2016 12:00

Removed obsolete line from SharingStage.

687d714

Unhooked event handler for SharingManagerConnected on object destruct…

0d5090f

…ion.

StephenHodgson reviewed Nov 2, 2016

View reviewed changes

Holo-Krzysztof added 2 commits November 3, 2016 12:50

Private all the things!

b8e46d7

Removed extra line.

c835f8d

StephenHodgson reviewed Nov 3, 2016

View reviewed changes

Previously public members are public again.

fa00b71

ImportExportState is now private.

5b8ab53

NeerajW merged commit 915cd7b into microsoft:master Nov 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sharing fixes #306

Sharing fixes #306

Holo-Krzysztof commented Oct 24, 2016

msftclas commented Oct 24, 2016

NeerajW left a comment

NeerajW Oct 24, 2016

keveleigh Oct 26, 2016 •

edited

StephenHodgson Oct 31, 2016

NeerajW Oct 31, 2016

StephenHodgson Oct 31, 2016 •

edited

Holo-Krzysztof Nov 2, 2016

NeerajW Oct 24, 2016

NeerajW Oct 24, 2016

keveleigh Oct 26, 2016

Holo-Krzysztof Nov 2, 2016 •

edited

StephenHodgson Nov 2, 2016 •

edited

keveleigh Nov 3, 2016

msftclas commented Nov 2, 2016

StephenHodgson left a comment •

edited

StephenHodgson Nov 2, 2016

StephenHodgson Nov 2, 2016 •

edited

Holo-Krzysztof commented Nov 3, 2016

StephenHodgson Nov 3, 2016 •

edited

StephenHodgson Nov 3, 2016 •

edited

StephenHodgson Nov 3, 2016

StephenHodgson Nov 3, 2016

StephenHodgson commented Nov 3, 2016 •

edited

Sharing fixes #306

Sharing fixes #306

Conversation

Holo-Krzysztof commented Oct 24, 2016

msftclas commented Oct 24, 2016

NeerajW left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keveleigh Oct 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Oct 31, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holo-Krzysztof Nov 2, 2016 • edited

Choose a reason for hiding this comment

StephenHodgson Nov 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msftclas commented Nov 2, 2016

StephenHodgson left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Nov 2, 2016 • edited

Choose a reason for hiding this comment

Holo-Krzysztof commented Nov 3, 2016

StephenHodgson Nov 3, 2016 • edited

Choose a reason for hiding this comment

StephenHodgson Nov 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson commented Nov 3, 2016 • edited

keveleigh Oct 26, 2016 •

edited

StephenHodgson Oct 31, 2016 •

edited

Holo-Krzysztof Nov 2, 2016 •

edited

StephenHodgson Nov 2, 2016 •

edited

StephenHodgson left a comment •

edited

StephenHodgson Nov 2, 2016 •

edited

StephenHodgson Nov 3, 2016 •

edited

StephenHodgson Nov 3, 2016 •

edited

StephenHodgson commented Nov 3, 2016 •

edited