Skip to content

feat(runtime): Add OnBeforeValueChange callback to NetworkVariable #668

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.

Sign up for GitHub

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

Closed
tonygiang wants to merge 2 commits into from
Closed

feat(runtime): Add OnBeforeValueChange callback to NetworkVariable #668

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d4a395
feat(runtime): Add a callback that invokes before a NetworkVariable's…
tonygiang Mar 25, 2021
69dacff
Merge branch 'develop' into develop
mattwalsh-unity Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions com.unity.multiplayer.mlapi/Runtime/NetworkVariable/NetworkVariable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,15 @@ public class NetworkVariable<T> : INetworkVariable
/// <summary>
/// Delegate type for value changed event
/// </summary>
/// <param name="previousValue">The value before the change</param>
/// <param name="oldValue">The value before the change</param>
/// <param name="newValue">The new value</param>
public delegate void OnValueChangedDelegate(T previousValue, T newValue);
public delegate void OnValueChangedDelegate(T oldValue, T newValue);
/// <summary>
/// The callback to be invoked when the value gets changed
/// The callback to be invoked before the value gets changed
/// </summary>
public OnValueChangedDelegate OnBeforeValueChange;
Copy link
Contributor

@LukeStampfli LukeStampfli Apr 29, 2021

Choose a reason for hiding this comment

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

Do we have an example use case for this? You should be able to achieve everything with OnValueChanged already by accessing the previousValue.

This increases the public API of NetworkVariable which means more documentation, code to maintain, just more potential issues for us. If this solves an existing issue then yeah lets merge this. If this is just adding an additional event for convenience then I would suggest we don't add this.

Copy link
Author

@tonygiang tonygiang May 1, 2021

Choose a reason for hiding this comment

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

The use-case for this delegate is strictly to ensure order-of-execution. The previous value of a network variable itself may had been passed along OnValueChange, but what if there are other resources associated with this value upon it being changed? We typically would like the order-of-execution to be:

  1. Take a snapshot of values associated with the network variable and send it elsewhere - could be a logger, could be a SQL utility to compile a single Update statement.
  2. Update associated values so that they now reflect the network variable's new value.

If this had been UnityEvent, we could easily guarantee this order of execution by just swapping around serialized callbacks in the Inspector, order-of-execution would have been a trivial issue. But since NetworkVariable uses plain C# delegates, the order of execution is less clear. I'm aware that the order of execution of listeners registered to a C# delegate can be guaranteed by registering them in the same order you'd like them to be executed, but this presents a big potential for a gotcha, where seemingly unrelated changes in a codebase could change the order of registration and thus change the order of execution silently. Everything would still run but we'd be unknowingly sending false data back to our database. We will take 1000+ compilation errors over such a circumstance any day.

In non-Unity .Net apps, I use this wrapper class as a way to have a better control over order-of-execution:

using System;
using System.Linq;
using System.Collections.Generic;

public class SortableAction<TO> where TO : IComparable<TO>
{
  protected Action _action;
  // can be a byte to save memory, or can be System.DateTime or System.Version
  // for some reason
  public TO ExecutionOrder = default;

  public static SortableAction<TO> From(Action action, TO eo)
  { return new SortableAction<TO> { _action = action, ExecutionOrder = eo }; }

  public void Invoke() { _action.Invoke(); }
}

public static class SortableActionExtensions
{
  public static void InvokeInOrder<TO>(
    this IEnumerable<SortableAction<TO>> source) where TO : IComparable<TO>
  {
    var sortedActions = source.OrderBy(sa => sa.ExecutionOrder);
    foreach (var sa in sortedActions) sa.Invoke();
  }
}

But that solution is out of the question for this project. So I figured a "before change" event could at the very least guarantee some degree of control over execution order and hoped that other Unity developers find other use cases for it.

Copy link
Contributor

@LukeStampfli LukeStampfli May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the explanation. Something like your wrapper class would indeed be a much better solution because it providers more fine grained control over execution order. But I can see that there are some situations where a BeforeValueChange can help to some extend.

/// <summary>
/// The callback to be invoked after the value gets changed
/// </summary>
public OnValueChangedDelegate OnValueChanged;

Expand Down Expand Up @@ -91,9 +95,10 @@ public T Value
RemoteTick = NetworkTickSystem.NoTick;

m_IsDirty = true;
T previousValue = m_InternalValue;
T oldValue = m_InternalValue;
OnBeforeValueChange?.Invoke(oldValue, value);
m_InternalValue = value;
OnValueChanged?.Invoke(previousValue, m_InternalValue);
OnValueChanged?.Invoke(oldValue, m_InternalValue);
}
}

Expand Down Expand Up @@ -183,12 +188,14 @@ public void ReadDelta(Stream stream, bool keepDirtyDelta, ushort localTick, usho

using (var reader = PooledNetworkReader.Get(stream))
{
T previousValue = m_InternalValue;
m_InternalValue = (T)reader.ReadObjectPacked(typeof(T));
T oldValue = m_InternalValue;
T newValue = (T)reader.ReadObjectPacked(typeof(T));
OnBeforeValueChange?.Invoke(oldValue, newValue);
m_InternalValue = newValue;

if (keepDirtyDelta) m_IsDirty = true;

OnValueChanged?.Invoke(previousValue, m_InternalValue);
OnValueChanged?.Invoke(oldValue, newValue);
}
}

Expand Down