Revert "Partial buckling refactor (#29031)"

This reverts commit 1741356.

Content.Client/Buckle/BuckleSystem.cs

@@ -3,7 +3,6 @@
 using Content.Shared.Buckle.Components;
 using Content.Shared.Rotation;
 using Robust.Client.GameObjects;
-using Robust.Shared.GameStates;
 
 namespace Content.Client.Buckle;
 
@@ -15,63 +14,40 @@ public override void Initialize()
     {
         base.Initialize();
 
-        SubscribeLocalEvent<BuckleComponent, ComponentHandleState>(OnHandleState);
+        SubscribeLocalEvent<BuckleComponent, AfterAutoHandleStateEvent>(OnBuckleAfterAutoHandleState);
         SubscribeLocalEvent<BuckleComponent, AppearanceChangeEvent>(OnAppearanceChange);
-        SubscribeLocalEvent<StrapComponent, MoveEvent>(OnStrapMoveEvent);
     }
 
-    private void OnStrapMoveEvent(EntityUid uid, StrapComponent component, ref MoveEvent args)
+    private void OnBuckleAfterAutoHandleState(EntityUid uid, BuckleComponent component, ref AfterAutoHandleStateEvent args)
     {
-        // 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
+        ActionBlocker.UpdateCanMove(uid);
 
-        // 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)
+        if (!TryComp<SpriteComponent>(uid, out var ownerSprite))
             return;
 
-        if (!TryComp<SpriteComponent>(uid, out var strapSprite))
+        // 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;
             return;
+        }
 
-        var isNorth = Transform(uid).LocalRotation.GetCardinalDir() == Direction.North;
-        foreach (var buckledEntity in component.BuckledEntities)
+        // If here, we're not turning north and should restore the saved draw depth.
+        if (component.OriginalDrawDepth.HasValue)
         {
-            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;
-            }
+            ownerSprite.DrawDepth = component.OriginalDrawDepth.Value;
+            component.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/BuckleTest.cs

@@ -90,6 +90,7 @@ 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
@@ -109,6 +110,8 @@ 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.
@@ -118,7 +121,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.TryUnbuckle(human, human), Is.False);
+                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
                 Assert.That(buckle.Buckled);
 #pragma warning restore NUnit2045
             });
@@ -145,6 +148,7 @@ await server.WaitAssertion(() =>
 
                     // Unbuckle, strap
                     Assert.That(strap.BuckledEntities, Is.Empty);
+                    Assert.That(strap.OccupiedSize, Is.Zero);
                 });
 
 #pragma warning disable NUnit2045 // Interdependent asserts.
@@ -155,9 +159,9 @@ await server.WaitAssertion(() =>
                 // On cooldown
                 Assert.That(buckleSystem.TryUnbuckle(human, human, buckleComp: buckle), Is.False);
                 Assert.That(buckle.Buckled);
-                Assert.That(buckleSystem.TryUnbuckle(human, human), Is.False);
+                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
                 Assert.That(buckle.Buckled);
-                Assert.That(buckleSystem.TryUnbuckle(human, human), Is.False);
+                Assert.That(buckleSystem.ToggleBuckle(human, human, chair, buckle: buckle), Is.False);
                 Assert.That(buckle.Buckled);
 #pragma warning restore NUnit2045
             });
@@ -184,6 +188,7 @@ 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
@@ -196,10 +201,12 @@ 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
-                buckleSystem.Unbuckle(human, human);
+                Assert.That(buckleSystem.TryUnbuckle(human, human, true, buckleComp: buckle));
                 Assert.Multiple(() =>
                 {
                     Assert.That(buckle.Buckled, Is.False);
@@ -303,7 +310,7 @@ await server.WaitAssertion(() =>
                 // Break our guy's kneecaps
                 foreach (var leg in legs)
                 {
-                    entityManager.DeleteEntity(leg.Id);
+                    xformSystem.DetachParentToNull(leg.Id, entityManager.GetComponent<TransformComponent>(leg.Id));
                 }
             });
 
@@ -320,8 +327,7 @@ await server.WaitAssertion(() =>
                     Assert.That(hand.HeldEntity, Is.Null);
                 }
 
-                buckleSystem.Unbuckle(human, human);
-                Assert.That(buckle.Buckled, Is.False);
+                buckleSystem.TryUnbuckle(human, human, true, buckleComp: buckle);
             });
 
             await pair.CleanReturnAsync();

Content.IntegrationTests/Tests/Climbing/ClimbingTest.cs

@@ -1,6 +1,5 @@
 #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,6 +59,11 @@ 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>
@@ -88,22 +93,28 @@ 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.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.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));
 
-        await FindEntity(Spear, shouldSucceed: false);
+            await FindEntity(Spear, shouldSucceed: false);
+        });
 
         // Cancel the DoAfter. Should drop ingredients to the floor.
         await CancelDoAfters();
-        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.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));
+        });
 
         // Re-attempt the do-after
 #pragma warning disable CS4014 // Legacy construction code uses DoAfterAwait. See above.
@@ -112,17 +123,24 @@ public async Task CancelCraft()
         await RunTicks(1);
 
         // DoAfter is in progress. Entity not spawned, ingredients are in a container.
-        Assert.That(ActiveDoAfters.Count(), Is.EqualTo(1));
-        Assert.That(sys.IsEntityInContainer(shard), Is.True);
-        await FindEntity(Spear, shouldSucceed: false);
+        Assert.Multiple(async () =>
+        {
+            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.
-        await FindEntity(Spear);
-        Assert.That(sys.IsEntityInContainer(rods), Is.False);
-        Assert.That(sys.IsEntityInContainer(wires), Is.False);
-        Assert.That(SEntMan.Deleted(shard));
+        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));
+        });
     }
+#endif
 }

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

@@ -82,9 +82,8 @@ protected async Task CraftItem(string prototype, bool shouldSucceed = true)
     /// <summary>
     /// Spawn an entity entity and set it as the target.
     /// </summary>
-    [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)
+    [MemberNotNull(nameof(Target))]
+    protected async Task SpawnTarget(string prototype)
     {
         Target = NetEntity.Invalid;
         await Server.WaitPost(() =>
@@ -94,9 +93,7 @@ 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
@@ -1012,17 +1009,14 @@ 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 = null)
+        NetEntity cursorEntity = default)
     {
         await SetKey(key, BoundKeyState.Down, coordinates, cursorEntity);
         await RunTicks(ticks);
@@ -1031,17 +1025,15 @@ protected async Task PressKey(
     }
 
     /// <summary>
-    ///     Make the client press or release a key.
-    ///     This will default to using the <see cref="Target"/> entity and <see cref="TargetCoords"/> coordinates.
+    ///     Make the client press or release a key
     /// </summary>
     protected async Task SetKey(
         BoundKeyFunction key,
         BoundKeyState state,
         NetCoordinates? coordinates = null,
-        NetEntity? cursorEntity = null)
+        NetEntity cursorEntity = default)
     {
         var coords = coordinates ?? TargetCoords;
-        var target = cursorEntity ?? Target ?? default;
         ScreenCoordinates screen = default;
 
         var funcId = InputManager.NetworkBindMap.KeyFunctionID(key);
@@ -1050,7 +1042,7 @@ protected async Task SetKey(
             State = state,
             Coordinates = CEntMan.GetCoordinates(coords),
             ScreenCoordinates = screen,
-            Uid = CEntMan.GetEntity(target),
+            Uid = CEntMan.GetEntity(cursorEntity),
         };
 
         await Client.WaitPost(() => InputSystem.HandleInputCommand(ClientSession, key, message));