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

Sync System: root object added listener does not raise events properly #311

Closed StephenHodgson closed 7 years ago

StephenHodgson commented 7 years ago

I'm just having trouble getting a listener on the root object to raise events properly.

RootSyncObject = SharingStage.Instance.Manager.GetRootSyncObject();
objElementAdapter = new ObjectElementAdapter();
objElementAdapter.ElementAddedEvent += OnElementAdded;
objElementAdapter.ElementDeletedEvent += OnElementDeleted;
RootSyncObject.AddListener(objElementAdapter);

I'm getting the root object and adding a listener but that listener never raises ElementAddedEvent nor ElementDeletedEvent.

Here's the branch I've set up for proper testing: https://github.com/HodgsonSDAS/HoloToolkit-Unity/tree/SyncSystemTests

  1. Open SyncSystemTests.unity
  2. Add breakpoints to ObjectElementAdapter.cs lines 34, 123, & 137.
  3. Start Debugging in visual studio and attach to Unity Editor.
  4. Run the SyncSystemTest.unity scene.
  5. Create/Delete object object and int elements.
  6. Increment and decrement int value.
  7. Observe events not raised.
StephenHodgson commented 7 years ago

Blocks https://github.com/Microsoft/HoloToolkit-Unity/issues/298

StephenHodgson commented 7 years ago

@jwittner any ideas?

Ziugy commented 7 years ago

I've noticed similar behaviour in regards to this. Listening to the SyncMessage on the server connection and registering a callback on the NetworkConnectionAdapter's MessageReceivedCallback you would find that the other clients get the changed events. I think there's something in the underlying HoloToolkit.Sharing that's not processing the operations.

jwittner commented 7 years ago

I'm using a version of this library in an application that's a couple months older than the latest. I'm wondering if we're seeing a recently introduced issue.

StephenHodgson commented 7 years ago

Debugging shows that the listener registers the delegates properly in Unity, but it seems like the events never get raised.

Ziugy commented 7 years ago

I uncommented #define SYNC_DEBUG in CommonSettings.h to enable further debugging of this. Looks like Modify and Ack operations are getting sent between my clients, but like you said the Unity events aren't getting raised. To investigate this further I added extra debugging in the ModifyOperation's Notify method. For me it looks like the parentElement is always "Root". The modifiedElement is correct. I would have thought the parent element was the ObjectElement name that I attached my primitive elements to. After seeing this I listened directly to the root sync and I'm seeing those events in Unity fire.

Ziugy commented 7 years ago

I can confirm that events are firing properly. I just had an initialization issue within my project. Fixing that cause all the handlers to execute correctly. I'm going to compare the setup of my project with what you've created to see where we differ.

StephenHodgson commented 7 years ago

Thanks @Ziugy that would be really helpful.

Yeah I was looking though the same code in C++ but I currently have an issue where I can't compile the HoloToolkit source on my work PC so I'm unable to debug the source.

I tried to follow the same pattern used for other parts of the sharing service like the other Adapter classes like SessionManagerAdapter, RoomManagerAdapter, and NetworkConnectionAdapter, but I've so far been unsuccessful.

Ziugy commented 7 years ago

I've attached a patch with the changes I've made to your repository that fixes the int case. The room connection really isn't needed. You needed to listen to the IntChangedEvent on the ObjectElement where the IntElement is a child of. In the OnElementAdded and OnElementDeleted methods you also needed to properly handle the IntElement being past in. Hopes this helps!

patch.zip

StephenHodgson commented 7 years ago

@Ziugy You sir are a scholar and a saint.

But your zip file is empty. haha. Might be simpler to just do a pull request on my fork?

I think I can get it working from your description of the errors of my ways but I'd still like to see what was in your patch.

StephenHodgson commented 7 years ago

@Ziugy were you able to get the RootObject to raise the OnElementAdded event when adding ObjectElements?

Ziugy commented 7 years ago

D'oh, looks like it was. Pull request makes much more sense! Done. You should see the add/delete event for ObjectElements too, but I didn't test that. Only investigated the IntElement for you.

Note that for every ObjectElement you need to AddListener to receive that ObjectElement's events. The change events don't propagate up to the root.

StephenHodgson commented 7 years ago

Hehe, no worries. I really appreciate you looking at this.

StephenHodgson commented 7 years ago

Merged your pull request but I still can't get the events to fire though, not even for OnIntChanged.

See my latest updates to the branch. I removed all the inValueText changes in the button presses and it's now only handled by OnIntChanged

Ziugy commented 7 years ago

Oh, local changes do not raise events on the local client. Events only get raised by remote client changes. You need to keep any local modifications done to intValueText.text.

StephenHodgson commented 7 years ago

Oh, okay so then it doesn't raise events locally by design.