Partial buckling refactor (#29031)

* partial buckling refactor

* git mv test

* change test namespace

* git mv test

* Update test namespace

* Add pulling test

* Network BuckleTime

* Add two more tests

* smelly

Content.Client/Buckle/BuckleSystem.cs

@@ -3,6 +3,7 @@
 using Content.Shared.Buckle.Components;
 using Content.Shared.Rotation;
 using Robust.Client.GameObjects;
+using Robust.Shared.GameStates;
 
 namespace Content.Client.Buckle;
 
@@ -14,40 +15,63 @@ public override void Initialize()
     {
         base.Initialize();
 
-        SubscribeLocalEvent<BuckleComponent, AfterAutoHandleStateEvent>(OnBuckleAfterAutoHandleState);
+        SubscribeLocalEvent<BuckleComponent, ComponentHandleState>(OnHandleState);
         SubscribeLocalEvent<BuckleComponent, AppearanceChangeEvent>(OnAppearanceChange);
+        SubscribeLocalEvent<StrapComponent, MoveEvent>(OnStrapMoveEvent);
     }
 
-    private void OnBuckleAfterAutoHandleState(EntityUid uid, BuckleComponent component, ref AfterAutoHandleStateEvent args)
+    private void OnStrapMoveEvent(EntityUid uid, StrapComponent component, ref MoveEvent args)
     {
-        ActionBlocker.UpdateCanMove(uid);
+        // I'm moving this to the client-side system, but for the sake of posterity let's keep this comment:
+        // > This is mega cursed. Please somebody save me from Mr Buckle's wild ride
 
-        if (!TryComp<SpriteComponent>(uid, out var ownerSprite))
+        // The nice thing is its still true, this is quite cursed, though maybe not omega cursed anymore.
+        // This code is garbage, it doesn't work with rotated viewports. I need to finally get around to reworking
+        // sprite rendering for entity layers & direction dependent sorting.
+
+        if (args.NewRotation == args.OldRotation)
             return;
 
-        // Adjust draw depth when the chair faces north so that the seat back is drawn over the player.
-        // Reset the draw depth when rotated in any other direction.
-        // TODO when ECSing, make this a visualizer
-        // This code was written before rotatable viewports were introduced, so hard-coding Direction.North
-        // and comparing it against LocalRotation now breaks this in other rotations. This is a FIXME, but
-        // better to get it working for most people before we look at a more permanent solution.
-        if (component is { Buckled: true, LastEntityBuckledTo: { } } &&
-            Transform(component.LastEntityBuckledTo.Value).LocalRotation.GetCardinalDir() == Direction.North &&
-            TryComp<SpriteComponent>(component.LastEntityBuckledTo, out var buckledSprite))
-        {
-            component.OriginalDrawDepth ??= ownerSprite.DrawDepth;
-            ownerSprite.DrawDepth = buckledSprite.DrawDepth - 1;
+        if (!TryComp<SpriteComponent>(uid, out var strapSprite))
             return;
-        }
 
-        // If here, we're not turning north and should restore the saved draw depth.
-        if (component.OriginalDrawDepth.HasValue)
+        var isNorth = Transform(uid).LocalRotation.GetCardinalDir() == Direction.North;
+        foreach (var buckledEntity in component.BuckledEntities)
         {
-            ownerSprite.DrawDepth = component.OriginalDrawDepth.Value;
-            component.OriginalDrawDepth = null;
+            if (!TryComp<BuckleComponent>(buckledEntity, out var buckle))
+                continue;
+
+            if (!TryComp<SpriteComponent>(buckledEntity, out var buckledSprite))
+                continue;
+
+            if (isNorth)
+            {
+                buckle.OriginalDrawDepth ??= buckledSprite.DrawDepth;
+                buckledSprite.DrawDepth = strapSprite.DrawDepth - 1;
+            }
+            else if (buckle.OriginalDrawDepth.HasValue)
+            {
+                buckledSprite.DrawDepth = buckle.OriginalDrawDepth.Value;
+                buckle.OriginalDrawDepth = null;
+            }
         }
     }
 
+    private void OnHandleState(Entity<BuckleComponent> ent, ref ComponentHandleState args)
+    {
+        if (args.Current is not BuckleState state)
+            return;
+
+        ent.Comp.DontCollide = state.DontCollide;
+        ent.Comp.BuckleTime = state.BuckleTime;
+        var strapUid = EnsureEntity<BuckleComponent>(state.BuckledTo, ent);
+
+        SetBuckledTo(ent, strapUid == null ? null : new (strapUid.Value, null));
+
+        var (uid, component) = ent;
+
+    }
+
     private void OnAppearanceChange(EntityUid uid, BuckleComponent component, ref AppearanceChangeEvent args)
     {
         if (!TryComp<RotationVisualsComponent>(uid, out var rotVisuals)

Content.IntegrationTests/Tests/Buckle/BuckleDragTest.cs

@@ -0,0 +1,56 @@
+using Content.IntegrationTests.Tests.Interaction;
+using Content.Shared.Buckle;
+using Content.Shared.Buckle.Components;
+using Content.Shared.Input;
+using Content.Shared.Movement.Pulling.Components;
+
+namespace Content.IntegrationTests.Tests.Buckle;
+
+public sealed class BuckleDragTest : InteractionTest
+{
+    // Check that dragging a buckled player unbuckles them.
+    [Test]
+    public async Task BucklePullTest()
+    {
+        var urist = await SpawnTarget("MobHuman");
+        var sUrist = ToServer(urist);
+        await SpawnTarget("Chair");
+
+        var buckle = Comp<BuckleComponent>(urist);
+        var strap = Comp<StrapComponent>(Target);
+        var puller = Comp<PullerComponent>(Player);
+        var pullable = Comp<PullableComponent>(urist);
+
+#pragma warning disable RA0002
+        buckle.Delay = TimeSpan.Zero;
+#pragma warning restore RA0002
+
+        // Initially not buckled to the chair and not pulling anything
+        Assert.That(buckle.Buckled, Is.False);
+        Assert.That(buckle.BuckledTo, Is.Null);
+        Assert.That(strap.BuckledEntities, Is.Empty);
+        Assert.That(puller.Pulling, Is.Null);
+        Assert.That(pullable.Puller, Is.Null);
+        Assert.That(pullable.BeingPulled, Is.False);
+
+        // Strap the human to the chair
+        Assert.That(Server.System<SharedBuckleSystem>().TryBuckle(sUrist, SPlayer, STarget.Value));
+        await RunTicks(5);
+        Assert.That(buckle.Buckled, Is.True);
+        Assert.That(buckle.BuckledTo, Is.EqualTo(STarget));
+        Assert.That(strap.BuckledEntities, Is.EquivalentTo(new[]{sUrist}));
+        Assert.That(puller.Pulling, Is.Null);
+        Assert.That(pullable.Puller, Is.Null);
+        Assert.That(pullable.BeingPulled, Is.False);
+
+        // Start pulling, and thus unbuckle them
+        await PressKey(ContentKeyFunctions.TryPullObject, cursorEntity:urist);
+        await RunTicks(5);
+        Assert.That(buckle.Buckled, Is.False);
+        Assert.That(buckle.BuckledTo, Is.Null);
+        Assert.That(strap.BuckledEntities, Is.Empty);
+        Assert.That(puller.Pulling, Is.EqualTo(sUrist));
+        Assert.That(pullable.Puller, Is.EqualTo(SPlayer));
+        Assert.That(pullable.BeingPulled, Is.True);
+    }
+}

Content.IntegrationTests/Tests/Buckle/BuckleTest.cs

@@ -90,7 +90,6 @@ await server.WaitAssertion(() =>
                 {
                     Assert.That(strap, Is.Not.Null);
                     Assert.That(strap.BuckledEntities, Is.Empty);
-                    Assert.That(strap.OccupiedSize, Is.Zero);
                 });
 
                 // Side effects of buckling
@@ -110,8 +109,6 @@ await server.WaitAssertion(() =>
 
                     // Side effects of buckling for the strap
                     Assert.That(strap.BuckledEntities, Does.Contain(human));
-                    Assert.That(strap.OccupiedSize, Is.EqualTo(buckle.Size));
-                    Assert.That(strap.OccupiedSize, Is.Positive);
                 });
 
 #pragma warning disable NUnit2045 // Interdependent asserts.
@@ -121,7 +118,7 @@ await server.WaitAssertion(() =>
                 // Trying to unbuckle too quickly fails
                 Assert.That(buckleSystem.TryUnbuckle(human, human, buckleComp: buckle), Is.False);
                 Assert.That(buckle.Buckled);
-                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
+                Assert.That(buckleSystem.TryUnbuckle(human, human), Is.False);
                 Assert.That(buckle.Buckled);
 #pragma warning restore NUnit2045
             });
@@ -148,7 +145,6 @@ await server.WaitAssertion(() =>
 
                     // Unbuckle, strap
                     Assert.That(strap.BuckledEntities, Is.Empty);
-                    Assert.That(strap.OccupiedSize, Is.Zero);
                 });
 
 #pragma warning disable NUnit2045 // Interdependent asserts.
@@ -159,9 +155,9 @@ await server.WaitAssertion(() =>
                 // On cooldown
                 Assert.That(buckleSystem.TryUnbuckle(human, human, buckleComp: buckle), Is.False);
                 Assert.That(buckle.Buckled);
-                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
+                Assert.That(buckleSystem.TryUnbuckle(human, human), Is.False);
                 Assert.That(buckle.Buckled);
-                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
+                Assert.That(buckleSystem.TryUnbuckle(human, human), Is.False);
                 Assert.That(buckle.Buckled);
 #pragma warning restore NUnit2045
             });
@@ -188,7 +184,6 @@ await server.WaitAssertion(() =>
 #pragma warning disable NUnit2045 // Interdependent asserts.
                 Assert.That(buckleSystem.TryBuckle(human, human, chair, buckleComp: buckle), Is.False);
                 Assert.That(buckleSystem.TryUnbuckle(human, human, buckleComp: buckle), Is.False);
-                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
 #pragma warning restore NUnit2045
 
                 // Move near the chair
@@ -201,12 +196,10 @@ await server.WaitAssertion(() =>
                 Assert.That(buckle.Buckled);
                 Assert.That(buckleSystem.TryUnbuckle(human, human, buckleComp: buckle), Is.False);
                 Assert.That(buckle.Buckled);
-                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
-                Assert.That(buckle.Buckled);
 #pragma warning restore NUnit2045
 
                 // Force unbuckle
-                Assert.That(buckleSystem.TryUnbuckle(human, human, true, buckleComp: buckle));
+                buckleSystem.Unbuckle(human, human);
                 Assert.Multiple(() =>
                 {
                     Assert.That(buckle.Buckled, Is.False);
@@ -310,7 +303,7 @@ await server.WaitAssertion(() =>
                 // Break our guy's kneecaps
                 foreach (var leg in legs)
                 {
-                    xformSystem.DetachParentToNull(leg.Id, entityManager.GetComponent<TransformComponent>(leg.Id));
+                    entityManager.DeleteEntity(leg.Id);
                 }
             });
 
@@ -327,7 +320,8 @@ await server.WaitAssertion(() =>
                     Assert.That(hand.HeldEntity, Is.Null);
                 }
 
-                buckleSystem.TryUnbuckle(human, human, true, buckleComp: buckle);
+                buckleSystem.Unbuckle(human, human);
+                Assert.That(buckle.Buckled, Is.False);
             });
 
             await pair.CleanReturnAsync();

Content.IntegrationTests/Tests/Climbing/ClimbingTest.cs

@@ -1,5 +1,6 @@
 #nullable enable
 using Content.IntegrationTests.Tests.Interaction;
+using Content.IntegrationTests.Tests.Movement;
 using Robust.Shared.Maths;
 using ClimbingComponent = Content.Shared.Climbing.Components.ClimbingComponent;
 using ClimbSystem = Content.Shared.Climbing.Systems.ClimbSystem;

Content.IntegrationTests/Tests/Construction/Interaction/CraftingTests.cs

@@ -59,11 +59,6 @@ public async Task CraftSpear()
         await AssertEntityLookup((Rod, 2), (Cable, 7), (ShardGlass, 2), (Spear, 1));
     }
 
-    // The following is wrapped in an if DEBUG. This is because of cursed state handling bugs. Tests don't (de)serialize
-    // net messages and just copy objects by reference. This means that the server will directly modify cached server
-    // states on the client's end. Crude fix at the moment is to used modified state handling while in debug mode
-    // Otherwise, this test cannot work.
-#if DEBUG
     /// <summary>
     /// Cancel crafting a complex recipe.
     /// </summary>
@@ -93,28 +88,22 @@ public async Task CancelCraft()
         await RunTicks(1);
 
         // DoAfter is in progress. Entity not spawned, stacks have been split and someingredients are in a container.
-        Assert.Multiple(async () =>
-        {
-            Assert.That(ActiveDoAfters.Count(), Is.EqualTo(1));
-            Assert.That(sys.IsEntityInContainer(shard), Is.True);
-            Assert.That(sys.IsEntityInContainer(rods), Is.False);
-            Assert.That(sys.IsEntityInContainer(wires), Is.False);
-            Assert.That(rodStack, Has.Count.EqualTo(8));
-            Assert.That(wireStack, Has.Count.EqualTo(7));
+        Assert.That(ActiveDoAfters.Count(), Is.EqualTo(1));
+        Assert.That(sys.IsEntityInContainer(shard), Is.True);
+        Assert.That(sys.IsEntityInContainer(rods), Is.False);
+        Assert.That(sys.IsEntityInContainer(wires), Is.False);
+        Assert.That(rodStack, Has.Count.EqualTo(8));
+        Assert.That(wireStack, Has.Count.EqualTo(7));
 
-            await FindEntity(Spear, shouldSucceed: false);
-        });
+        await FindEntity(Spear, shouldSucceed: false);
 
         // Cancel the DoAfter. Should drop ingredients to the floor.
         await CancelDoAfters();
-        Assert.Multiple(async () =>
-        {
-            Assert.That(sys.IsEntityInContainer(rods), Is.False);
-            Assert.That(sys.IsEntityInContainer(wires), Is.False);
-            Assert.That(sys.IsEntityInContainer(shard), Is.False);
-            await FindEntity(Spear, shouldSucceed: false);
-            await AssertEntityLookup((Rod, 10), (Cable, 10), (ShardGlass, 1));
-        });
+        Assert.That(sys.IsEntityInContainer(rods), Is.False);
+        Assert.That(sys.IsEntityInContainer(wires), Is.False);
+        Assert.That(sys.IsEntityInContainer(shard), Is.False);
+        await FindEntity(Spear, shouldSucceed: false);
+        await AssertEntityLookup((Rod, 10), (Cable, 10), (ShardGlass, 1));
 
         // Re-attempt the do-after
 #pragma warning disable CS4014 // Legacy construction code uses DoAfterAwait. See above.
@@ -123,24 +112,17 @@ public async Task CancelCraft()
         await RunTicks(1);
 
         // DoAfter is in progress. Entity not spawned, ingredients are in a container.
-        Assert.Multiple(async () =>
-        {
-            Assert.That(ActiveDoAfters.Count(), Is.EqualTo(1));
-            Assert.That(sys.IsEntityInContainer(shard), Is.True);
-            await FindEntity(Spear, shouldSucceed: false);
-        });
+        Assert.That(ActiveDoAfters.Count(), Is.EqualTo(1));
+        Assert.That(sys.IsEntityInContainer(shard), Is.True);
+        await FindEntity(Spear, shouldSucceed: false);
 
         // Finish the DoAfter
         await AwaitDoAfters();
 
         // Spear has been crafted. Rods and wires are no longer contained. Glass has been consumed.
-        Assert.Multiple(async () =>
-        {
-            await FindEntity(Spear);
-            Assert.That(sys.IsEntityInContainer(rods), Is.False);
-            Assert.That(sys.IsEntityInContainer(wires), Is.False);
-            Assert.That(SEntMan.Deleted(shard));
-        });
+        await FindEntity(Spear);
+        Assert.That(sys.IsEntityInContainer(rods), Is.False);
+        Assert.That(sys.IsEntityInContainer(wires), Is.False);
+        Assert.That(SEntMan.Deleted(shard));
     }
-#endif
 }

Content.IntegrationTests/Tests/Interaction/InteractionTest.Helpers.cs

@@ -84,8 +84,9 @@ protected async Task CraftItem(string prototype, bool shouldSucceed = true)
     /// <summary>
     /// Spawn an entity entity and set it as the target.
     /// </summary>
-    [MemberNotNull(nameof(Target))]
-    protected async Task SpawnTarget(string prototype)
+    [MemberNotNull(nameof(Target), nameof(STarget), nameof(CTarget))]
+#pragma warning disable CS8774 // Member must have a non-null value when exiting.
+    protected async Task<NetEntity> SpawnTarget(string prototype)
     {
         Target = NetEntity.Invalid;
         await Server.WaitPost(() =>
@@ -95,7 +96,9 @@ await Server.WaitPost(() =>
 
         await RunTicks(5);
         AssertPrototype(prototype);
+        return Target!.Value;
     }
+#pragma warning restore CS8774 // Member must have a non-null value when exiting.
 
     /// <summary>
     /// Spawn an entity in preparation for deconstruction
@@ -1170,14 +1173,17 @@ await Server.WaitPost(() =>
 
     #region Inputs
 
+
+
     /// <summary>
     ///     Make the client press and then release a key. This assumes the key is currently released.
+    ///     This will default to using the <see cref="Target"/> entity and <see cref="TargetCoords"/> coordinates.
     /// </summary>
     protected async Task PressKey(
         BoundKeyFunction key,
         int ticks = 1,
         NetCoordinates? coordinates = null,
-        NetEntity cursorEntity = default)
+        NetEntity? cursorEntity = null)
     {
         await SetKey(key, BoundKeyState.Down, coordinates, cursorEntity);
         await RunTicks(ticks);
@@ -1186,15 +1192,17 @@ protected async Task PressKey(
     }
 
     /// <summary>
-    ///     Make the client press or release a key
+    ///     Make the client press or release a key.
+    ///     This will default to using the <see cref="Target"/> entity and <see cref="TargetCoords"/> coordinates.
     /// </summary>
     protected async Task SetKey(
         BoundKeyFunction key,
         BoundKeyState state,
         NetCoordinates? coordinates = null,
-        NetEntity cursorEntity = default)
+        NetEntity? cursorEntity = null)
     {
         var coords = coordinates ?? TargetCoords;
+        var target = cursorEntity ?? Target ?? default;
         ScreenCoordinates screen = default;
 
         var funcId = InputManager.NetworkBindMap.KeyFunctionID(key);
@@ -1203,7 +1211,7 @@ protected async Task SetKey(
             State = state,
             Coordinates = CEntMan.GetCoordinates(coords),
             ScreenCoordinates = screen,
-            Uid = CEntMan.GetEntity(cursorEntity),
+            Uid = CEntMan.GetEntity(target),
         };
 
         await Client.WaitPost(() => InputSystem.HandleInputCommand(ClientSession, key, message));