NetworkShow and CheckObjectVisibility limiting visibiliy management flexibility (and does not work as intened in documentation)

[JackCher]

Is your feature request related to a problem? Please describe. Using a NetworkShow and CheckObjectVisibility is known to be impossible, and there were already some discussions about it in the past (see https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues/2546 and https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues/2609 )

A resolution of the above issues was an introduction of SpawnWithObservers property on NetworkObject. That indeed solved situations where it was necessary to spawn object with no observers. However, in my case, I want to spawn a player object with owner-only visibility, and later add additional objects. However if I do it with:

go.GetComponent<NetworkObject>().CheckObjectVisibility = ((cId) =>
      {
          return cId == go.GetComponent<NetworkObject>().OwnerClientId;
      });

It results in fully blocking me from any further network visibility management with NetworkShow / NetworkHide.

Interesting that the documentation tells that the above approach is actually valid: https://docs-multiplayer.unity3d.com/netcode/current/basics/object-visibility/ It's said that CheckObjectVisibility is checked only at object spawn, and it's advised to use NetworkShow/NetworkHide after that. However currently it's not how it works, and it's necessary to both update the delegate method and do NetworkShow every time I need to show some object to some client. I feel there is a redundancy in updating a delegate in addition to NetworkShow.

Describe the solution you'd like

• Currently, in my local branch of NGO I just commented out a visibility check in NetworkShow that prevents the method from doing what is supposed to do. This is the fastest solution.
• More beautiful solution would be to have a new argument to spawn a function to setup initial observes of the object:

  SpawnAsPlayerObject(clientId, observers: new List<ulong>{.....});
  Spawn(observers: new List<ulong>{.....});

• Or as it was proposed previously in https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues/2546 some sort of new delegate such as CheckObjectInitialVisibility would also fix the issue

[g3trans]

I've also encountered this issue recently.

I was in 1.2.0 and would set CheckObjectVisibility -> false prior to spawning. In this Unity version NetworkShow would not test CheckObjectVisibility, and I would use NetworkShow to add to the observers list.

        public void NetworkShow(ulong clientId)
        {
            if (!IsSpawned) // throw
            if (!NetworkManager.IsServer) // throw
            if (Observers.Contains(clientId)) // throw

            Observers.Add(clientId);
            NetworkManager.SpawnManager.SendSpawnCallForObject(clientId, this);
        }

In 1.4.0 the CheckObjectVisibility check was added to NetworkShow:

            if (CheckObjectVisibility != null && !CheckObjectVisibility(clientId))
            {
                if (NetworkManager.LogLevel <= LogLevel.Normal)
                {
                    NetworkLog.LogWarning($"[NetworkShow] Trying to make {nameof(NetworkObject)} {gameObject.name} visible to client ({clientId}) but {nameof(CheckObjectVisibility)} returned false!");
                }
                return;
            }
            NetworkManager.SpawnManager.MarkObjectForShowingTo(this, clientId);
            Observers.Add(clientId);

This change meant that my default CheckObjectVisibility -> false will prevent NetworkShow from applying.

This is the summary of #2546 which lead to the SpawnWithObservers flag.

This flag had a bug where new connecting clients would spawn by default, this is discussed in depth in #2609 This was fixed in Pull #2623

The key change was to introduce a 'do not spawn if SpawnWithObservers' check:

if (!sobj.Observers.Contains(clientId) && (sobj.SpawnWithObservers || clientId == NetworkManager.ServerClientId))
{  sobj.Observers.Add(clientId); }

However this fix only applies when CheckObjectVisibility is false, so in my case the default false CheckObjectVisibility would still mean that new clients are observers by default.

if (sobj.CheckObjectVisibility == null)
                {
                    // If the client is not part of the observers and spawn with observers is enabled on this instance or the clientId is the server
                    if (!sobj.Observers.Contains(clientId) && (sobj.SpawnWithObservers || clientId == NetworkManager.ServerClientId))
                    {
                        sobj.Observers.Add(clientId);
                    }
                }
                else
                {
                    // CheckObject visibility overrides SpawnWithObservers under this condition
                    if (sobj.CheckObjectVisibility(clientId))
                    {
                        sobj.Observers.Add(clientId);
                    }
                    else // Otherwise, if the observers contains the clientId (shouldn't happen) then remove it since CheckObjectVisibility returned false
                    if (sobj.Observers.Contains(clientId))
                    {
                        sobj.Observers.Remove(clientId);
                    }
                } 

This is unintuitive that CheckObjectVisibility leads to SpawnWithObservers false not having an effect for late joining clients (and depending on log levels, the warning is missed).

If we remove the CheckObjectVisibility delegate, the issue with late joining clients is resolved, they are not observers since SpawnWithObservers is false.

So we have two options:

1. Use CheckObjectVisibility, don't use NetworkShow/Hide, manage visibility ourselves with a wrapper for the the delegate.
2. Don't use CheckObjectVisibility, set SpawnWithObservers to false and NetworkShow/Hide.

The downside of 2, which @JackCher makes use of (Correct me if I'm misunderstanding) is that the object needs to be spawned prior to NetworkShow. When relying on CheckObjectVisibility, we could set an observer prior to Spawning and immediately RPC in the same frame as spawning. When relying on SpawnWithObservers, the object must first be spawned, and then NetworkShow to clients, effectively making all clients 'late joining' and require synced NetworkVars for all states.

[NoelStephensUnity]

Hi @g3trans! I would recommend updating to the latest version of NGO as a first step. Next, the idea behind the CheckObjectVisibility is that it was meant to be flexible enough that one could implement a more simplified visibility check or have a more detailed/complex visibility check depending upon the implementation. SpawnWithObservers is only meant to be used if you want the very initial observers (server-side) to be none but can be used in tandem with CheckObjectVisbility...but again...it depends upon the visibility check implementation as to how everything "works out in the end".

There are 3 places where CheckObjectVisibility is invoked on spawned NetworkObjects (only server-side, clients never use it):

1. When a client is first being synchronized (if scene management is enabled it checks there otherwise it checks when serializing the ConnectionApprovedMessage)
2. During the initial spawning of NetworkObjects
3. When NetworkShow is invoked (when making it visible to a client)

The order of operations when spawning a NetworkObject (from the server-side perspective):

• NetworkObject is instantiated
• NetworkObject is spawned
  • During the local server-side spawning it will check SpawnWithObservers and if that is set then use CheckObjectVisibility
  • It does this before invoking each associated NetworkBehaviour's OnNetworkSpawn method.
• Based on the observers, it will then send the CreateObjectMessage to all observers.

In one of the earlier PRs I provided an example that I have since updated. This should work with the SpawnWithObservers property set to false or true and could be adjusted to automatically make the owner have visibility:

public class MyVisibilityHandler : NetworkBehaviour
{
    public bool DefaultVisibility;
    public bool OwnerAlwaysHasVisbility;
    private Dictionary<ulong, bool> m_ClientVisibilityTable = new Dictionary<ulong, bool>();

    /// <summary>
    /// Set visibility for a specific client
    /// </summary>
    public void SetObjectVisibility(ulong clientId, bool isVisible)
    {
        if (!m_ClientVisibilityTable.ContainsKey(clientId))
        {
            m_ClientVisibilityTable.Add(clientId, isVisible);
        }
        else
        {
            m_ClientVisibilityTable[clientId] = isVisible;
        }

        if (isVisible)
        {
            NetworkObject.NetworkShow(clientId);
        }
        else
        {
            NetworkObject.NetworkHide(clientId);
        }
    }

    /// <summary>
    /// Set visibility status for all connected clients
    /// </summary>
    public void SetObjectVisibility(bool isVisible = false)
    {
        if (!IsServer || !IsSpawned)
        {
            return;
        }

        SetObjectVisibility(NetworkManager.ConnectedClientsIds.ToList(), isVisible);
    }

    /// <summary>
    /// Set visibility for one or more clients
    /// </summary>
    public void SetObjectVisibility(List<ulong> identifiers, bool isVisible = false)
    {
        // Only the server can show, and if it is not spawned yet then we should not be trying to show the object
        if (!IsServer || !IsSpawned)
        {
            return;
        }

        foreach (var clientId in NetworkManager.ConnectedClientsIds)
        {
            SetObjectVisibility(clientId, isVisible);
        }
    }

    private bool CheckObjectVisibility(ulong clientId)
    {
        if (m_ClientVisibilityTable.ContainsKey(clientId))
        {
            return m_ClientVisibilityTable[clientId];
        }
        return DefaultVisibility;
    }

    public override void OnNetworkSpawn()
    {
        if (IsServer)
        {
            NetworkManager.OnClientConnectedCallback += OnClientConnectedCallback;
            NetworkManager.OnClientDisconnectCallback += OnClientDisconnectCallback;
            NetworkObject.CheckObjectVisibility = CheckObjectVisibility;

            // Default to server always having visibility
            SetObjectVisibility(NetworkManager.LocalClientId, true);

            // If you want owners to always have visibility
            if (OwnerAlwaysHasVisbility && OwnerClientId != NetworkManager.LocalClientId)
            {
                SetObjectVisibility(OwnerClientId, true);
            }                
        }
    }

    public override void OnNetworkDespawn()
    {
        NetworkManager.OnClientConnectedCallback -= OnClientConnectedCallback;
        NetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
        m_ClientVisibilityTable.Clear();
    }

    private void OnClientConnectedCallback(ulong clientId)
    {
        if (NetworkObject.SpawnWithObservers)
        {
            SetObjectVisibility(clientId, DefaultVisibility);
        }
    }

    private void OnClientDisconnectCallback(ulong clientId)
    {
        // Just remove the client identifier entry when the client disconnects
        m_ClientVisibilityTable.Remove(clientId);
    }
}

The flow of the above is:

• When a server instantiates and spawns the NetworkObject, it will either:
  • Initially have no observers if SpawnWithObservers is false, but then when OnNetworkSpawn is invoked both the server and/or the owner (if OwnerAlwaysHasVisbility is set) will be provided visibility.
  • If SpawnWithObservers is true, then visibility will be based on what you set the DefaultVisibility to be (_since there are initially no clients registered with the mClientVisibilityTable).
• When a new client connects:
  • If SpawnWithObservers is set, then that client's visibility of the NetworkObject will be based on the DefaultVisibility setting.
  • If SpawnWithObservers is false, then the object is not visible to the connected client until some other game state logic determines the client should have visibility of that object (i.e. sets visibility through the above component).

Let me know if the above example helps you achieve your goals?

[JackCher]

Hi @NoelStephensUnity Not sure, if the above message was addressed to me or to @g3trans, but if you don't mind let me answer it. I opened this issue as a feature request, as I relate this issue to general problems of visibility management API. I've seen quite a lot of people encounter the same questions (here in github and in discord) - and this is what gave me a reason to raise this question. here are 3 places where CheckObjectVisibility is invoked on spawned NetworkObjects (only server-side, clients never use it): When a client is first being synchronized (if scene management is enabled it checks there otherwise it checks when serializing the ConnectionApprovedMessage) During the initial spawning of NetworkObjects When NetworkShow is invoked (when making it visible to a client)

Yes, I've already seen that in NGO source code, that's why I felt safe to comment out code in NetworkShow knowing that nothing will be broken because of that. After spawning is done the delegate has no impact on actual visibility management, and the actual change can be only done with NetworkShow/NetworkHide methods. In fact, changing CheckObjectVisibility delegate after spawning is only necessary here to bypass filtering for NetworkShow.

Thank you for the above code example. I completely agree that we can solve this issue with this wrapper on NetworkShow/NetworkHide that will duplicate changes in CheckObjectVisibility. However, as a result, all NGO users will end up having the Dictionary<ulong, bool> m_ClientVisibilityTable with identical data as the one in NetworkObject.Observers, and NGO users must maintain this data the same way as NetworkObject.Observers are maintained (with the wrapper you provided). In my opinion, this data duplication and data management is redundant, but with the current API everyone who wants to spawn an object with a custom list of observers and who wants to use NetworkShow must always have their own wrapper in order to make it work.