From bd4597c5ca8a3c55b17868a2c8aef543d3ea503d Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Mon, 19 Feb 2024 21:08:41 -0500 Subject: [PATCH] Fix action state handling bug (#25395) * Rejig action state handling * Fix entity arg * Fix deserialization --- Content.Client/Actions/ActionsSystem.cs | 7 ++- .../Tests/Actions/ActionPvsDetachTest.cs | 59 +++++++++++++++++++ .../Actions/ActionContainerSystem.cs | 35 ++++------- Content.Shared/Actions/ActionsComponent.cs | 2 - Content.Shared/Actions/BaseActionComponent.cs | 11 ++-- Content.Shared/Actions/SharedActionsSystem.cs | 26 +++++--- .../EntitySystems/ToggleableClothingSystem.cs | 15 +---- 7 files changed, 103 insertions(+), 52 deletions(-) create mode 100644 Content.IntegrationTests/Tests/Actions/ActionPvsDetachTest.cs diff --git a/Content.Client/Actions/ActionsSystem.cs b/Content.Client/Actions/ActionsSystem.cs index 2343b9eac1..4eaf06b691 100644 --- a/Content.Client/Actions/ActionsSystem.cs +++ b/Content.Client/Actions/ActionsSystem.cs @@ -78,10 +78,12 @@ namespace Content.Client.Actions private void BaseHandleState(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(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) - ActionsUpdated?.Invoke(); + UpdateAction(uid, component); } protected override void UpdateAction(EntityUid? actionId, BaseActionComponent? action = null) diff --git a/Content.IntegrationTests/Tests/Actions/ActionPvsDetachTest.cs b/Content.IntegrationTests/Tests/Actions/ActionPvsDetachTest.cs new file mode 100644 index 0000000000..420a90a50b --- /dev/null +++ b/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(); + var cSys = client.System(); + + // 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(); + 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(); + } +} diff --git a/Content.Shared/Actions/ActionContainerSystem.cs b/Content.Shared/Actions/ActionContainerSystem.cs index 17bcf11bff..1c5a3ba0d9 100644 --- a/Content.Shared/Actions/ActionContainerSystem.cs +++ b/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(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); } /// /// Transfers all actions from one container to another, while keeping the attached entity the same. /// - /// <remarks> + /// /// While the attached entity should be the same at the end, this will actually remove and then re-grant the action. - /// </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); - DebugTools.Assert(data.Container == null || data.Container == uid); - - data.Container = uid; - Dirty(uid, component); + if (data.Container != uid) + { + data.Container = uid; + Dirty(args.Entity, data); + } 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) diff --git a/Content.Shared/Actions/ActionsComponent.cs b/Content.Shared/Actions/ActionsComponent.cs index b810e98d4d..a081a23867 100644 --- a/Content.Shared/Actions/ActionsComponent.cs +++ b/Content.Shared/Actions/ActionsComponent.cs @@ -26,8 +26,6 @@ public sealed class ActionsComponentState : ComponentState } } -public readonly record struct ActionMetaData(bool ClientExclusive); - /// /// Determines how the action icon appears in the hotbar for item actions. /// diff --git a/Content.Shared/Actions/BaseActionComponent.cs b/Content.Shared/Actions/BaseActionComponent.cs index 291d9a3ea2..cce7b912c7 100644 --- a/Content.Shared/Actions/BaseActionComponent.cs +++ b/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; /// - /// The max charges this action has, set automatically from + /// The max charges this action has. If null, this is set automatically from on mapinit. /// - public int MaxCharges; + [DataField] public int? MaxCharges; /// /// If enabled, charges will regenerate after a is complete @@ -130,7 +131,7 @@ public abstract partial class BaseActionComponent : Component /// /// What entity, if any, currently has this action in the actions component? /// - [ViewVariables] public EntityUid? AttachedEntity; + [DataField] public EntityUid? AttachedEntity; /// /// 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; diff --git a/Content.Shared/Actions/SharedActionsSystem.cs b/Content.Shared/Actions/SharedActionsSystem.cs index 90508bff9d..a6c40c7ae3 100644 --- a/Content.Shared/Actions/SharedActionsSystem.cs +++ b/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(OnInit); - SubscribeLocalEvent(OnInit); - SubscribeLocalEvent(OnInit); + SubscribeLocalEvent(OnActionMapInit); + SubscribeLocalEvent(OnActionMapInit); + SubscribeLocalEvent(OnActionMapInit); + + SubscribeLocalEvent(OnActionShutdown); + SubscribeLocalEvent(OnActionShutdown); + SubscribeLocalEvent(OnActionShutdown); SubscribeLocalEvent(OnDidEquip); SubscribeLocalEvent(OnHandEquipped); @@ -60,10 +63,19 @@ public abstract class SharedActionsSystem : EntitySystem SubscribeAllEvent(OnActionRequest); } - private void OnInit(EntityUid uid, BaseActionComponent component, MapInitEvent args) + private void OnActionMapInit(EntityUid uid, BaseActionComponent component, MapInitEvent args) { - if (component.Charges != null) - component.MaxCharges = component.Charges.Value; + if (component.Charges == null) + 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) diff --git a/Content.Shared/Clothing/EntitySystems/ToggleableClothingSystem.cs b/Content.Shared/Clothing/EntitySystems/ToggleableClothingSystem.cs index f03745006f..0138de7a98 100644 --- a/Content.Shared/Clothing/EntitySystems/ToggleableClothingSystem.cs +++ b/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. - if (_actionsSystem.TryGetActionData(component.ActionEntity, out var action) && - action.AttachedEntity != null) - { - _actionsSystem.RemoveAction(action.AttachedEntity.Value, component.ActionEntity); - } + _actionsSystem.RemoveAction(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. - if (_actionsSystem.TryGetActionData(toggleComp.ActionEntity, out var action) && - action.AttachedEntity != null) - { - _actionsSystem.RemoveAction(action.AttachedEntity.Value, toggleComp.ActionEntity); - } - + _actionsSystem.RemoveAction(toggleComp.ActionEntity); RemComp(component.AttachedUid, toggleComp); }