Fix authority changes ordering problem (#1465)

Author: Jamie Brynes

CHANGELOG.md

@@ -18,6 +18,10 @@
 - Migrated launch configurations to latest game templates. [#1457](https://github.com/spatialos/gdk-for-unity/pull/1457)
 - Multithreaded component serialization through `SystemBase` jobs. [#1454](https://github.com/spatialos/gdk-for-unity/pull/1454)
 
+### Fixed
+
+- Fixed an issue where authority changes returned by `ComponentUpdateSystem.GetAuthorityChangesReceived()` were returned in order from newest to oldest. [#1465](https://github.com/spatialos/gdk-for-unity/pull/1465)
+
 ## `0.3.10` - 2020-08-18
 
 ### Breaking Changes

workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs

@@ -0,0 +1,79 @@
+using Improbable.Gdk.Core;
+using Improbable.Gdk.Test;
+using Improbable.Gdk.TestUtils;
+using Improbable.Worker.CInterop;
+using NUnit.Framework;
+
+namespace Improbable.Gdk.EditmodeTests
+{
+    [TestFixture]
+    public class Test : MockBase
+    {
+        private const long EntityId = 1;
+
+        [Test]
+        public void Authority_changes_are_kept_in_order_and_applied_in_order()
+        {
+            World
+                .Step(world => world.Connection.CreateEntity(EntityId, GetTemplate()))
+                .Step(world =>
+                {
+                    world.Connection.ChangeAuthority(EntityId, 54, Authority.Authoritative);
+                    world.Connection.ChangeAuthority(EntityId, 54, Authority.NotAuthoritative);
+                })
+                .Step(world =>
+                {
+                    var authChanges = world
+                        .GetSystem<ComponentUpdateSystem>()
+                        .GetAuthorityChangesReceived(new EntityId(EntityId), 54);
+
+                    Assert.AreEqual(2, authChanges.Count);
+                    Assert.AreEqual(Authority.Authoritative, authChanges[0].Authority);
+                    Assert.AreEqual(Authority.NotAuthoritative, authChanges[1].Authority);
+
+                    var ecsEntity = world.GetSystem<WorkerSystem>().GetEntity(new EntityId(EntityId));
+
+                    Assert.IsFalse(world.Worker.World.EntityManager.HasComponent<Position.HasAuthority>(ecsEntity));
+                });
+        }
+
+        [Test]
+        public void Component_updates_are_kept_in_order()
+        {
+            World
+                .Step(world => world.Connection.CreateEntity(EntityId, GetTemplate()))
+                .Step(world =>
+                {
+                    world.Connection.UpdateComponent(EntityId, 54, new Position.Update
+                    {
+                        Coords = new Coordinates(1, 0, 0)
+                    });
+                    world.Connection.UpdateComponent(EntityId, 54, new Position.Update
+                    {
+                        Coords = new Coordinates(-1, 0, 0)
+                    });
+                })
+                .Step(world =>
+                {
+                    var componentUpdates = world
+                        .GetSystem<ComponentUpdateSystem>()
+                        .GetComponentUpdatesReceived<Position.Update>();
+
+                    Assert.AreEqual(2, componentUpdates.Count);
+                    Assert.AreEqual(1, componentUpdates[0].Update.Coords.Value.X);
+                    Assert.AreEqual(-1, componentUpdates[1].Update.Coords.Value.X);
+
+                    var ecsEntity = world.GetSystem<WorkerSystem>().GetEntity(new EntityId(EntityId));
+
+                    Assert.AreEqual(-1, world.Worker.World.EntityManager.GetComponentData<Position.Component>(ecsEntity).Coords.X);
+                });
+        }
+
+        private static EntityTemplate GetTemplate()
+        {
+            var template = new EntityTemplate();
+            template.AddComponent(new Position.Snapshot());
+            return template;
+        }
+    }
+}

workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs.meta

@@ -6,7 +6,14 @@ internal class AuthorityComparer : IComparer<AuthorityChangeReceived>
     {
         public int Compare(AuthorityChangeReceived x, AuthorityChangeReceived y)
         {
-            return x.EntityId.Id.CompareTo(y.EntityId.Id);
+            var entityIdCompare = x.EntityId.Id.CompareTo(y.EntityId.Id);
+
+            if (entityIdCompare == 0)
+            {
+                return x.AuthorityChangeId.CompareTo(y.AuthorityChangeId);
+            }
+
+            return entityIdCompare;
         }
     }
 }

workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/AuthorityComparer.cs

@@ -62,11 +62,13 @@ public ComponentEventReceived(T @event, EntityId entityId, ulong updateId)
     {
         public readonly Authority Authority;
         public readonly EntityId EntityId;
+        public readonly uint AuthorityChangeId;
 
-        public AuthorityChangeReceived(Authority authority, EntityId entityId)
+        public AuthorityChangeReceived(Authority authority, EntityId entityId, uint authorityChangeId)
         {
             Authority = authority;
             EntityId = entityId;
+            AuthorityChangeId = authorityChangeId;
         }
 
         EntityId IReceivedEntityMessage.EntityId => EntityId;

workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/ComponentUpdateToSend.cs

@@ -7,6 +7,7 @@ internal class UpdateComparer<T> : IComparer<ComponentUpdateReceived<T>> where T
         public int Compare(ComponentUpdateReceived<T> x, ComponentUpdateReceived<T> y)
         {
             var entityIdCompare = x.EntityId.Id.CompareTo(y.EntityId.Id);
+
             if (entityIdCompare == 0)
             {
                 return x.UpdateId.CompareTo(y.UpdateId);

workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/UpdateComparer.cs

@@ -46,6 +46,7 @@ internal MockConnectionHandler()
         }
 
         private uint updateId;
+        private uint authorityChangeId;
 
         private ViewDiff CurrentDiff => diffs[currentDiffIndex];
 
@@ -64,7 +65,7 @@ public void CreateEntity(long entityId, EntityTemplate template)
 
         public void ChangeAuthority(long entityId, uint componentId, Authority newAuthority)
         {
-            CurrentDiff.SetAuthority(entityId, componentId, newAuthority);
+            CurrentDiff.SetAuthority(entityId, componentId, newAuthority, authorityChangeId++);
         }
 
         public void UpdateComponent<T>(long entityId, uint componentId, T update) where T : ISpatialComponentUpdate

workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/MockConnectionHandler.cs

@@ -10,6 +10,7 @@ internal class OpListConverter
         private readonly ViewDiff viewDiff = new ViewDiff();
 
         private uint componentUpdateId;
+        private uint authorityChangeId;
 
         private bool shouldClear;
 
@@ -86,7 +87,9 @@ public bool ParseOpListIntoDiff(OpList opList, CommandMetaData commandMetaData)
                         break;
                     case OpType.AuthorityChange:
                         var authorityOp = opList.GetAuthorityChangeOp(i);
-                        viewDiff.SetAuthority(authorityOp.EntityId, authorityOp.ComponentId, authorityOp.Authority);
+                        viewDiff.SetAuthority(authorityOp.EntityId, authorityOp.ComponentId, authorityOp.Authority,
+                            authorityChangeId);
+                        authorityChangeId++;
                         break;
                     case OpType.ComponentUpdate:
                         var updateOp = opList.GetComponentUpdateOp(i);
@@ -118,6 +121,7 @@ public bool ParseOpListIntoDiff(OpList opList, CommandMetaData commandMetaData)
         public ViewDiff GetViewDiff()
         {
             componentUpdateId = 1;
+            authorityChangeId = 1;
             shouldClear = true;
             return viewDiff;
         }

workers/unity/Packages/io.improbable.gdk.core/Worker/OpListConverter.cs

@@ -162,7 +162,7 @@ public void RemoveComponent(long entityId, uint componentId)
             storage.RemoveEntityComponent(entityId);
         }
 
-        public void SetAuthority(long entityId, uint componentId, Authority authority)
+        public void SetAuthority(long entityId, uint componentId, Authority authority, uint authorityChangeId)
         {
             if (!componentIdToComponentStorage.TryGetValue(componentId, out var authorityStorage))
             {
@@ -177,7 +177,7 @@ public void SetAuthority(long entityId, uint componentId, Authority authority)
             }
 
             ((IDiffAuthorityStorage) authorityStorage).AddAuthorityChange(
-                new AuthorityChangeReceived(authority, new EntityId(entityId)));
+                new AuthorityChangeReceived(authority, new EntityId(entityId), authorityChangeId));
 
             // Remove received command requests if authority has been lost
             if (authority == Authority.NotAuthoritative)

workers/unity/Packages/io.improbable.gdk.core/Worker/ViewDiff.cs

@@ -84,8 +84,7 @@ private void CreateAuthChangeAction<T>(AuthChangeFunc<T> authFunc)
                         return;
                     }
 
-                    // The first element is actually the latest value!
-                    var auth = changes[0];
+                    var auth = changes[changes.Count - 1];
 
                     authFunc(ref kinematicStateWhenAuth, auth, component);
                 }));