Refactor DetachParent (#1188)

Robust.Server/Console/Commands/PlayerCommands.cs

@@ -35,7 +35,7 @@ namespace Robust.Server.Console.Commands
             if(transform == null)
                 return;
 
-            transform.DetachParent();
+            transform.AttachToGridOrMap();
 
             MapId mapId;
             if (args.Length == 3 && int.TryParse(args[2], out var intMapId))

Robust.Server/GameObjects/Components/Container/Container.cs

@@ -178,7 +178,7 @@ namespace Robust.Server.GameObjects.Components.Container
             if (!toremove.IsValid())
                 return true;
 
-            toremove.Transform.DetachParent();
+            ContainerHelpers.AttachParentToContainerOrGrid(toremove.Transform);
             return true;
         }
 

Robust.Shared/Containers/ContainerHelpers.cs

@@ -65,6 +65,32 @@ namespace Robust.Shared.Containers
             return false;
         }
 
+        public static void AttachParentToContainerOrGrid(ITransformComponent transform)
+        {
+            if (transform.Parent == null
+                || !TryGetContainer(transform.Parent.Owner, out var container)
+                || !TryInsertIntoContainer(transform, container))
+            {
+                transform.AttachToGridOrMap();
+            }
+        }
+
+        private static bool TryInsertIntoContainer(ITransformComponent transform, IContainer container)
+        {
+            if (container.Insert(transform.Owner))
+            {
+                return true;
+            }
+
+            if (container.Owner.Transform.Parent != null
+                && TryGetContainer(container.Owner, out var newContainer))
+            {
+                return TryInsertIntoContainer(transform, newContainer);
+            }
+
+            return false;
+        }
+
         private static bool TryGetManagerComp(IEntity entity, [NotNullWhen(true)] out IContainerManager? manager)
         {
             DebugTools.AssertNotNull(entity);

Robust.Shared/GameObjects/Components/Transform/TransformComponent.cs

@@ -133,7 +133,7 @@ namespace Robust.Shared.GameObjects.Components.Transform
                 }
                 else
                 {
-                    DetachParent();
+                    AttachToGridOrMap();
                 }
             }
         }
@@ -432,7 +432,7 @@ namespace Robust.Shared.GameObjects.Components.Transform
         /// <summary>
         /// Detaches this entity from its parent.
         /// </summary>
-        public void DetachParent()
+        public void AttachToGridOrMap()
         {
             // nothing to do
             var oldParent = Parent;
@@ -445,9 +445,18 @@ namespace Robust.Shared.GameObjects.Components.Transform
 
             IEntity newMapEntity;
             if (_mapManager.TryFindGridAt(mapPos, out var mapGrid))
+            {
                 newMapEntity = _entityManager.GetEntity(mapGrid.GridEntityId);
-            else
+            }
+            else if (_mapManager.HasMapEntity(mapPos.MapId))
+            {
                 newMapEntity = _mapManager.GetMapEntity(mapPos.MapId);
+            }
+            else
+            {
+                DetachParentToNull();
+                return;
+            }
 
             // this would be a no-op
             var oldParentEnt = oldParent.Owner;

Robust.Shared/Interfaces/GameObjects/Components/ITransformComponent.cs

@@ -99,7 +99,7 @@ namespace Robust.Shared.Interfaces.GameObjects.Components
         /// </summary>
         GridId GridID { get; }
 
-        void DetachParent();
+        void AttachToGridOrMap();
         void AttachParent(ITransformComponent parent);
         void AttachParent(IEntity parent);
 

Robust.UnitTesting/Server/GameObjects/Components/Container_Test.cs

@@ -3,6 +3,7 @@ using System.IO;
 using NUnit.Framework;
 using Robust.Server.GameObjects.Components.Container;
 using Robust.Server.Interfaces.GameObjects;
+using Robust.Shared.Containers;
 using Robust.Shared.Interfaces.GameObjects;
 using Robust.Shared.Interfaces.GameObjects.Components;
 using Robust.Shared.Interfaces.Map;
@@ -17,6 +18,14 @@ namespace Robust.UnitTesting.Server.GameObjects.Components
     {
         private IServerEntityManager EntityManager = default!;
 
+        const string PROTOTYPES = @"
+- type: entity
+  name: dummy
+  id: dummy
+  components:
+  - type: Transform
+";
+
         [OneTimeSetUp]
         public void Setup()
         {
@@ -98,12 +107,107 @@ namespace Robust.UnitTesting.Server.GameObjects.Components
             Assert.That(transform.Deleted, Is.True);
         }
 
-        const string PROTOTYPES = @"
-- type: entity
-  name: dummy
-  id: dummy
-  components:
-  - type: Transform
-";
+        [Test]
+        public void TestNestedRemoval()
+        {
+            var owner = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+            var inserted = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+            var transform = inserted.Transform;
+            var entity = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+
+            var container = ContainerManagerComponent.Create<Container>("dummy", owner);
+            Assert.That(container.Insert(inserted), Is.True);
+            Assert.That(transform.Parent!.Owner, Is.EqualTo(owner));
+
+            var container2 = ContainerManagerComponent.Create<Container>("dummy", inserted);
+            Assert.That(container2.Insert(entity), Is.True);
+            Assert.That(entity.Transform.Parent!.Owner, Is.EqualTo(inserted));
+
+            Assert.That(container2.Remove(entity), Is.True);
+            Assert.That(container.Contains(entity), Is.True);
+            Assert.That(entity.Transform.Parent!.Owner, Is.EqualTo(owner));
+
+            owner.Delete();
+            Assert.That(transform.Deleted, Is.True);
+        }
+
+        [Test]
+        public void TestNestedRemovalWithDenial()
+        {
+            var entityOne = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+            var entityTwo = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+            var entityThree = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+            var entityItem = EntityManager.SpawnEntity("dummy", new GridCoordinates(0, 0, new GridId(1)));
+
+            var container = ContainerManagerComponent.Create<Container>("dummy", entityOne);
+            var container2 = ContainerManagerComponent.Create<ContainerOnlyContainer>("dummy", entityTwo);
+            var container3 = ContainerManagerComponent.Create<Container>("dummy", entityThree);
+
+            Assert.That(container.Insert(entityTwo), Is.True);
+            Assert.That(entityTwo.Transform.Parent!.Owner, Is.EqualTo(entityOne));
+
+            Assert.That(container2.Insert(entityThree), Is.True);
+            Assert.That(entityThree.Transform.Parent!.Owner, Is.EqualTo(entityTwo));
+
+            Assert.That(container3.Insert(entityItem), Is.True);
+            Assert.That(entityItem.Transform.Parent!.Owner, Is.EqualTo(entityThree));
+
+            Assert.That(container3.Remove(entityItem), Is.True);
+            Assert.That(container.Contains(entityItem), Is.True);
+            Assert.That(entityItem.Transform.Parent!.Owner, Is.EqualTo(entityOne));
+
+            entityOne.Delete();
+            Assert.That(entityTwo.Transform.Deleted, Is.True);
+        }
+
+        private class ContainerOnlyContainer : BaseContainer
+        {
+            /// <summary>
+            /// The generic container class uses a list of entities
+            /// </summary>
+            private readonly List<IEntity> _containerList = new List<IEntity>();
+
+            /// <inheritdoc />
+            public ContainerOnlyContainer(string id, IContainerManager manager) : base(id, manager) { }
+
+            /// <inheritdoc />
+            public override IReadOnlyList<IEntity> ContainedEntities => _containerList;
+
+            /// <inheritdoc />
+            protected override void InternalInsert(IEntity toinsert)
+            {
+                _containerList.Add(toinsert);
+                base.InternalInsert(toinsert);
+            }
+
+            /// <inheritdoc />
+            protected override void InternalRemove(IEntity toremove)
+            {
+                _containerList.Remove(toremove);
+                base.InternalRemove(toremove);
+            }
+
+            /// <inheritdoc />
+            public override bool Contains(IEntity contained)
+            {
+                return _containerList.Contains(contained);
+            }
+
+            /// <inheritdoc />
+            public override void Shutdown()
+            {
+                base.Shutdown();
+
+                foreach (var entity in _containerList)
+                {
+                    entity.Delete();
+                }
+            }
+
+            public override bool CanInsert(IEntity toinsert)
+            {
+                return toinsert.TryGetComponent(out IContainerManager _);
+            }
+        }
     }
 }

Robust.UnitTesting/Server/GameObjects/Components/Transform_Test.cs

@@ -115,7 +115,7 @@ namespace Robust.UnitTesting.Server.GameObjects.Components
             var oldLpos = childTrans.GridPosition;
             var oldWpos = childTrans.WorldPosition;
 
-            childTrans.DetachParent();
+            childTrans.AttachToGridOrMap();
 
             // the gridId won't match, because we just detached from the grid entity