-
Notifications
You must be signed in to change notification settings - Fork 430
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
feat(runtime): Add OnBeforeValueChange callback to NetworkVariable #668
Conversation
… value changes. Clarify when its value changes for the comment of OnValueChanged. The new OnBeforeValueChange callback will guarantee the order-of-execution of certain actions that have to be executed before a NetworkVariable's value changes. As a part of this commit, the comment for OnValueChanged will also be clearer about when it is invoked in relative to the value change.
@jeffreyrainy this looks like a pretty straightforward & useful addition. Wondered what your take is and wanted for your awareness for your network variable work |
I like the improvement. Good stuff, thanks! The only reservations I have are: #426 Naming change from |
I made the change from All instances of variables named |
@jeffreyrainy Could you drive this and get it merged or closed? Thanks |
/// The callback to be invoked when the value gets changed | ||
/// The callback to be invoked before the value gets changed | ||
/// </summary> | ||
public OnValueChangedDelegate OnBeforeValueChange; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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.
- 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.
There was a problem hiding this comment.
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.
We discussed this internally and decided to not add this into MLAPI for now. Reason being that this could also be achieved by a user by subscribing their own high level construct to the event which then calls two functions. We have some work planned in this area with the snapshot system we are building and will potentially change how events like OnValueChanged works in general. |
This feels a bit too much like a workaround around a problem for comfort. But if the team is open to the possibility of changing how events like I've been thinking about a pure-C# solution to statically guarantee order-of-execution like I first thought of the idea while coming across this C# proposal and coming up with a use-case for it. And while that proposal was for declaring generic alias, you can right now create something resembling a quasi-multicast delegate by subclassing
(The invocation semantic can be anything - member method, extension method or just a raw foreach loop) This I think is the cleanest way to force pure C# delegates to guarantee order-of-execution like |
The new
OnBeforeValueChange
callback will guarantee the order-of-execution of certain actions that have to be executed before aNetworkVariable
's value changes. As a part of this commit, the comment forOnValueChanged
will also be clearer about when it is invoked in relative to the value change.