From c6896e9bd957efcf67c0b7258b30c9b15069aadf Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Thu, 3 Nov 2022 14:30:14 +1300 Subject: [PATCH] More broadphase fixes (#3429) --- .../GameStates/ClientGameStateManager.cs | 45 +++++++++-- Robust.Shared/Containers/BaseContainer.cs | 19 +++-- Robust.Shared/Containers/Container.cs | 2 +- .../Containers/ContainerManagerComponent.cs | 3 +- Robust.Shared/Containers/ContainerSlot.cs | 2 +- Robust.Shared/Containers/IContainer.cs | 3 - Robust.Shared/Containers/IContainerManager.cs | 3 - .../Containers/SharedContainerSystem.cs | 7 +- .../Transform/TransformComponent.cs | 9 +++ .../GameObjects/Systems/EntityLookupSystem.cs | 77 ++++++++++++++----- .../GameObjects/Components/Container_Test.cs | 2 +- 11 files changed, 125 insertions(+), 47 deletions(-) diff --git a/Robust.Client/GameStates/ClientGameStateManager.cs b/Robust.Client/GameStates/ClientGameStateManager.cs index 41455d967..8cfe74683 100644 --- a/Robust.Client/GameStates/ClientGameStateManager.cs +++ b/Robust.Client/GameStates/ClientGameStateManager.cs @@ -25,6 +25,8 @@ using Robust.Shared.Log; using Robust.Shared.Map; using Robust.Shared.Network; using Robust.Shared.Network.Messages; +using Robust.Shared.Physics; +using Robust.Shared.Physics.Components; using Robust.Shared.Profiling; using Robust.Shared.Timing; using Robust.Shared.Utility; @@ -664,7 +666,7 @@ namespace Robust.Client.GameStates var xformSys = _entitySystemManager.GetEntitySystem(); var containerSys = _entitySystemManager.GetEntitySystem(); var lookupSys = _entitySystemManager.GetEntitySystem(); - var detached = ProcessPvsDeparture(curState.ToSequence, metas, xforms, xformSys, containerSys); + var detached = ProcessPvsDeparture(curState.ToSequence, metas, xforms, xformSys, containerSys, lookupSys); // Check next state (AFTER having created new entities introduced in curstate) if (nextState != null) @@ -687,6 +689,12 @@ namespace Robust.Client.GameStates } } + var contQuery = _entities.GetEntityQuery(); + var physicsQuery = _entities.GetEntityQuery(); + var fixturesQuery = _entities.GetEntityQuery(); + var broadQuery = _entities.GetEntityQuery(); + var queuedBroadphaseUpdates = new List<(EntityUid, TransformComponent)>(enteringPvs); + // Apply entity states. using (_prof.Group("Apply States")) { @@ -695,12 +703,30 @@ namespace Robust.Client.GameStates HandleEntityState(entity, _entities.EventBus, data.curState, data.nextState, data.LastApplied, curState.ToSequence, data.EnteringPvs); - if (data.EnteringPvs) - lookupSys.FindAndAddToEntityTree(entity); + if (!data.EnteringPvs) + continue; + + // Now that things like collision data, fixtures, and positions have been updated, we queue a + // broadphase update. However, if this entity is parented to some other entity also re-entering PVS, + // we only need to update it's parent (as it recursively updates children anyways). + var xform = xforms.GetComponent(entity); + DebugTools.Assert(xform.Broadphase == BroadphaseData.Invalid); + xform.Broadphase = null; + if (!toApply.TryGetValue(xform.ParentUid, out var parent) || !parent.EnteringPvs) + queuedBroadphaseUpdates.Add((entity, xform)); } _prof.WriteValue("Count", ProfData.Int32(toApply.Count)); } + // Add entering entities back to broadphase. + using (_prof.Group("Update Broadphase")) + { + foreach (var (uid, xform) in queuedBroadphaseUpdates) + { + lookupSys.FindAndAddToEntityTree(uid, xform, xforms, metas, contQuery, physicsQuery, fixturesQuery, broadQuery); + } + } + var delSpan = curState.EntityDeletions.Span; if (delSpan.Length > 0) ProcessDeletions(delSpan, xforms, metas, xformSys); @@ -754,7 +780,13 @@ namespace Robust.Client.GameStates _prof.WriteValue("Count", ProfData.Int32(delSpan.Length)); } - private List ProcessPvsDeparture(GameTick toTick, EntityQuery metas, EntityQuery xforms, SharedTransformSystem xformSys, ContainerSystem containerSys) + private List ProcessPvsDeparture( + GameTick toTick, + EntityQuery metas, + EntityQuery xforms, + SharedTransformSystem xformSys, + ContainerSystem containerSys, + EntityLookupSystem lookupSys) { var toDetach = _processor.GetEntitiesToDetach(toTick, _pvsDetachBudget); var detached = new List(); @@ -790,6 +822,9 @@ namespace Robust.Client.GameStates var xform = xforms.GetComponent(ent); if (xform.ParentUid.IsValid()) { + lookupSys.RemoveFromEntityTree(ent, xform, xforms); + xform.Broadphase = BroadphaseData.Invalid; + // In some cursed scenarios an entity inside of a container can leave PVS without the container itself leaving PVS. // In those situations, we need to add the entity back to the list of expected entities after detaching. IContainer? container = null; @@ -798,7 +833,7 @@ namespace Robust.Client.GameStates (containerMeta.Flags & MetaDataFlags.Detached) == 0 && containerSys.TryGetContainingContainer(xform.ParentUid, ent, out container, null, true)) { - container.Remove(ent, _entities, xform, meta, false, false, true); + container.Remove(ent, _entities, xform, meta, false, true); } meta._flags |= MetaDataFlags.Detached; diff --git a/Robust.Shared/Containers/BaseContainer.cs b/Robust.Shared/Containers/BaseContainer.cs index 23d856e55..0bf8ef75e 100644 --- a/Robust.Shared/Containers/BaseContainer.cs +++ b/Robust.Shared/Containers/BaseContainer.cs @@ -114,6 +114,11 @@ namespace Robust.Shared.Containers // This is done before changing can collide to avoid unecceary updates. // TODO maybe combine with RecursivelyUpdatePhysics to avoid fetching components and iterating parents twice? lookupSys.RemoveFromEntityTree(toinsert, transform, transformQuery); + DebugTools.Assert(transform.Broadphase == null || !transform.Broadphase.Value.IsValid()); + + // Avoid unnecessary broadphase updates while unanchoring, changing physics collision, and re-parenting. + var old = transform.Broadphase; + transform.Broadphase = BroadphaseData.Invalid; // Unanchor the entity (without changing physics body types). xformSys.Unanchor(transform, false); @@ -128,6 +133,7 @@ namespace Robust.Shared.Containers // Attach to new parent var oldParent = transform.ParentUid; xformSys.SetCoordinates(transform, new EntityCoordinates(Owner, Vector2.Zero), Angle.Zero); + transform.Broadphase = old; // the transform.AttachParent() could previously result in the flag being unset, so check that this hasn't happened. DebugTools.Assert((meta.Flags & MetaDataFlags.InContainer) != 0); @@ -144,7 +150,6 @@ namespace Robust.Shared.Containers DebugTools.Assert(!transform.Anchored); DebugTools.Assert(transform.LocalPosition == Vector2.Zero); DebugTools.Assert(transform.LocalRotation == Angle.Zero); - DebugTools.Assert(transform.Broadphase == null); DebugTools.Assert(!physicsQuery.TryGetComponent(toinsert, out var phys) || (!phys.Awake && !phys.CanCollide)); entMan.Dirty(Manager); @@ -225,7 +230,6 @@ namespace Robust.Shared.Containers TransformComponent? xform = null, MetaDataComponent? meta = null, bool reparent = true, - bool addToBroadphase = true, bool force = false, EntityCoordinates? destination = null, Angle? localRotation = null) @@ -250,18 +254,18 @@ namespace Robust.Shared.Containers } DebugTools.Assert(meta.EntityLifeStage < EntityLifeStage.Terminating || (force && !reparent)); - DebugTools.Assert(xform.Broadphase == null); + DebugTools.Assert(xform.Broadphase == null || !xform.Broadphase.Value.IsValid()); DebugTools.Assert(!xform.Anchored); DebugTools.Assert((meta.Flags & MetaDataFlags.InContainer) != 0x0); DebugTools.Assert(!entMan.TryGetComponent(toRemove, out PhysicsComponent? phys) || (!phys.Awake && !phys.CanCollide)); // Unset flag (before parent change events are raised). meta.Flags &= ~MetaDataFlags.InContainer; - // Implementation specific remove logic InternalRemove(toRemove, entMan); DebugTools.Assert((meta.Flags & MetaDataFlags.InContainer) == 0x0); + var oldParent = xform.ParentUid; if (destination != null) { @@ -276,9 +280,10 @@ namespace Robust.Shared.Containers entMan.EntitySysManager.GetEntitySystem().SetLocalRotation(xform, localRotation.Value); } - if (addToBroadphase) + // Add to new broadphase + if (xform.ParentUid == oldParent // move event should already have handled it + && xform.Broadphase == null) // broadphase explicitly invalid? { - // Container ECS when. entMan.EntitySysManager.GetEntitySystem().FindAndAddToEntityTree(toRemove, xform); } @@ -286,6 +291,8 @@ namespace Robust.Shared.Containers entMan.EventBus.RaiseLocalEvent(Owner, new EntRemovedFromContainerMessage(toRemove, this), true); entMan.EventBus.RaiseLocalEvent(toRemove, new EntGotRemovedFromContainerMessage(toRemove, this), false); + DebugTools.Assert(destination == null || xform.Coordinates.Equals(destination.Value)); + entMan.Dirty(Manager); return true; } diff --git a/Robust.Shared/Containers/Container.cs b/Robust.Shared/Containers/Container.cs index 811633669..aeafac80d 100644 --- a/Robust.Shared/Containers/Container.cs +++ b/Robust.Shared/Containers/Container.cs @@ -69,7 +69,7 @@ namespace Robust.Shared.Containers if (!isClient) entMan.DeleteEntity(entity); else if (entMan.EntityExists(entity)) - Remove(entity, entMan, reparent: false, addToBroadphase: true, force: true); + Remove(entity, entMan, reparent: false, force: true); } } } diff --git a/Robust.Shared/Containers/ContainerManagerComponent.cs b/Robust.Shared/Containers/ContainerManagerComponent.cs index 2e7b3bdc6..5f4b7b0a5 100644 --- a/Robust.Shared/Containers/ContainerManagerComponent.cs +++ b/Robust.Shared/Containers/ContainerManagerComponent.cs @@ -138,7 +138,6 @@ namespace Robust.Shared.Containers TransformComponent? xform = null, MetaDataComponent? meta = null, bool reparent = true, - bool addToBroadphase = true, bool force = false, EntityCoordinates? destination = null, Angle? localRotation = null) @@ -146,7 +145,7 @@ namespace Robust.Shared.Containers foreach (var containers in Containers.Values) { if (containers.Contains(toremove)) - return containers.Remove(toremove, _entMan, xform, meta, reparent, addToBroadphase, force, destination, localRotation); + return containers.Remove(toremove, _entMan, xform, meta, reparent, force, destination, localRotation); } return true; // If we don't contain the entity, it will always be removed diff --git a/Robust.Shared/Containers/ContainerSlot.cs b/Robust.Shared/Containers/ContainerSlot.cs index 671f2e012..3733d34d6 100644 --- a/Robust.Shared/Containers/ContainerSlot.cs +++ b/Robust.Shared/Containers/ContainerSlot.cs @@ -107,7 +107,7 @@ namespace Robust.Shared.Containers if (!isClient) entMan.DeleteEntity(entity); else if (entMan.EntityExists(entity)) - Remove(entity, entMan, reparent: false, addToBroadphase: true, force: true); + Remove(entity, entMan, reparent: false, force: true); } } } diff --git a/Robust.Shared/Containers/IContainer.cs b/Robust.Shared/Containers/IContainer.cs index d21170c20..f284bc958 100644 --- a/Robust.Shared/Containers/IContainer.cs +++ b/Robust.Shared/Containers/IContainer.cs @@ -112,8 +112,6 @@ namespace Robust.Shared.Containers /// /// If false, this operation will not rigger a move or parent change event. Ignored if /// destination is not null - /// If false, this entity will not get re-added to broadphases after removal - /// Useful if the entity is about to move or be re-inserted into another container. /// If true, this will not perform can-remove checks. /// Where to place the entity after removing. Avoids unnecessary broadphase updates. /// If not specified, and reparent option is true, then the entity will either be inserted into a parent @@ -125,7 +123,6 @@ namespace Robust.Shared.Containers TransformComponent? xform = null, MetaDataComponent? meta = null, bool reparent = true, - bool addToBroadphase = true, bool force = false, EntityCoordinates? destination = null, Angle? localRotation = null); diff --git a/Robust.Shared/Containers/IContainerManager.cs b/Robust.Shared/Containers/IContainerManager.cs index 4c430895c..924e5db41 100644 --- a/Robust.Shared/Containers/IContainerManager.cs +++ b/Robust.Shared/Containers/IContainerManager.cs @@ -28,8 +28,6 @@ namespace Robust.Shared.Containers /// /// If false, this operation will not rigger a move or parent change event. Ignored if /// destination is not null - /// If false, this entity will not get re-added to broadphases after removal - /// Useful if the entity is about to move or be re-inserted into another container. /// If true, this will not perform can-remove checks. /// Where to place the entity after removing. Avoids unnecessary broadphase updates. /// If not specified, and reparent option is true, then the entity will either be inserted into a parent @@ -39,7 +37,6 @@ namespace Robust.Shared.Containers TransformComponent? xform = null, MetaDataComponent? meta = null, bool reparent = true, - bool addToBroadphase = true, bool force = false, EntityCoordinates? destination = null, Angle? localRotation = null); diff --git a/Robust.Shared/Containers/SharedContainerSystem.cs b/Robust.Shared/Containers/SharedContainerSystem.cs index cf31fd9da..d8ae8b417 100644 --- a/Robust.Shared/Containers/SharedContainerSystem.cs +++ b/Robust.Shared/Containers/SharedContainerSystem.cs @@ -13,11 +13,9 @@ namespace Robust.Shared.Containers { public abstract partial class SharedContainerSystem : EntitySystem { - [Dependency] private readonly SharedTransformSystem _xforms = default!; [Dependency] private readonly SharedPhysicsSystem _physics = default!; [Dependency] private readonly SharedJointSystem _joint = default!; [Dependency] private readonly EntityLookupSystem _lookup = default!; - [Dependency] private readonly INetManager _netMan = default!; /// public override void Initialize() @@ -116,7 +114,6 @@ namespace Robust.Shared.Containers TransformComponent? containedXform = null, MetaDataComponent? containedMeta = null, bool reparent = true, - bool addToBroadphase = true, bool force = false, EntityCoordinates? destination = null, Angle? localRotation = null) @@ -124,7 +121,7 @@ namespace Robust.Shared.Containers if (!Resolve(uid, ref containerManager) || !Resolve(toremove, ref containedMeta, ref containedXform)) return; - containerManager.Remove(toremove, containedXform, containedMeta, reparent, addToBroadphase, force, destination, localRotation); + containerManager.Remove(toremove, containedXform, containedMeta, reparent, force, destination, localRotation); } public ContainerManagerComponent.AllContainersEnumerable GetAllContainers(EntityUid uid, ContainerManagerComponent? containerManager = null) @@ -377,7 +374,7 @@ namespace Robust.Shared.Containers foreach (var entity in container.ContainedEntities.ToArray()) { if (query.TryGetComponent(entity, out var xform)) - container.Remove(entity, entMan, xform, null, attachToGridOrMap, true, force, moveTo); + container.Remove(entity, entMan, xform, null, attachToGridOrMap, force, moveTo); } } diff --git a/Robust.Shared/GameObjects/Components/Transform/TransformComponent.cs b/Robust.Shared/GameObjects/Components/Transform/TransformComponent.cs index 8c96316ae..a87eeaf9f 100644 --- a/Robust.Shared/GameObjects/Components/Transform/TransformComponent.cs +++ b/Robust.Shared/GameObjects/Components/Transform/TransformComponent.cs @@ -843,8 +843,17 @@ namespace Robust.Shared.GameObjects /// /// Data used to store information about the broad-phase that any given entity is currently on. /// + /// + /// A null value means that this entity is simply not on a broadphase (e.g., in null-space or in a container). + /// An invalid entity UID indicates that this entity has intentionally been removed from broadphases and should + /// not automatically be re-added by movement events.. + /// internal record struct BroadphaseData(EntityUid Uid, bool CanCollide, bool Static) { + public bool IsValid() => Uid.IsValid(); + public bool Valid => IsValid(); + public readonly static BroadphaseData Invalid = default; + // TODO include MapId if ever grids are allowed to enter null-space (leave PVS). } } diff --git a/Robust.Shared/GameObjects/Systems/EntityLookupSystem.cs b/Robust.Shared/GameObjects/Systems/EntityLookupSystem.cs index b6a795059..f8eb99634 100644 --- a/Robust.Shared/GameObjects/Systems/EntityLookupSystem.cs +++ b/Robust.Shared/GameObjects/Systems/EntityLookupSystem.cs @@ -199,6 +199,7 @@ namespace Robust.Shared.GameObjects // ensure that the cached broadphase is correct. DebugTools.Assert(_timing.ApplyingState || xform.Broadphase == null + || !xform.Broadphase.Value.IsValid() || ((xform.Broadphase.Value.CanCollide == ev.Body.CanCollide) && (xform.Broadphase.Value.Static == (ev.Body.BodyType == BodyType.Static)))); } @@ -218,7 +219,7 @@ namespace Robust.Shared.GameObjects return; DebugTools.Assert(!_mapManager.IsGrid(uid)); - if (xform.Broadphase is not { } old) + if (xform.Broadphase is not { Valid: true } old) return; // entity is not on any broadphase xform.Broadphase = null; @@ -397,10 +398,23 @@ namespace Robust.Shared.GameObjects private void UpdateParent(EntityUid uid, TransformComponent xform, EntityUid oldParent) { - if (!TryGetCurrentBroadphase(xform, out var oldBroadphase)) - return; // If the entity was not already in a broadphase, parent changes will not automatically add it. + var broadQuery = GetEntityQuery(); + var xformQuery = GetEntityQuery(); - if (oldBroadphase != null && Transform(oldParent).MapID == MapId.Nullspace) + BroadphaseComponent? oldBroadphase = null; + if (xform.Broadphase != null) + { + if (!xform.Broadphase.Value.IsValid()) + return; // Entity is intentionally not on a broadphase (deferred updating?). + + if (!broadQuery.TryGetComponent(xform.Broadphase.Value.Uid, out oldBroadphase)) + { + // broadphase was probably deleted + xform.Broadphase = null; + } + } + + if (oldBroadphase != null && xformQuery.GetComponent(oldParent).MapID == MapId.Nullspace) { oldBroadphase = null; // Note that the parentXform.MapID != MapId.Nullspace is required because currently grids are not allowed to @@ -412,12 +426,10 @@ namespace Robust.Shared.GameObjects // generally save a component lookup. } - var broadQuery = GetEntityQuery(); - var xformQuery = GetEntityQuery(); - var physicsQuery = GetEntityQuery(); - var fixturesQuery = GetEntityQuery(); TryFindBroadphase(xform, broadQuery, xformQuery, out var newBroadphase); + var physicsQuery = GetEntityQuery(); + var fixturesQuery = GetEntityQuery(); if (oldBroadphase != null && oldBroadphase != newBroadphase) { @@ -465,10 +477,21 @@ namespace Robust.Shared.GameObjects return; var broadQuery = GetEntityQuery(); - if (!TryFindBroadphase(xform, broadQuery, xformQuery, out var broadphase)) - return; + if (TryFindBroadphase(xform, broadQuery, xformQuery, out var broadphase)) + AddToEntityTree(broadphase, uid, xform, xformQuery); + } - AddToEntityTree(broadphase, uid, xform, xformQuery); + public void FindAndAddToEntityTree(EntityUid uid, + TransformComponent xform, + EntityQuery xformQuery, + EntityQuery metaQuery, + EntityQuery contQuery, + EntityQuery physicsQuery, + EntityQuery fixturesQuery, + EntityQuery broadQuery) + { + if (TryFindBroadphase(xform, broadQuery, xformQuery, out var broadphase)) + AddToEntityTree(broadphase.Owner, broadphase, uid, xform, xformQuery, metaQuery, contQuery, physicsQuery, fixturesQuery, true); } /// @@ -498,6 +521,20 @@ namespace Robust.Shared.GameObjects var physicsQuery = GetEntityQuery(); var fixturesQuery = GetEntityQuery(); + AddToEntityTree(broadphase.Owner, broadphase, uid, xform, xformQuery, metaQuery, contQuery, physicsQuery, fixturesQuery, recursive); + } + + private void AddToEntityTree(EntityUid broadUid, + BroadphaseComponent broadphase, + EntityUid uid, + TransformComponent xform, + EntityQuery xformQuery, + EntityQuery metaQuery, + EntityQuery contQuery, + EntityQuery physicsQuery, + EntityQuery fixturesQuery, + bool recursive) + { var broadphaseXform = xformQuery.GetComponent(broadphase.Owner); if (!TryComp(broadphaseXform.MapUid, out SharedPhysicsMapComponent? physMap)) { @@ -506,7 +543,7 @@ namespace Robust.Shared.GameObjects } AddToEntityTree( - broadphase.Owner, + broadUid, broadphase, broadphaseXform, physMap, @@ -613,19 +650,19 @@ namespace Robust.Shared.GameObjects EntityQuery fixturesQuery, bool recursive = true) { - if (xform.Broadphase == null) + if (xform.Broadphase is not { Valid: true } old) { // this entity was probably inside of a container during a recursive iteration. This should mean all of // its own children are also not on any broadphase. return; } - if (xform.Broadphase.Value.Uid != broadUid) + if (old.Uid != broadUid) { // This may happen when the client has deferred broadphase updates, where maybe an entity from one // broadphase was parented to one from another. DebugTools.Assert(_netMan.IsClient); - broadUid = xform.Broadphase.Value.Uid; + broadUid = old.Uid; broadphaseXform = xformQuery.GetComponent(broadUid); if (broadphaseXform.MapID == MapId.Nullspace) return; @@ -638,9 +675,9 @@ namespace Robust.Shared.GameObjects physicsMap = map; } - if (xform.Broadphase.Value.CanCollide) - RemoveBroadTree(broadphase, fixturesQuery.GetComponent(uid), xform.Broadphase.Value.Static); - else if (xform.Broadphase.Value.Static) + if (old.CanCollide) + RemoveBroadTree(broadphase, fixturesQuery.GetComponent(uid), old.Static); + else if (old.Static) broadphase.StaticSundriesTree.Remove(uid); else broadphase.SundriesTree.Remove(uid); @@ -668,12 +705,12 @@ namespace Robust.Shared.GameObjects public bool TryGetCurrentBroadphase(TransformComponent xform, [NotNullWhen(true)] out BroadphaseComponent? broadphase) { broadphase = null; - if (xform.Broadphase == null) + if (xform.Broadphase is not { Valid: true } old) return false; if (!TryComp(xform.Broadphase.Value.Uid, out broadphase)) { - // broadphase was probably deleted during + // broadphase was probably deleted xform.Broadphase = null; return false; } diff --git a/Robust.UnitTesting/Server/GameObjects/Components/Container_Test.cs b/Robust.UnitTesting/Server/GameObjects/Components/Container_Test.cs index f8dba1a0c..6f0ce47fa 100644 --- a/Robust.UnitTesting/Server/GameObjects/Components/Container_Test.cs +++ b/Robust.UnitTesting/Server/GameObjects/Components/Container_Test.cs @@ -336,7 +336,7 @@ namespace Robust.UnitTesting.Server.GameObjects.Components if (!isClient) entMan.DeleteEntity(entity); else if (entMan.EntityExists(entity)) - Remove(entity, entMan, reparent: false, addToBroadphase: true, force: true); + Remove(entity, entMan, reparent: false, force: true); } }