Fix action state handling bug (#25395)

* Rejig action state handling

* Fix entity arg

* Fix deserialization

Content.Client/Actions/ActionsSystem.cs

@@ -78,10 +78,12 @@ namespace Content.Client.Actions
 
         private void BaseHandleState<T>(EntityUid uid, BaseActionComponent component, BaseActionComponentState state) where T : BaseActionComponent
         {
+            // TODO ACTIONS use auto comp states
             component.Icon = state.Icon;
             component.IconOn = state.IconOn;
             component.IconColor = state.IconColor;
-            component.Keywords = new HashSet<string>(state.Keywords);
+            component.Keywords.Clear();
+            component.Keywords.UnionWith(state.Keywords);
             component.Enabled = state.Enabled;
             component.Toggled = state.Toggled;
             component.Cooldown = state.Cooldown;
@@ -101,8 +103,7 @@ namespace Content.Client.Actions
             component.ItemIconStyle = state.ItemIconStyle;
             component.Sound = state.Sound;
 
-            if (_playerManager.LocalEntity == component.AttachedEntity)
+            UpdateAction(uid, component);
-                ActionsUpdated?.Invoke();
         }
 
         protected override void UpdateAction(EntityUid? actionId, BaseActionComponent? action = null)

Content.IntegrationTests/Tests/Actions/ActionPvsDetachTest.cs

@@ -0,0 +1,59 @@
+using System.Linq;
+using Content.Shared.Actions;
+using Content.Shared.Eye;
+using Robust.Server.GameObjects;
+using Robust.Shared.GameObjects;
+
+namespace Content.IntegrationTests.Tests.Actions;
+
+[TestFixture]
+public sealed class ActionPvsDetachTest
+{
+    [Test]
+    public async Task TestActionDetach()
+    {
+        await using var pair = await PoolManager.GetServerClient(new PoolSettings { Connected = true });
+        var (server, client) = pair;
+        var sys = server.System<SharedActionsSystem>();
+        var cSys = client.System<SharedActionsSystem>();
+
+        // Spawn mob that has some actions
+        EntityUid ent = default;
+        var map = await pair.CreateTestMap();
+        await server.WaitPost(() => ent = server.EntMan.SpawnAtPosition("MobHuman", map.GridCoords));
+        await pair.RunTicksSync(5);
+        var cEnt = pair.ToClientUid(ent);
+
+        // Verify that both the client & server agree on the number of actions
+        var initActions = sys.GetActions(ent).Count();
+        Assert.That(initActions, Is.GreaterThan(0));
+        Assert.That(initActions, Is.EqualTo(cSys.GetActions(cEnt).Count()));
+
+        // PVS-detach action entities
+        // We do this by just giving them the ghost layer
+        var visSys = server.System<VisibilitySystem>();
+        var enumerator = server.Transform(ent).ChildEnumerator;
+        while (enumerator.MoveNext(out var child))
+        {
+            visSys.AddLayer(child, (int) VisibilityFlags.Ghost);
+        }
+        await pair.RunTicksSync(5);
+
+        // Client's actions have left been detached / are out of view, but action comp state has not changed
+        Assert.That(sys.GetActions(ent).Count(), Is.EqualTo(initActions));
+        Assert.That(cSys.GetActions(cEnt).Count(), Is.EqualTo(initActions));
+
+        // Re-enter PVS view
+        enumerator = server.Transform(ent).ChildEnumerator;
+        while (enumerator.MoveNext(out var child))
+        {
+            visSys.RemoveLayer(child, (int) VisibilityFlags.Ghost);
+        }
+        await pair.RunTicksSync(5);
+        Assert.That(sys.GetActions(ent).Count(), Is.EqualTo(initActions));
+        Assert.That(cSys.GetActions(cEnt).Count(), Is.EqualTo(initActions));
+
+        await server.WaitPost(() => server.EntMan.DeleteEntity(map.MapUid));
+        await pair.CleanReturnAsync();
+    }
+}

Content.Shared/Actions/ActionContainerSystem.cs

@@ -37,7 +37,7 @@ public sealed class ActionContainerSystem : EntitySystem
 
     private void OnMindAdded(EntityUid uid, ActionsContainerComponent component, MindAddedMessage args)
     {
-        if(!_mind.TryGetMind(uid, out var mindId, out _))
+        if (!_mind.TryGetMind(uid, out var mindId, out _))
             return;
         if (!TryComp<ActionsContainerComponent>(mindId, out var mindActionContainerComp))
             return;
@@ -143,20 +143,15 @@ public sealed class ActionContainerSystem : EntitySystem
             return;
 
         DebugTools.AssertEqual(action.Container, newContainer);
-        DebugTools.AssertNull(action.AttachedEntity);
-
-        if (attached != null)
-            _actions.AddActionDirect(attached.Value, actionId, action: action);
-
         DebugTools.AssertEqual(action.AttachedEntity, attached);
     }
 
     /// <summary>
     /// Transfers all actions from one container to another, while keeping the attached entity the same.
     /// </summary>
-    /// &lt;remarks&gt;
+    /// <remarks>
     /// While the attached entity should be the same at the end, this will actually remove and then re-grant the action.
-    /// &lt;/remarks&gt;
+    /// </remarks>
     public void TransferAllActions(
         EntityUid from,
         EntityUid to,
@@ -305,11 +300,11 @@ public sealed class ActionContainerSystem : EntitySystem
         if (!_actions.TryGetActionData(args.Entity, out var data))
             return;
 
-        DebugTools.Assert(data.AttachedEntity == null || data.Container != EntityUid.Invalid);
+        if (data.Container != uid)
-        DebugTools.Assert(data.Container == null || data.Container == uid);
+        {
-
+            data.Container = uid;
-        data.Container = uid;
+            Dirty(args.Entity, data);
-        Dirty(uid, component);
+        }
 
         var ev = new ActionAddedEvent(args.Entity, data);
         RaiseLocalEvent(uid, ref ev);
@@ -320,21 +315,17 @@ public sealed class ActionContainerSystem : EntitySystem
         if (args.Container.ID != ActionsContainerComponent.ContainerId)
             return;
 
-        // Actions should only be getting removed while terminating or moving outside of PVS range.
-        DebugTools.Assert(Terminating(args.Entity)
-                          || _netMan.IsServer // I love gibbing code
-                          || _timing.ApplyingState);
-
         if (!_actions.TryGetActionData(args.Entity, out var data, false))
             return;
 
-        // No event - the only entity that should care about this is the entity that the action was provided to.
-        if (data.AttachedEntity != null)
-            _actions.RemoveAction(data.AttachedEntity.Value, args.Entity, null, data);
-
         var ev = new ActionRemovedEvent(args.Entity, data);
         RaiseLocalEvent(uid, ref ev);
+
+        if (data.Container == null)
+            return;
+
         data.Container = null;
+        Dirty(args.Entity, data);
     }
 
     private void OnActionAdded(EntityUid uid, ActionsContainerComponent component, ActionAddedEvent args)

Content.Shared/Actions/ActionsComponent.cs

@@ -26,8 +26,6 @@ public sealed class ActionsComponentState : ComponentState
     }
 }
 
-public readonly record struct ActionMetaData(bool ClientExclusive);
-
 /// <summary>
 ///     Determines how the action icon appears in the hotbar for item actions.
 /// </summary>

Content.Shared/Actions/BaseActionComponent.cs

@@ -4,7 +4,8 @@ using Robust.Shared.Utility;
 
 namespace Content.Shared.Actions;
 
-// TODO this should be an IncludeDataFields of each action component type, not use inheritance
+// TODO ACTIONS make this a seprate component and remove the inheritance stuff.
+// TODO ACTIONS convert to auto comp state?
 
 // TODO add access attribute. Need to figure out what to do with decal & mapping actions.
 // [Access(typeof(SharedActionsSystem))]
@@ -72,9 +73,9 @@ public abstract partial class BaseActionComponent : Component
     [DataField("charges")] public int? Charges;
 
     /// <summary>
-    ///     The max charges this action has, set automatically from <see cref="Charges"/>
+    ///     The max charges this action has. If null, this is set automatically from <see cref="Charges"/> on mapinit.
     /// </summary>
-    public int MaxCharges;
+    [DataField] public int? MaxCharges;
 
     /// <summary>
     ///     If enabled, charges will regenerate after a <see cref="Cooldown"/> is complete
@@ -130,7 +131,7 @@ public abstract partial class BaseActionComponent : Component
     /// <summary>
     ///     What entity, if any, currently has this action in the actions component?
     /// </summary>
-    [ViewVariables] public EntityUid? AttachedEntity;
+    [DataField] public EntityUid? AttachedEntity;
 
     /// <summary>
     ///     If true, this will cause the the action event to always be raised directed at the action performer/user instead of the action's container/provider.
@@ -171,7 +172,7 @@ public abstract class BaseActionComponentState : ComponentState
     public (TimeSpan Start, TimeSpan End)? Cooldown;
     public TimeSpan? UseDelay;
     public int? Charges;
-    public int MaxCharges;
+    public int? MaxCharges;
     public bool RenewCharges;
     public NetEntity? Container;
     public NetEntity? EntityIcon;

Content.Shared/Actions/SharedActionsSystem.cs

@@ -8,7 +8,6 @@ using Content.Shared.Hands;
 using Content.Shared.Interaction;
 using Content.Shared.Inventory.Events;
 using Content.Shared.Mind;
-using Robust.Shared.Audio;
 using Robust.Shared.Audio.Systems;
 using Robust.Shared.Containers;
 using Robust.Shared.GameStates;
@@ -35,9 +34,13 @@ public abstract class SharedActionsSystem : EntitySystem
     {
         base.Initialize();
 
-        SubscribeLocalEvent<InstantActionComponent, MapInitEvent>(OnInit);
+        SubscribeLocalEvent<InstantActionComponent, MapInitEvent>(OnActionMapInit);
-        SubscribeLocalEvent<EntityTargetActionComponent, MapInitEvent>(OnInit);
+        SubscribeLocalEvent<EntityTargetActionComponent, MapInitEvent>(OnActionMapInit);
-        SubscribeLocalEvent<WorldTargetActionComponent, MapInitEvent>(OnInit);
+        SubscribeLocalEvent<WorldTargetActionComponent, MapInitEvent>(OnActionMapInit);
+
+        SubscribeLocalEvent<InstantActionComponent, ComponentShutdown>(OnActionShutdown);
+        SubscribeLocalEvent<EntityTargetActionComponent, ComponentShutdown>(OnActionShutdown);
+        SubscribeLocalEvent<WorldTargetActionComponent, ComponentShutdown>(OnActionShutdown);
 
         SubscribeLocalEvent<ActionsComponent, DidEquipEvent>(OnDidEquip);
         SubscribeLocalEvent<ActionsComponent, DidEquipHandEvent>(OnHandEquipped);
@@ -60,10 +63,19 @@ public abstract class SharedActionsSystem : EntitySystem
         SubscribeAllEvent<RequestPerformActionEvent>(OnActionRequest);
     }
 
-    private void OnInit(EntityUid uid, BaseActionComponent component, MapInitEvent args)
+    private void OnActionMapInit(EntityUid uid, BaseActionComponent component, MapInitEvent args)
     {
-        if (component.Charges != null)
+        if (component.Charges == null)
-            component.MaxCharges = component.Charges.Value;
+            return;
+
+        component.MaxCharges ??= component.Charges.Value;
+        Dirty(uid, component);
+    }
+
+    private void OnActionShutdown(EntityUid uid, BaseActionComponent component, ComponentShutdown args)
+    {
+        if (component.AttachedEntity != null && !TerminatingOrDeleted(component.AttachedEntity.Value))
+            RemoveAction(component.AttachedEntity.Value, uid, action: component);
     }
 
     private void OnShutdown(EntityUid uid, ActionsComponent component, ComponentShutdown args)

Content.Shared/Clothing/EntitySystems/ToggleableClothingSystem.cs

@@ -170,12 +170,7 @@ public sealed class ToggleableClothingSystem : EntitySystem
         // "outside" of the container or not. This means that if a hardsuit takes too much damage, the helmet will also
         // automatically be deleted.
 
-        // remove action.
+        _actionsSystem.RemoveAction(component.ActionEntity);
-        if (_actionsSystem.TryGetActionData(component.ActionEntity, out var action) &&
-            action.AttachedEntity != null)
-        {
-            _actionsSystem.RemoveAction(action.AttachedEntity.Value, component.ActionEntity);
-        }
 
         if (component.ClothingUid != null && !_netMan.IsClient)
             QueueDel(component.ClothingUid.Value);
@@ -199,13 +194,7 @@ public sealed class ToggleableClothingSystem : EntitySystem
         if (toggleComp.LifeStage > ComponentLifeStage.Running)
             return;
 
-        // remove action.
+        _actionsSystem.RemoveAction(toggleComp.ActionEntity);
-        if (_actionsSystem.TryGetActionData(toggleComp.ActionEntity, out var action) &&
-            action.AttachedEntity != null)
-        {
-            _actionsSystem.RemoveAction(action.AttachedEntity.Value, toggleComp.ActionEntity);
-        }
-
         RemComp(component.AttachedUid, toggleComp);
     }