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

@@ -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<LubedComponent, ComponentInit>(OnInit);
-        SubscribeLocalEvent<LubedComponent, ContainerGettingInsertedAttemptEvent>(OnHandPickUp);
+        SubscribeLocalEvent<LubedComponent, BeforeGettingEquippedHandEvent>(OnHandPickUp);
         SubscribeLocalEvent<LubedComponent, RefreshNameModifiersEvent>(OnRefreshNameModifiers);
     }
 
@@ -30,21 +32,38 @@ public sealed class LubedSystem : EntitySystem
         _nameMod.RefreshNameModifiers(uid);
     }
 
-    private void OnHandPickUp(EntityUid uid, LubedComponent component, ContainerGettingInsertedAttemptEvent args)
+    /// <remarks>
+    /// 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 <see cref="SharedHandsSystem"/>
+    /// which would (probably) make this effect look less funny. You will
+    /// probably want to either tweak <see cref="BeforeGettingEquippedHandEvent"/>
+    /// 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
+    /// <see cref="MultiHandedItemSystem.OnBeforeEquipped"/>.
+    /// </remarks>
+    private void OnHandPickUp(Entity<LubedComponent> ent, ref BeforeGettingEquippedHandEvent args)
     {
-        if (component.SlipsLeft <= 0)
+        if (args.Cancelled)
+            return;
+
+        if (ent.Comp.SlipsLeft <= 0)
         {
-            RemComp<LubedComponent>(uid);
-            _nameMod.RefreshNameModifiers(uid);
+            RemComp<LubedComponent>(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<LubedComponent> entity, ref RefreshNameModifiersEvent args)

Content.Shared/ActionBlocker/ActionBlockerSystem.cs

@@ -167,15 +167,21 @@ namespace Content.Shared.ActionBlocker
             return !ev.Cancelled;
         }
 
-        public bool CanPickup(EntityUid user, EntityUid item)
+        /// <summary>
+        /// Whether a user can pickup the given item.
+        /// </summary>
+        /// <param name="user">The mob trying to pick up the item.</param>
+        /// <param name="item">The item being picked up.</param>
+        /// <param name="showPopup">Whether or not to show a popup to the player telling them why the attempt failed.</param>
+        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;

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);

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);

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)
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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);
     }
 
     /// <summary>
-    ///     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.
     /// </summary>
-    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
         }
     }
 
+    /// <summary>
+    /// Small helper function meant as a last step before <see cref="DoPickup"/>
+    /// is called. Used to run a cancelable before pickup event that can have
+    /// side effects, unlike the side effect free <see cref="GettingPickedUpAttemptEvent"/>.
+    /// </summary>
+    private bool BeforeDoPickup(Entity<HandsComponent?> 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;
+    }
+
     /// <summary>
     ///     Puts an entity into the player's hand, assumes that the insertion is allowed. In general, you should not be calling this function directly.
     /// </summary>

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
         }
     }
 
+    /// <summary>
+    /// 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
+    /// <see cref="SharedHandsSystem.CanPickupToHand"/> and similar don't see.
+    /// </summary>
+    /// <param name="User">The user picking up the item.</param>
+    /// <param name="Cancelled">
+    /// If true, the item will not be equipped into the user's hand.
+    /// </param>
+    [ByRefEvent]
+    public record struct BeforeGettingEquippedHandEvent(EntityUid User, bool Cancelled = false);
+
+    /// <summary>
+    /// 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
+    /// <see cref="SharedHandsSystem.CanPickupToHand"/> and similar don't see.
+    /// </summary>
+    /// <param name="Item">The item being picked up.</param>
+    /// <param name="Cancelled">
+    /// If true, the item will not be equipped into the user's hand.
+    /// </param>
+    [ByRefEvent]
+    public record struct BeforeEquippingHandEvent(EntityUid Item, bool Cancelled = false);
+
     /// <summary>
     ///     Raised when putting an entity into a hand slot
     /// </summary>

Content.Shared/Item/MultiHandedItemSystem.cs

@@ -37,12 +37,17 @@ public sealed class MultiHandedItemSystem : EntitySystem
 
     private void OnAttemptPickup(Entity<MultiHandedItemComponent> 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<MultiHandedItemComponent> ent, ref VirtualItemDeletedEvent args)

Content.Shared/Item/PickupAttemptEvent.cs

@@ -1,30 +1,47 @@
 namespace Content.Shared.Item;
 
 /// <summary>
-///     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!
 /// </summary>
 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) { }
 }
 
 /// <summary>
-///     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!
 /// </summary>
 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
 {
+    /// <summary>
+    /// The mob that is picking up the item.
+    /// </summary>
     public readonly EntityUid User;
+    /// <summary>
+    /// The item being picked up.
+    /// </summary>
+
     public readonly EntityUid Item;
 
-    public BasePickupAttemptEvent(EntityUid user, EntityUid item)
+    /// <summary>
+    /// 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.
+    /// </summary>
+    public bool ShowPopup;
+
+    public BasePickupAttemptEvent(EntityUid user, EntityUid item, bool showPopup)
     {
         User = user;
         Item = item;
+        ShowPopup = showPopup;
     }
 }

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;