From 5ade6688e78548175c86d60ea5211137d252b87e Mon Sep 17 00:00:00 2001 From: Perry Fraser Date: Sat, 11 Oct 2025 15:42:10 -0400 Subject: [PATCH] HOTFIX Fix pickup effects occurring with verb creation (#38705) * fix: don't run pickup effects on verb creation * review * redundant --------- Co-authored-by: slarticodefast <161409025+slarticodefast@users.noreply.github.com> --- Content.Server/Lube/LubedSystem.cs | 45 +++++++++++++----- .../ActionBlocker/ActionBlockerSystem.cs | 12 +++-- .../Containers/ItemSlot/ItemSlotsSystem.cs | 2 +- .../SharedHandsSystem.Interactions.cs | 2 +- .../EntitySystems/SharedHandsSystem.Pickup.cs | 47 ++++++++++++++++--- Content.Shared/Hands/HandEvents.cs | 27 +++++++++++ Content.Shared/Item/MultiHandedItemSystem.cs | 11 +++-- Content.Shared/Item/PickupAttemptEvent.cs | 27 +++++++++-- .../Strip/SharedStrippableSystem.cs | 2 +- 9 files changed, 141 insertions(+), 34 deletions(-) diff --git a/Content.Server/Lube/LubedSystem.cs b/Content.Server/Lube/LubedSystem.cs index 3c536dcceb..01a2fa8dde 100644 --- a/Content.Server/Lube/LubedSystem.cs +++ b/Content.Server/Lube/LubedSystem.cs @@ -1,9 +1,11 @@ +using Content.Shared.Hands; +using Content.Shared.Hands.EntitySystems; using Content.Shared.IdentityManagement; +using Content.Shared.Item; using Content.Shared.Lube; using Content.Shared.NameModifier.EntitySystems; using Content.Shared.Popups; using Content.Shared.Throwing; -using Robust.Shared.Containers; using Robust.Shared.Random; namespace Content.Server.Lube; @@ -21,7 +23,7 @@ public sealed class LubedSystem : EntitySystem base.Initialize(); SubscribeLocalEvent(OnInit); - SubscribeLocalEvent(OnHandPickUp); + SubscribeLocalEvent(OnHandPickUp); SubscribeLocalEvent(OnRefreshNameModifiers); } @@ -30,21 +32,38 @@ public sealed class LubedSystem : EntitySystem _nameMod.RefreshNameModifiers(uid); } - private void OnHandPickUp(EntityUid uid, LubedComponent component, ContainerGettingInsertedAttemptEvent args) + /// + /// Note to whoever makes this predicted—there is a mispredict here that + /// would be nice to keep! If this is in shared, the client will predict + /// this and not run the pickup animation in + /// which would (probably) make this effect look less funny. You will + /// probably want to either tweak + /// to be able to cancel but still run the animation or something—we do want + /// the event to run before the animation for stuff like + /// . + /// + private void OnHandPickUp(Entity ent, ref BeforeGettingEquippedHandEvent args) { - if (component.SlipsLeft <= 0) + if (args.Cancelled) + return; + + if (ent.Comp.SlipsLeft <= 0) { - RemComp(uid); - _nameMod.RefreshNameModifiers(uid); + RemComp(ent); + _nameMod.RefreshNameModifiers(ent.Owner); return; } - component.SlipsLeft--; - args.Cancel(); - var user = args.Container.Owner; - _transform.SetCoordinates(uid, Transform(user).Coordinates); - _transform.AttachToGridOrMap(uid); - _throwing.TryThrow(uid, _random.NextVector2(), baseThrowSpeed: component.SlipStrength); - _popup.PopupEntity(Loc.GetString("lube-slip", ("target", Identity.Entity(uid, EntityManager))), user, user, PopupType.MediumCaution); + + ent.Comp.SlipsLeft--; + args.Cancelled = true; + + _transform.SetCoordinates(ent, Transform(args.User).Coordinates); + _transform.AttachToGridOrMap(ent); + _throwing.TryThrow(ent, _random.NextVector2(), ent.Comp.SlipStrength); + _popup.PopupEntity(Loc.GetString("lube-slip", ("target", Identity.Entity(ent, EntityManager))), + args.User, + args.User, + PopupType.MediumCaution); } private void OnRefreshNameModifiers(Entity entity, ref RefreshNameModifiersEvent args) diff --git a/Content.Shared/ActionBlocker/ActionBlockerSystem.cs b/Content.Shared/ActionBlocker/ActionBlockerSystem.cs index 08eac657c0..c256872cc7 100644 --- a/Content.Shared/ActionBlocker/ActionBlockerSystem.cs +++ b/Content.Shared/ActionBlocker/ActionBlockerSystem.cs @@ -167,15 +167,21 @@ namespace Content.Shared.ActionBlocker return !ev.Cancelled; } - public bool CanPickup(EntityUid user, EntityUid item) + /// + /// Whether a user can pickup the given item. + /// + /// The mob trying to pick up the item. + /// The item being picked up. + /// Whether or not to show a popup to the player telling them why the attempt failed. + public bool CanPickup(EntityUid user, EntityUid item, bool showPopup = false) { - var userEv = new PickupAttemptEvent(user, item); + var userEv = new PickupAttemptEvent(user, item, showPopup); RaiseLocalEvent(user, userEv); if (userEv.Cancelled) return false; - var itemEv = new GettingPickedUpAttemptEvent(user, item); + var itemEv = new GettingPickedUpAttemptEvent(user, item, showPopup); RaiseLocalEvent(item, itemEv); return !itemEv.Cancelled; diff --git a/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs b/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs index 88e6ca44dc..718d2a0524 100644 --- a/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs +++ b/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs @@ -573,7 +573,7 @@ namespace Content.Shared.Containers.ItemSlots item = slot.Item; // This handles user logic - if (user != null && item != null && !_actionBlockerSystem.CanPickup(user.Value, item.Value)) + if (user != null && item != null && !_actionBlockerSystem.CanPickup(user.Value, item.Value, showPopup: true)) return false; Eject(uid, slot, item!.Value, user, excludeUserAudio); diff --git a/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Interactions.cs b/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Interactions.cs index 0e8d7a7556..cd6085535a 100644 --- a/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Interactions.cs +++ b/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Interactions.cs @@ -181,7 +181,7 @@ public abstract partial class SharedHandsSystem : EntitySystem if (!CanDropHeld(uid, handName, checkActionBlocker)) return false; - if (!CanPickupToHand(uid, entity.Value, handsComp.ActiveHandId, checkActionBlocker, handsComp)) + if (!CanPickupToHand(uid, entity.Value, handsComp.ActiveHandId, checkActionBlocker: checkActionBlocker, handsComp: handsComp)) return false; DoDrop(uid, handName, false, log: false); diff --git a/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Pickup.cs b/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Pickup.cs index ea13004313..c19de9629a 100644 --- a/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Pickup.cs +++ b/Content.Shared/Hands/EntitySystems/SharedHandsSystem.Pickup.cs @@ -1,4 +1,3 @@ -using System.Diagnostics; using Content.Shared.Database; using Content.Shared.Hands.Components; using Content.Shared.Item; @@ -84,7 +83,10 @@ public abstract partial class SharedHandsSystem if (!Resolve(entity, ref item, false)) return false; - if (!CanPickupToHand(uid, entity, handId, checkActionBlocker, handsComp, item)) + if (!CanPickupToHand(uid, entity, handId, checkActionBlocker: checkActionBlocker, showPopup: true, handsComp: handsComp, item: item)) + return false; + + if (!BeforeDoPickup((uid, handsComp), entity)) return false; if (animate) @@ -151,7 +153,11 @@ public abstract partial class SharedHandsSystem return false; } - public bool CanPickupAnyHand(EntityUid uid, EntityUid entity, bool checkActionBlocker = true, HandsComponent? handsComp = null, ItemComponent? item = null) + /// + /// Checks whether a given item will fit into the user's first free hand. + /// Unless otherwise specified, this will also check the general CanPickup action blocker. + /// + public bool CanPickupAnyHand(EntityUid uid, EntityUid entity, bool checkActionBlocker = true, bool showPopup = false, HandsComponent? handsComp = null, ItemComponent? item = null) { if (!Resolve(uid, ref handsComp, false)) return false; @@ -159,13 +165,14 @@ public abstract partial class SharedHandsSystem if (!TryGetEmptyHand((uid, handsComp), out var hand)) return false; - return CanPickupToHand(uid, entity, hand, checkActionBlocker, handsComp, item); + return CanPickupToHand(uid, entity, hand, checkActionBlocker, showPopup, handsComp, item); } /// - /// Checks whether a given item will fit into a specific user's hand. Unless otherwise specified, this will also check the general CanPickup action blocker. + /// Checks whether a given item will fit into a specific user's hand. + /// Unless otherwise specified, this will also check the general CanPickup action blocker. /// - public bool CanPickupToHand(EntityUid uid, EntityUid entity, string handId, bool checkActionBlocker = true, HandsComponent? handsComp = null, ItemComponent? item = null) + public bool CanPickupToHand(EntityUid uid, EntityUid entity, string handId, bool checkActionBlocker = true, bool showPopup = false, HandsComponent? handsComp = null, ItemComponent? item = null) { if (!Resolve(uid, ref handsComp, false)) return false; @@ -176,13 +183,17 @@ public abstract partial class SharedHandsSystem if (handContainer.ContainedEntities.FirstOrNull() != null) return false; + // Huh, seems kinda weird that this system passes item comp around + // everywhere but it's never actually used besides being resolved. + // I wouldn't be surprised if there's some API simplifications that + // could be made with respect to that. if (!Resolve(entity, ref item, false)) return false; if (TryComp(entity, out PhysicsComponent? physics) && physics.BodyType == BodyType.Static) return false; - if (checkActionBlocker && !_actionBlocker.CanPickup(uid, entity)) + if (checkActionBlocker && !_actionBlocker.CanPickup(uid, entity, showPopup)) return false; if (!CheckWhitelists((uid, handsComp), handId, entity)) @@ -232,6 +243,28 @@ public abstract partial class SharedHandsSystem } } + /// + /// Small helper function meant as a last step before + /// is called. Used to run a cancelable before pickup event that can have + /// side effects, unlike the side effect free . + /// + private bool BeforeDoPickup(Entity user, EntityUid item) + { + if (!Resolve(user, ref user.Comp)) + return false; + + var userEv = new BeforeEquippingHandEvent(item); + RaiseLocalEvent(user, ref userEv); + + if (userEv.Cancelled) + return false; + + var itemEv = new BeforeGettingEquippedHandEvent(user); + RaiseLocalEvent(item, ref itemEv); + + return !itemEv.Cancelled; + } + /// /// Puts an entity into the player's hand, assumes that the insertion is allowed. In general, you should not be calling this function directly. /// diff --git a/Content.Shared/Hands/HandEvents.cs b/Content.Shared/Hands/HandEvents.cs index 0499c05f42..3d3cb9a322 100644 --- a/Content.Shared/Hands/HandEvents.cs +++ b/Content.Shared/Hands/HandEvents.cs @@ -1,5 +1,6 @@ using System.Numerics; using Content.Shared.Hands.Components; +using Content.Shared.Hands.EntitySystems; using JetBrains.Annotations; using Robust.Shared.Map; using Robust.Shared.Serialization; @@ -156,6 +157,32 @@ namespace Content.Shared.Hands } } + /// + /// Raised against an item being picked up before it is actually inserted + /// into the pick-up-ers hand container. This can be handled with side + /// effects, and may be canceled preventing the pickup in a way that + /// and similar don't see. + /// + /// The user picking up the item. + /// + /// If true, the item will not be equipped into the user's hand. + /// + [ByRefEvent] + public record struct BeforeGettingEquippedHandEvent(EntityUid User, bool Cancelled = false); + + /// + /// Raised against a mob picking up and item before it is actually inserted + /// into the pick-up-ers hand container. This can be handled with side + /// effects, and may be canceled preventing the pickup in a way that + /// and similar don't see. + /// + /// The item being picked up. + /// + /// If true, the item will not be equipped into the user's hand. + /// + [ByRefEvent] + public record struct BeforeEquippingHandEvent(EntityUid Item, bool Cancelled = false); + /// /// Raised when putting an entity into a hand slot /// diff --git a/Content.Shared/Item/MultiHandedItemSystem.cs b/Content.Shared/Item/MultiHandedItemSystem.cs index db64610eae..9d30d57a91 100644 --- a/Content.Shared/Item/MultiHandedItemSystem.cs +++ b/Content.Shared/Item/MultiHandedItemSystem.cs @@ -37,12 +37,17 @@ public sealed class MultiHandedItemSystem : EntitySystem private void OnAttemptPickup(Entity ent, ref GettingPickedUpAttemptEvent args) { - if (_hands.CountFreeHands(args.User) >= ent.Comp.HandsNeeded) + if (args.Cancelled || _hands.CountFreeHands(args.User) >= ent.Comp.HandsNeeded) return; args.Cancel(); - _popup.PopupPredictedCursor(Loc.GetString("multi-handed-item-pick-up-fail", - ("number", ent.Comp.HandsNeeded - 1), ("item", ent.Owner)), args.User); + + if (args.ShowPopup) + _popup.PopupPredictedCursor( + Loc.GetString("multi-handed-item-pick-up-fail", + ("number", ent.Comp.HandsNeeded - 1), + ("item", ent.Owner)), + args.User); } private void OnVirtualItemDeleted(Entity ent, ref VirtualItemDeletedEvent args) diff --git a/Content.Shared/Item/PickupAttemptEvent.cs b/Content.Shared/Item/PickupAttemptEvent.cs index eb0b4c8dcc..c8382fa08f 100644 --- a/Content.Shared/Item/PickupAttemptEvent.cs +++ b/Content.Shared/Item/PickupAttemptEvent.cs @@ -1,30 +1,47 @@ namespace Content.Shared.Item; /// -/// Raised on a *mob* when it tries to pickup something +/// Raised on a *mob* when it tries to pickup something. +/// IMPORTANT: Attempt event subscriptions should not be doing any state changes like throwing items, opening UIs, playing sounds etc! /// public sealed class PickupAttemptEvent : BasePickupAttemptEvent { - public PickupAttemptEvent(EntityUid user, EntityUid item) : base(user, item) { } + public PickupAttemptEvent(EntityUid user, EntityUid item, bool showPopup) : base(user, item, showPopup) { } } /// -/// Raised directed at entity being picked up when someone tries to pick it up +/// Raised directed at entity being picked up when someone tries to pick it up. +/// IMPORTANT: Attempt event subscriptions should not be doing any state changes like throwing items, opening UIs, playing sounds etc! /// public sealed class GettingPickedUpAttemptEvent : BasePickupAttemptEvent { - public GettingPickedUpAttemptEvent(EntityUid user, EntityUid item) : base(user, item) { } + public GettingPickedUpAttemptEvent(EntityUid user, EntityUid item, bool showPopup) : base(user, item, showPopup) { } } [Virtual] public class BasePickupAttemptEvent : CancellableEntityEventArgs { + /// + /// The mob that is picking up the item. + /// public readonly EntityUid User; + /// + /// The item being picked up. + /// + public readonly EntityUid Item; - public BasePickupAttemptEvent(EntityUid user, EntityUid item) + /// + /// Whether or not to show a popup message to the player telling them why the attempt was cancelled. + /// This should be true when this event is raised during interactions, and false when it is raised + /// for disabling verbs or similar that do not do the actual pickup. + /// + public bool ShowPopup; + + public BasePickupAttemptEvent(EntityUid user, EntityUid item, bool showPopup) { User = user; Item = item; + ShowPopup = showPopup; } } diff --git a/Content.Shared/Strip/SharedStrippableSystem.cs b/Content.Shared/Strip/SharedStrippableSystem.cs index 49be180503..aca0e42945 100644 --- a/Content.Shared/Strip/SharedStrippableSystem.cs +++ b/Content.Shared/Strip/SharedStrippableSystem.cs @@ -379,7 +379,7 @@ public abstract class SharedStrippableSystem : EntitySystem return false; } - if (!_handsSystem.CanPickupToHand(target, activeItem.Value, handName, checkActionBlocker: false, target.Comp)) + if (!_handsSystem.CanPickupToHand(target, activeItem.Value, handName, checkActionBlocker: false, handsComp: target.Comp)) { _popupSystem.PopupCursor(Loc.GetString("strippable-component-cannot-put-message", ("owner", Identity.Entity(target, EntityManager)))); return false;