Fix teleport scroll teleporting into walls (#40815)

* Move Ulam spiral generator to its own class

* Make UlamSpiral.Point return a zero vector for n<=0

* Add UlamSpiral.PointsForMaxDistance

* Fix TeleportLocationsSystem can teleport user into a wall

Uses an Ulam spiral pattern to check for valid locations around the target entity.

* Handle teleportation failures more gracefully

* Fix unconventional name for delay system field

* Fix client crashes when target entity is paused/in nullspace

* Show popup when teleportation destination is obstructed

---------

Co-authored-by: opl <4833621+opl@users.noreply.github.com>

Content.Server/Tabletop/TabletopSystem.Map.cs

@@ -1,5 +1,6 @@
 using System.Numerics;
 using Content.Shared.GameTicking;
+using Content.Shared.Maths;
 using Robust.Shared.Map;
 using Robust.Shared.Map.Components;
 
@@ -37,7 +38,7 @@ namespace Content.Server.Tabletop
         /// <returns></returns>
         private Vector2 GetNextTabletopPosition()
         {
-            return UlamSpiral(++_tabletops) * TabletopSeparation;
+            return UlamSpiral.Point(++_tabletops) * TabletopSeparation;
         }
 
         /// <summary>
@@ -59,34 +60,6 @@ namespace Content.Server.Tabletop
             Dirty(mapUid, mapComp);
         }
 
-        /// <summary>
-        ///     Algorithm for mapping scalars to 2D positions in the same pattern as an Ulam Spiral.
-        /// </summary>
-        /// <param name="n">Scalar to map to a 2D position. Must be greater than or equal to 1.</param>
-        /// <returns>The mapped 2D position for the scalar.</returns>
-        private Vector2i UlamSpiral(int n)
-        {
-            var k = (int)MathF.Ceiling((MathF.Sqrt(n) - 1) / 2);
-            var t = 2 * k + 1;
-            var m = (int)MathF.Pow(t, 2);
-            t--;
-
-            if (n >= m - t)
-                return new Vector2i(k - (m - n), -k);
-
-            m -= t;
-
-            if (n >= m - t)
-                return new Vector2i(-k, -k + (m - n));
-
-            m -= t;
-
-            if (n >= m - t)
-                return new Vector2i(-k + (m - n), k);
-
-            return new Vector2i(k, k - (m - n - t));
-        }
-
         private void OnRoundRestart(RoundRestartCleanupEvent _)
         {
             if (TabletopMap == MapId.Nullspace || !_map.MapExists(TabletopMap))

Content.Server/Teleportation/TeleportLocationsSystem.cs

@@ -1,5 +1,6 @@
 using Content.Server.Chat.Systems;
 using Content.Shared.Chat;
+using Content.Shared.Popups;
 using Content.Shared.Teleportation;
 using Content.Shared.Teleportation.Components;
 using Content.Shared.Teleportation.Systems;
@@ -15,6 +16,7 @@ namespace Content.Server.Teleportation;
 public sealed partial class TeleportLocationsSystem : SharedTeleportLocationsSystem
 {
     [Dependency] private ChatSystem _chat = default!;
+    [Dependency] private SharedPopupSystem _popup = default!;
     [Dependency] private EntityWhitelistSystem _whitelist = default!;
 
     public override void Initialize()
@@ -37,7 +39,7 @@ public sealed partial class TeleportLocationsSystem : SharedTeleportLocationsSys
 
     protected override void OnTeleportToLocationRequest(Entity<TeleportLocationsComponent> ent, ref TeleportLocationDestinationMessage args)
     {
-        if (Delay.IsDelayed(ent.Owner, TeleportDelay))
+        if (IsDelayed(ent))
             return;
 
         if (!string.IsNullOrWhiteSpace(ent.Comp.Speech))
@@ -49,6 +51,13 @@ public sealed partial class TeleportLocationsSystem : SharedTeleportLocationsSys
         base.OnTeleportToLocationRequest(ent, ref args);
     }
 
+    protected override void HandleTeleportDestinationObstructed(EntityUid userUid)
+    {
+        // Client can't know if the destination is obstructed if it's not loaded.
+        var msg = Loc.GetString("teleportation-menu-destination-obstructed");
+        _popup.PopupEntity(msg, userUid, userUid, PopupType.MediumCaution);
+    }
+
     // If it's in shared this doesn't populate the points on the UI
     /// <summary>
     ///     Gets the teleport points to send to the BUI

Content.Shared/Maths/UlamSpiral.cs

@@ -0,0 +1,44 @@
+namespace Content.Shared.Maths;
+
+public static class UlamSpiral
+{
+    /// <summary>
+    ///     Algorithm for mapping scalars to 2D positions in the same pattern as an Ulam Spiral.
+    /// </summary>
+    /// <param name="n">Scalar to map to a 2D position. Returns a zero vector for values smaller than 1.</param>
+    /// <returns>The mapped 2D position for the scalar.</returns>
+    public static Vector2i Point(int n)
+    {
+        if (n <= 0)
+            return new Vector2i(0, 0);
+
+        var k = (int)MathF.Ceiling((MathF.Sqrt(n) - 1) / 2);
+        var t = 2 * k + 1;
+        var m = (int)MathF.Pow(t, 2);
+        t--;
+
+        if (n >= m - t)
+            return new Vector2i(k - (m - n), -k);
+
+        m -= t;
+
+        if (n >= m - t)
+            return new Vector2i(-k, -k + (m - n));
+
+        m -= t;
+
+        if (n >= m - t)
+            return new Vector2i(-k + (m - n), k);
+
+        return new Vector2i(k, k - (m - n - t));
+    }
+
+    /// <summary>
+    ///     Returns the largest value for which <see cref="Point"> will generate a point within a <paramref name="maxDistance"> Chebyshev distance from origin.
+    /// </summary>
+    public static int PointsForMaxDistance(int maxDistance)
+    {
+        var x = maxDistance * 2 + 1;
+        return x * x;
+    }
+}

Content.Shared/Teleportation/Systems/SharedTeleportLocationsSystem.cs

@@ -1,7 +1,10 @@
-using Content.Shared.Teleportation.Components;
+using Content.Shared.Maths;
+using Content.Shared.Teleportation.Components;
 using Content.Shared.Timing;
 using Content.Shared.UserInterface;
 using Content.Shared.Warps;
+using Robust.Shared.Map;
+using System.Numerics;
 
 namespace Content.Shared.Teleportation.Systems;
 
@@ -10,12 +13,14 @@ namespace Content.Shared.Teleportation.Systems;
 /// </summary>
 public abstract partial class SharedTeleportLocationsSystem : EntitySystem
 {
-    [Dependency] protected UseDelaySystem Delay = default!;
+    [Dependency] private UseDelaySystem _delay = default!;
 
+    [Dependency] private EntityLookupSystem _lookup = default!;
     [Dependency] private SharedUserInterfaceSystem _ui = default!;
     [Dependency] private SharedTransformSystem _xform = default!;
 
     protected const string TeleportDelay = "TeleportDelay";
+    protected const string TeleportFailedDelay = "TeleportFailedDelay";
 
     public override void Initialize()
     {
@@ -25,9 +30,14 @@ public abstract partial class SharedTeleportLocationsSystem : EntitySystem
         SubscribeLocalEvent<TeleportLocationsComponent, TeleportLocationDestinationMessage>(OnTeleportToLocationRequest);
     }
 
+    protected bool IsDelayed(EntityUid entityUid)
+    {
+        return _delay.IsDelayed(entityUid, TeleportDelay) || _delay.IsDelayed(entityUid, TeleportFailedDelay);
+    }
+
     private void OnUiOpenAttempt(Entity<TeleportLocationsComponent> ent, ref ActivatableUIOpenAttemptEvent args)
     {
-        if (!Delay.IsDelayed(ent.Owner, TeleportDelay))
+        if (!IsDelayed(ent))
             return;
 
         args.Cancel();
@@ -35,20 +45,31 @@ public abstract partial class SharedTeleportLocationsSystem : EntitySystem
 
     protected virtual void OnTeleportToLocationRequest(Entity<TeleportLocationsComponent> ent, ref TeleportLocationDestinationMessage args)
     {
-        if (!TryGetEntity(args.NetEnt, out var telePointEnt) || TerminatingOrDeleted(telePointEnt) || !HasComp<WarpPointComponent>(telePointEnt) || Delay.IsDelayed(ent.Owner, TeleportDelay))
+        if (!TryGetEntity(args.NetEnt, out var telePointEnt) || TerminatingOrDeleted(telePointEnt) || !HasComp<WarpPointComponent>(telePointEnt) || IsDelayed(ent))
             return;
 
         var comp = ent.Comp;
         var originEnt = args.Actor;
         var telePointXForm = Transform(telePointEnt.Value);
 
-        SpawnAtPosition(comp.TeleportEffect, Transform(originEnt).Coordinates);
+        // Spawn effect even if the target is unsafe - the failure is funny.
+        var originEntXForm = Transform(originEnt);
+        SpawnAtPosition(comp.TeleportEffect, originEntXForm.Coordinates);
 
-        _xform.SetMapCoordinates(originEnt, _xform.GetMapCoordinates(telePointEnt.Value, telePointXForm));
+        if (ChooseSafeLocation((telePointEnt.Value, telePointXForm), maxDistance: 3) is not { } safeTargetMapCoords)
+        {
+            // Prevent spamming effects if the target is obstructed.
+            _delay.TryResetDelay(ent.Owner, true, id: TeleportFailedDelay);
 
-        SpawnAtPosition(comp.TeleportEffect, telePointXForm.Coordinates);
+            HandleTeleportDestinationObstructed(originEnt);
 
-        Delay.TryResetDelay(ent.Owner, true, id: TeleportDelay);
+            return;
+        }
+
+        _xform.SetMapCoordinates(originEnt, safeTargetMapCoords);
+        SpawnAtPosition(comp.TeleportEffect, originEntXForm.Coordinates);
+
+        _delay.TryResetDelay(ent.Owner, true, id: TeleportDelay);
 
         if (!ent.Comp.CloseAfterTeleport)
             return;
@@ -56,4 +77,51 @@ public abstract partial class SharedTeleportLocationsSystem : EntitySystem
         // Teleport's done, now tell the BUI to close if needed.
         _ui.CloseUi(ent.Owner, TeleportLocationUiKey.Key);
     }
+
+    /// <summary>
+    /// Called when the destination was obstructed and the user wasn't teleported.
+    /// </summary>
+    protected virtual void HandleTeleportDestinationObstructed(EntityUid userUid)
+    {
+    }
+
+    /// <remarks>
+    /// The result of this overload might be different between the client and server due to PVS.
+    /// </remarks>
+    private MapCoordinates? ChooseSafeLocation(Entity<TransformComponent> targetEntity, int maxDistance)
+    {
+        // If the target point is on a grid, use that grid's rotation.
+        var gridTransform = targetEntity.Comp.GridUid is { } grid
+            ? Matrix3Helpers.CreateTransform(Vector2.Zero, _xform.GetWorldRotation(Transform(grid)))
+            : Matrix3x2.Identity;
+
+        var targetCoords = _xform.GetMapCoordinates(targetEntity);
+        // The entity might've left PVS on the client.
+        if (targetCoords.MapId == MapId.Nullspace)
+            return null;
+
+        return ChooseSafeLocation(targetCoords, maxDistance, gridTransform);
+    }
+
+    private MapCoordinates? ChooseSafeLocation(MapCoordinates targetCoords, int maxDistance, Matrix3x2 gridTransform)
+    {
+        var maxAttempts = UlamSpiral.PointsForMaxDistance(maxDistance);
+        // Transforms an offset from the target entity into the final world space position.
+        var worldToGridSpacePlusTargetPos = gridTransform * Matrix3x2.CreateTranslation(targetCoords.Position);
+
+        for (var attempt = 0; attempt <= maxAttempts; attempt++)
+        {
+            var offset = UlamSpiral.Point(attempt);
+            var offsetWorldPos = Vector2.Transform(new Vector2(offset.X, offset.Y), worldToGridSpacePlusTargetPos);
+            var offsetCoords = new MapCoordinates(offsetWorldPos, targetCoords.MapId);
+
+            if (!_lookup.AnyEntitiesIntersecting(offsetCoords, LookupFlags.Static))
+            {
+                // Selected location is not inside a wall.
+                return offsetCoords;
+            }
+        }
+
+        return null;
+    }
 }

Resources/Locale/en-US/teleportation/teleportation-menu-gui.ftl

@@ -1,5 +1,6 @@
 ## Default
 teleportation-menu-default-window-title = Teleportation Menu
+teleportation-menu-destination-obstructed = You don't feel like you went anywhere...
 
 ## Wizard
 teleportation-scroll-window-title = Teleportation Scroll

Resources/Prototypes/Magic/teleport_scroll.yml

@@ -30,3 +30,5 @@
     delays:
       TeleportDelay: !type:UseDelayInfo
         length: 300
+      TeleportFailedDelay: !type:UseDelayInfo
+        length: 10