From a073a240cb7725c6c20e7b2f140fd13b27250866 Mon Sep 17 00:00:00 2001 From: Jamie Brynes Date: Thu, 27 Aug 2020 11:30:53 +0100 Subject: [PATCH] Fix authority changes ordering problem (#1465) --- CHANGELOG.md | 4 + .../Correctness/Core/OpOrderingTests.cs | 79 +++++++++++++++++++ .../Correctness/Core/OpOrderingTests.cs.meta | 3 + .../UpdatesAndEvents/AuthorityComparer.cs | 9 ++- .../UpdatesAndEvents/ComponentUpdateToSend.cs | 4 +- .../UpdatesAndEvents/UpdateComparer.cs | 1 + .../MockConnectionHandler.cs | 3 +- .../Worker/OpListConverter.cs | 6 +- .../io.improbable.gdk.core/Worker/ViewDiff.cs | 4 +- .../SetKinematicFromAuthoritySystem.cs | 3 +- 10 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs create mode 100644 workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs.meta diff --git a/CHANGELOG.md b/CHANGELOG.md index 58e20bb8d4..40c1a8a53d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs b/workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs new file mode 100644 index 0000000000..44c40e7dd9 --- /dev/null +++ b/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() + .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().GetEntity(new EntityId(EntityId)); + + Assert.IsFalse(world.Worker.World.EntityManager.HasComponent(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() + .GetComponentUpdatesReceived(); + + 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().GetEntity(new EntityId(EntityId)); + + Assert.AreEqual(-1, world.Worker.World.EntityManager.GetComponentData(ecsEntity).Coords.X); + }); + } + + private static EntityTemplate GetTemplate() + { + var template = new EntityTemplate(); + template.AddComponent(new Position.Snapshot()); + return template; + } + } +} diff --git a/workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs.meta b/workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs.meta new file mode 100644 index 0000000000..b59224a00f --- /dev/null +++ b/workers/unity/Assets/Tests/EditmodeTests/Correctness/Core/OpOrderingTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: 49480b8b8e104e9b9b8714957da231dc +timeCreated: 1598453188 \ No newline at end of file diff --git a/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/AuthorityComparer.cs b/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/AuthorityComparer.cs index 4365490a15..c5ab4fc23b 100644 --- a/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/AuthorityComparer.cs +++ b/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/AuthorityComparer.cs @@ -6,7 +6,14 @@ internal class AuthorityComparer : IComparer { 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; } } } diff --git a/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/ComponentUpdateToSend.cs b/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/ComponentUpdateToSend.cs index be6abca674..7fb36f65cb 100644 --- a/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/ComponentUpdateToSend.cs +++ b/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/ComponentUpdateToSend.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; diff --git a/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/UpdateComparer.cs b/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/UpdateComparer.cs index 9a1883b363..3149b2d806 100644 --- a/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/UpdateComparer.cs +++ b/workers/unity/Packages/io.improbable.gdk.core/UpdatesAndEvents/UpdateComparer.cs @@ -7,6 +7,7 @@ internal class UpdateComparer : IComparer> where T public int Compare(ComponentUpdateReceived x, ComponentUpdateReceived y) { var entityIdCompare = x.EntityId.Id.CompareTo(y.EntityId.Id); + if (entityIdCompare == 0) { return x.UpdateId.CompareTo(y.UpdateId); diff --git a/workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/MockConnectionHandler.cs b/workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/MockConnectionHandler.cs index 66b80ca8f5..d4275c3360 100644 --- a/workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/MockConnectionHandler.cs +++ b/workers/unity/Packages/io.improbable.gdk.core/Worker/ConnectionHandlers/MockConnectionHandler.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(long entityId, uint componentId, T update) where T : ISpatialComponentUpdate diff --git a/workers/unity/Packages/io.improbable.gdk.core/Worker/OpListConverter.cs b/workers/unity/Packages/io.improbable.gdk.core/Worker/OpListConverter.cs index ab8b465a31..7fbf620bd9 100644 --- a/workers/unity/Packages/io.improbable.gdk.core/Worker/OpListConverter.cs +++ b/workers/unity/Packages/io.improbable.gdk.core/Worker/OpListConverter.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; } diff --git a/workers/unity/Packages/io.improbable.gdk.core/Worker/ViewDiff.cs b/workers/unity/Packages/io.improbable.gdk.core/Worker/ViewDiff.cs index 54c571b142..06ad3303ca 100644 --- a/workers/unity/Packages/io.improbable.gdk.core/Worker/ViewDiff.cs +++ b/workers/unity/Packages/io.improbable.gdk.core/Worker/ViewDiff.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) diff --git a/workers/unity/Packages/io.improbable.gdk.transformsynchronization/Systems/SetKinematicFromAuthoritySystem.cs b/workers/unity/Packages/io.improbable.gdk.transformsynchronization/Systems/SetKinematicFromAuthoritySystem.cs index 68d53e8a98..0654b96a6b 100644 --- a/workers/unity/Packages/io.improbable.gdk.transformsynchronization/Systems/SetKinematicFromAuthoritySystem.cs +++ b/workers/unity/Packages/io.improbable.gdk.transformsynchronization/Systems/SetKinematicFromAuthoritySystem.cs @@ -84,8 +84,7 @@ private void CreateAuthChangeAction(AuthChangeFunc 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); }));