Optimise PVS broadphase leaves

Robust.Benchmarks/Physics/BroadphaseDetachBenchmark.cs

@@ -0,0 +1,78 @@
+using BenchmarkDotNet.Attributes;
+using JetBrains.Annotations;
+using System.Numerics;
+using Robust.Shared.Analyzers;
+using Robust.Shared.GameObjects;
+using Robust.Shared.Map;
+using Robust.Shared.Map.Components;
+using Robust.Shared.Maths;
+using Robust.UnitTesting.Server;
+
+namespace Robust.Benchmarks.Physics;
+
+[Virtual, MemoryDiagnoser]
+public class BroadphaseDetachBenchmark
+{
+    private ISimulation _simulation = default!;
+    private IEntityManager _entManager = default!;
+    private EntityLookupSystem _lookup = default!;
+    private SharedMapSystem _map = default!;
+
+    private EntityUid _root;
+    private TransformComponent _rootXform = default!;
+    private EntityUid[] _children = default!;
+
+    [UsedImplicitly]
+    [Params(0, 20, 50, 100)]
+    public int Children;
+
+    [GlobalSetup]
+    public void GlobalSetup()
+    {
+        _simulation = RobustServerSimulation.NewSimulation().InitializeInstance();
+        _entManager = _simulation.Resolve<IEntityManager>();
+        var systems = _entManager.EntitySysManager;
+        _lookup = systems.GetEntitySystem<EntityLookupSystem>();
+        _map = systems.GetEntitySystem<SharedMapSystem>();
+
+        var (mapUid, mapId) = _simulation.CreateMap();
+        var grid = _map.CreateGridEntity(mapId);
+        _map.SetTile(grid, Vector2i.Zero, new Tile(1));
+
+        _root = _entManager.SpawnEntity(null, new EntityCoordinates(grid.Owner, new Vector2(0.5f, 0.5f)));
+        _rootXform = _entManager.GetComponent<TransformComponent>(_root);
+        _children = new EntityUid[Children];
+
+        var parent = _root;
+        for (var i = 0; i < Children; i++)
+        {
+            var child = _entManager.SpawnEntity(null, new EntityCoordinates(parent, new Vector2(0.001f, 0f)));
+            _children[i] = child;
+            parent = child;
+        }
+    }
+
+    [IterationSetup]
+    public void IterationSetup()
+    {
+        _lookup.FindAndAddToEntityTree(_root, true, _rootXform);
+    }
+
+    [Benchmark(Baseline = true)]
+    public void RemoveEveryEntityRecursive()
+    {
+        for (var i = _children.Length - 1; i >= 0; i--)
+        {
+            var child = _children[i];
+            _lookup.RemoveFromEntityTree(child, _entManager.GetComponent<TransformComponent>(child));
+        }
+
+        _lookup.RemoveFromEntityTree(_root, _rootXform);
+    }
+
+    [Benchmark]
+    public void RemoveRootOnly()
+    {
+        _lookup.RemoveFromEntityTree(_root, _rootXform);
+    }
+}

Robust.Client/GameStates/ClientGameStateManager.cs

@@ -59,6 +59,7 @@ public sealed partial class ClientGameStateManager : IClientGameStateManager
         private readonly HashSet<EntityUid> _sorted = new();
         private readonly List<NetEntity> _created = new();
         private readonly List<NetEntity> _detached = new();
+        private readonly HashSet<EntityUid> _detachBatch = new();
 
         private readonly record struct StateData(
             EntityUid Uid,
@@ -1301,63 +1302,111 @@ private void Detach(GameTick maxTick,
             ContainerSystem containerSys,
             EntityLookupSystem lookupSys)
         {
+            _detachBatch.Clear();
+
             foreach (var netEntity in entities)
             {
-                if (!_entities.TryGetEntityData(netEntity, out var ent, out var meta))
+                if (!ShouldDetach(netEntity, maxTick, out var ent, out _))
                     continue;
 
-                if (meta.LastStateApplied > maxTick)
-                {
-                    // Server sent a new state for this entity sometime after the detach message was sent. The
-                    // detach message probably just arrived late or was initially dropped.
-                    continue;
-                }
+                _detachBatch.Add(ent!.Value);
+            }
+
+            var broadphaseRoots = 0;
 
-                if ((meta.Flags & (MetaDataFlags.Detached | MetaDataFlags.Undetachable)) != 0)
+            foreach (var netEntity in entities)
+            {
+                if (!ShouldDetach(netEntity, maxTick, out var ent, out var meta))
                     continue;
 
+                var uid = ent!.Value;
+                var metadata = meta!;
+
                 if (lastStateApplied.HasValue)
-                    meta.LastStateApplied = lastStateApplied.Value;
+                    metadata.LastStateApplied = lastStateApplied.Value;
 
-                var xform = xforms.GetComponent(ent.Value);
+                var xform = xforms.GetComponent(uid);
 
                 // TODO PVS DETACH
                 // Why is this if block here again? If a null-space entity gets sent to a player via some PVS override,
                 // and then later on it gets removed, you would assume that the client marks it as detached?
                 // I.e., modifying the metadata flag & pausing the entity should probably happen outside of this block.
                 if (xform.ParentUid.IsValid())
                 {
-                    lookupSys.RemoveFromEntityTree(ent.Value, xform);
+                    if (!HasDetachingParent(xform, xforms))
+                    {
+                        lookupSys.RemoveFromEntityTree(uid, xform);
+                        broadphaseRoots++;
+                    }
+
                     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.
                     BaseContainer? container = null;
-                    if ((meta.Flags & MetaDataFlags.InContainer) != 0 &&
+                    if ((metadata.Flags & MetaDataFlags.InContainer) != 0 &&
                         metas.TryGetComponent(xform.ParentUid, out var containerMeta) &&
                         (containerMeta.Flags & MetaDataFlags.Detached) == 0 &&
-                        containerSys.TryGetContainingContainer(xform.ParentUid, ent.Value, out container))
+                        containerSys.TryGetContainingContainer(xform.ParentUid, uid, out container))
                     {
-                        containerSys.Remove((ent.Value, xform, meta), container, false, true);
+                        containerSys.Remove((uid, xform, metadata), container, false, true);
                     }
 
-                    meta._flags |= MetaDataFlags.Detached;
-                    xformSys.DetachEntity(ent.Value, xform);
-                    DebugTools.Assert((meta.Flags & MetaDataFlags.InContainer) == 0);
+                    metadata._flags |= MetaDataFlags.Detached;
+                    xformSys.DetachEntity(uid, xform);
+                    DebugTools.Assert((metadata.Flags & MetaDataFlags.InContainer) == 0);
 
                     // We mark the entity as paused, without raising a pause-event.
                     // The entity gets un-paused when the metadata's comp-state is reapplied (which also does not raise
                     // an un-pause event). The assumption is that game logic that has to handle the pausing should be
                     // getting networked anyway. And if its some client-side timer on a networked entity, the timer
                     // shouldn't actually be getting paused just because the entity has left the players view.
-                    meta.PauseTime = TimeSpan.Zero;
+                    metadata.PauseTime = TimeSpan.Zero;
 
                     if (container != null)
                         containerSys.AddExpectedEntity(netEntity, container);
                 }
 
                 _detached.Add(netEntity);
             }
+
+            _prof.WriteValue("Broadphase roots", ProfData.Int32(broadphaseRoots));
+            _prof.WriteValue("Batch", ProfData.Int32(_detachBatch.Count));
+            _detachBatch.Clear();
+        }
+
+        private bool ShouldDetach(
+            NetEntity netEntity,
+            GameTick maxTick,
+            out EntityUid? ent,
+            out MetaDataComponent? meta)
+        {
+            if (!_entities.TryGetEntityData(netEntity, out ent, out meta))
+                return false;
+
+            if (meta.LastStateApplied > maxTick)
+            {
+                // Server sent a new state for this entity sometime after the detach message was sent. The
+                // detach message probably just arrived late or was initially dropped.
+                return false;
+            }
+
+            return (meta.Flags & (MetaDataFlags.Detached | MetaDataFlags.Undetachable)) == 0;
+        }
+
+        private bool HasDetachingParent(TransformComponent xform, EntityQuery<TransformComponent> xforms)
+        {
+            var parent = xform.ParentUid;
+
+            while (parent.IsValid())
+            {
+                if (_detachBatch.Contains(parent))
+                    return true;
+
+                parent = xforms.GetComponent(parent).ParentUid;
+            }
+
+            return false;
         }
 
         private void HandleEntityState(in StateData data, IEventBus bus, GameTick toTick)

Robust.Shared/GameObjects/Systems/EntityLookupSystem.cs

@@ -242,7 +242,7 @@ private void InitializeChild(
 
             if (!_broadQuery.TryGetComponent(xform.Broadphase.Value.Uid, out var oldBroadphase))
             {
-                DebugTools.Assert("Encountered deleted broadphase.");
+                AssertMissingBroadphaseExpected(xform.Broadphase.Value.Uid);
                 if (_fixturesQuery.TryGetComponent(child, out var fixtures))
                 {
                     foreach (var fixture in fixtures.Fixtures.Values)
@@ -567,7 +567,7 @@ private void UpdateParent(EntityUid uid, TransformComponent xform)
 
             if (!_broadQuery.TryGetComponent(xform.Broadphase.Value.Uid, out oldBroadphase))
             {
-                DebugTools.Assert("Encountered deleted broadphase.");
+                AssertMissingBroadphaseExpected(xform.Broadphase.Value.Uid);
 
                 // broadphase was probably deleted.
                 if (_fixturesQuery.TryGetComponent(uid, out var fixtures))
@@ -794,6 +794,16 @@ private void RemoveFromEntityTree(
             // parented to one from another.
             DebugTools.Assert(_netMan.IsClient);
             broadUid = old.Uid;
+
+            if (!_broadQuery.TryGetComponent(broadUid, out var currentBroadphase))
+            {
+                AssertMissingBroadphaseExpected(broadUid);
+                ClearFixtureProxies(uid);
+                xform.Broadphase = null;
+                return;
+            }
+
+            broadphase = currentBroadphase;
         }
 
         if (old.CanCollide)
@@ -828,24 +838,36 @@ public bool TryGetCurrentBroadphase(TransformComponent xform, [NotNullWhen(true)
         if (!_broadQuery.TryGetComponent(old.Uid, out broadphase))
         {
             // broadphase was probably deleted
-            DebugTools.Assert("Encountered deleted broadphase.");
-
-            if (_fixturesQuery.TryGetComponent(xform.Owner, out FixturesComponent? fixtures))
-            {
-                foreach (var fixture in fixtures.Fixtures.Values)
-                {
-                    fixture.ProxyCount = 0;
-                    fixture.Proxies = Array.Empty<FixtureProxy>();
-                }
-            }
+            AssertMissingBroadphaseExpected(old.Uid);
 
+            ClearFixtureProxies(xform.Owner);
             xform.Broadphase = null;
             return false;
         }
 
         return true;
     }
 
+    private void AssertMissingBroadphaseExpected(EntityUid broadphaseUid)
+    {
+        if (TerminatingOrDeleted(broadphaseUid))
+            return;
+
+        DebugTools.Assert("Encountered deleted broadphase.");
+    }
+
+    private void ClearFixtureProxies(EntityUid uid)
+    {
+        if (!_fixturesQuery.TryGetComponent(uid, out FixturesComponent? fixtures))
+            return;
+
+        foreach (var fixture in fixtures.Fixtures.Values)
+        {
+            fixture.ProxyCount = 0;
+            fixture.Proxies = Array.Empty<FixtureProxy>();
+        }
+    }
+
     public BroadphaseComponent? GetCurrentBroadphase(TransformComponent xform)
     {
         TryGetCurrentBroadphase(xform, out var broadphase);