-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sharing fixes #306
Sharing fixes #306
Changes from 5 commits
03faa43
5a6da82
cec0c75
447efbd
b4615ac
687d714
0d5090f
b8e46d7
c835f8d
fa00b71
5b8ab53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,16 @@ | ||
using System; | ||
using HoloToolkit.Unity; | ||
using System; | ||
using UnityEngine; | ||
|
||
namespace HoloToolkit.Sharing | ||
{ | ||
public class SharingStage : MonoBehaviour | ||
public class SharingStage : Singleton<SharingStage> | ||
{ | ||
/// <summary> | ||
/// SharingManagerConnected event notifies when the sharing manager is created and connected. | ||
/// </summary> | ||
public event EventHandler SharingManagerConnected; | ||
|
||
public static SharingStage Instance = null; | ||
|
||
/// <summary> | ||
/// Set whether this app should be a Primary or Secondary client. | ||
/// Primary: Connects directly to the Session Server, can create/join/leave sessions | ||
|
@@ -48,29 +47,39 @@ public class SharingStage : MonoBehaviour | |
/// Provides callbacks when server is discovered or lost. | ||
/// </summary> | ||
private DiscoveryClientAdapter discoveryClientAdapter; | ||
|
||
/// <summary> | ||
/// Provides callbacks for when we connect to a server. | ||
/// </summary> | ||
private NetworkConnectionAdapter networkConnectionAdapter = null; | ||
|
||
private NetworkConnection networkConnection = null; | ||
|
||
private float pingIntervalCurrent = 0; | ||
private bool isTryingToFindServer = false; | ||
|
||
private void Awake() | ||
{ | ||
Instance = this; | ||
|
||
this.logWriter = new ConsoleLogWriter(); | ||
|
||
if (AutoDiscoverServer) | ||
{ | ||
AutoDiscoverInit(); | ||
} | ||
else | ||
|
||
networkConnectionAdapter = new NetworkConnectionAdapter(); | ||
} | ||
|
||
private void Start() | ||
{ | ||
if (!AutoDiscoverServer) | ||
{ | ||
Connect(); | ||
} | ||
} | ||
|
||
protected void OnDestroy() | ||
{ | ||
Instance = null; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Extra line |
||
if (this.discoveryClient != null) | ||
{ | ||
|
@@ -85,6 +94,19 @@ protected void OnDestroy() | |
} | ||
} | ||
|
||
if (this.networkConnection != null) | ||
{ | ||
networkConnection.RemoveListener((byte)MessageID.StatusOnly, networkConnectionAdapter); | ||
networkConnection.Dispose(); | ||
networkConnection = null; | ||
|
||
if (networkConnectionAdapter != null) | ||
{ | ||
networkConnectionAdapter.Dispose(); | ||
networkConnectionAdapter = null; | ||
} | ||
} | ||
|
||
if (this.sharingMgr != null) | ||
{ | ||
// Force a disconnection so that we can stop and start Unity without connections hanging around | ||
|
@@ -133,8 +155,19 @@ private void Connect() | |
|
||
this.sharingMgr = SharingManager.Create(config); | ||
|
||
//set up callbacks so that we know when we've connected successfully | ||
this.networkConnection = sharingMgr.GetServerConnection(); | ||
this.networkConnectionAdapter = new NetworkConnectionAdapter(); | ||
networkConnectionAdapter.ConnectedCallback += NetworkConnectionAdapter_ConnectedCallback; | ||
networkConnection.AddListener((byte)MessageID.StatusOnly, networkConnectionAdapter); | ||
|
||
//delay sending notification so everything is initialized properly | ||
Invoke("SendConnectedNotification", 1); | ||
//Invoke("SendConnectedNotification", 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove these lines then? |
||
} | ||
|
||
private void NetworkConnectionAdapter_ConnectedCallback(NetworkConnection obj) | ||
{ | ||
SendConnectedNotification(); | ||
} | ||
|
||
private void SendConnectedNotification() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,18 +116,19 @@ public bool AnchorEstablished | |
/// </summary> | ||
RoomManagerAdapter roomManagerCallbacks; | ||
|
||
void Start() | ||
void Awake() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't scene objects loaded before the first Awake call is made? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep in mind that the Singleton pattern is being updated due to the new input module #277 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(). |
||
{ | ||
Debug.Log("Import Export Manager starting"); | ||
|
||
currentState = ImportExportState.Ready; | ||
|
||
// We need to get our local anchor store started up. | ||
currentState = ImportExportState.AnchorStore_Initializing; | ||
WorldAnchorStore.GetAsync(AnchorStoreReady); | ||
|
||
//Wait for a notification that the sharing manager has been initialized (connected to sever) | ||
SharingStage.Instance.SharingManagerConnected += SharingManagerConnected; | ||
|
||
// 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; | ||
} | ||
|
||
void OnDestroy() | ||
|
@@ -153,10 +154,6 @@ private void SharingManagerConnected(object sender, EventArgs e) | |
roomManagerCallbacks.AnchorsDownloadedEvent += RoomManagerCallbacks_AnchorsDownloaded; | ||
roomManagerCallbacks.AnchorUploadedEvent += RoomManagerCallbacks_AnchorUploaded; | ||
roomManager.AddListener(roomManagerCallbacks); | ||
|
||
// 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; | ||
} | ||
|
||
/// <summary> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be removing the event handler in OnDestroy? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol, yeah this actually is in the SharingManagerConneced method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am asking for it to be -= in the OnDestroy :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting that it gets removed after we raise the We also never unsubscribe to We should probably be unsubscribing to any/all of these events whenever we call OnDestroy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a statement to unregister that handler in OnDestroy. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)