Skip to content

fix: applying CheckObjectVisibility upon NetworkShow #2463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 22, 2023
Merged

Conversation

jeffreyrainy
Copy link
Contributor

Prevents NetworkShow from overriding CheckObjectVisibility.

Found while investigating #2454 which is already fixed.

@jeffreyrainy jeffreyrainy requested a review from a team as a code owner March 21, 2023 16:22
@jeffreyrainy
Copy link
Contributor Author

I'm not 100% we want this as it modifies the API slightly.

One could argue either for or against this.

Various arguments that could be made:

  • it is silly to call NetworkShow() while providing a contrary visibility delegate
  • if the client code calls NetworkShow(), well, they mean to show the object
  • we shouldn't have two APIs for the same feature
  • we shouldn't break/change APIs

What are the thoughts of other members of the team?

@NoelStephensUnity
Copy link
Collaborator

I'm not 100% we want this as it modifies the API slightly.

One could argue either for or against this.

Various arguments that could be made:

  • it is silly to call NetworkShow() while providing a contrary visibility delegate
  • if the client code calls NetworkShow(), well, they mean to show the object
  • we shouldn't have two APIs for the same feature
  • we shouldn't break/change APIs

What are the thoughts of other members of the team?

I think this PR helps remove user confusion with the two potentially conflicting visibility directives. By providing some form of feedback (i.e. warning) users can more easily come to the conclusion that their object's visibility delegate is still returning false and make the appropriate adjustments in their implementation.

👍

@@ -331,6 +331,15 @@ public void NetworkShow(ulong clientId)
throw new VisibilityChangeException("The object is already visible");
}

if (CheckObjectVisibility != null && !CheckObjectVisibility(clientId))
{
if (NetworkManager.LogLevel == LogLevel.Developer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this message log if NetworkManager.LogLevel <= LogLevel.Normal?
A user would be more likely to see this message and it wouldn't be something that would get spammed all of the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the optional suggested modification to the loglevel.
The rest looks good.

@0xFA11 0xFA11 enabled auto-merge (squash) March 22, 2023 14:09
@0xFA11 0xFA11 merged commit 8b01cbe into develop Mar 22, 2023
@0xFA11 0xFA11 deleted the fix/aside-2454 branch March 22, 2023 16:22
@A1win
Copy link

A1win commented Apr 5, 2023

With this change, what's the intended way of creating a NetworkObject that's hidden from all clients by default (even those who connect after the object has been spawned) but has the ability to be shown to a specific client at a later time? The simplest way I can think to do it is as follows, which does seem to work but is a little strange:

Spawning the NetworkObject, hidden from all clients by default:

no.CheckObjectVisibility = (id) => false;
no.Spawn();

Showing the NetworkObject to a specific clientId:

no.CheckObjectVisibility = (id) => id = clientId;
no.NetworkShow(clientId);

Hiding the NetworkObject from clientId again:

no.CheckObjectVisibility = (id) => false;
no.NetworkHide(clientId);

Perhaps it would make sense to have some kind of NetworkObject.EvaluateVisibility() function that would show/hide the object for clients based on the CheckObjectVisibility delegate? Then the code would look like this:

Spawning the NetworkObject, hidden from all clients by default:

no.CheckObjectVisibility = (id) => false;
no.Spawn();

Showing the NetworkObject to a specific clientId:

no.CheckObjectVisibility = (id) => id = clientId;
no.EvaluateVisibility();

Hiding the NetworkObject from clientId again:

no.CheckObjectVisibility = (id) => false;
no.EvaluateVisibility();

@NoelStephensUnity
Copy link
Collaborator

NoelStephensUnity commented Apr 5, 2023

@A1win
Something along the lines of this implementation might be what you are looking for:

public class PreHiddenObject : NetworkBehaviour
{
    private Dictionary<ulong, VisibilityStates> m_ClientVisibility = new Dictionary<ulong, VisibilityStates>();

    public enum VisibilityStates
    {
        Hidden,
        Visible
    }

    public VisibilityStates DefaultVisibility;

    private bool IsObjectVisibileToClient(ulong clientId)
    {
        if (m_ClientVisibility.ContainsKey(clientId))
        {
            return m_ClientVisibility[clientId] == VisibilityStates.Visible;
        }
        // If the client identifier has no entry then, for your scenario, it would default to false
        return DefaultVisibility == VisibilityStates.Visible;
    }

    /// <summary>
    /// One potential way to unify changing a client's visibility of the NetworkObject
    /// </summary>
    public void ChangeClientObjectVisibility(ulong clientId, VisibilityStates visibilityState)
    {
        if (!IsServer)
        {
            return;
        }

        if (!m_ClientVisibility.ContainsKey(clientId))
        {
            m_ClientVisibility.Add(clientId, visibilityState);
        }
        else
        {
            m_ClientVisibility[clientId] = visibilityState;
        }
        if (visibilityState == VisibilityStates.Visible)
        {
            NetworkObject.NetworkShow(clientId);
        }
        else
        {
            NetworkObject.NetworkHide(clientId);
        }
    }

    private void OnClientDisconnectCallback(ulong clientId)
    {
        // Remove the client identifier when a client disconnects
        m_ClientVisibility.Remove(clientId);
    }

    private void OnClientConnectedCallback(ulong clientId)
    {
        // If it doesn't already exist, then add the client identifier when a client connects
        if (!m_ClientVisibility.ContainsKey(clientId))
        {
            m_ClientVisibility.Add(clientId, DefaultVisibility);
        }
    }

    public override void OnNetworkSpawn()
    {
        if (IsServer)
        {
            NetworkManager.OnClientConnectedCallback += OnClientConnectedCallback;
            NetworkManager.OnClientDisconnectCallback += OnClientDisconnectCallback;
            m_ClientVisibility.Add(NetworkManager.LocalClientId, VisibilityStates.Visible);
            NetworkObject.CheckObjectVisibility = IsObjectVisibileToClient;
        }
    }

    public override void OnNetworkDespawn()
    {
        // Optional/Preference
        // Only when the object is despawned on the server does it reset the visibility table.
        if (IsServer)
        {
            NetworkManager.OnClientConnectedCallback -= OnClientConnectedCallback;
            NetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
            NetworkObject.CheckObjectVisibility = null;
            m_ClientVisibility.Clear();
        }
        base.OnNetworkDespawn();
    }
}

It isn't the "simplest" in regards to lines of code, but it shows the general flow of how you can start out with a specific visibility state and make client specific visibility changes.

You can also just use NetworkObject.NetworkShow and NetworkObject.NetworkHide.

If you are wanting something to simply be "not visually visible" but still maintain an instance on the client side, then using NetworkObject.NetworkShow, NetworkObject.NetworkHide, and NetworkObject.CheckObjectVisibility wouldn't be the best way to handle that since when a NetworkObject is "hidden" it is actually despawned/destroyed on the client side.

_In order to do that, I would just disable the visual elements but not disable any GameObject with a NetworkObject or any NetworkBehaviours. Typically under this scenario I recommend placing all visual elements on a child under the root GameObject (not parented netcode relative, but when creating the prefab) while keeping all netcode related components on the root or other children...so all you need to do there is just disable the "child visual components GameObject" and then re-enable it when you are ready for it to be visible. Of course, you would need to track the "visible" state in any NetworkBehaviour updates and exit early if it was "hidden". _

NetworkObject.NetworkShow and NetworkObject.NetworkHide are to make the object "network visible or network hidden". It is often used to cull out objects not pertinent to the client and/or to help reduce bandwidth for objects out of a client's line of sight (but would not recommend using it based on what is or is not visible in the view frustum of the client as spawning does have a bandwidth cost).

@A1win
Copy link

A1win commented Apr 5, 2023

I did mean network visibility, and as long as calling NetworkHide on an object immediately after spawning it won't even briefly show it to the wrong clients, something along the lines of the code you posted is probably good enough. 👍

@NoelStephensUnity
Copy link
Collaborator

NoelStephensUnity commented Apr 5, 2023

The name of the class is fubar (just re-read it), but yeah that class should do the trick (might rename it to whatever makes sense to you).

@CosmicStud
Copy link

CosmicStud commented May 2, 2023

Dang, I'm late to the debate lol... But why not just a simple default boolean in the NetworkShow method...

#2546

@CosmicStud
Copy link

CosmicStud commented May 2, 2023

I'm not 100% we want this as it modifies the API slightly.

One could argue either for or against this.

Various arguments that could be made:

  • it is silly to call NetworkShow() while providing a contrary visibility delegate
  • if the client code calls NetworkShow(), well, they mean to show the object
  • we shouldn't have two APIs for the same feature
  • we shouldn't break/change APIs

What are the thoughts of other members of the team?

Not on the team, but I concur, on the second point. I am manually calling the visibility due to a change in the application/gameplay. Right now since the NGO API modification, it kinda blocks my choice. Since all objects are visible by default, its just a lot of bandwidth for the spawns on the client side for objects I do not want to be visible.

So I use the check object delegate to return false on all objects. Then use the NetworkShow to manually adjust by my needs. I do have several objects that use different implementations for the delegate besides returning false. So I do think they are the same feature but they have different uses per developer and use case.

Also I have a lot of dynamic objects spawning and despawning from the client so, its just full control for me to decide how to design.

I propose a simple default boolean so the developer can choose to skip the "CheckObjectVisibility" delegate when manually calling "NetworkShow" from the network object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants