NetworkShow and the CheckObjectVisibility Delegate

[CosmicStud]

Description

i think to fix several other issues in the NetworkList, a major feature was overlooked and removed. Currently if a "CheckObjectVisibility" delegate is set on a network object and returns false. Then a manual call with "NetworkShow" is made on the network object. No matter what it will never spawn on the client. I think this should be regressed. The developer should have full control over the objects visibility. This change removes developer choice.

Currently, all objects are set to hidden in my project. I would assume that would be the case for a good majority of devs. Or otherwise a lot of bandwidth will undoubtedly be used. If I ever wanted to show an objects visibility forcibly, I cannot anymore. Unless I create another set of workarounds. The only two options are lambda functions, which can be designed with more local data in the method. Or using a delegate, which only the ID of the client is usable...

Before NGO 1.4 release, this was not an issue. If possible can we design NGO to add a parameter to force the "NetworkShow" call and ignore the "CheckObjectVisibility"...

Reproduce Steps

1. Assign "CheckObjectVisibility" delegate, return false.
2. Spawn the network object.
3. Show visibility of network object with "NetworkShow."

Actual Outcome

A warning appears with method execution also ending before spawn implementation.

Expected Outcome

Visibility of network object is shown to client. Due to server having a call on the network object to show visibility.

Environment

• OS: Mac OS Catalina
• Unity Version: 2022.2.15
• Netcode Version: 1.4
• Netcode Commit: [e.g. https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/commit/ba418fa5b600ad9eb61fab0575f12fbecc2c6520]

Additional Context

Current "NetworkObject" class, starting line number 339

https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/blob/27176860f9f25eb089d807ae61413ed44b1bdd1a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs#L339

public void NetworkShow(ulong clientId)
        {
            if (!IsSpawned)
            {
                throw new SpawnStateException("Object is not spawned");
            }

            if (!NetworkManager.IsServer)
            {
                throw new NotServerException("Only server can change visibility");
            }

            if (Observers.Contains(clientId))
            {
                throw new VisibilityChangeException("The object is already visible");
            }

            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);
        }

A possible fix for this would be...

public void NetworkShow(ulong clientId, bool isForced = false)
        {
            if (!IsSpawned)
            {
                throw new SpawnStateException("Object is not spawned");
            }

            if (!NetworkManager.IsServer)
            {
                throw new NotServerException("Only server can change visibility");
            }

            if (Observers.Contains(clientId))
            {
                throw new VisibilityChangeException("The object is already visible");
            }

            if (!isForced)
            {
                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);
        }

[ezoray]

This is a problem for me as well. I've just come back to an older project and updated to 1.4.0. The project is running server-only with custom scene management so I'm making a lot of NetworkShow and NetworkHide calls. With this change it's currently broken, calling NetworkShow gives a warning on the server and no client spawn, and an error log on the client.

How do I check it's safe to make the NetworkShow call?

I'm currently running network object pools, how do I make the change work with these?

Will there be a corresponding check in NetworkHide to make sure it doesn't hide an object that should be visible?

I might be being a bit slow on the uptake here, can you explain the benefit of this change?

Thanks!

[NoelStephensUnity]

@CosmicStud I think possibly there could be confusion as to what CheckObjectVisibility does.

/// <summary>
/// Delegate invoked when the netcode needs to know if the object should be visible to a client, if null it will assume true
/// </summary>
public VisibilityDelegate CheckObjectVisibility = null;

The intended meaning behind the above XML API documentation is that if you want to have "per client" control (i.e. full control) over NetworkObject visibility then you can use CheckObjectVisibility.

When the CheckObjectVisibility is not set the default is to consider the NetworkObject to be always visible for all clients when NetworkShow is invoked. If the CheckObjectVisibility is set but always returns false, no matter which clientId is passed to it, then that NetworkObject will never be visible to any clients when NetworkShow is invoked.

The one thing I think was missed in that last update was to not check CheckObjectVisibility within NetworkHide before hiding the NetworkObject in question. However, it really depends upon how visibility is being managed within your project that should determine this...the API is just the mechanism to "show" or "hide" the NetworkObject in question.

Regarding the proposed fix, adding a public void NetworkShow(ulong clientId, bool isForced = false) where you would be passing in a clientId and a "true or false" value as to whether to show or not show a NetworkObject is effectively the same as having the CheckObjectVisibility return "true or false" based on the clientId passed into it. If you wanted to apply this globally, then there are ways to handle this as is shown in the example implementation below.

Here would be one example of how CheckObjectVisibility could be used and would address all issues, as I (currently) understand them, that you are having:

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

    public void SetClientVisibility(ulong clientId, bool isVisible)
    {
        if (!m_ClientVisibilityTable.ContainsKey(clientId))
        {
            m_ClientVisibilityTable.Add(clientId, isVisible);
        }
        else
        {
            m_ClientVisibilityTable[clientId] = isVisible;
        }
    }

    public void ShowObject(bool setClientVisibility = false, bool forceShow = 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;
        }
        // If true, then the object will become visible to all clients no matter what their visibility setting is
        m_ForceObjectVisibility = forceShow;
        foreach (var clientId in NetworkManager.ConnectedClientsIds)
        {
            // Optional, but you can keep client visibility synchronized here
            if (setClientVisibility)
            {
                SetClientVisibility(clientId, true);
            }
            NetworkObject.NetworkShow(clientId);
        }
    }

    public void HideObject(bool useClientVisibility = false, bool setClientVisibility = false)
    {
        // Only the server can hide, and if it is not spawned yet then we should not be trying to hide the object
        if (!IsServer || !IsSpawned)
        {
            return;
        }

        foreach (var clientId in NetworkManager.ConnectedClientsIds)
        {
            // If useClientVisibility is true then we override the hiding of a NetworkObject by using each client's visibility settings.
            // If a client's visibility is true then we ignore the NetworkHide for that client.
            // (otherwise if useClientVisibility is false then we will hide the NetworkObject for all clients)
            if (useClientVisibility && m_ClientVisibilityTable.ContainsKey(clientId) && m_ClientVisibilityTable[clientId])
            {
                continue;
            }
            // Optional, but you can keep client visibility synchronized here
            if (setClientVisibility)
            {
                SetClientVisibility(clientId, false);
            }                
            NetworkObject.NetworkHide(clientId);
        }
    }

    private bool CheckObjectVisibility(ulong clientId)
    {
        if (m_ForceObjectVisibility)
        {
            return true;
        }
        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;
        }
        base.OnNetworkSpawn();
    }

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

    private void OnClientConnectedCallback(ulong clientId)
    {
        SetClientVisibility(clientId, DefaultVisibility);
    }

    private void OnClientDisconnectCallback(ulong clientId)
    {
        m_ClientVisibilityTable.Remove(clientId);
    }
}

The above provides you with a way to handle what the default visibility setting will be and provides a way to set, on a per client basis, what the NetworkObject's visibility is...which overrides any call to NetworkShow if (and only if) CheckObjectVisibility returns false. It also allows you to control when a NetworkObject should be hidden on a per client basis based on the client's current object visibility setting and provides a way to force showing to all clients if that is needed as well.

Take a look over the MyVisibilityHandler implementation and perhaps we can use this as a "reference point" class to be compared to your project's usage of CheckObjectVisibility so that I might better understand the issue you are having?

[CosmicStud]

Hey there @NoelStephensUnity thanks for responding but that's not really a solution to my issue. I just want to be able to call a network show a network object to a client without checking the delegate. I have a lot of pools and objects spawning and respawning that the majority of my clients will not see.

Your implementation is a great one with the current case of the code we have in front of us. Although now I need a separate set of data structures for maintaining visibility.

But the current NGO kinda blocks my option to show an object to a client regardless of the checkobjectvisibility delegate. That's all I'm saying, perhaps a force option for showing network objects without having to create so many extra workarounds per object in my pools. I'm cool with adapting, it's just extra bloat for my project.

[CosmicStud]

It's just this is a recent change to the NGO code as of 1.4. I've been using previous build versions for some time now. Just wondering if the new change to Network show was append-able with a force option

[NoelStephensUnity]

@CosmicStud

But the current NGO kinda blocks my option to show an object to a client regardless of the checkobjectvisibility delegate.

CheckObjectVisibility was used to handle whether or not a NetworkObject should be "observed" by clients. This extends out to whether an object is visible to a client or not but there were changes a long time ago that did not take that delegate into account. So, if it always was returning false then it would impact how a visible NetworkObject was updated (i.e. updates to NetworkVariables and the like) since that is also used to determine who "observes" the NetworkObject.

If CheckObjectVisibility is null (i.e. not assigned) then it should show to all clients. It could be a deeper issue in how objects are spawned that I need to find so if you could you provide me with an example usage or more details in why you were using this originally it would help me better understand the issue you are experiencing and possibly find a different type of bug and/or missing functionality that might help you still accomplish what you were using it for originally.

[ezoray]

One issue I have with this is that it changes API behaviour breaking current projects. The previous behaviour of NetworkShow has been around for, I don't know, 1.5 years plus. Could the previous behaviour not have been deprecated?

I also don't think the naming is all that descriptive, shouldn't CheckObjectVisibility be something like CheckObjectShouldBeVisible. If NetworkShow is no longer a command, shouldn't it be TryNetworkShow?

This all feels a bit befuddling to me, I understand the reasoning but something doesn't sit right. For what I and what it sounds like CosmicStud are doing losing the simple ability to show and hide objects on demand is painful and if you are to give a command like NetworkShow it should really be overriding whatever CheckObjectVisibility is returning.

Just my 2 cents.

[NoelStephensUnity]

@ezoray I hear what you are saying and it is possible the changes made did not take into consideration an alternate (unexpected) usage of CheckObjectVisibility...under the condition that it always returns false...and that the update to NetworkShow would impact this alternate usage (i.e. CheckObjectVisibility says it is not visible but when invoking NetworkShow even though user code deems the object not visible the object should be made visible).

Personally, I think the term "visible" in itself is confusing enough as is (i.e. meaning whether it is visible from a network perspective to a specific client).

Overview of the Issue Currently:

There was a usage pattern where a server could spawn a NetworkObject locally that would not add any clients to the "observers" of the NetworkObject because CheckObjectVisibility always returned false which, in turn, would mean the NetworkObject would not be spawned on any client until it became visible. Users that used this usage pattern, where the assigned CheckObjectVisibility handler never changed its returned value, NetworkObject.NetworkShow was effectively broken for those users.

The change made in #2454 to fix an issue where NetworkList was populating the list twice for another usage pattern where users were also using a CheckObjectVisibility handler that always returned false.

I am going to mark this bug for import and we will need to noodle over the best way to handle this conundrum in a way that makes sense for all use cases.

Until then, you might contemplate something like:

    public class ObjectVisibilityHandler : NetworkBehaviour
    {
        public bool IsObjectVisible;

        private bool ObjectVisibility(ulong clientId)
        {
            return IsObjectVisible;
        }

        private void SetObjectVisibility(bool isVisible)
        {
            IsObjectVisible = isVisible;
            foreach (var clientId in NetworkManager.ConnectedClientsIds)
            {
                if (IsObjectVisible)
                {
                    NetworkObject.NetworkShow(clientId);
                }
                else
                {
                    NetworkObject.NetworkHide(clientId);
                }
            }
        }

        public void ShowObject()
        {
            SetObjectVisibility(true);
        }

        public void HideObject()
        {
            SetObjectVisibility(false);
        }

        public override void OnNetworkSpawn()
        {
            if (IsServer)
            {
                NetworkObject.CheckObjectVisibility = ObjectVisibility;
            }
            base.OnNetworkSpawn();
        }
    }

Where the returned value is changed based on whether it is considered visible or not since it seems like the usage pattern being described is either global (i.e. applied to all clients) or is being tracked elsewhere as to which client has visibility or not (i.e. it is shown only to specific clients based on some other factors being taken into consideration).

Apologies for the inconvenience as this usage pattern was not taken into consideration when the fix for #2454 change was made.

[ezoray]

@NoelStephensUnity many thanks for the explanation and for putting this under consideration. In the mean time I'll either go back to 1.3.1 or will look into implementing your example code should I need to update.

[NoelStephensUnity]

@ezoray @CosmicStud I have a couple of fixes in mind for this, but they will require a change to the API and we are planning a patch sometime soon (which means no changes to the public API for that update). However, it is imported and we will have a fix for this issue in the next minor version update proceeding the patch.

[CosmicStud]

@NoelStephensUnity @ezoray Thanks for helping out!

I've been thinking over fixes myself, the fix I recommended earlier would break the two if-else checks in "NetworkSpawnManager" if the object was ever respawned. But we should I think always let a NetworkShow override the CheckObjectVisibility delegate ultimately

A possible fix could be another delegate check in the NetworkObject, used only for clients that are joining. A "CheckObjectInitialVisibility" delegate.

That way object visibility use cases can be handled for the actual clients joining in. Also not to hefty to add as its null unless it's filled.

This new delegate check would only apply for the "ConnectionApprovedMessage" class and that one if-else check inside it. Just a suggestion. You guys are awesome, thanks for the help again!

[NoelStephensUnity]

@CosmicStud Interesting thoughts indeed.

Now that I understand the usage pattern (I believe)... it might be that we flip our thinking on this and have the ability to disable assigning observers when a NetworkObject is spawned... possibly a property (i.e. NetworkObject.SpawnWithNoObservers). This way you could just remove your CheckObjectVisiblity handler assignment and then set that property. The NetworkObject would spawn with no observers (but the server would always be an observer) and then when you are ready for a client to be an observer (i.e. the NetworkObject becomes "visible" to the client) you would just use NetworkShow normally.

This approach would require you to make a minor adjustment, but it sort of makes more sense based on how I believe you are/were using CheckObjectVisiblity.

How does that sound to you?

[ezoray]

That makes sense from my perspective as currently I set CheckObjectVisibility to always return false as a way to prevent objects being shown to clients on spawn. This is due to the possibility of clients being in different scenes and a spawned object may not have any relevance to other clients. Or some clients are in the same scene but at a distance for example playing on a map, if one client raises a new army object it would only be shown to other clients when it comes into their field of view.

[CosmicStud]

@NoelStephensUnity Yeah, that would be perfect actually. I'm all in for it!

[NoelStephensUnity]

@CosmicStud @ezoray In the next update you will have a NetworkObject.SpawnWithObservers property (default is true) that you can set to false and it will spawn the NetworkObject with no observers. The only change you will have to make on your end is to remove the CheckObjectVisibility handler and just set NetworkObject.SpawnWithObservers to false. 👍

[ezoray]

@NoelStephensUnity that addition is much appreciated thank you.