fix: Moving tick writing out of NetworkVariable and into NetworkBehav… - #590
Conversation
…iour. Allows proper handling of INetworkVariable that are not plain NetworkVariable. Forces extension of the interface. issues/573, MTT-532
| /// <summary> | ||
| /// Accessor for the RemoteTick stored in the networkVariable, list, set or dictionary | ||
| /// </summary> | ||
| ushort RemoteTick { get; } |
There was a problem hiding this comment.
Do you mind describing the problem and the proposed fix in this PR?
I have very little to zero context, what I'm reviewing? :)
There was a problem hiding this comment.
See below comment
|
Explaining the overall situation: A previous PR introduced ticks to networked variables. The remote tick is the tick at which the other machine changed a variable. This will be needed for lag compensation. The remote tick used to be written in and read in Notice the asymmetry. When another class than NetworkVariable implementing INetworkVariable is used, the Read would occur, but not the Write. As such, the other INetworkVariable could not be received. This PR corrects the situation by moving the Write out of NetworkVariable so that it applies to all INetworkVariable. This comes at a cost. The cost is that INetworkVariable now needs to expose a property for the remote tick. At the moment, this property is set to "NoTick" for all other INetworkVariables. In the longer-run, I would advocate for replacing the interface INetworkVariable with a proper base class that would handle stuff like ticks. But such a change is out of the scope of the upcoming release. |
| @@ -145,14 +145,6 @@ public bool CanClientRead(ulong clientId) | |||
| /// <param name="stream">The stream to write the value to</param> | |||
| public void WriteDelta(Stream stream) | |||
There was a problem hiding this comment.
similar to what we are doing here, can we have ReadDelta(Stream stream, bool keepDirtyDelta) instead of ReadDelta(Stream stream, bool keepDirtyDelta, ushort localTick, ushort remoteTick) and push these down localTick/remoteTick reading from NetworkBehaviour.HandleNetworkVariableDeltas(...) method body.
There was a problem hiding this comment.
That is an awesome point and forced be to go back to re-thinking the whole rationale.
Where we start from:
- a set of individual Netvars that individually travel between machines.
Where we want to go:
- a snapshot system where a single message carries all the vars for a given frame. This message would have a single local tick (when it is sent) and many remote tick (when each individual var was modified). The dictionaries, sets and lists would (could?) go in too.
This experimental release, sadly, sits somewhere in between. Netvars (all types) looks like they carry remote tick and local tick. But only NetworkedVariables actually do. But we don't have the full snapshot system either.
I'm unwilling to revert back the whole tick transport into the NetworkedVariables just to not change stuff. In the end, the NetworkBehaviour will be an integral part of writing the snapshot. So, it feels OK to have some of the feature there. Similarly, in the end, the sets, lists, and dictionaries will have to be able to fit in the snapshot, so... This change is needed.
It's not optimal and, in hindsight, the part that goes in the experimental release would have been cut out differently.
But for this release, I'd go forward with this PR, adding the tests that Matt suggests below. Would that be acceptable ?
|
I'd like to, for test coverage, ask that we augment the |
mattwalsh-unity
left a comment
mattwalsh-unity
left a comment
There was a problem hiding this comment.
Code looks good, let's add the test coverage I put in the comment. Not only will we get more test coverage but it will be a thing we can build off of when we have the in-process testing working
Added tests for the NetworkDictionary, NetworkSet and NetworkList. Please review. Thanks! |
4 participants
…iour. Allows proper handling of INetworkVariable that are not plain NetworkVariable. Forces extension of the interface. issues/573, MTT-532