Add new entity spawn test & fix misc bugs (#19953)

Content.IntegrationTests/Tests/EntityTest.cs

@@ -1,6 +1,8 @@
 using System.Collections.Generic;
 using System.Linq;
+using Content.Server.Humanoid.Components;
 using Content.Shared.Coordinates;
+using Content.Shared.Prototypes;
 using Robust.Shared;
 using Robust.Shared.Configuration;
 using Robust.Shared.GameObjects;
@@ -183,7 +185,7 @@ await server.WaitPost(() =>
                     var query = entityMan.AllEntityQueryEnumerator<TComp>();
                     while (query.MoveNext(out var uid, out var meta))
                         yield return (uid, meta);
-                };
+                }
 
                 var entityMetas = Query<MetaDataComponent>(sEntMan).ToList();
                 foreach (var (uid, meta) in entityMetas)
@@ -198,6 +200,100 @@ await server.WaitPost(() =>
             await pair.CleanReturnAsync();
         }
 
+        /// <summary>
+        /// This test checks that spawning and deleting an entity doesn't somehow create other unrelated entities.
+        /// </summary>
+        /// <remarks>
+        /// Unless an entity is intentionally designed to spawn other entities (e.g., mob spawners), they should
+        /// generally not spawn unrelated / detached entities. Any entities that do get spawned should be parented to
+        /// the spawned entity (e.g., in a container). If an entity needs to spawn an entity somewhere in null-space,
+        /// it should delete that entity when it is no longer required. This test mainly exists to prevent "entity leak"
+        /// bugs, where spawning some entity starts spawning unrelated entities in null space.
+        /// </remarks>
+        [Test]
+        public async Task SpawnAndDeleteEntityCountTest()
+        {
+            var settings = new PoolSettings { Connected = true, Dirty = true };
+            await using var pair = await PoolManager.GetServerClient(settings);
+            var server = pair.Server;
+            var client = pair.Client;
+
+            var excluded = new[]
+            {
+                "MapGrid",
+                "StationEvent",
+                "TimedDespawn",
+
+                // Spawner entities
+                "DragonRift",
+                "RandomHumanoidSpawner",
+                "RandomSpawner",
+                "ConditionalSpawner",
+                "GhostRoleMobSpawner",
+                "NukeOperativeSpawner",
+                "TimedSpawner",
+            };
+
+            Assert.That(server.CfgMan.GetCVar(CVars.NetPVS), Is.False);
+
+            var protoIds = server.ProtoMan
+                .EnumeratePrototypes<EntityPrototype>()
+                .Where(p => !p.Abstract)
+                .Where(p => !pair.IsTestPrototype(p))
+                .Where(p => !excluded.Any(p.Components.ContainsKey))
+                .Select(p => p.ID)
+                .ToList();
+
+            MapCoordinates coords = default;
+            await server.WaitPost(() =>
+            {
+                var map = server.MapMan.CreateMap();
+                coords = new MapCoordinates(default, map);
+            });
+
+            await pair.RunTicksSync(3);
+
+            List<string> badPrototypes = new();
+            foreach (var protoId in protoIds)
+            {
+                // TODO fix ninja
+                // Currently ninja fails to equip their own loadout.
+                if (protoId == "MobHumanSpaceNinja")
+                    continue;
+
+                var count = server.EntMan.EntityCount;
+                var clientCount = client.EntMan.EntityCount;
+                EntityUid uid = default;
+                await server.WaitPost(() => uid = server.EntMan.SpawnEntity(protoId, coords));
+                await pair.RunTicksSync(3);
+
+                // If the entity deleted itself, check that it didn't spawn other entities
+                if (!server.EntMan.EntityExists(uid))
+                {
+                    if (server.EntMan.EntityCount != count || client.EntMan.EntityCount != clientCount)
+                        badPrototypes.Add(protoId);
+                    continue;
+                }
+
+                // Check that the number of entities has increased.
+                if (server.EntMan.EntityCount <= count || client.EntMan.EntityCount <= clientCount)
+                {
+                    badPrototypes.Add(protoId);
+                    continue;
+                }
+
+                await server.WaitPost(() => server.EntMan.DeleteEntity(uid));
+                await pair.RunTicksSync(3);
+
+                // Check that the number of entities has gone back to the original value.
+                if (server.EntMan.EntityCount != count || client.EntMan.EntityCount != clientCount)
+                    badPrototypes.Add(protoId);
+            }
+
+            Assert.That(badPrototypes, Is.Empty);
+            await pair.CleanReturnAsync();
+        }
+
         [Test]
         public async Task AllComponentsOneToOneDeleteTest()
         {

Content.Server/GameTicking/Rules/DeathMatchRuleSystem.cs

@@ -1,3 +1,4 @@
+using System.Linq;
 using Content.Server.Administration.Commands;
 using Content.Server.GameTicking.Rules.Components;
 using Content.Server.KillTracking;
@@ -95,7 +96,7 @@ private void OnKillReported(ref KillReportedEvent ev)
             if (ev.Assist is KillPlayerSource assist && dm.Victor == null)
                 _point.AdjustPointValue(assist.PlayerId, 1, uid, point);
 
-            var spawns = EntitySpawnCollection.GetSpawns(dm.RewardSpawns);
+            var spawns = EntitySpawnCollection.GetSpawns(dm.RewardSpawns).Cast<string?>().ToList();
             EntityManager.SpawnEntities(Transform(ev.Entity).MapPosition, spawns);
         }
     }

Content.Server/Gatherable/GatherableSystem.cs

@@ -70,7 +70,7 @@ public void Gather(EntityUid gatheredUid, EntityUid? gatherer = null, Gatherable
                     continue;
             }
             var getLoot = _prototypeManager.Index<EntityLootTablePrototype>(table);
-            var spawnLoot = getLoot.GetSpawns();
+            var spawnLoot = getLoot.GetSpawns(_random);
             var spawnPos = pos.Offset(_random.NextVector2(0.3f));
             Spawn(spawnLoot[0], spawnPos);
         }

Content.Server/IdentityManagement/IdentitySystem.cs

@@ -35,6 +35,7 @@ public override void Initialize()
         SubscribeLocalEvent<IdentityComponent, DidEquipHandEvent>((uid, _, _) => QueueIdentityUpdate(uid));
         SubscribeLocalEvent<IdentityComponent, DidUnequipEvent>((uid, _, _) => QueueIdentityUpdate(uid));
         SubscribeLocalEvent<IdentityComponent, DidUnequipHandEvent>((uid, _, _) => QueueIdentityUpdate(uid));
+        SubscribeLocalEvent<IdentityComponent, MapInitEvent>(OnMapInit);
     }
 
     public override void Update(float frameTime)
@@ -53,10 +54,8 @@ public override void Update(float frameTime)
     }
 
     // This is where the magic happens
-    protected override void OnComponentInit(EntityUid uid, IdentityComponent component, ComponentInit args)
+    private void OnMapInit(EntityUid uid, IdentityComponent component, MapInitEvent args)
     {
-        base.OnComponentInit(uid, component, args);
-
         var ident = Spawn(null, Transform(uid).Coordinates);
 
         QueueIdentityUpdate(uid);

Content.Server/Kitchen/EntitySystems/KitchenSpikeSystem.cs

@@ -110,7 +110,8 @@ private void OnInteractHand(EntityUid uid, KitchenSpikeComponent component, Inte
             if (args.Handled)
                 return;
 
-            if (component.PrototypesToSpawn?.Count > 0) {
+            if (component.PrototypesToSpawn?.Count > 0)
+            {
                 _popupSystem.PopupEntity(Loc.GetString("comp-kitchen-spike-knife-needed"), uid, args.User);
                 args.Handled = true;
             }

Content.Server/StationEvents/Events/StationEventSystem.cs

@@ -136,12 +136,7 @@ protected void ForceEndSelf(EntityUid uid, GameRuleComponent? component = null)
 
     protected bool TryGetRandomStation([NotNullWhen(true)] out EntityUid? station, Func<EntityUid, bool>? filter = null)
     {
-        var stations = new ValueList<EntityUid>();
-
-        if (filter == null)
-        {
-            stations.EnsureCapacity(Count<StationEventEligibleComponent>());
-        }
+        var stations = new ValueList<EntityUid>(Count<StationEventEligibleComponent>());
 
         filter ??= _ => true;
         var query = AllEntityQuery<StationEventEligibleComponent>();

Content.Server/Storage/EntitySystems/StorageSystem.Fill.cs

@@ -1,6 +1,10 @@
+using Content.Server.Spawners.Components;
 using Content.Server.Storage.Components;
+using Content.Shared.Prototypes;
 using Content.Shared.Storage;
 using Content.Shared.Storage.Components;
+using Robust.Shared.Prototypes;
+using Robust.Shared.Utility;
 
 namespace Content.Server.Storage.EntitySystems;
 
@@ -25,10 +29,13 @@ private void OnStorageFillMapInit(EntityUid uid, StorageFillComponent component,
         var spawnItems = EntitySpawnCollection.GetSpawns(component.Contents, Random);
         foreach (var item in spawnItems)
         {
+            // No, you are not allowed to fill a container with entity spawners.
+            DebugTools.Assert(!_prototype.Index<EntityPrototype>(item)
+                .HasComponent(typeof(RandomSpawnerComponent)));
             var ent = EntityManager.SpawnEntity(item, coordinates);
 
             // handle depending on storage component, again this should be unified after ECS
-            if (entityStorageComp != null && EntityStorage.Insert(ent, uid))
+            if (entityStorageComp != null && EntityStorage.Insert(ent, uid, entityStorageComp))
                 continue;
 
             if (storageComp != null && Insert(uid, ent, out _, storageComp: storageComp, playSound: false))

Content.Server/Storage/EntitySystems/StorageSystem.cs

@@ -12,14 +12,15 @@
 using Robust.Server.Player;
 using Robust.Shared.Map;
 using Robust.Shared.Player;
-using Robust.Shared.Players;
+using Robust.Shared.Prototypes;
 using Robust.Shared.Utility;
 
 namespace Content.Server.Storage.EntitySystems;
 
 public sealed partial class StorageSystem : SharedStorageSystem
 {
     [Dependency] private readonly IAdminManager _admin = default!;
+    [Dependency] private readonly IPrototypeManager _prototype = default!;
     [Dependency] private readonly UserInterfaceSystem _uiSystem = default!;
 
     public override void Initialize()

Content.Shared/EntityList/EntityLootTablePrototype.cs

@@ -15,7 +15,7 @@ public sealed class EntityLootTablePrototype : IPrototype
     public ImmutableList<EntitySpawnEntry> Entries = ImmutableList<EntitySpawnEntry>.Empty;
 
     /// <inheritdoc cref="EntitySpawnCollection.GetSpawns"/>
-    public List<string?> GetSpawns(IRobustRandom? random = null)
+    public List<string> GetSpawns(IRobustRandom random)
     {
         return EntitySpawnCollection.GetSpawns(Entries, random);
     }

Content.Shared/Kitchen/Components/KitchenSpikeComponent.cs

@@ -15,7 +15,7 @@ public sealed partial class KitchenSpikeComponent : Component
     [DataField("sound")]
     public SoundSpecifier SpikeSound = new SoundPathSpecifier("/Audio/Effects/Fluids/splat.ogg");
 
-    public List<string?>? PrototypesToSpawn;
+    public List<string>? PrototypesToSpawn;
 
     // TODO: Spiking alive mobs? (Replace with uid) (deal damage to their limbs on spiking, kill on first butcher attempt?)
     public string MeatSource1p = "?";

Content.Shared/Station/SharedStationSpawningSystem.cs

@@ -27,7 +27,7 @@ public void EquipStartingGear(EntityUid entity, StartingGearPrototype startingGe
                 if (!string.IsNullOrEmpty(equipmentStr))
                 {
                     var equipmentEntity = EntityManager.SpawnEntity(equipmentStr, EntityManager.GetComponent<TransformComponent>(entity).Coordinates);
-                    InventorySystem.TryEquip(entity, equipmentEntity, slot.Name, true);
+                    InventorySystem.TryEquip(entity, equipmentEntity, slot.Name, true, force:true);
                 }
             }
         }

Content.Shared/Storage/EntitySpawnEntry.cs

@@ -78,12 +78,12 @@ public sealed class OrGroup
     /// <param name="entries">The entity spawn entries.</param>
     /// <param name="random">Resolve param.</param>
     /// <returns>A list of entity prototypes that should be spawned.</returns>
-    public static List<string?> GetSpawns(IEnumerable<EntitySpawnEntry> entries,
+    public static List<string> GetSpawns(IEnumerable<EntitySpawnEntry> entries,
         IRobustRandom? random = null)
     {
         IoCManager.Resolve(ref random);
 
-        var spawned = new List<string?>();
+        var spawned = new List<string>();
         var ungrouped = CollectOrGroups(entries, out var orGroupedSpawns);
 
         foreach (var entry in ungrouped)
@@ -93,6 +93,9 @@ public sealed class OrGroup
             if (entry.SpawnProbability != 1f && !random.Prob(entry.SpawnProbability))
                 continue;
 
+            if (entry.PrototypeId == null)
+                continue;
+
             var amount = (int) entry.GetAmount(random);
 
             for (var i = 0; i < amount; i++)
@@ -116,6 +119,9 @@ public sealed class OrGroup
                 if (diceRoll > cumulative)
                     continue;
 
+                if (entry.PrototypeId == null)
+                    break;
+
                 // Dice roll succeeded, add item and break loop
                 var amount = (int) entry.GetAmount(random);
 

Content.Shared/Storage/EntitySystems/SharedEntityStorageSystem.cs

@@ -22,6 +22,7 @@
 using Robust.Shared.Physics;
 using Robust.Shared.Physics.Components;
 using Robust.Shared.Physics.Systems;
+using Robust.Shared.Prototypes;
 using Robust.Shared.Timing;
 using Robust.Shared.Utility;
 
@@ -260,9 +261,12 @@ public bool Insert(EntityUid toInsert, EntityUid container, SharedEntityStorageC
         }
 
         _joints.RecursiveClearJoints(toInsert);
+        if (!component.Contents.Insert(toInsert, EntityManager))
+            return false;
+
         var inside = EnsureComp<InsideEntityStorageComponent>(toInsert);
         inside.Storage = container;
-        return component.Contents.Insert(toInsert, EntityManager);
+        return true;
     }
 
     public bool Remove(EntityUid toRemove, EntityUid container, SharedEntityStorageComponent? component = null, TransformComponent? xform = null)

Resources/Prototypes/Catalog/Fills/Crates/salvage.yml

@@ -92,30 +92,33 @@
   id: CratePartsT3
   name: tier 3 parts crate
   description: Contains 5 random tier 3 parts for upgrading machines.
-  components:
-  - type: StorageFill
-    contents:
-    - id: SalvagePartsT3Spawner
-      amount: 5
+  # TODO add contents.
+  #components:
+  #- type: StorageFill
+  #  contents:
+  #   - id: SalvagePartsT3Spawner
+  #    amount: 5
 
 - type: entity
   parent: CrateGenericSteel
   id: CratePartsT3T4
   name: tier 3/4 parts crate
   description: Contains 5 random tier 3 or 4 parts for upgrading machines.
-  components:
-  - type: StorageFill
-    contents:
-    - id: SalvagePartsT3T4Spawner
-      amount: 5
+  # TODO add contents.
+  #components:
+  # type: StorageFill
+  #  contents:
+  #  - id: SalvagePartsT3T4Spawner
+  #     amount: 5
 
 - type: entity
   parent: CrateGenericSteel
   id: CratePartsT4
   name: tier 4 parts crate
   description: Contains 5 random tier 4 parts for upgrading machines.
-  components:
-  - type: StorageFill
-    contents:
-    - id: SalvagePartsT4Spawner
-      amount: 5
+  # TODO add contents.
+  #components:
+  #- type: StorageFill
+  #  contents:
+  #  - id: SalvagePartsT4Spawner
+  #    amount: 5