Fix some machine construction bugs. (#18179)

Content.Server/Construction/MachineFrameSystem.cs

@@ -7,6 +7,7 @@ using Content.Shared.Stacks;
 using Content.Shared.Tag;
 using Content.Shared.Popups;
 using Robust.Shared.Containers;
+using Robust.Shared.Utility;
 
 namespace Content.Server.Construction;
 
@@ -48,83 +49,40 @@ public sealed class MachineFrameSystem : EntitySystem
 
     private void OnInteractUsing(EntityUid uid, MachineFrameComponent component, InteractUsingEvent args)
     {
-        if (!component.HasBoard && TryComp<MachineBoardComponent?>(args.Used, out var machineBoard))
+        if (args.Handled)
-        {
+            return;
-            if (_container.TryRemoveFromContainer(args.Used))
-            {
-                // Valid board!
-                component.BoardContainer.Insert(args.Used);
 
-                // Setup requirements and progress...
+        if (!component.HasBoard)
-                ResetProgressAndRequirements(component, machineBoard);
+        {
+            if (TryInsertBoard(uid, args.Used, component))
+                args.Handled = true;
+            return;
+        }
 
-                if (TryComp(uid, out ConstructionComponent? construction))
+        // Machine parts cannot currently satisfy stack/component/tag restrictions. Similarly stacks cannot satisfy
-                {
+        // component/tag restrictions. However, there is no reason this cannot be supported in the future. If this
-                    // So prying the components off works correctly.
+        // changes, then RegenerateProgress() also needs to be updated.
-                    _construction.ResetEdge(uid, construction);
+        //
-                }
+        // Note that one entity is ALLOWED to satisfy more than one kind of component or tag requirements. This is
-            }
+        // necessary in order to avoid weird entity-ordering shenanigans in RegenerateProgress().
-        }
+
-        else if (component.HasBoard)
+        // Handle parts
-        {
         if (TryComp<MachinePartComponent>(args.Used, out var machinePart))
         {
-                if (!component.Requirements.ContainsKey(machinePart.PartType))
+            if (TryInsertPart(uid, args.Used, component, machinePart))
-                    return;
-
-                if (component.Progress[machinePart.PartType] != component.Requirements[machinePart.PartType]
-                    && _container.TryRemoveFromContainer(args.Used) && component.PartContainer.Insert(args.Used))
-                {
-                    component.Progress[machinePart.PartType]++;
-                    args.Handled = true;
-                }
-            }
-
-            if (!args.Handled && TryComp<StackComponent?>(args.Used, out var stack))
-            {
-                var type = stack.StackTypeId;
-                if (type == null)
-                    return;
-                if (!component.MaterialRequirements.ContainsKey(type))
-                    return;
-
-                if (component.MaterialProgress[type] == component.MaterialRequirements[type])
-                    return;
-
-                var needed = component.MaterialRequirements[type] - component.MaterialProgress[type];
-                var count = stack.Count;
-
-                if (count < needed)
-                {
-                    if (!component.PartContainer.Insert(stack.Owner))
-                        return;
-
-                    component.MaterialProgress[type] += count;
                 args.Handled = true;
             return;
         }
 
-                var splitStack = _stack.Split(args.Used, needed,
+        // Handle stacks
-                    Comp<TransformComponent>(uid).Coordinates, stack);
+        if (TryComp<StackComponent?>(args.Used, out var stack))
-
-                if (splitStack == null)
-                    return;
-
-                if (!component.PartContainer.Insert(splitStack.Value))
-                    return;
-
-                component.MaterialProgress[type] += needed;
-                args.Handled = true;
-            }
-
-            if (args.Handled)
         {
-                if (IsComplete(component)) {
+            if (TryInsertStack(uid, args.Used, component, stack))
-                    _popupSystem.PopupEntity(Loc.GetString("machine-frame-component-on-complete"), uid);
+                args.Handled = true;
-                }
             return;
         }
 
+        // Handle component requirements
         foreach (var (compName, info) in component.ComponentRequirements)
         {
             if (component.ComponentProgress[compName] >= info.Amount)
@@ -135,30 +93,147 @@ public sealed class MachineFrameSystem : EntitySystem
             if (!HasComp(args.Used, registration.Type))
                 continue;
 
-                if (!_container.TryRemoveFromContainer(args.Used) || !component.PartContainer.Insert(args.Used))
+            // Insert the entity, if it hasn't already been inserted
-                    continue;
+            if (!args.Handled)
-                component.ComponentProgress[compName]++;
+            {
+                if (!_container.TryRemoveFromContainer(args.Used))
+                    return;
+
                 args.Handled = true;
+                if (!component.PartContainer.Insert(args.Used))
                     return;
             }
 
+            component.ComponentProgress[compName]++;
+
+            if (IsComplete(component))
+            {
+                _popupSystem.PopupEntity(Loc.GetString("machine-frame-component-on-complete"), uid);
+                return;
+            }
+        }
+
+        // Handle tag requirements
+        if (!TryComp<TagComponent>(args.Used, out var tagComp))
+            return;
+
         foreach (var (tagName, info) in component.TagRequirements)
         {
             if (component.TagProgress[tagName] >= info.Amount)
                 continue;
 
-                if (!_tag.HasTag(args.Used, tagName))
+            if (!_tag.HasTag(tagComp, tagName))
                 continue;
 
-                if (!_container.TryRemoveFromContainer(args.Used) || !component.PartContainer.Insert(args.Used))
+            // Insert the entity, if it hasn't already been inserted
-                    continue;
+            if (!args.Handled)
+            {
+                if (!_container.TryRemoveFromContainer(args.Used))
+                    return;
+
+                args.Handled = true;
+                if (!component.PartContainer.Insert(args.Used))
+                    return;
+            }
+
             component.TagProgress[tagName]++;
             args.Handled = true;
+
+            if (IsComplete(component))
+            {
+                _popupSystem.PopupEntity(Loc.GetString("machine-frame-component-on-complete"), uid);
                 return;
             }
         }
     }
 
+    /// <returns>Whether or not the function had any effect. Does not indicate success.</returns>
+    private bool TryInsertBoard(EntityUid uid, EntityUid used, MachineFrameComponent component)
+    {
+        if (!TryComp<MachineBoardComponent?>(used, out var machineBoard))
+            return false;
+
+        if (!_container.TryRemoveFromContainer(used))
+            return false;
+
+        if (!component.BoardContainer.Insert(used))
+            return true;
+
+        ResetProgressAndRequirements(component, machineBoard);
+
+        // Reset edge so that prying the components off works correctly.
+        if (TryComp(uid, out ConstructionComponent? construction))
+            _construction.ResetEdge(uid, construction);
+
+        return true;
+    }
+
+    /// <returns>Whether or not the function had any effect. Does not indicate success.</returns>
+    private bool TryInsertPart(EntityUid uid, EntityUid used, MachineFrameComponent component, MachinePartComponent machinePart)
+    {
+        DebugTools.Assert(!HasComp<StackComponent>(uid));
+        if (!component.Requirements.ContainsKey(machinePart.PartType))
+            return false;
+
+        if (component.Progress[machinePart.PartType] >= component.Requirements[machinePart.PartType])
+            return false;
+
+        if (!_container.TryRemoveFromContainer(used))
+            return false;
+
+        if (!component.PartContainer.Insert(used))
+            return true;
+
+        component.Progress[machinePart.PartType]++;
+        if (IsComplete(component))
+            _popupSystem.PopupEntity(Loc.GetString("machine-frame-component-on-complete"), uid);
+
+        return true;
+    }
+
+    /// <returns>Whether or not the function had any effect. Does not indicate success.</returns>
+    private bool TryInsertStack(EntityUid uid, EntityUid used, MachineFrameComponent component, StackComponent stack)
+    {
+        var type = stack.StackTypeId;
+
+        if (!component.MaterialRequirements.ContainsKey(type))
+            return false;
+
+        var progress = component.MaterialProgress[type];
+        var requirement = component.MaterialRequirements[type];
+        var needed = requirement - progress;
+
+        if (needed <= 0)
+            return false;
+
+        var count = stack.Count;
+        if (count < needed)
+        {
+            if (!_container.TryRemoveFromContainer(used))
+                return false;
+
+            if (!component.PartContainer.Insert(used))
+                return true;
+
+            component.MaterialProgress[type] += count;
+            return true;
+        }
+
+        var splitStack = _stack.Split(used, needed, Transform(uid).Coordinates, stack);
+
+        if (splitStack == null)
+            return false;
+
+        if (!component.PartContainer.Insert(splitStack.Value))
+            return true;
+
+        component.MaterialProgress[type] += needed;
+        if (IsComplete(component))
+            _popupSystem.PopupEntity(Loc.GetString("machine-frame-component-on-complete"), uid);
+
+        return true;
+    }
+
     public bool IsComplete(MachineFrameComponent component)
     {
         if (!component.HasBoard)
@@ -247,10 +322,14 @@ public sealed class MachineFrameSystem : EntitySystem
 
         ResetProgressAndRequirements(component, machineBoard);
 
+        // If the following code is updated, you need to make sure that it matches the logic in OnInteractUsing()
+
         foreach (var part in component.PartContainer.ContainedEntities)
         {
             if (TryComp<MachinePartComponent>(part, out var machinePart))
             {
+                DebugTools.Assert(!HasComp<StackComponent>(part));
+
                 // Check this is part of the requirements...
                 if (!component.Requirements.ContainsKey(machinePart.PartType))
                     continue;
@@ -259,21 +338,23 @@ public sealed class MachineFrameSystem : EntitySystem
                     component.Progress[machinePart.PartType] = 1;
                 else
                     component.Progress[machinePart.PartType]++;
+
+                continue;
             }
 
             if (TryComp<StackComponent>(part, out var stack))
             {
                 var type = stack.StackTypeId;
-                // Check this is part of the requirements...
+
-                if (type == null)
-                    continue;
                 if (!component.MaterialRequirements.ContainsKey(type))
                     continue;
 
                 if (!component.MaterialProgress.ContainsKey(type))
-                    component.MaterialProgress[type] = 1;
+                    component.MaterialProgress[type] = stack.Count;
                 else
-                    component.MaterialProgress[type]++;
+                    component.MaterialProgress[type] += stack.Count;
+
+                continue;
             }
 
             // I have many regrets.
@@ -290,10 +371,13 @@ public sealed class MachineFrameSystem : EntitySystem
                     component.ComponentProgress[compName]++;
             }
 
+            if (!TryComp<TagComponent>(part, out var tagComp))
+                continue;
+
             // I have MANY regrets.
-            foreach (var (tagName, _) in component.TagRequirements)
+            foreach (var tagName in component.TagRequirements.Keys)
             {
-                if (!_tag.HasTag(part, tagName))
+                if (!_tag.HasTag(tagComp, tagName))
                     continue;
 
                 if (!component.TagProgress.ContainsKey(tagName))

Content.Server/Stack/StackSystem.cs

@@ -50,15 +50,15 @@ namespace Content.Server.Stack
             if (!Resolve(uid, ref stack))
                 return null;
 
+            // Try to remove the amount of things we want to split from the original stack...
+            if (!Use(uid, amount, stack))
+                return null;
+
             // Get a prototype ID to spawn the new entity. Null is also valid, although it should rarely be picked...
             var prototype = _prototypeManager.TryIndex<StackPrototype>(stack.StackTypeId, out var stackType)
                 ? stackType.Spawn
                 : Prototype(uid)?.ID;
 
-            // Try to remove the amount of things we want to split from the original stack...
-            if (!Use(uid, amount, stack))
-                return null;
-
             // Set the output parameter in the event instance to the newly split stack.
             var entity = Spawn(prototype, spawnPosition);
 

Content.Shared/Stacks/SharedStackSystem.cs

@@ -167,6 +167,7 @@ namespace Content.Shared.Stacks
             amount = Math.Min(amount, GetMaxCount(component));
             amount = Math.Max(amount, 0);
 
+            // Server-side override deletes the entity if count == 0
             component.Count = amount;
             Dirty(component);
 

Content.Shared/Stacks/StackComponent.cs

@@ -28,6 +28,7 @@ namespace Content.Shared.Stacks
 
         /// <summary>
         ///     Set to true to not reduce the count when used.
+        ///     Note that <see cref="Count"/> still limits the amount that can be used at any one time.
         /// </summary>
         [DataField("unlimited")]
         [ViewVariables(VVAccess.ReadOnly)]