Hiding a network object does not happen for client if another object was shown to them on the same frame

[kjohnston-ooi]

Description

When hiding network objects for a client via NetworkObject.NetworkHide, if any network objects have been shown to the client via NetworkObject.NetworkShow on the same frame prior to the hide calls, they will fail with a VisibilityChangeException on the server and remain visible for the client.

Reproduce Steps

1. Start a server and connect a single client to it
2. Spawn two network objects (A and B), both with visibility for the client
3. Wait at least a frame and remove visibility for object A via NetworkObject.NetworkHide
4. Wait at least a frame again, and then on the same frame, show object A to the client again via NetworkObject.NetworkShow and hide object B via NetworkObject.NetworkHide (in that order)
5. Observe outcome

Actual Outcome

On the server, a VisibilityChangeException will be logged for object B with the message "The object is already hidden", and on the client they will have visibility of both object A and object B.

Expected Outcome

No exception would be logged, and the client would have visibility of object A only.

Environment

• OS: Windows 10
• Unity Version: 2021.3.16f1
• Netcode Version: 1.3.1
• Netcode Commit: 469b46f

Additional Context

This issue appears to have been introduced in a commit on December 12 of last year, where the NetworkObject.NetworkHide call was changed to incorporate a new NetworkManager.RemoveObjectFromShowingTo call. The purpose of that call appears to be for checking if the object being hidden was just told to show for the client by checking if it's in the ObjectsToShowToClient dictionary, as seen here:

// returns whether any matching objects would have become visible and were returned to hidden state
internal bool RemoveObjectFromShowingTo(NetworkObject networkObject, ulong clientId)
{
    var ret = false;
    if (!ObjectsToShowToClient.ContainsKey(clientId))
    {
        return false;
    }

    // probably overkill, but deals with multiple entries
    while (ObjectsToShowToClient[clientId].Contains(networkObject))
    {
        Debug.LogWarning(
            "Object was shown and hidden from the same client in the same Network frame. As a result, the client will _not_ receive a NetworkSpawn");
        ObjectsToShowToClient[clientId].Remove(networkObject);
        ret = true;
    }

    networkObject.Observers.Remove(clientId);

    return ret;
}

However, this does not actually check if that dictionary contains the object in question as an entry to its list value before calling networkObject.Observers.Remove(clientId);, meaning any object that's being shown to the client on this frame/tick will cause the initial return statement to be bypassed and the object to be removed from the Observers list here, rather than in NetworkObject.NetworkHide. As a result, in the following block in the NetworkHide call, the exception noted in the outcome will be triggered and no DestroyObjectMessage will be sent to the client informing them to hide the object:

if (!NetworkManager.RemoveObjectFromShowingTo(this, clientId))
{
    if (!Observers.Contains(clientId))
    {
        throw new VisibilityChangeException("The object is already hidden");
    }
    Observers.Remove(clientId);

    var message = new DestroyObjectMessage
    {
        NetworkObjectId = NetworkObjectId,
        DestroyGameObject = !IsSceneObject.Value
    };
    // Send destroy call
    var size = NetworkManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, clientId);
    NetworkManager.NetworkMetrics.TrackObjectDestroySent(clientId, this, size);
}

I'm currently investigating a fix on my organization's branch of NGO, but I believe it may be that the networkObject.Observers.Remove(clientId); should just have an if (ret) check to ensure that the object was actually in the ObjectsToShowToClient list value. This would also prevent the exception as the code would no longer enter the above block in NetworkObject.NetworkHide for this scenario.

NOTE: at the time of this report the RemoveObjectFromShowingTo has been moved from the NetworkManager to the SpawnManager, but the code otherwise appears identical.

[NoelStephensUnity]

@kjohnston-ooi
Nice find!
Will setup a small replication on our end and either fix it myself or get someone assigned to it!
👍

[jeffreyrainy]

Thanks for the report and debugging. This should now be fixed on the develop branch.

[kjohnston-ooi]

Great, thanks folks!