Skip to content
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

Automatically invoke NetworkVariable.OnValueChanged when spawning #3186

Open
babaq opened this issue Dec 30, 2024 · 4 comments
Open

Automatically invoke NetworkVariable.OnValueChanged when spawning #3186

babaq opened this issue Dec 30, 2024 · 4 comments
Labels
priority:medium This issue has medium priority and may take some time to be resolved stat:awaiting triage Status - Awaiting triage from the Netcode team. Tracking Has been added to tracking type:feature New feature, request or improvement type:feature 2.x New NGO 2.0.0 feature, request or improvement

Comments

@babaq
Copy link

babaq commented Dec 30, 2024

Feedback
When load scene with in-scene placed NetworkObject or dynamically spawn NetworkObject, connected client would spawn the object and NetworkVariable value synced with the server. But the OnValueChanged is not called, even if the value has been synced with the value of server, and is different from the default value that has been set in NetworkBehaviour. The current behavior of skipping OnValueChanged when spawning also doesn't depend on where the OnValueChanged has been registered, in OnNetworkSpawn , OnNetworkPreSpawn or even in Awake.

This caused a lot of problems, because NetworkVariable may be a bool that enable/disable a renderer, or set a shader field through OnValueChanged. Skipping OnValueChanged would make render frame inconsistent with what's intended. This is specially annoying when late-join client starts to spawn all NetworkObject, without OnValueChanged the game states would drastically different from server. The problem also shows up when a previously NetworkHide object become visible again and client respwan the object but with inconsistent states from server.

Suggested Changes
When spawning, all NetworkVariable should be synchronized, and corresponding OnValueChanged should all get called, because clients are supposed to keep the same states as server. The server has likely changed the value of NetworkVariable and have already called the OnValueChanged on server side, so when spawning happens, client need to not only sync all values of NetworkVariable, but also call all OnValueChanged to keep the same states as server.

An intuitive way is to register OnValueChanged in OnNetworkPreSpawn, so that during subsequent spawning any valid callbacks registered will get called.

@babaq babaq added stat:awaiting triage Status - Awaiting triage from the Netcode team. type:feedback Issue contains feedback and not specific bug or feature requests. labels Dec 30, 2024
@NoelStephensUnity
Copy link
Collaborator

Hi @babaq,

The documentation provides the description of the behavior behind NetworkVariables, but I am going to see if we can improve upon that documentation to include additional information that will help clarify the state of NetworkVariables and when OnValueChanged is invoked.

  • The server (or client) instance that has write permissions sets the initial value.
  • While one might think of the network prefab's value as being the "previous state" for non-authority instances (i.e. does not have write permissions), this is actually an incorrect assumption as the "previous state" is considered the "current state" when a NetworkObject is spawned.

Take the following example into consideration:

public class MyNetworkBehaviour : NetworkBehaviour
{
    // One way to check for default previous valu
    private const int InitialSomeIntValue = 10;
    private NetworkVariable<int> m_SomeInt = new NetworkVariable<int>(InitialSomeIntValue);
    private int m_InitialSomeIntValue;

    protected override void OnNetworkPreSpawn(ref NetworkManager networkManager)
    {
        m_InitialSomeIntValue = m_SomeInt.Value;
        base.OnNetworkPreSpawn(ref networkManager);
    }

    public override void OnNetworkSpawn()
    {
        m_SomeInt.OnValueChanged += OnSomeIntChanged;
        if (IsServer)
        {
            m_SomeInt.Value = Random.Range(20, 1000);
        }
        else
        {
            // Non-authority (no write permissions) instances can initialize
            // from the initial value of the NetworkVariable this way.
            // The initial value is when the previous and current values are
            // the same.
            OnSomeIntChanged(m_InitialSomeIntValue, m_SomeInt.Value);
        }
        base.OnNetworkSpawn();
    }

    private void OnSomeIntChanged(int previous, int current)
    {
        Debug.Log($"SomeInt changed from ({previous}) to ({current})");
    }
}

What will happen is that the server will invoke OnValueChanged during OnNetworkSpawn because the value (server/authority relative) does indeed change.
For clients (non-authority when write permissions are set to server), the newly instantiated network prefab is considered to have "no state" until it is spawned. Upon being spawned, the new instance's NetworkVariable(s) are considered to be set to their "initial value" (client relative). If you need a client to do additional initialization based on NetworkVariable values when spawning, then it is recommended to create a central method (other than the subscribed OnValueChanged method) so you can invoke it both when spawned and when (after being spawned) the initial value changes.

Why is it this way?
While it might seem "counter intuitive" you have to think a bit more about what being "spawned" really means. When a NetworkObject and the associated NetworkBehaviour components are considered "not spawned" then there is no known previous state. This is primarily because they have yet to be spawned and have no reference point to determine a "previous state".

I know...it might sound confusing but I can help further clarify. When a NetworkObject is spawned on a non-authority/non-write permission instance the previous state and current state are considered "the same". This is to provide the baseline in which that instance can determine any changes to the state from that point in time forward. Then there is the value assigned to the NetworkVarible when it is instantiated (typically during the declaration portion of the instantiation) that may or may not be a value that a user wants to have as the "previous state" during the spawning of a non-authority instance. Some designs might not even care about the default value as that is configured by the authority instance during OnNetworkSpawn and/or a users script might not want OnValueChanged to be triggered when an instance is spawned and only cares about when it changes from the point of being instantiated and spawned forward.

Finally, if you were to have a NetworkObject pool where you are re-using instances, you could eventually end up spawning an instance that was already spawned...which you can't set the value of a NetworkVariable on a non-authority instance... so the "current" value of a despawned pooled object could be some value that you might not want to have as the previous value since it was last used. As such, it made more sense to make the previous state and current state be the same values when spawned and for delta states to only occur after the NetworkObject is spawned.

With that said, under the scenario you are describing it is recommended to just invoke the same method you are using to subscribe to the OnValueChanged event when a non-authority instance is spawned (or alternately you can create a third method that is invoked by OnNetworkSpawn and the method used to subscribe to the OnValueChanged event.

This is a base behavior of NetworkVariable that we most likely will not change.
However, you can replicate NetworkVariable by deriving from NetworkVariableBase and define your own type of NetworkVariable to get the behavior specific to your project's needs.

@NoelStephensUnity NoelStephensUnity removed the stat:awaiting triage Status - Awaiting triage from the Netcode team. label Jan 7, 2025
@babaq
Copy link
Author

babaq commented Jan 8, 2025

Hi @NoelStephensUnity,

I agree that when spawning, the instance of a prefab in a non-authority client should consider spawned value as current and previous value. The main concern is not about which should be current and previous values, but to sync the game states with server, not just sync NetworkVariable, because NetworkVariable could hook to an external function that do something to the game states.

For example, if a NetworkVariable<float> Freq = new(10f) have a OnValueChanged callback that set a shader property. Then the intended initialization should be set the Freq value and also invoke OnValueChanged(Freq.Value, Freq.Value), so that rendered noise texture freq will be the intended. Meanwhile, at the non-authority client, the spawned value synced with server, but without also invoke OnValueChanged, rendered texture would be different. My suggestion is that allow user to decide whether they consider the OnValueChanged is part of initialization or not.

One easy way to differentiate the two is to register OnValueChanged in OnNetworkPrespawn or OnNetworkSpawn. Since the previous value and current value are set to same when spawned, we only need to check whether OnValueChanged is null, and if not, invoke OnValueChanged(previous, current), which is the same as OnValueChanged(current, current) when spawned.

My current solution is to gather all NetworkVaribles and call Notify (essentially SetDirty(true)) on server side to trigger OnValueChanged, so game states are completely synced across the network, which is why i also suggested to add Notify function for NetworkVariable in #3184.

one of other solutions as the way you suggested, is to call all OnValueChanged at OnNetworkSpawn like this:

public override void OnNetworkSpawn()
    {
        nv1 += Onnv1;
        nv2 += Onnv2;
        nv3 += Onnv3;
        .
        .
        .
        Onnv1(nv1.Value,nv1.Value);
        Onnv2(nv2.Value,nv2.Value);
        Onnv3(nv3.Value,nv3.Value);
        .
        .
        .
        base.OnNetworkSpawn();
    }

But this is a tedious repeated work which i don't like(especially when there are many NetowrkVariables). I think the first solution is a proper reuse the mechanism Netcode already has.

@NoelStephensUnity
Copy link
Collaborator

@babaq
Just so I can understand how frequently this occurs. How many derived NetworkBehaviours do you have in your project and do you know (roughly) how many unique NetworkVariables each NetworkBehaviour has? I know it might seem like I am resisting a change to this area of NetworkVariable but we have to always consider the impact it might have by changing the way something currently works (i.e. current projects that are designed around the way something works vs making a change that could effectively "break" their script logic because suddenly it is invoking OnValueChanged prior to invoking OnNetworkSpawn which would cause script using the pattern you outlined above to invoke twice during spawn).

Of course, we could always look into the possibility to add another property to NetworkObject that you could set which would force every NetworkVariable on any associated NetworkBehaviour to trigger OnValueChanged during spawn. Adding something like this to each NetworkVariable would yield close to the same amount of work one would have to run through (typing a few less characters but still having to specify this kind of behavior), and so making it an "all or none" property on NetworkObject seems to be a balance between the two....where you could still have some form of granular control.

Would adding another property to NetworkObject that enabled the automatic triggering of NetworkVaraibles on the associated NetworkBehaviours help improve your development experience?

@babaq
Copy link
Author

babaq commented Jan 9, 2025

@NoelStephensUnity Your suggested solution would definitely help a lot of people's developing experience. In the current state of our project, we have ~15 NetworkBehaviours, and the number of NetworkVariables are within 5-50, ~20 on average, and we know we will have much more NetworkBehaviours as project develop.

The Documentation have only shown examples where OnValueChanged are all registered in OnNetworkSpawn. This will remain the same for old project.

But if user want to invoke their OnValueChanged as part of initialization, they will need to register OnValueChanged in OnNetworkPrespawn, so that when NetworkVariables are initialized, their OnValueChanged are not null and then invoked with OnValueChanged(current, current). For extra safeguard, that some current project have already intended or accidentally registered OnValueChanged in OnNetworkPrespawn, A SpawnWithInvokeingOnValueChanged property can be added to NetworkObject that toggle the check for OnValueChanged, if not null, then invoke them.

@NoelStephensUnity NoelStephensUnity added type:feature New feature, request or improvement type:feature 2.x New NGO 2.0.0 feature, request or improvement Tracking Has been added to tracking labels Jan 9, 2025
@NoelStephensUnity NoelStephensUnity added priority:medium This issue has medium priority and may take some time to be resolved and removed type:feedback Issue contains feedback and not specific bug or feature requests. labels Jan 9, 2025
@michalChrobot michalChrobot added the stat:awaiting triage Status - Awaiting triage from the Netcode team. label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:medium This issue has medium priority and may take some time to be resolved stat:awaiting triage Status - Awaiting triage from the Netcode team. Tracking Has been added to tracking type:feature New feature, request or improvement type:feature 2.x New NGO 2.0.0 feature, request or improvement
Projects
None yet
Development

No branches or pull requests

3 participants