Improve InteractionSystem range & BUI checks (#27999)

* Improve InteractionSystem range & BUI checks

* Ghost fixes

* AAA

* Fix test

* fix nullable

* revert to broadcast event

* Fixes for eengine PR

* Ah buckle code

* )

Content.Client/Instruments/UI/InstrumentBoundUserInterface.cs

@@ -37,14 +37,8 @@ namespace Content.Client.Instruments.UI
 
         protected override void ReceiveMessage(BoundUserInterfaceMessage message)
         {
-            switch (message)
+            if (message is InstrumentBandResponseBuiMessage bandRx)
-            {
+                _bandMenu?.Populate(bandRx.Nearby, EntMan);
-                case InstrumentBandResponseBuiMessage bandRx:
-                    _bandMenu?.Populate(bandRx.Nearby, EntMan);
-                    break;
-                default:
-                    break;
-            }
         }
 
         protected override void Open()

Content.Client/Interactable/InteractionSystem.cs

@@ -4,24 +4,6 @@ using Robust.Shared.Containers;
 
 namespace Content.Client.Interactable
 {
-    public sealed class InteractionSystem : SharedInteractionSystem
+    // TODO Remove Shared prefix
-    {
+    public sealed class InteractionSystem : SharedInteractionSystem;
-        [Dependency] private readonly SharedContainerSystem _container = default!;
-
-        public override bool CanAccessViaStorage(EntityUid user, EntityUid target)
-        {
-            if (!EntityManager.EntityExists(target))
-                return false;
-
-            if (!_container.TryGetContainingContainer(target, out var container))
-                return false;
-
-            if (!HasComp<StorageComponent>(container.Owner))
-                return false;
-
-            // we don't check if the user can access the storage entity itself. This should be handed by the UI system.
-            // Need to return if UI is open or not
-            return true;
-        }
-    }
 }

Content.IntegrationTests/Tests/EntityTest.cs

@@ -350,8 +350,12 @@ namespace Content.IntegrationTests.Tests
                 "DebrisFeaturePlacerController", // Above.
                 "LoadedChunk", // Worldgen chunk loading malding.
                 "BiomeSelection", // Whaddya know, requires config.
+                "ActivatableUI", // Requires enum key
             };
 
+            // TODO TESTS
+            // auto ignore any components that have a "required" data field.
+
             await using var pair = await PoolManager.GetServerClient();
             var server = pair.Server;
             var entityManager = server.ResolveDependency<IEntityManager>();

Content.Server/Configurable/ConfigurationSystem.cs

@@ -24,6 +24,7 @@ public sealed class ConfigurationSystem : EntitySystem
 
     private void OnInteractUsing(EntityUid uid, ConfigurationComponent component, InteractUsingEvent args)
     {
+        // TODO use activatable ui system
         if (args.Handled)
             return;
 

Content.Server/Interaction/InteractionSystem.cs

@@ -7,31 +7,6 @@ using Robust.Shared.Player;
 
 namespace Content.Server.Interaction
 {
-    /// <summary>
+    // TODO Remove Shared prefix
-    /// Governs interactions during clicking on entities
+    public sealed class InteractionSystem : SharedInteractionSystem;
-    /// </summary>
-    [UsedImplicitly]
-    public sealed partial class InteractionSystem : SharedInteractionSystem
-    {
-        [Dependency] private readonly SharedContainerSystem _container = default!;
-        [Dependency] private readonly UserInterfaceSystem _uiSystem = default!;
-
-        public override bool CanAccessViaStorage(EntityUid user, EntityUid target)
-        {
-            if (Deleted(target))
-                return false;
-
-            if (!_container.TryGetContainingContainer(target, out var container))
-                return false;
-
-            if (!TryComp(container.Owner, out StorageComponent? storage))
-                return false;
-
-            if (storage.Container?.ID != container.ID)
-                return false;
-
-            // we don't check if the user can access the storage entity itself. This should be handed by the UI system.
-            return _uiSystem.IsUiOpen(container.Owner, StorageComponent.StorageUiKey.Key, user);
-        }
-    }
 }

Content.Server/StationEvents/Events/ImmovableRodRule.cs

@@ -28,7 +28,9 @@ public sealed class ImmovableRodRule : StationEventSystem<ImmovableRodRuleCompon
 
         if (proto.TryGetComponent<ImmovableRodComponent>(out var rod) && proto.TryGetComponent<TimedDespawnComponent>(out var despawn))
         {
-            TryFindRandomTile(out _, out _, out _, out var targetCoords);
+            if (!TryFindRandomTile(out _, out _, out _, out var targetCoords))
+                return;
+
             var speed = RobustRandom.NextFloat(rod.MinSpeed, rod.MaxSpeed);
             var angle = RobustRandom.NextAngle();
             var direction = angle.ToVec();

Content.Server/Weapons/Melee/MeleeWeaponSystem.cs

@@ -187,15 +187,10 @@ public sealed class MeleeWeaponSystem : SharedMeleeWeaponSystem
         if (session is { } pSession)
         {
             (targetCoordinates, targetLocalAngle) = _lag.GetCoordinatesAngle(target, pSession);
-        }
+            return Interaction.InRangeUnobstructed(user, target, targetCoordinates, targetLocalAngle, range);
-        else
-        {
-            var xform = Transform(target);
-            targetCoordinates = xform.Coordinates;
-            targetLocalAngle = xform.LocalRotation;
         }
 
-        return Interaction.InRangeUnobstructed(user, target, targetCoordinates, targetLocalAngle, range);
+        return Interaction.InRangeUnobstructed(user, target, range);
     }
 
     protected override void DoDamageEffect(List<EntityUid> targets, EntityUid? user, TransformComponent targetXform)

Content.Shared/Actions/SharedActionsSystem.cs

@@ -502,13 +502,7 @@ public abstract class SharedActionsSystem : EntitySystem
             return distance <= action.Range;
         }
 
-        if (_interactionSystem.InRangeUnobstructed(user, target, range: action.Range)
+        return _interactionSystem.InRangeAndAccessible(user, target, range: action.Range);
-            && _containerSystem.IsInSameOrParentContainer(user, target))
-        {
-            return true;
-        }
-
-        return _interactionSystem.CanAccessViaStorage(user, target);
     }
 
     public bool ValidateWorldTarget(EntityUid user, EntityCoordinates coords, Entity<WorldTargetActionComponent> action)

Content.Shared/Buckle/SharedBuckleSystem.Buckle.cs

@@ -64,7 +64,7 @@ public abstract partial class SharedBuckleSystem
             return;
 
         var strapPosition = Transform(strapUid).Coordinates;
-        if (ev.NewPosition.InRange(EntityManager, _transform, strapPosition, strapComp.MaxBuckleDistance))
+        if (ev.NewPosition.EntityId.IsValid() && ev.NewPosition.InRange(EntityManager, _transform, strapPosition, strapComp.MaxBuckleDistance))
             return;
 
         TryUnbuckle(uid, uid, true, component);

Content.Shared/Interaction/SharedInteractionSystem.cs

@@ -1,11 +1,10 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using Content.Shared.ActionBlocker;
-using Content.Shared.Administration;
 using Content.Shared.Administration.Logs;
-using Content.Shared.Administration.Managers;
 using Content.Shared.CombatMode;
 using Content.Shared.Database;
+using Content.Shared.Ghost;
 using Content.Shared.Hands;
 using Content.Shared.Hands.Components;
 using Content.Shared.Input;
@@ -15,12 +14,13 @@ using Content.Shared.Inventory;
 using Content.Shared.Inventory.Events;
 using Content.Shared.Item;
 using Content.Shared.Movement.Components;
-using Content.Shared.Movement.Pulling.Components;
 using Content.Shared.Movement.Pulling.Systems;
 using Content.Shared.Physics;
 using Content.Shared.Popups;
+using Content.Shared.Storage;
 using Content.Shared.Tag;
 using Content.Shared.Timing;
+using Content.Shared.UserInterface;
 using Content.Shared.Verbs;
 using Content.Shared.Wall;
 using JetBrains.Annotations;
@@ -36,6 +36,7 @@ using Robust.Shared.Player;
 using Robust.Shared.Random;
 using Robust.Shared.Serialization;
 using Robust.Shared.Timing;
+using Robust.Shared.Utility;
 
 #pragma warning disable 618
 
@@ -50,12 +51,11 @@ namespace Content.Shared.Interaction
         [Dependency] private readonly IGameTiming _gameTiming = default!;
         [Dependency] private readonly INetManager _net = default!;
         [Dependency] private readonly IMapManager _mapManager = default!;
-        [Dependency] private readonly ISharedAdminManager _adminManager = default!;
         [Dependency] private readonly ISharedAdminLogManager _adminLogger = default!;
         [Dependency] private readonly ActionBlockerSystem _actionBlockerSystem = default!;
         [Dependency] private readonly RotateToFaceSystem _rotateToFaceSystem = default!;
         [Dependency] private readonly SharedContainerSystem _containerSystem = default!;
-        [Dependency] private readonly SharedPhysicsSystem _sharedBroadphaseSystem = default!;
+        [Dependency] private readonly SharedPhysicsSystem _broadphase = default!;
         [Dependency] private readonly SharedTransformSystem _transform = default!;
         [Dependency] private readonly SharedVerbSystem _verbSystem = default!;
         [Dependency] private readonly SharedPopupSystem _popupSystem = default!;
@@ -64,6 +64,18 @@ namespace Content.Shared.Interaction
         [Dependency] private readonly InventorySystem _inventory = default!;
         [Dependency] private readonly IRobustRandom _random = default!;
         [Dependency] private readonly TagSystem _tagSystem = default!;
+        [Dependency] private readonly SharedUserInterfaceSystem _ui = default!;
+
+        private EntityQuery<IgnoreUIRangeComponent> _ignoreUiRangeQuery;
+        private EntityQuery<FixturesComponent> _fixtureQuery;
+        private EntityQuery<ItemComponent> _itemQuery;
+        private EntityQuery<PhysicsComponent> _physicsQuery;
+        private EntityQuery<HandsComponent> _handsQuery;
+        private EntityQuery<InteractionRelayComponent> _relayQuery;
+        private EntityQuery<CombatModeComponent> _combatQuery;
+        private EntityQuery<WallMountComponent> _wallMountQuery;
+        private EntityQuery<UseDelayComponent> _delayQuery;
+        private EntityQuery<ActivatableUIComponent> _uiQuery;
 
         private const CollisionGroup InRangeUnobstructedMask = CollisionGroup.Impassable | CollisionGroup.InteractImpassable;
 
@@ -76,6 +88,17 @@ namespace Content.Shared.Interaction
 
         public override void Initialize()
         {
+            _ignoreUiRangeQuery = GetEntityQuery<IgnoreUIRangeComponent>();
+            _fixtureQuery = GetEntityQuery<FixturesComponent>();
+            _itemQuery = GetEntityQuery<ItemComponent>();
+            _physicsQuery = GetEntityQuery<PhysicsComponent>();
+            _handsQuery = GetEntityQuery<HandsComponent>();
+            _relayQuery = GetEntityQuery<InteractionRelayComponent>();
+            _combatQuery = GetEntityQuery<CombatModeComponent>();
+            _wallMountQuery = GetEntityQuery<WallMountComponent>();
+            _delayQuery = GetEntityQuery<UseDelayComponent>();
+            _uiQuery = GetEntityQuery<ActivatableUIComponent>();
+
             SubscribeLocalEvent<BoundUserInterfaceCheckRangeEvent>(HandleUserInterfaceRangeCheck);
             SubscribeLocalEvent<BoundUserInterfaceMessageAttempt>(OnBoundInterfaceInteractAttempt);
 
@@ -111,29 +134,57 @@ namespace Content.Shared.Interaction
         /// </summary>
         private void OnBoundInterfaceInteractAttempt(BoundUserInterfaceMessageAttempt ev)
         {
-            var user = ev.Actor;
+            _uiQuery.TryComp(ev.Target, out var uiComp);
+            if (!_actionBlockerSystem.CanInteract(ev.Actor, ev.Target))
+            {
+                // We permit ghosts to open uis unless explicitly blocked
+                if (ev.Message is not OpenBoundInterfaceMessage || !HasComp<GhostComponent>(ev.Actor) || uiComp?.BlockSpectators == true)
+                {
+                    ev.Cancel();
+                    return;
+                }
+            }
 
-            if (!_actionBlockerSystem.CanInteract(user, ev.Target))
+            var range = _ui.GetUiRange(ev.Target, ev.UiKey);
+
+            // As long as range>0, the UI frame updates should have auto-closed the UI if it is out of range.
+            DebugTools.Assert(range <= 0 || UiRangeCheck(ev.Actor, ev.Target, range));
+
+            if (range <= 0 && !IsAccessible(ev.Actor, ev.Target))
             {
                 ev.Cancel();
                 return;
             }
 
-            // Check if the bound entity is accessible. Note that we allow admins to ignore this restriction, so that
+            if (uiComp == null)
-            // they can fiddle with UI's that people can't normally interact with (e.g., placing things directly into
+                return;
-            // other people's backpacks).
+
-            if (!_containerSystem.IsInSameOrParentContainer(user, ev.Target)
+            if (uiComp.SingleUser && uiComp.CurrentSingleUser != ev.Actor)
-                && !CanAccessViaStorage(user, ev.Target)
-                && !_adminManager.HasAdminFlag(user, AdminFlags.Admin))
             {
                 ev.Cancel();
                 return;
             }
 
-            if (!InRangeUnobstructed(user, ev.Target))
+            if (!uiComp.RequireHands)
-            {
+                return;
+
+            if (!_handsQuery.TryComp(ev.Actor, out var hands) || hands.Hands.Count == 0)
                 ev.Cancel();
-            }
+        }
+
+        private bool UiRangeCheck(Entity<TransformComponent?> user, Entity<TransformComponent?> target, float range)
+        {
+            if (!Resolve(target, ref target.Comp))
+                return false;
+
+            if (user.Owner == target.Owner)
+                return true;
+
+            // Fast check: if the user is the parent of the entity (e.g., holding it), we always assume that it is in range
+            if (target.Comp.ParentUid == user.Owner)
+                return true;
+
+            return InRangeAndAccessible(user, target, range) || _ignoreUiRangeQuery.HasComp(user);
         }
 
         /// <summary>
@@ -190,10 +241,7 @@ namespace Content.Shared.Interaction
             if (!InRangeUnobstructed(userEntity.Value, uid, popup: true))
                 return false;
 
-            if (!TryComp(uid, out PullableComponent? pull))
+            _pullSystem.TogglePull(uid, userEntity.Value);
-                return false;
-
-            _pullSystem.TogglePull(uid, userEntity.Value, pull);
             return false;
         }
 
@@ -269,7 +317,7 @@ namespace Content.Shared.Interaction
         public bool CombatModeCanHandInteract(EntityUid user, EntityUid? target)
         {
             // Always allow attack in these cases
-            if (target == null || !TryComp<HandsComponent>(user, out var hands) || hands.ActiveHand?.HeldEntity is not null)
+            if (target == null || !_handsQuery.TryComp(user, out var hands) || hands.ActiveHand?.HeldEntity is not null)
                 return false;
 
             // Only eat input if:
@@ -277,7 +325,7 @@ namespace Content.Shared.Interaction
             // - Target doesn't cancel should-interact event
             // This is intended to allow items to be picked up in combat mode,
             // but to also allow items to force attacks anyway (like mobs which are items, e.g. mice)
-            if (!HasComp<ItemComponent>(target))
+            if (!_itemQuery.HasComp(target))
                 return false;
 
             var combatEv = new CombatModeShouldHandInteractEvent();
@@ -307,7 +355,7 @@ namespace Content.Shared.Interaction
             bool checkAccess = true,
             bool checkCanUse = true)
         {
-            if (TryComp<InteractionRelayComponent>(user, out var relay) && relay.RelayEntity is not null)
+            if (_relayQuery.TryComp(user, out var relay) && relay.RelayEntity is not null)
             {
                 // TODO this needs to be handled better. This probably bypasses many complex can-interact checks in weird roundabout ways.
                 if (_actionBlockerSystem.CanInteract(user, target))
@@ -321,7 +369,7 @@ namespace Content.Shared.Interaction
             if (target != null && Deleted(target.Value))
                 return;
 
-            if (!altInteract && TryComp<CombatModeComponent>(user, out var combatMode) && combatMode.IsInCombatMode)
+            if (!altInteract && _combatQuery.TryComp(user, out var combatMode) && combatMode.IsInCombatMode)
             {
                 if (!CombatModeCanHandInteract(user, target))
                     return;
@@ -343,10 +391,7 @@ namespace Content.Shared.Interaction
 
             // Check if interacted entity is in the same container, the direct child, or direct parent of the user.
             // Also checks if the item is accessible via some storage UI (e.g., open backpack)
-            if (checkAccess
+            if (checkAccess && target != null && !IsAccessible(user, target.Value))
-                && target != null
-                && !_containerSystem.IsInSameOrParentContainer(user, target.Value)
-                && !CanAccessViaStorage(user, target.Value))
                 return;
 
             var inRangeUnobstructed = target == null
@@ -354,7 +399,7 @@ namespace Content.Shared.Interaction
                 : !checkAccess || InRangeUnobstructed(user, target.Value); // permits interactions with wall mounted entities
 
             // Does the user have hands?
-            if (!TryComp<HandsComponent>(user, out var hands) || hands.ActiveHand == null)
+            if (!_handsQuery.TryComp(user, out var hands) || hands.ActiveHand == null)
             {
                 var ev = new InteractNoHandEvent(user, target, coordinates);
                 RaiseLocalEvent(user, ev);
@@ -494,7 +539,7 @@ namespace Content.Shared.Interaction
 
             predicate ??= _ => false;
             var ray = new CollisionRay(origin.Position, dir.Normalized(), collisionMask);
-            var rayResults = _sharedBroadphaseSystem.IntersectRayWithPredicate(origin.MapId, ray, dir.Length(), predicate.Invoke, false).ToList();
+            var rayResults = _broadphase.IntersectRayWithPredicate(origin.MapId, ray, dir.Length(), predicate.Invoke, false).ToList();
 
             if (rayResults.Count == 0)
                 return dir.Length();
@@ -557,23 +602,29 @@ namespace Content.Shared.Interaction
             }
 
             var ray = new CollisionRay(origin.Position, dir.Normalized(), (int) collisionMask);
-            var rayResults = _sharedBroadphaseSystem.IntersectRayWithPredicate(origin.MapId, ray, length, predicate.Invoke, false).ToList();
+            var rayResults = _broadphase.IntersectRayWithPredicate(origin.MapId, ray, length, predicate.Invoke, false).ToList();
 
             return rayResults.Count == 0;
         }
 
         public bool InRangeUnobstructed(
-            EntityUid origin,
+            Entity<TransformComponent?> origin,
-            EntityUid other,
+            Entity<TransformComponent?> other,
             float range = InteractionRange,
             CollisionGroup collisionMask = InRangeUnobstructedMask,
             Ignored? predicate = null,
             bool popup = false)
         {
-            if (!TryComp(other, out TransformComponent? otherXform))
+            if (!Resolve(other, ref other.Comp))
                 return false;
 
-            return InRangeUnobstructed(origin, other, otherXform.Coordinates, otherXform.LocalRotation, range, collisionMask, predicate,
+            return InRangeUnobstructed(origin,
+                other,
+                other.Comp.Coordinates,
+                other.Comp.LocalRotation,
+                range,
+                collisionMask,
+                predicate,
                 popup);
         }
 
@@ -605,8 +656,8 @@ namespace Content.Shared.Interaction
         ///     True if the two points are within a given range without being obstructed.
         /// </returns>
         public bool InRangeUnobstructed(
-            EntityUid origin,
+            Entity<TransformComponent?> origin,
-            EntityUid other,
+            Entity<TransformComponent?> other,
             EntityCoordinates otherCoordinates,
             Angle otherAngle,
             float range = InteractionRange,
@@ -614,10 +665,10 @@ namespace Content.Shared.Interaction
             Ignored? predicate = null,
             bool popup = false)
         {
-            Ignored combinedPredicate = e => e == origin || (predicate?.Invoke(e) ?? false);
+            Ignored combinedPredicate = e => e == origin.Owner || (predicate?.Invoke(e) ?? false);
             var inRange = true;
             MapCoordinates originPos = default;
-            var targetPos = otherCoordinates.ToMap(EntityManager, _transform);
+            var targetPos = _transform.ToMapCoordinates(otherCoordinates);
             Angle targetRot = default;
 
             // So essentially:
@@ -627,23 +678,30 @@ namespace Content.Shared.Interaction
             // Alternatively we could check centre distances first though
             // that means we wouldn't be able to easily check overlap interactions.
             if (range > 0f &&
-                TryComp<FixturesComponent>(origin, out var fixtureA) &&
+                _fixtureQuery.TryComp(origin, out var fixtureA) &&
                 // These fixture counts are stuff that has the component but no fixtures for <reasons> (e.g. buttons).
                 // At least until they get removed.
                 fixtureA.FixtureCount > 0 &&
-                TryComp<FixturesComponent>(other, out var fixtureB) &&
+                _fixtureQuery.TryComp(other, out var fixtureB) &&
                 fixtureB.FixtureCount > 0 &&
-                TryComp(origin, out TransformComponent? xformA))
+                Resolve(origin, ref origin.Comp))
             {
-                var (worldPosA, worldRotA) = xformA.GetWorldPositionRotation();
+                var (worldPosA, worldRotA) = origin.Comp.GetWorldPositionRotation();
                 var xfA = new Transform(worldPosA, worldRotA);
                 var parentRotB = _transform.GetWorldRotation(otherCoordinates.EntityId);
                 var xfB = new Transform(targetPos.Position, parentRotB + otherAngle);
 
                 // Different map or the likes.
-                if (!_sharedBroadphaseSystem.TryGetNearest(origin, other,
+                if (!_broadphase.TryGetNearest(
-                        out _, out _, out var distance,
+                        origin,
-                        xfA, xfB, fixtureA, fixtureB))
+                        other,
+                        out _,
+                        out _,
+                        out var distance,
+                        xfA,
+                        xfB,
+                        fixtureA,
+                        fixtureB))
                 {
                     inRange = false;
                 }
@@ -665,15 +723,15 @@ namespace Content.Shared.Interaction
                 else
                 {
                     // We'll still do the raycast from the centres but we'll bump the range as we know they're in range.
-                    originPos = _transform.GetMapCoordinates(origin, xform: xformA);
+                    originPos = _transform.GetMapCoordinates(origin, xform: origin.Comp);
                     range = (originPos.Position - targetPos.Position).Length();
                 }
             }
             // No fixtures, e.g. wallmounts.
             else
             {
-                originPos = _transform.GetMapCoordinates(origin);
+                originPos = _transform.GetMapCoordinates(origin, origin);
-                var otherParent = Transform(other).ParentUid;
+                var otherParent = (other.Comp ?? Transform(other)).ParentUid;
                 targetRot = otherParent.IsValid() ? Transform(otherParent).LocalRotation + otherAngle : otherAngle;
             }
 
@@ -724,13 +782,13 @@ namespace Content.Shared.Interaction
         {
             HashSet<EntityUid> ignored = new();
 
-            if (HasComp<ItemComponent>(target) && TryComp(target, out PhysicsComponent? physics) && physics.CanCollide)
+            if (_itemQuery.HasComp(target) && _physicsQuery.TryComp(target, out var physics) && physics.CanCollide)
             {
                 // If the target is an item, we ignore any colliding entities. Currently done so that if items get stuck
                 // inside of walls, users can still pick them up.
-                ignored.UnionWith(_sharedBroadphaseSystem.GetEntitiesIntersectingBody(target, (int) collisionMask, false, physics));
+                ignored.UnionWith(_broadphase.GetEntitiesIntersectingBody(target, (int) collisionMask, false, physics));
             }
-            else if (TryComp(target, out WallMountComponent? wallMount))
+            else if (_wallMountQuery.TryComp(target, out var wallMount))
             {
                 // wall-mount exemptions may be restricted to a specific angle range.da
 
@@ -748,13 +806,7 @@ namespace Content.Shared.Interaction
                     ignored.UnionWith(grid.GetAnchoredEntities(targetCoords));
             }
 
-            Ignored combinedPredicate = e =>
+            Ignored combinedPredicate = e => e == target || (predicate?.Invoke(e) ?? false) || ignored.Contains(e);
-            {
-                return e == target
-                       || (predicate?.Invoke(e) ?? false)
-                       || ignored.Contains(e);
-            };
-
             return combinedPredicate;
         }
 
@@ -951,10 +1003,8 @@ namespace Content.Shared.Interaction
             bool checkUseDelay = true,
             bool checkAccess = true)
         {
-            UseDelayComponent? delayComponent = null;
+            _delayQuery.TryComp(used, out var delayComponent);
-            if (checkUseDelay
+            if (checkUseDelay && delayComponent != null && _useDelay.IsDelayed((used, delayComponent)))
-                && TryComp(used, out delayComponent)
-                && _useDelay.IsDelayed((used, delayComponent)))
                 return false;
 
             if (checkCanInteract && !_actionBlockerSystem.CanInteract(user, used))
@@ -965,11 +1015,11 @@ namespace Content.Shared.Interaction
 
             // Check if interacted entity is in the same container, the direct child, or direct parent of the user.
             // This is bypassed IF the interaction happened through an item slot (e.g., backpack UI)
-            if (checkAccess && !_containerSystem.IsInSameOrParentContainer(user, used) && !CanAccessViaStorage(user, used))
+            if (checkAccess && !IsAccessible(user, used))
                 return false;
 
             // Does the user have hands?
-            if (!HasComp<HandsComponent>(user))
+            if (!_handsQuery.HasComp(user))
                 return false;
 
             var activateMsg = new ActivateInWorldEvent(user, used);
@@ -979,7 +1029,9 @@ namespace Content.Shared.Interaction
 
             DoContactInteraction(user, used, activateMsg);
             // Still need to call this even without checkUseDelay in case this gets relayed from Activate.
-            _useDelay.TryResetDelay(used, component: delayComponent);
+            if (delayComponent != null)
+                _useDelay.TryResetDelay(used, component: delayComponent);
+
             if (!activateMsg.WasLogged)
                 _adminLogger.Add(LogType.InteractActivate, LogImpact.Low, $"{ToPrettyString(user):user} activated {ToPrettyString(used):used}");
             return true;
@@ -1000,11 +1052,8 @@ namespace Content.Shared.Interaction
             bool checkCanInteract = true,
             bool checkUseDelay = true)
         {
-            UseDelayComponent? delayComponent = null;
+            _delayQuery.TryComp(used, out var delayComponent);
-
+            if (checkUseDelay && delayComponent != null && _useDelay.IsDelayed((used, delayComponent)))
-            if (checkUseDelay
-                && TryComp(used, out delayComponent)
-                && _useDelay.IsDelayed((used, delayComponent)))
                 return true; // if the item is on cooldown, we consider this handled.
 
             if (checkCanInteract && !_actionBlockerSystem.CanInteract(user, used))
@@ -1066,11 +1115,60 @@ namespace Content.Shared.Interaction
         }
         #endregion
 
+        /// <summary>
+        /// Check if a user can access a target (stored in the same containers) and is in range without obstructions.
+        /// </summary>
+        public bool InRangeAndAccessible(
+            Entity<TransformComponent?> user,
+            Entity<TransformComponent?> target,
+            float range = InteractionRange,
+            CollisionGroup collisionMask = InRangeUnobstructedMask,
+            Ignored? predicate = null)
+        {
+            if (user == target)
+                return true;
+
+            if (!Resolve(user, ref user.Comp))
+                return false;
+
+            if (!Resolve(target, ref target.Comp))
+                return false;
+
+            return IsAccessible(user, target) && InRangeUnobstructed(user, target, range, collisionMask, predicate);
+        }
+
+        /// <summary>
+        /// Check if a user can access a target or if they are stored in different containers.
+        /// </summary>
+        public bool IsAccessible(Entity<TransformComponent?> user, Entity<TransformComponent?> target)
+        {
+            if (_containerSystem.IsInSameOrParentContainer(user, target, out _, out var container))
+                return true;
+
+            return container != null && CanAccessViaStorage(user, target, container);
+        }
+
         /// <summary>
         ///     If a target is in range, but not in the same container as the user, it may be inside of a backpack. This
         ///     checks if the user can access the item in these situations.
         /// </summary>
-        public abstract bool CanAccessViaStorage(EntityUid user, EntityUid target);
+        public bool CanAccessViaStorage(EntityUid user, EntityUid target)
+        {
+            if (!_containerSystem.TryGetContainingContainer(target, out var container))
+                return false;
+
+            return CanAccessViaStorage(user, target, container);
+        }
+
+        /// <inheritdoc cref="CanAccessViaStorage(Robust.Shared.GameObjects.EntityUid,Robust.Shared.GameObjects.EntityUid)"/>
+        public bool CanAccessViaStorage(EntityUid user, EntityUid target, BaseContainer container)
+        {
+            if (StorageComponent.ContainerId != container.ID)
+                return false;
+
+            // we don't check if the user can access the storage entity itself. This should be handed by the UI system.
+            return _ui.IsUiOpen(target, StorageComponent.StorageUiKey.Key, user);
+        }
 
         /// <summary>
         ///     Checks whether an entity currently equipped by another player is accessible to some user. This shouldn't
@@ -1151,19 +1249,15 @@ namespace Content.Shared.Interaction
             RaiseLocalEvent(uidB.Value, new ContactInteractionEvent(uidA));
         }
 
+
         private void HandleUserInterfaceRangeCheck(ref BoundUserInterfaceCheckRangeEvent ev)
         {
             if (ev.Result == BoundUserInterfaceRangeResult.Fail)
                 return;
 
-            if (InRangeUnobstructed(ev.Actor, ev.Target, ev.Data.InteractionRange))
+            ev.Result = UiRangeCheck(ev.Actor!, ev.Target, ev.Data.InteractionRange)
-            {
+                    ? BoundUserInterfaceRangeResult.Pass
-                ev.Result = BoundUserInterfaceRangeResult.Pass;
+                    : BoundUserInterfaceRangeResult.Fail;
-            }
-            else
-            {
-                ev.Result = BoundUserInterfaceRangeResult.Fail;
-            }
         }
     }
 

Content.Shared/Inventory/InventorySystem.Equip.cs

@@ -210,11 +210,7 @@ public abstract partial class InventorySystem
             return false;
 
         // Can the actor reach the item?
-        if (_interactionSystem.InRangeUnobstructed(actor, itemUid) && _containerSystem.IsInSameOrParentContainer(actor, itemUid))
+        if (_interactionSystem.InRangeAndAccessible(actor, itemUid))
-            return true;
-
-        // Is the item in an open storage UI, i.e., is the user quick-equipping from an open backpack?
-        if (_interactionSystem.CanAccessViaStorage(actor, itemUid))
             return true;
 
         // Is the actor currently stripping the target? Here we could check if the actor has the stripping UI open, but

Content.Shared/MagicMirror/SharedMagicMirrorSystem.cs

@@ -4,6 +4,7 @@ using Content.Shared.Humanoid.Markings;
 using Content.Shared.Interaction;
 using Content.Shared.UserInterface;
 using Robust.Shared.Serialization;
+using Robust.Shared.Utility;
 
 namespace Content.Shared.MagicMirror;
 
@@ -21,10 +22,13 @@ public abstract class SharedMagicMirrorSystem : EntitySystem
 
     private void OnMirrorRangeCheck(EntityUid uid, MagicMirrorComponent component, ref BoundUserInterfaceCheckRangeEvent args)
     {
-        if (!Exists(component.Target) || !_interaction.InRangeUnobstructed(uid, component.Target.Value))
+        if (args.Result == BoundUserInterfaceRangeResult.Fail)
-        {
+            return;
+
+        DebugTools.Assert(component.Target != null && Exists(component.Target));
+
+        if (!_interaction.InRangeUnobstructed(uid, component.Target.Value))
             args.Result = BoundUserInterfaceRangeResult.Fail;
-        }
     }
 
     private void OnBeforeUIOpen(Entity<MagicMirrorComponent> ent, ref BeforeActivatableUIOpenEvent args)

Content.Shared/Movement/Pulling/Systems/PullingSystem.cs

@@ -362,14 +362,17 @@ public sealed class PullingSystem : EntitySystem
         return !startPull.Cancelled && !getPulled.Cancelled;
     }
 
-    public bool TogglePull(EntityUid pullableUid, EntityUid pullerUid, PullableComponent pullable)
+    public bool TogglePull(Entity<PullableComponent?> pullable, EntityUid pullerUid)
     {
-        if (pullable.Puller == pullerUid)
+        if (!Resolve(pullable, ref pullable.Comp, false))
+            return false;
+
+        if (pullable.Comp.Puller == pullerUid)
         {
-            return TryStopPull(pullableUid, pullable);
+            return TryStopPull(pullable, pullable.Comp);
         }
 
-        return TryStartPull(pullerUid, pullableUid, pullableComp: pullable);
+        return TryStartPull(pullerUid, pullable, pullableComp: pullable);
     }
 
     public bool TogglePull(EntityUid pullerUid, PullerComponent puller)
@@ -377,7 +380,7 @@ public sealed class PullingSystem : EntitySystem
         if (!TryComp<PullableComponent>(puller.Pulling, out var pullable))
             return false;
 
-        return TogglePull(puller.Pulling.Value, pullerUid, pullable);
+        return TogglePull((puller.Pulling.Value, pullable), pullerUid);
     }
 
     public bool TryStartPull(EntityUid pullerUid, EntityUid pullableUid,

Content.Shared/Storage/EntitySystems/SharedStorageSystem.cs

@@ -1078,7 +1078,7 @@ public abstract class SharedStorageSystem : EntitySystem
     /// <returns>true if inserted, false otherwise</returns>
     public bool PlayerInsertEntityInWorld(Entity<StorageComponent?> uid, EntityUid player, EntityUid toInsert)
     {
-        if (!Resolve(uid, ref uid.Comp) || !_interactionSystem.InRangeUnobstructed(player, uid))
+        if (!Resolve(uid, ref uid.Comp) || !_interactionSystem.InRangeUnobstructed(player, uid.Owner))
             return false;
 
         if (!Insert(uid, toInsert, out _, user: player, uid.Comp))

Content.Shared/UserInterface/ActivatableUIComponent.cs

@@ -57,7 +57,7 @@ namespace Content.Shared.UserInterface
         /// </summary>
         [ViewVariables(VVAccess.ReadWrite)]
         [DataField]
-        public bool AllowSpectator = true;
+        public bool BlockSpectators;
 
         /// <summary>
         ///     Whether the item must be in the user's currently selected/active hand.

Content.Shared/UserInterface/ActivatableUISystem.cs

@@ -8,7 +8,7 @@ using Content.Shared.Interaction;
 using Content.Shared.Interaction.Events;
 using Content.Shared.Popups;
 using Content.Shared.Verbs;
-using Robust.Shared.Containers;
+using Robust.Shared.Utility;
 
 namespace Content.Shared.UserInterface;
 
@@ -19,15 +19,12 @@ public sealed partial class ActivatableUISystem : EntitySystem
     [Dependency] private readonly SharedUserInterfaceSystem _uiSystem = default!;
     [Dependency] private readonly SharedPopupSystem _popupSystem = default!;
     [Dependency] private readonly SharedHandsSystem _hands = default!;
-    [Dependency] private readonly SharedContainerSystem _container = default!;
-    [Dependency] private readonly SharedInteractionSystem _interaction = default!;
-
-    private readonly List<EntityUid> _toClose = new();
 
     public override void Initialize()
     {
         base.Initialize();
 
+        SubscribeLocalEvent<ActivatableUIComponent, ComponentStartup>(OnStartup);
         SubscribeLocalEvent<ActivatableUIComponent, UseInHandEvent>(OnUseInHand);
         SubscribeLocalEvent<ActivatableUIComponent, ActivateInWorldEvent>(OnActivate);
         SubscribeLocalEvent<ActivatableUIComponent, InteractUsingEvent>(OnInteractUsing);
@@ -37,28 +34,24 @@ public sealed partial class ActivatableUISystem : EntitySystem
         SubscribeLocalEvent<ActivatableUIComponent, GetVerbsEvent<ActivationVerb>>(GetActivationVerb);
         SubscribeLocalEvent<ActivatableUIComponent, GetVerbsEvent<Verb>>(GetVerb);
 
-        // TODO ActivatableUI
-        // Add UI-user component, and listen for user container changes.
-        // I.e., should lose a computer UI if a player gets shut into a locker.
-        SubscribeLocalEvent<ActivatableUIComponent, EntGotInsertedIntoContainerMessage>(OnGotInserted);
-        SubscribeLocalEvent<ActivatableUIComponent, EntGotRemovedFromContainerMessage>(OnGotRemoved);
-
-        SubscribeLocalEvent<BoundUserInterfaceMessageAttempt>(OnBoundInterfaceInteractAttempt);
         SubscribeLocalEvent<UserInterfaceComponent, OpenUiActionEvent>(OnActionPerform);
 
         InitializePower();
     }
 
-    private void OnBoundInterfaceInteractAttempt(BoundUserInterfaceMessageAttempt ev)
+    private void OnStartup(Entity<ActivatableUIComponent> ent, ref ComponentStartup args)
     {
-        if (!TryComp(ev.Target, out ActivatableUIComponent? comp))
+        if (ent.Comp.Key == null)
+        {
+            Log.Error($"Missing UI Key for entity: {ToPrettyString(ent)}");
             return;
+        }
 
-        if (!comp.RequireHands)
+        // TODO BUI
-            return;
+        // set interaction range to zero to avoid constant range checks.
-
+        //
-        if (!TryComp(ev.Actor, out HandsComponent? hands) || hands.Hands.Count == 0)
+        // if (ent.Comp.InHandsOnly && _uiSystem.TryGetInterfaceData(ent.Owner, ent.Comp.Key, out var data))
-            ev.Cancel();
+        //     data.InteractionRange = 0;
     }
 
     private void OnActionPerform(EntityUid uid, UserInterfaceComponent component, OpenUiActionEvent args)
@@ -77,9 +70,10 @@ public sealed partial class ActivatableUISystem : EntitySystem
 
         args.Verbs.Add(new ActivationVerb
         {
-            // TODO VERBS add "open UI" icon
             Act = () => InteractUI(args.User, uid, component),
-            Text = Loc.GetString(component.VerbText)
+            Text = Loc.GetString(component.VerbText),
+            // TODO VERB ICON find a better icon
+            Icon = new SpriteSpecifier.Texture(new ResPath("/Textures/Interface/VerbIcons/settings.svg.192dpi.png")),
         });
     }
 
@@ -90,9 +84,10 @@ public sealed partial class ActivatableUISystem : EntitySystem
 
         args.Verbs.Add(new Verb
         {
-            // TODO VERBS add "open UI" icon
             Act = () => InteractUI(args.User, uid, component),
-            Text = Loc.GetString(component.VerbText)
+            Text = Loc.GetString(component.VerbText),
+            // TODO VERB ICON find a better icon
+            Icon = new SpriteSpecifier.Texture(new ResPath("/Textures/Interface/VerbIcons/settings.svg.192dpi.png")),
         });
     }
 
@@ -119,7 +114,7 @@ public sealed partial class ActivatableUISystem : EntitySystem
             }
         }
 
-        return args.CanInteract || component.AllowSpectator && HasComp<GhostComponent>(args.User);
+        return args.CanInteract || HasComp<GhostComponent>(args.User) && !component.BlockSpectators;
     }
 
     private void OnUseInHand(EntityUid uid, ActivatableUIComponent component, UseInHandEvent args)
@@ -191,7 +186,7 @@ public sealed partial class ActivatableUISystem : EntitySystem
             return true;
         }
 
-        if (!_blockerSystem.CanInteract(user, uiEntity) && (!aui.AllowSpectator || !HasComp<GhostComponent>(user)))
+        if (!_blockerSystem.CanInteract(user, uiEntity) && (!HasComp<GhostComponent>(user) || aui.BlockSpectators))
             return false;
 
         if (aui.RequireHands)
@@ -286,47 +281,4 @@ public sealed partial class ActivatableUISystem : EntitySystem
         if (ent.Comp.RequireHands && ent.Comp.InHandsOnly)
             CloseAll(ent, ent);
     }
-
-    private void OnGotInserted(Entity<ActivatableUIComponent> ent, ref EntGotInsertedIntoContainerMessage args)
-    {
-        CheckAccess((ent, ent));
-    }
-
-    private void OnGotRemoved(Entity<ActivatableUIComponent> ent, ref EntGotRemovedFromContainerMessage args)
-    {
-        CheckAccess((ent, ent));
-    }
-
-    public void CheckAccess(Entity<ActivatableUIComponent?> ent)
-    {
-        if (!Resolve(ent, ref ent.Comp))
-            return;
-
-        if (ent.Comp.Key == null)
-        {
-            Log.Error($"Encountered null key in activatable ui on entity {ToPrettyString(ent)}");
-            return;
-        }
-
-        foreach (var user in _uiSystem.GetActors(ent.Owner, ent.Comp.Key))
-        {
-            if (!_container.IsInSameOrParentContainer(user, ent)
-                && !_interaction.CanAccessViaStorage(user, ent))
-            {
-                _toClose.Add(user);
-                continue;
-
-            }
-
-            if (!_interaction.InRangeUnobstructed(user, ent))
-                _toClose.Add(user);
-        }
-
-        foreach (var user in _toClose)
-        {
-            _uiSystem.CloseUi(ent.Owner, ent.Comp.Key, user);
-        }
-
-        _toClose.Clear();
-    }
 }

Content.Shared/Verbs/SharedVerbSystem.cs

@@ -72,19 +72,7 @@ namespace Content.Shared.Verbs
             extraCategories = new();
 
             // accessibility checks
-            bool canAccess = false;
+            var canAccess = force || _interactionSystem.InRangeAndAccessible(user, target);
-            if (force || target == user)
-                canAccess = true;
-            else if (_interactionSystem.InRangeUnobstructed(user, target))
-            {
-                // Note that being in a container does not count as an obstruction for InRangeUnobstructed
-                // Therefore, we need extra checks to ensure the item is actually accessible:
-                if (ContainerSystem.IsInSameOrParentContainer(user, target))
-                    canAccess = true;
-                else
-                    // the item might be in a backpack that the user has open
-                    canAccess = _interactionSystem.CanAccessViaStorage(user, target);
-            }
 
             // A large number of verbs need to check action blockers. Instead of repeatedly having each system individually
             // call ActionBlocker checks, just cache it for the verb request.

Resources/Prototypes/Entities/Objects/Fun/Instruments/base_instruments.yml

@@ -29,7 +29,7 @@
   components:
   - type: Instrument
   - type: ActivatableUI
-    allowSpectator: false # otherwise they can play client-side music
+    blockSpectators: true # otherwise they can play client-side music
     inHandsOnly: false
     singleUser: true
     requireHands: true