Migrate pulling logic into SharedPullableComponent from PullController (#2627)

* Pulling: Migrate critical pull state from PullController to SharedPullableComponent, fixing two bugs in the process

Bug 1: PullController can be just summoned for no reason when the verb is queried
Bug 2: PullController keeps it's own independent pull state which can (and will) go out of sync (See https://github.com/space-wizards/space-station-14/issues/2619 )

* Pulling: Fix issues with previous commit (or possibly in general?) causing transferring a pull to cause alerts to go wrong

The primary problem is that there is one "pulling slot" for a puller, so SharedPullableComponent needs to stop the existing pull.

* Pulling: Remove debug logs (whoops)

Content.Server/GameObjects/Components/Pulling/PullableComponent.cs

@@ -41,26 +41,17 @@ namespace Content.Server.GameObjects.Components.Pulling
                     return;
                 }
 
-                var controller = targetPhysics.EnsureController<PullController>();
-
                 data.Visibility = VerbVisibility.Visible;
-                data.Text = controller.Puller == userPhysics
+                data.Text = component.Puller == userPhysics
                     ? Loc.GetString("Stop pulling")
                     : Loc.GetString("Pull");
             }
 
             protected override void Activate(IEntity user, PullableComponent component)
             {
-                if (!user.TryGetComponent(out IPhysicsComponent? userCollidable) ||
+                // There used to be sanity checks here for no reason.
-                    !component.Owner.TryGetComponent(out IPhysicsComponent? targetCollidable) ||
+                // Why no reason? Because they're supposed to be performed in TryStartPull.
-                    targetCollidable.Anchored)
+                if (component.Puller == user)
-                {
-                    return;
-                }
-
-                var controller = targetCollidable.EnsureController<PullController>();
-
-                if (controller.Puller == userCollidable)
                 {
                     component.TryStopPull();
                 }

Content.Shared/GameObjects/Components/Pulling/SharedPullableComponent.cs

@@ -25,46 +25,139 @@ namespace Content.Shared.GameObjects.Components.Pulling
 
         [ComponentDependency] private readonly IPhysicsComponent? _physics = default!;
 
+        /// <summary>
+        /// Only set in Puller->set! Only set in unison with _pullerPhysics!
+        /// </summary>
         private IEntity? _puller;
+        private IPhysicsComponent? _pullerPhysics;
+        public IPhysicsComponent? PullerPhysics => _pullerPhysics;
 
+        /// <summary>
+        /// The current entity pulling this component.
+        /// Setting this performs the entire setup process for pulling.
+        /// </summary>
         public virtual IEntity? Puller
         {
             get => _puller;
-            private set
+            set
             {
                 if (_puller == value)
                 {
                     return;
                 }
 
-                _puller = value;
+                // New value. Abandon being pulled by any existing object.
-                Dirty();
+                if (_puller != null)
+                {
+                    var oldPuller = _puller;
+                    var oldPullerPhysics = _pullerPhysics;
 
+                    _puller = null;
+                    _pullerPhysics = null;
+
+                    if (_physics != null)
+                    {
+                        var message = new PullStoppedMessage(oldPullerPhysics, _physics);
+
+                        oldPuller.SendMessage(null, message);
+                        Owner.SendMessage(null, message);
+
+                        oldPuller.EntityManager.EventBus.RaiseEvent(EventSource.Local, message);
+                        _physics.WakeBody();
+                        _physics.TryRemoveController<PullController>();
+                    }
+                    // else-branch warning is handled below
+                }
+
+                // Now that is settled, prepare to be pulled by a new object.
                 if (_physics == null)
                 {
+                    Logger.WarningS("c.go.c.pulling", "Well now you've done it, haven't you? SharedPullableComponent on {0} didn't have an IPhysicsComponent.", Owner);
                     return;
                 }
 
-                PullController controller;
+                if (value != null)
-
-                if (value == null)
                 {
-                    if (_physics.TryGetController(out controller))
+                    // Pulling a new object : Perform sanity checks.
+
+                    if (!_canStartPull(value))
                     {
-                        controller.StopPull();
+                        return;
                     }
 
-                    return;
+                    if (!value.TryGetComponent<IPhysicsComponent>(out var valuePhysics))
-                }
+                    {
+                        return;
+                    }
 
-                controller = _physics.EnsureController<PullController>();
+                    if (!value.TryGetComponent<SharedPullerComponent>(out var valuePuller))
-                controller.StartPull(value);
+                    {
+                        return;
+                    }
+
+                    // Ensure that the puller is not currently pulling anything.
+                    // If this isn't done, then it happens too late, and the start/stop messages go out of order,
+                    //  and next thing you know it thinks it's not pulling anything even though it is!
+
+                    var oldPulling = valuePuller.Pulling;
+                    if (oldPulling != null)
+                    {
+                        if (oldPulling.TryGetComponent(out SharedPullableComponent? pullable))
+                        {
+                            pullable.Puller = null;
+                        }
+                        else
+                        {
+                            Logger.WarningS("c.go.c.pulling", "Well now you've done it, haven't you? Someone transferred pulling to this component (on {0}) while presently pulling something that has no Pullable component (on {1})!", Owner, oldPulling);
+                            return;
+                        }
+                    }
+
+                    // Continue with pulling process.
+
+                    var pullAttempt = new PullAttemptMessage(valuePhysics, _physics);
+
+                    value.SendMessage(null, pullAttempt);
+
+                    if (pullAttempt.Cancelled)
+                    {
+                        return;
+                    }
+
+                    Owner.SendMessage(null, pullAttempt);
+
+                    if (pullAttempt.Cancelled)
+                    {
+                        return;
+                    }
+
+                    // Pull start confirm
+
+                    _puller = value;
+                    _pullerPhysics = valuePhysics;
+
+                    _physics.EnsureController<PullController>().Manager = this;
+                    var message = new PullStartedMessage(_pullerPhysics, _physics);
+
+                    _puller.SendMessage(null, message);
+                    Owner.SendMessage(null, message);
+
+                    _puller.EntityManager.EventBus.RaiseEvent(EventSource.Local, message);
+
+                    _physics.WakeBody();
+                }
+                // Code here will not run if pulling a new object was attempted and failed because of the returns from the refactor.
             }
         }
 
         public bool BeingPulled => Puller != null;
 
-        public bool CanStartPull(IEntity puller)
+        /// <summary>
+        /// Sanity-check pull. This is called from Puller setter, so it will never deny a pull that's valid by setting Puller.
+        /// It might allow an impossible pull (i.e: puller has no PhysicsComponent somehow).
+        /// Ultimately this is only used separately to stop TryStartPull from cancelling a pull for no reason.
+        /// </summary>
+        private bool _canStartPull(IEntity puller)
         {
             if (!puller.HasComponent<SharedPullerComponent>())
             {
@@ -96,7 +189,7 @@ namespace Content.Shared.GameObjects.Components.Pulling
 
         public bool TryStartPull(IEntity puller)
         {
-            if (!CanStartPull(puller))
+            if (!_canStartPull(puller))
             {
                 return false;
             }
@@ -201,30 +294,25 @@ namespace Content.Shared.GameObjects.Components.Pulling
                 return;
             }
 
+            SharedAlertsComponent? pulledStatus = Owner.GetComponentOrNull<SharedAlertsComponent>();
+
             switch (message)
             {
                 case PullStartedMessage msg:
-                    AddPullingStatuses(msg.Puller.Owner);
+                    if (pulledStatus != null)
+                    {
+                        pulledStatus.ShowAlert(AlertType.Pulled);
+                    }
                     break;
                 case PullStoppedMessage msg:
-                    RemovePullingStatuses(msg.Puller.Owner);
+                    if (pulledStatus != null)
+                    {
+                        pulledStatus.ClearAlert(AlertType.Pulled);
+                    }
                     break;
             }
         }
 
-        private void AddPullingStatuses(IEntity puller)
-        {
-            if (Owner.TryGetComponent(out SharedAlertsComponent? pulledStatus))
-            {
-                pulledStatus.ShowAlert(AlertType.Pulled);
-            }
-
-            if (puller.TryGetComponent(out SharedAlertsComponent? ownerStatus))
-            {
-                ownerStatus.ShowAlert(AlertType.Pulling, onClickAlert: OnClickAlert);
-            }
-        }
-
         private void OnClickAlert(ClickAlertEventArgs args)
         {
             EntitySystem
@@ -234,19 +322,6 @@ namespace Content.Shared.GameObjects.Components.Pulling
                 .TryStopPull();
         }
 
-        private void RemovePullingStatuses(IEntity puller)
-        {
-            if (Owner.TryGetComponent(out SharedAlertsComponent? pulledStatus))
-            {
-                pulledStatus.ClearAlert(AlertType.Pulled);
-            }
-
-            if (puller.TryGetComponent(out SharedAlertsComponent? ownerStatus))
-            {
-                ownerStatus.ClearAlert(AlertType.Pulling);
-            }
-        }
-
         public override void OnRemove()
         {
             TryStopPull();

Content.Shared/GameObjects/Components/Pulling/SharedPullerComponent.cs

@@ -1,9 +1,14 @@
 #nullable enable
+using Content.Shared.Alert;
+using Content.Shared.GameObjects.Components.Mobs;
 using Content.Shared.GameObjects.Components.Movement;
+using Content.Shared.GameObjects.EntitySystems;
 using Content.Shared.Physics.Pull;
 using Robust.Shared.GameObjects;
+using Robust.Shared.GameObjects.Systems;
 using Robust.Shared.Interfaces.GameObjects;
 using Component = Robust.Shared.GameObjects.Component;
+using Robust.Shared.Log;
 
 namespace Content.Shared.GameObjects.Components.Pulling
 {
@@ -58,15 +63,34 @@ namespace Content.Shared.GameObjects.Components.Pulling
                 return;
             }
 
+            SharedAlertsComponent? ownerStatus = Owner.GetComponentOrNull<SharedAlertsComponent>();
+
             switch (message)
             {
                 case PullStartedMessage msg:
                     Pulling = msg.Pulled.Owner;
+                    if (ownerStatus != null)
+                    {
+                        ownerStatus.ShowAlert(AlertType.Pulling, onClickAlert: OnClickAlert);
+                    }
                     break;
                 case PullStoppedMessage _:
                     Pulling = null;
+                    if (ownerStatus != null)
+                    {
+                        ownerStatus.ClearAlert(AlertType.Pulling);
+                    }
                     break;
             }
         }
+
+        private void OnClickAlert(ClickAlertEventArgs args)
+        {
+            EntitySystem
+                .Get<SharedPullingSystem>()
+                .GetPulled(args.Player)?
+                .GetComponentOrNull<SharedPullableComponent>()?
+                .TryStopPull();
+        }
     }
 }

Content.Shared/Physics/Pull/PullController.cs

@@ -11,10 +11,15 @@ using Robust.Shared.Map;
 using Robust.Shared.Maths;
 using Robust.Shared.Physics;
 using Robust.Shared.Utility;
+using Content.Shared.GameObjects.Components.Pulling;
 using static Content.Shared.GameObjects.EntitySystems.SharedInteractionSystem;
 
 namespace Content.Shared.Physics.Pull
 {
+    /// <summary>
+    /// This is applied upon a Pullable object when that object is being pulled.
+    /// It lives only to serve that Pullable object.
+    /// </summary>
     public class PullController : VirtualController
     {
         [Dependency] private readonly IEntityManager _entityManager = default!;
@@ -24,12 +29,16 @@ namespace Content.Shared.Physics.Pull
 
         private const float StopMoveThreshold = 0.25f;
 
-        private IPhysicsComponent? _puller;
+        /// <summary>
+        /// The managing SharedPullableComponent of this PullController.
+        /// MUST BE SET! If you go attaching PullControllers yourself, YOU ARE DOING IT WRONG.
+        /// If you get a crash based on such, then, well, see previous note.
+        /// This is set by the SharedPullableComponent attaching the PullController.
+        /// </summary>
+        public SharedPullableComponent Manager = default!;
 
         private EntityCoordinates? _movingTo;
 
-        public IPhysicsComponent? Puller => _puller;
-
         public EntityCoordinates? MovingTo
         {
             get => _movingTo;
@@ -47,6 +56,7 @@ namespace Content.Shared.Physics.Pull
 
         private bool PullerMovingTowardsPulled()
         {
+            var _puller = Manager.PullerPhysics;
             if (_puller == null)
             {
                 return false;
@@ -74,93 +84,9 @@ namespace Content.Shared.Physics.Pull
             return rayResults.Any();
         }
 
-        public bool StartPull(IEntity entity)
-        {
-            DebugTools.AssertNotNull(entity);
-
-            if (!entity.TryGetComponent(out IPhysicsComponent? physics))
-            {
-                return false;
-            }
-
-            return StartPull(physics);
-        }
-
-        public bool StartPull(IPhysicsComponent puller)
-        {
-            DebugTools.AssertNotNull(puller);
-
-            if (_puller == puller)
-            {
-                return false;
-            }
-
-            if (ControlledComponent == null)
-            {
-                return false;
-            }
-
-            var pullAttempt = new PullAttemptMessage(puller, ControlledComponent);
-
-            puller.Owner.SendMessage(null, pullAttempt);
-
-            if (pullAttempt.Cancelled)
-            {
-                return false;
-            }
-
-            ControlledComponent.Owner.SendMessage(null, pullAttempt);
-
-            if (pullAttempt.Cancelled)
-            {
-                return false;
-            }
-
-            _puller = puller;
-
-            var message = new PullStartedMessage(this, _puller, ControlledComponent);
-
-            _puller.Owner.SendMessage(null, message);
-            ControlledComponent.Owner.SendMessage(null, message);
-
-            _puller.Owner.EntityManager.EventBus.RaiseEvent(EventSource.Local, message);
-
-            ControlledComponent.WakeBody();
-
-            return true;
-        }
-
-        public bool StopPull()
-        {
-            var oldPuller = _puller;
-
-            if (oldPuller == null)
-            {
-                return false;
-            }
-
-            _puller = null;
-
-            if (ControlledComponent == null)
-            {
-                return false;
-            }
-
-            var message = new PullStoppedMessage(oldPuller, ControlledComponent);
-
-            oldPuller.Owner.SendMessage(null, message);
-            ControlledComponent.Owner.SendMessage(null, message);
-
-            oldPuller.Owner.EntityManager.EventBus.RaiseEvent(EventSource.Local, message);
-
-            ControlledComponent.WakeBody();
-            ControlledComponent.TryRemoveController<PullController>();
-
-            return true;
-        }
-
         public bool TryMoveTo(EntityCoordinates from, EntityCoordinates to)
         {
+            var _puller = Manager.PullerPhysics;
             if (_puller == null || ControlledComponent == null)
             {
                 return false;
@@ -199,6 +125,7 @@ namespace Content.Shared.Physics.Pull
 
         public override void UpdateBeforeProcessing()
         {
+            var _puller = Manager.PullerPhysics;
             if (_puller == null || ControlledComponent == null)
             {
                 return;
@@ -206,7 +133,7 @@ namespace Content.Shared.Physics.Pull
 
             if (!_puller.Owner.IsInSameOrNoContainer(ControlledComponent.Owner))
             {
-                StopPull();
+                Manager.Puller = null;
                 return;
             }
 
@@ -214,7 +141,7 @@ namespace Content.Shared.Physics.Pull
 
             if (distance.Length > DistBeforeStopPull)
             {
-                StopPull();
+                Manager.Puller = null;
             }
             else if (MovingTo.HasValue)
             {

Content.Shared/Physics/Pull/PullStartedMessage.cs

@@ -4,7 +4,7 @@ namespace Content.Shared.Physics.Pull
 {
     public class PullStartedMessage : PullMessage
     {
-        public PullStartedMessage(PullController controller, IPhysicsComponent puller, IPhysicsComponent pulled) :
+        public PullStartedMessage(IPhysicsComponent puller, IPhysicsComponent pulled) :
             base(puller, pulled)
         {
         }