Make clickable 1% nicer (#29706)

* Make vox roundstart

I believe all the issues are fixed.

* Click detection bandaid

* Make clickable 1% nicer

Still bad. Still doesn't handle multi-viewports well.

Content.Client/Clickable/ClickableComponent.cs

@@ -1,148 +1,17 @@
-using System.Numerics;
+namespace Content.Client.Clickable;
-using Robust.Client.GameObjects;
-using Robust.Client.Graphics;
-using Robust.Client.Utility;
-using Robust.Shared.Graphics;
-using static Robust.Client.GameObjects.SpriteComponent;
-using Direction = Robust.Shared.Maths.Direction;
 
-namespace Content.Client.Clickable
+[RegisterComponent]
+public sealed partial class ClickableComponent : Component
 {
-    [RegisterComponent]
+    [DataField] public DirBoundData? Bounds;
-    public sealed partial class ClickableComponent : Component
-    {
-        [Dependency] private readonly IClickMapManager _clickMapManager = default!;
-
-        [DataField("bounds")] public DirBoundData? Bounds;
-
-        /// <summary>
-        /// Used to check whether a click worked. Will first check if the click falls inside of some explicit bounding
-        /// boxes (see <see cref="Bounds"/>). If that fails, attempts to use automatically generated click maps.
-        /// </summary>
-        /// <param name="worldPos">The world position that was clicked.</param>
-        /// <param name="drawDepth">
-        /// The draw depth for the sprite that captured the click.
-        /// </param>
-        /// <returns>True if the click worked, false otherwise.</returns>
-        public bool CheckClick(SpriteComponent sprite, TransformComponent transform, EntityQuery<TransformComponent> xformQuery, Vector2 worldPos, IEye eye, out int drawDepth, out uint renderOrder, out float bottom)
-        {
-            if (!sprite.Visible)
-            {
-                drawDepth = default;
-                renderOrder = default;
-                bottom = default;
-                return false;
-            }
-
-            drawDepth = sprite.DrawDepth;
-            renderOrder = sprite.RenderOrder;
-            var (spritePos, spriteRot) = transform.GetWorldPositionRotation(xformQuery);
-            var spriteBB = sprite.CalculateRotatedBoundingBox(spritePos, spriteRot, eye.Rotation);
-            bottom = Matrix3Helpers.CreateRotation(eye.Rotation).TransformBox(spriteBB).Bottom;
-
-            Matrix3x2.Invert(sprite.GetLocalMatrix(), out var invSpriteMatrix);
-
-            // This should have been the rotation of the sprite relative to the screen, but this is not the case with no-rot or directional sprites.
-            var relativeRotation = (spriteRot + eye.Rotation).Reduced().FlipPositive();
-
-            Angle cardinalSnapping = sprite.SnapCardinals ? relativeRotation.GetCardinalDir().ToAngle() : Angle.Zero;
-
-            // First we get `localPos`, the clicked location in the sprite-coordinate frame.
-            var entityXform = Matrix3Helpers.CreateInverseTransform(spritePos, sprite.NoRotation ? -eye.Rotation : spriteRot - cardinalSnapping);
-            var localPos = Vector2.Transform(Vector2.Transform(worldPos, entityXform), invSpriteMatrix);
-
-            // Check explicitly defined click-able bounds
-            if (CheckDirBound(sprite, relativeRotation, localPos))
-                return true;
-
-            // Next check each individual sprite layer using automatically computed click maps.
-            foreach (var spriteLayer in sprite.AllLayers)
-            {
-                // TODO: Move this to a system and also use SpriteSystem.IsVisible instead.
-                if (!spriteLayer.Visible || spriteLayer is not Layer layer || layer.CopyToShaderParameters != null)
-                {
-                    continue;
-                }
-
-                // Check the layer's texture, if it has one
-                if (layer.Texture != null)
-                {
-                    // Convert to image coordinates
-                    var imagePos = (Vector2i) (localPos * EyeManager.PixelsPerMeter * new Vector2(1, -1) + layer.Texture.Size / 2f);
-
-                    if (_clickMapManager.IsOccluding(layer.Texture, imagePos))
-                        return true;
-                }
-
-                // Either we weren't clicking on the texture, or there wasn't one. In which case: check the RSI next
-                if (layer.ActualRsi is not { } rsi || !rsi.TryGetState(layer.State, out var rsiState))
-                    continue;
-
-                var dir = Layer.GetDirection(rsiState.RsiDirections, relativeRotation);
-
-                // convert to layer-local coordinates
-                layer.GetLayerDrawMatrix(dir, out var matrix);
-                Matrix3x2.Invert(matrix, out var inverseMatrix);
-                var layerLocal = Vector2.Transform(localPos, inverseMatrix);
-
-                // Convert to image coordinates
-                var layerImagePos = (Vector2i) (layerLocal * EyeManager.PixelsPerMeter * new Vector2(1, -1) + rsiState.Size / 2f);
-
-                // Next, to get the right click map we need the "direction" of this layer that is actually being used to draw the sprite on the screen.
-                // This **can** differ from the dir defined before, but can also just be the same.
-                if (sprite.EnableDirectionOverride)
-                    dir = sprite.DirectionOverride.Convert(rsiState.RsiDirections);
-                dir = dir.OffsetRsiDir(layer.DirOffset);
-
-                if (_clickMapManager.IsOccluding(layer.ActualRsi!, layer.State, dir, layer.AnimationFrame, layerImagePos))
-                    return true;
-            }
-
-            drawDepth = default;
-            renderOrder = default;
-            bottom = default;
-            return false;
-        }
-
-        public bool CheckDirBound(SpriteComponent sprite, Angle relativeRotation, Vector2 localPos)
-        {
-            if (Bounds == null)
-                return false;
-
-            // These explicit bounds only work for either 1 or 4 directional sprites.
-
-            // This would be the orientation of a 4-directional sprite.
-            var direction = relativeRotation.GetCardinalDir();
-
-            var modLocalPos = sprite.NoRotation
-                ? localPos
-                : direction.ToAngle().RotateVec(localPos);
-
-            // First, check the bounding box that is valid for all orientations
-            if (Bounds.All.Contains(modLocalPos))
-                return true;
-
-            // Next, get and check the appropriate bounding box for the current sprite orientation
-            var boundsForDir = (sprite.EnableDirectionOverride ? sprite.DirectionOverride : direction) switch
-            {
-                Direction.East => Bounds.East,
-                Direction.North => Bounds.North,
-                Direction.South => Bounds.South,
-                Direction.West => Bounds.West,
-                _ => throw new InvalidOperationException()
-            };
-
-            return boundsForDir.Contains(modLocalPos);
-        }
 
     [DataDefinition]
     public sealed partial class DirBoundData
     {
-            [DataField("all")] public Box2 All;
+        [DataField] public Box2 All;
-            [DataField("north")] public Box2 North;
+        [DataField] public Box2 North;
-            [DataField("south")] public Box2 South;
+        [DataField] public Box2 South;
-            [DataField("east")] public Box2 East;
+        [DataField] public Box2 East;
-            [DataField("west")] public Box2 West;
+        [DataField] public Box2 West;
-        }
     }
 }

Content.Client/Clickable/ClickableSystem.cs

@@ -0,0 +1,168 @@
+using System.Numerics;
+using Robust.Client.GameObjects;
+using Robust.Client.Graphics;
+using Robust.Client.Utility;
+using Robust.Shared.Graphics;
+
+namespace Content.Client.Clickable;
+
+/// <summary>
+/// Handles click detection for sprites.
+/// </summary>
+public sealed class ClickableSystem : EntitySystem
+{
+    [Dependency] private readonly IClickMapManager _clickMapManager = default!;
+    [Dependency] private readonly SharedTransformSystem _transforms = default!;
+    [Dependency] private readonly SpriteSystem _sprites = default!;
+
+    private EntityQuery<ClickableComponent> _clickableQuery;
+    private EntityQuery<TransformComponent> _xformQuery;
+
+    public override void Initialize()
+    {
+        base.Initialize();
+        _clickableQuery = GetEntityQuery<ClickableComponent>();
+        _xformQuery = GetEntityQuery<TransformComponent>();
+    }
+
+    /// <summary>
+    /// Used to check whether a click worked. Will first check if the click falls inside of some explicit bounding
+    /// boxes (see <see cref="Bounds"/>). If that fails, attempts to use automatically generated click maps.
+    /// </summary>
+    /// <param name="worldPos">The world position that was clicked.</param>
+    /// <param name="drawDepth">
+    /// The draw depth for the sprite that captured the click.
+    /// </param>
+    /// <returns>True if the click worked, false otherwise.</returns>
+    public bool CheckClick(Entity<ClickableComponent?, SpriteComponent, TransformComponent?> entity, Vector2 worldPos, IEye eye, out int drawDepth, out uint renderOrder, out float bottom)
+    {
+        if (!_clickableQuery.Resolve(entity.Owner, ref entity.Comp1, false))
+        {
+            drawDepth = default;
+            renderOrder = default;
+            bottom = default;
+            return false;
+        }
+
+        if (!_xformQuery.Resolve(entity.Owner, ref entity.Comp3))
+        {
+            drawDepth = default;
+            renderOrder = default;
+            bottom = default;
+            return false;
+        }
+
+        var sprite = entity.Comp2;
+        var transform = entity.Comp3;
+
+        if (!sprite.Visible)
+        {
+            drawDepth = default;
+            renderOrder = default;
+            bottom = default;
+            return false;
+        }
+
+        drawDepth = sprite.DrawDepth;
+        renderOrder = sprite.RenderOrder;
+        var (spritePos, spriteRot) = _transforms.GetWorldPositionRotation(transform);
+        var spriteBB = sprite.CalculateRotatedBoundingBox(spritePos, spriteRot, eye.Rotation);
+        bottom = Matrix3Helpers.CreateRotation(eye.Rotation).TransformBox(spriteBB).Bottom;
+
+        Matrix3x2.Invert(sprite.GetLocalMatrix(), out var invSpriteMatrix);
+
+        // This should have been the rotation of the sprite relative to the screen, but this is not the case with no-rot or directional sprites.
+        var relativeRotation = (spriteRot + eye.Rotation).Reduced().FlipPositive();
+
+        var cardinalSnapping = sprite.SnapCardinals ? relativeRotation.GetCardinalDir().ToAngle() : Angle.Zero;
+
+        // First we get `localPos`, the clicked location in the sprite-coordinate frame.
+        var entityXform = Matrix3Helpers.CreateInverseTransform(spritePos, sprite.NoRotation ? -eye.Rotation : spriteRot - cardinalSnapping);
+        var localPos = Vector2.Transform(Vector2.Transform(worldPos, entityXform), invSpriteMatrix);
+
+        // Check explicitly defined click-able bounds
+        if (CheckDirBound((entity.Owner, entity.Comp1, entity.Comp2), relativeRotation, localPos))
+            return true;
+
+        // Next check each individual sprite layer using automatically computed click maps.
+        foreach (var spriteLayer in sprite.AllLayers)
+        {
+            if (spriteLayer is not SpriteComponent.Layer layer || !_sprites.IsVisible(layer))
+            {
+                continue;
+            }
+
+            // Check the layer's texture, if it has one
+            if (layer.Texture != null)
+            {
+                // Convert to image coordinates
+                var imagePos = (Vector2i) (localPos * EyeManager.PixelsPerMeter * new Vector2(1, -1) + layer.Texture.Size / 2f);
+
+                if (_clickMapManager.IsOccluding(layer.Texture, imagePos))
+                    return true;
+            }
+
+            // Either we weren't clicking on the texture, or there wasn't one. In which case: check the RSI next
+            if (layer.ActualRsi is not { } rsi || !rsi.TryGetState(layer.State, out var rsiState))
+                continue;
+
+            var dir = SpriteComponent.Layer.GetDirection(rsiState.RsiDirections, relativeRotation);
+
+            // convert to layer-local coordinates
+            layer.GetLayerDrawMatrix(dir, out var matrix);
+            Matrix3x2.Invert(matrix, out var inverseMatrix);
+            var layerLocal = Vector2.Transform(localPos, inverseMatrix);
+
+            // Convert to image coordinates
+            var layerImagePos = (Vector2i) (layerLocal * EyeManager.PixelsPerMeter * new Vector2(1, -1) + rsiState.Size / 2f);
+
+            // Next, to get the right click map we need the "direction" of this layer that is actually being used to draw the sprite on the screen.
+            // This **can** differ from the dir defined before, but can also just be the same.
+            if (sprite.EnableDirectionOverride)
+                dir = sprite.DirectionOverride.Convert(rsiState.RsiDirections);
+            dir = dir.OffsetRsiDir(layer.DirOffset);
+
+            if (_clickMapManager.IsOccluding(layer.ActualRsi!, layer.State, dir, layer.AnimationFrame, layerImagePos))
+                return true;
+        }
+
+        drawDepth = default;
+        renderOrder = default;
+        bottom = default;
+        return false;
+    }
+
+    public bool CheckDirBound(Entity<ClickableComponent, SpriteComponent> entity, Angle relativeRotation, Vector2 localPos)
+    {
+        var clickable = entity.Comp1;
+        var sprite = entity.Comp2;
+
+        if (clickable.Bounds == null)
+            return false;
+
+        // These explicit bounds only work for either 1 or 4 directional sprites.
+
+        // This would be the orientation of a 4-directional sprite.
+        var direction = relativeRotation.GetCardinalDir();
+
+        var modLocalPos = sprite.NoRotation
+            ? localPos
+            : direction.ToAngle().RotateVec(localPos);
+
+        // First, check the bounding box that is valid for all orientations
+        if (clickable.Bounds.All.Contains(modLocalPos))
+            return true;
+
+        // Next, get and check the appropriate bounding box for the current sprite orientation
+        var boundsForDir = (sprite.EnableDirectionOverride ? sprite.DirectionOverride : direction) switch
+        {
+            Direction.East => clickable.Bounds.East,
+            Direction.North => clickable.Bounds.North,
+            Direction.South => clickable.Bounds.South,
+            Direction.West => clickable.Bounds.West,
+            _ => throw new InvalidOperationException()
+        };
+
+        return boundsForDir.Contains(modLocalPos);
+    }
+}

Content.Client/Gameplay/GameplayStateBase.cs

@@ -2,6 +2,7 @@ using System.Linq;
 using System.Numerics;
 using Content.Client.Clickable;
 using Content.Client.UserInterface;
+using Content.Client.Viewport;
 using Content.Shared.Input;
 using Robust.Client.ComponentTrees;
 using Robust.Client.GameObjects;
@@ -13,11 +14,13 @@ using Robust.Client.UserInterface;
 using Robust.Client.UserInterface.Controls;
 using Robust.Client.UserInterface.CustomControls;
 using Robust.Shared.Console;
+using Robust.Shared.Graphics;
 using Robust.Shared.Input;
 using Robust.Shared.Input.Binding;
 using Robust.Shared.Map;
 using Robust.Shared.Player;
 using Robust.Shared.Timing;
+using YamlDotNet.Serialization.TypeInspectors;
 
 namespace Content.Client.Gameplay
 {
@@ -98,7 +101,15 @@ namespace Content.Client.Gameplay
 
         public EntityUid? GetClickedEntity(MapCoordinates coordinates)
         {
-            var first = GetClickableEntities(coordinates).FirstOrDefault();
+            return GetClickedEntity(coordinates, _eyeManager.CurrentEye);
+        }
+
+        public EntityUid? GetClickedEntity(MapCoordinates coordinates, IEye? eye)
+        {
+            if (eye == null)
+                return null;
+
+            var first = GetClickableEntities(coordinates, eye).FirstOrDefault();
             return first.IsValid() ? first : null;
         }
 
@@ -110,6 +121,20 @@ namespace Content.Client.Gameplay
 
         public IEnumerable<EntityUid> GetClickableEntities(MapCoordinates coordinates)
         {
+            return GetClickableEntities(coordinates, _eyeManager.CurrentEye);
+        }
+
+        public IEnumerable<EntityUid> GetClickableEntities(MapCoordinates coordinates, IEye? eye)
+        {
+            /*
+             * TODO:
+             * 1. Stuff like MeleeWeaponSystem need an easy way to hook into viewport specific entities / entities under mouse
+             * 2. Cleanup the mess around InteractionOutlineSystem + below the keybind click detection.
+             */
+
+            if (eye == null)
+                return Array.Empty<EntityUid>();
+
             // Find all the entities intersecting our click
             var spriteTree = _entityManager.EntitySysManager.GetEntitySystem<SpriteTreeSystem>();
             var entities = spriteTree.QueryAabb(coordinates.MapId, Box2.CenteredAround(coordinates.Position, new Vector2(1, 1)));
@@ -117,15 +142,12 @@ namespace Content.Client.Gameplay
             // Check the entities against whether or not we can click them
             var foundEntities = new List<(EntityUid, int, uint, float)>(entities.Count);
             var clickQuery = _entityManager.GetEntityQuery<ClickableComponent>();
-            var xformQuery = _entityManager.GetEntityQuery<TransformComponent>();
+            var clickables = _entityManager.System<ClickableSystem>();
-
-            // TODO: Smelly
-            var eye = _eyeManager.CurrentEye;
 
             foreach (var entity in entities)
             {
                 if (clickQuery.TryGetComponent(entity.Uid, out var component) &&
-                    component.CheckClick(entity.Component, entity.Transform, xformQuery, coordinates.Position, eye,  out var drawDepthClicked, out var renderOrder, out var bottom))
+                    clickables.CheckClick((entity.Uid, component, entity.Component, entity.Transform), coordinates.Position, eye,  out var drawDepthClicked, out var renderOrder, out var bottom))
                 {
                     foundEntities.Add((entity.Uid, drawDepthClicked, renderOrder, bottom));
                 }
@@ -188,7 +210,15 @@ namespace Content.Client.Gameplay
             if (args.Viewport is IViewportControl vp && kArgs.PointerLocation.IsValid)
             {
                 var mousePosWorld = vp.PixelToMap(kArgs.PointerLocation.Position);
+
+                if (vp is ScalingViewport svp)
+                {
+                    entityToClick = GetClickedEntity(mousePosWorld, svp.Eye);
+                }
+                else
+                {
                     entityToClick = GetClickedEntity(mousePosWorld);
+                }
 
                 coordinates = _mapManager.TryFindGridAt(mousePosWorld, out _, out var grid) ?
                     grid.MapToGrid(mousePosWorld) :

Content.Client/Outline/InteractionOutlineSystem.cs

@@ -110,11 +110,15 @@ public sealed class InteractionOutlineSystem : EntitySystem
             && _inputManager.MouseScreenPosition.IsValid)
         {
             var mousePosWorld = vp.PixelToMap(_inputManager.MouseScreenPosition.Position);
-            entityToClick = screen.GetClickedEntity(mousePosWorld);
 
             if (vp is ScalingViewport svp)
             {
                 renderScale = svp.CurrentRenderScale;
+                entityToClick = screen.GetClickedEntity(mousePosWorld, svp.Eye);
+            }
+            else
+            {
+                entityToClick = screen.GetClickedEntity(mousePosWorld);
             }
         }
         else if (_uiManager.CurrentlyHovered is EntityMenuElement element)

Content.IntegrationTests/Tests/ClickableTest.cs

@@ -52,7 +52,6 @@ namespace Content.IntegrationTests.Tests
             var serverEntManager = server.ResolveDependency<IEntityManager>();
             var eyeManager = client.ResolveDependency<IEyeManager>();
             var spriteQuery = clientEntManager.GetEntityQuery<SpriteComponent>();
-            var xformQuery = clientEntManager.GetEntityQuery<TransformComponent>();
             var eye = client.ResolveDependency<IEyeManager>().CurrentEye;
 
             var testMap = await pair.CreateTestMap();
@@ -80,9 +79,8 @@ namespace Content.IntegrationTests.Tests
                 eyeManager.CurrentEye.Rotation = 0;
 
                 var pos = clientEntManager.System<SharedTransformSystem>().GetWorldPosition(clientEnt);
-                var clickable = clientEntManager.GetComponent<ClickableComponent>(clientEnt);
 
-                hit = clickable.CheckClick(sprite, xformQuery.GetComponent(clientEnt), xformQuery, new Vector2(clickPosX, clickPosY) + pos, eye, out _, out _, out _);
+                hit = clientEntManager.System<ClickableSystem>().CheckClick((clientEnt, null, sprite, null), new Vector2(clickPosX, clickPosY) + pos, eye, out _, out _, out _);
             });
 
             await server.WaitPost(() =>