Make PVS overrides respect vismasks

RELEASE-NOTES.md

@@ -35,15 +35,19 @@ END TEMPLATE-->
 
 ### Breaking changes
 
-*None yet*
+* The interaction between PVS overrides and visibility masks / layers have changed:
+  * Any forced entities (i.e., `PvsOverrideSystem.AddForceSend()`) now ignore visibility masks.
+  * Any global & session overrides (`PvsOverrideSystem.AddGlobalOverride()` & `PvsOverrideSystem.AddSessionOverride()`) now respect visibility masks.
+  * Entities added via the `ExpandPvsEvent` respect visibility masks.
+  * The mask used for any global/session overrides can be modified via `ExpandPvsEvent.Mask`.
 
 ### New features
 
 *None yet*
 
 ### Bugfixes
 
-*None yet*
+* Audio entities attached to invisible / masked entities should no longer be able to temporarily make those entities visible to all players.
 
 ### Other
 

Robust.Server/Audio/AudioSystem.cs

@@ -104,6 +104,11 @@ public override (EntityUid Entity, AudioComponent Component)? PlayEntity(string?
         var entity = SetupAudio(filename, audioParams);
         // Move it after setting it up
         XformSystem.SetCoordinates(entity, new EntityCoordinates(uid, Vector2.Zero));
+
+        // TODO AUDIO
+        // Add methods that allow for custom audio range.
+        // Some methods try to reduce the audio range, resulting in a custom filter which then unnecessarily has to
+        // use PVS overrides. PlayEntity with a reduced range shouldn't need PVS overrides at all.
         AddAudioFilter(entity, entity.Comp, playerFilter);
 
         return (entity, entity.Comp);

Robust.Server/GameStates/PvsData.cs

@@ -180,6 +180,9 @@ internal struct PvsMetadata
     public NetEntity NetEntity;
 
     public GameTick LastModifiedTick;
+
+    // TODO PVS maybe store as int?
+    // Theres extra space anyways, and the mask checks always need to convert to an int first, so it'd probably be faster too.
     public ushort VisMask;
     public EntityLifeStage LifeStage;
 #if DEBUG

Robust.Server/GameStates/PvsOverrideSystem.cs

@@ -132,7 +132,7 @@ private void Clear(EntityUid uid)
 
     /// <summary>
     /// Forces the entity, all of its parents, and all of its children to ignore normal PVS range limitations,
-    /// causing them to always be sent to all clients.
+    /// causing them to be sent to all clients. This will still respect visibility masks, it only overrides the range.
     /// </summary>
     public void AddGlobalOverride(EntityUid uid)
     {
@@ -154,8 +154,9 @@ public void RemoveGlobalOverride(EntityUid uid)
     /// This causes an entity and all of its parents to always be sent to all players.
     /// </summary>
     /// <remarks>
-    /// This differs from <see cref="AddGlobalOverride"/> as it does not send children, and will ignore a players usual
-    /// PVS budget. You generally shouldn't use this unless an entity absolutely always needs to be sent to all clients.
+    /// This differs from <see cref="AddGlobalOverride"/> as it does not send children, will ignore a players usual
+    /// PVS budget, and ignores visibility masks. You generally shouldn't use this unless an entity absolutely always
+    /// needs to be sent to all clients.
     /// </remarks>
     public void AddForceSend(EntityUid uid)
     {
@@ -171,11 +172,12 @@ public void RemoveForceSend(EntityUid uid)
     }
 
     /// <summary>
-    /// This causes an entity and all of its parents to always be sent to a player..
+    /// This causes an entity and all of its parents to always be sent to a player.
     /// </summary>
     /// <remarks>
-    /// This differs from <see cref="AddSessionOverride"/> as it does not send children, and will ignore a players usual
-    /// PVS budget. You generally shouldn't use this unless an entity absolutely always needs to be sent to a client.
+    /// This differs from <see cref="AddSessionOverride"/> as it does not send children, will ignore a players usual
+    /// PVS budget, and ignores visibility masks. You generally shouldn't use this unless an entity absolutely always
+    /// needs to be sent to a client.
     /// </remarks>
     public void AddForceSend(EntityUid uid, ICommonSession session)
     {
@@ -201,7 +203,7 @@ public void RemoveForceSend(EntityUid uid, ICommonSession session)
 
     /// <summary>
     /// Forces the entity, all of its parents, and all of its children to ignore normal PVS range limitations for a
-    /// specific session.
+    /// specific session. This will still respect visibility masks, it only overrides the range.
     /// </summary>
     public void AddSessionOverride(EntityUid uid, ICommonSession session)
     {
@@ -226,13 +228,15 @@ public void RemoveSessionOverride(EntityUid uid, ICommonSession session)
 
     /// <summary>
     /// Forces the entity, all of its parents, and all of its children to ignore normal PVS range limitations,
-    /// causing them to always be sent to all clients.
+    /// causing them to always be sent to the specified clients. This will still respect visibility masks, it only
+    /// overrides the range.
     /// </summary>
     public void AddSessionOverrides(EntityUid uid, Filter filter)
     {
+        _hasOverride.Add(uid);
         foreach (var session in filter.Recipients)
         {
-            AddSessionOverride(uid, session);
+            SessionOverrides.GetOrNew(session).Add(uid);
         }
     }
 

Robust.Server/GameStates/PvsSystem.Overrides.cs

@@ -19,14 +19,15 @@ internal sealed partial class PvsSystem
 
     private void AddAllOverrides(PvsSession session)
     {
-        var mask = session.VisMask;
         var fromTick = session.FromTick;
-        RaiseExpandEvent(session, fromTick);
+        var mask = RaiseExpandEvent(session, fromTick);
 
         foreach (ref var ent in CollectionsMarshal.AsSpan(_cachedGlobalOverride))
         {
             ref var meta = ref _metadataMemory.GetRef(ent.Ptr.Index);
             meta.Validate(ent.Meta);
+
+            // PVS overrides still respect visibility masks
             if ((mask & meta.VisMask) == meta.VisMask)
                 AddEntity(session, ref ent, ref meta, fromTick);
         }
@@ -36,7 +37,7 @@ private void AddAllOverrides(PvsSession session)
 
         foreach (var uid in sessionOverrides)
         {
-            RecursivelyAddOverride(session, uid, fromTick, addChildren: true);
+            RecursivelyAddOverride(session, uid, fromTick, addChildren: true, mask);
         }
     }
 
@@ -45,36 +46,37 @@ private void AddAllOverrides(PvsSession session)
     /// </summary>
     private void AddForcedEntities(PvsSession session)
     {
+        // Forced overrides do not respect visibility masks, so we set all bits.
+        var mask = -1;
+
         // Ignore PVS budgets
         session.Budget = new() {NewLimit = int.MaxValue, EnterLimit = int.MaxValue};
 
-        var mask = session.VisMask;
         var fromTick = session.FromTick;
         foreach (ref var ent in CollectionsMarshal.AsSpan(_cachedForceOverride))
         {
             ref var meta = ref _metadataMemory.GetRef(ent.Ptr.Index);
             meta.Validate(ent.Meta);
-            if ((mask & meta.VisMask) == meta.VisMask)
-                AddEntity(session, ref ent, ref meta, fromTick);
+            AddEntity(session, ref ent, ref meta, fromTick);
         }
 
         foreach (var uid in session.Viewers)
         {
-            RecursivelyAddOverride(session, uid, fromTick, addChildren: false);
+            RecursivelyAddOverride(session, uid, fromTick, addChildren: false, mask);
         }
 
         if (!_pvsOverride.SessionForceSend.TryGetValue(session.Session, out var sessionForce))
             return;
 
         foreach (var uid in sessionForce)
         {
-            RecursivelyAddOverride(session, uid, fromTick, addChildren: false);
+            RecursivelyAddOverride(session, uid, fromTick, addChildren: false, mask);
         }
     }
 
-    private void RaiseExpandEvent(PvsSession session, GameTick fromTick)
+    private int RaiseExpandEvent(PvsSession session, GameTick fromTick)
     {
-        var expandEvent = new ExpandPvsEvent(session.Session);
+        var expandEvent = new ExpandPvsEvent(session.Session, session.VisMask);
 
         if (session.Session.AttachedEntity != null)
             RaiseLocalEvent(session.Session.AttachedEntity.Value, ref expandEvent, true);
@@ -85,23 +87,25 @@ private void RaiseExpandEvent(PvsSession session, GameTick fromTick)
         {
             foreach (var uid in expandEvent.Entities)
             {
-                RecursivelyAddOverride(session, uid, fromTick, addChildren: false);
+                RecursivelyAddOverride(session, uid, fromTick, addChildren: false, expandEvent.VisMask);
             }
         }
 
         if (expandEvent.RecursiveEntities == null)
-            return;
+            return expandEvent.VisMask;
 
         foreach (var uid in expandEvent.RecursiveEntities)
         {
-            RecursivelyAddOverride(session, uid, fromTick, addChildren: true);
+            RecursivelyAddOverride(session, uid, fromTick, addChildren: true, expandEvent.VisMask);
         }
+
+        return expandEvent.VisMask;
     }
 
     /// <summary>
     /// Recursively add an entity and all of its parents to the to-send set. This optionally also adds all children.
     /// </summary>
-    private bool RecursivelyAddOverride(PvsSession session, EntityUid uid, GameTick fromTick, bool addChildren)
+    private bool RecursivelyAddOverride(PvsSession session, EntityUid uid, GameTick fromTick, bool addChildren, int mask)
     {
         if (!_xformQuery.TryGetComponent(uid, out var xform))
         {
@@ -116,25 +120,28 @@ private bool RecursivelyAddOverride(PvsSession session, EntityUid uid, GameTick
         // to the toSend set, it doesn't guarantee that its parents have been. E.g., if a player ghost just teleported
         // to follow a far away entity, the player's own entity is still being sent, but we need to ensure that we also
         // send the new parents, which may otherwise be delayed because of the PVS budget.
-        if (parent.IsValid() && !RecursivelyAddOverride(session, parent, fromTick, false))
+        if (parent.IsValid() && !RecursivelyAddOverride(session, parent, fromTick, false, mask))
             return false;
 
         if (!_metaQuery.TryGetComponent(uid, out var meta))
             return false;
 
+        if ((mask & meta.VisibilityMask) != meta.VisibilityMask)
+            return false;
+
         if (!AddEntity(session, (uid, meta), fromTick))
             return false;
 
         if (addChildren)
-            RecursivelyAddChildren(session, xform, fromTick);
+            RecursivelyAddChildren(session, xform, fromTick, mask);
 
         return true;
     }
 
     /// <summary>
     /// Recursively add an entity and all of its children to the to-send set.
     /// </summary>
-    private void RecursivelyAddChildren(PvsSession session, TransformComponent xform, GameTick fromTick)
+    private void RecursivelyAddChildren(PvsSession session, TransformComponent xform, GameTick fromTick, int mask)
     {
         foreach (var child in xform._children)
         {
@@ -145,10 +152,14 @@ private void RecursivelyAddChildren(PvsSession session, TransformComponent xform
             }
 
             var metadata = _metaQuery.GetComponent(child);
+
+            if ((mask & metadata.VisibilityMask) != metadata.VisibilityMask)
+                continue;
+
             if (!AddEntity(session, (child, metadata), fromTick))
-                return;
+                return; // Budget was exceeded (or some error occurred) -> return instead of continue.
 
-            RecursivelyAddChildren(session, childXform, fromTick);
+            RecursivelyAddChildren(session, childXform, fromTick, mask);
         }
     }
 

Robust.Server/GameStates/PvsSystem.ToSendSet.cs

@@ -142,18 +142,31 @@ private bool AddEntity(PvsSession session, Entity<MetaDataComponent> entity, Gam
 
         if (meta.EntityLifeStage >= EntityLifeStage.Terminating)
         {
-            var rep = new EntityStringRepresentation(entity);
-            Log.Error($"Attempted to add a deleted entity to PVS send set: '{rep}'. Deletion queued: {EntityManager.IsQueuedForDeletion(uid)}. Trace:\n{Environment.StackTrace}");
-
             // This can happen if some entity was some removed from it's parent while that parent was being deleted.
             // As a result the entity was marked for deletion but was never actually properly deleted.
-            EntityManager.QueueDeleteEntity(uid);
+
+            bool queued;
+            lock (_toDelete)
+            {
+                queued = EntityManager.IsQueuedForDeletion(uid) || _toDelete.Contains(uid);
+                _toDelete.Add(uid);
+            }
+
+            var rep = new EntityStringRepresentation(entity);
+            Log.Error($"Attempted to add a deleted entity to PVS send set: '{rep}'. Deletion queued: {queued}. Trace:\n{Environment.StackTrace}");
             return false;
         }
 
         data.LastSeen = _gameTiming.CurTick;
         session.ToSend!.Add(entity.Comp.PvsData);
 
+        // TODO PVS PERFORMANCE
+        // Investigate whether its better to defer actually creating the entity state & populating session.States here?
+        // I.e., should be be constructing the to-send list & to-get-states lists, and then separately getting all states
+        // after we have gotten all entities? If the CPU can focus on only processing data in session.DataMemory without
+        // having to access miscellaneous component info, maybe it will be faster?
+        // Though for that to work I guess it also has to avoid accessing the metadata component's lifestage?
+
         if (session.RequestedFull)
         {
             var state = GetFullEntityState(session.Session, uid, meta);

Robust.Server/GameStates/PvsSystem.cs

@@ -94,6 +94,8 @@ internal sealed partial class PvsSystem : EntitySystem
     /// </summary>
     private readonly List<GameTick> _deletedTick = new();
 
+    private readonly HashSet<EntityUid> _toDelete = new();
+
     /// <summary>
     /// The sessions that are currently being processed. Note that this is in general used by parallel & async tasks.
     /// Hence player disconnection processing is deferred and only run via <see cref="ProcessDisconnections"/>.
@@ -195,6 +197,12 @@ internal void SendGameStates(ICommonSession[] players)
         // Construct & serialize the game state for each player (and for the replay).
         SerializeStates();
 
+        foreach (var uid in _toDelete)
+        {
+            EntityManager.QueueDeleteEntity(uid);
+        }
+        _toDelete.Clear();
+
         // Compress & send the states.
         SendStates();
 
@@ -461,18 +469,27 @@ private void AfterSerializeStates()
 }
 
 [ByRefEvent]
-public struct ExpandPvsEvent(ICommonSession session)
+public struct ExpandPvsEvent(ICommonSession session, int mask)
 {
     public readonly ICommonSession Session = session;
 
     /// <summary>
-    /// List of entities that will get added to this session's PVS set.
+    /// List of entities that will get added to this session's PVS set. This will still respect visibility masks.
     /// </summary>
     public List<EntityUid>? Entities;
 
     /// <summary>
     /// List of entities that will get added to this session's PVS set. Unlike <see cref="Entities"/> this will also
-    /// recursively add all children of the given entity.
+    /// recursively add all children of the given entity. This will still respect visibility masks.
     /// </summary>
     public List<EntityUid>? RecursiveEntities;
+
+    /// <summary>
+    /// Visibility mask to use when adding entities. Defaults to the usual visibility mask for that client.
+    /// </summary>
+    /// <remarks>
+    /// Note that this mask will affect all global & session overrides from <see cref="PvsOverrideSystem"/> for this
+    /// client, not just the entities in <see cref="Entities"/> and <see cref="RecursiveEntities"/>.
+    /// </remarks>
+    public int VisMask = mask;
 }