Skip to content

fix: Moving tick writing out of NetworkVariable and into NetworkBehav… #590

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

Merged
mattwalsh-unity merged 3 commits into from
Mar 12, 2021
Merged

fix: Moving tick writing out of NetworkVariable and into NetworkBehav… #590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cf158e
fix: Moving tick writing out of NetworkVariable and into NetworkBehav…
jeffreyrainy Mar 11, 2021
a0e87b3
test[NetworkList]: Adding manual test for NetworkList
jeffreyrainy Mar 11, 2021
0553f30
test[NetworkList]: Adding manual test for NetworkDictionary and Netwo…
jeffreyrainy Mar 12, 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
4 changes: 4 additions & 0 deletions com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,10 @@ private void NetworkVariableUpdate(ulong clientId)
{
writtenAny = true;

// write the network tick at which this NetworkVariable was modified remotely
// this will allow lag-compensation
writer.WriteUInt16Packed(NetworkVariableFields[k].RemoteTick);

if (NetworkManager.Singleton.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
using (var varBuffer = PooledNetworkBuffer.Get())
Expand Down
11 changes: 10 additions & 1 deletion com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,15 @@ private void HandleAddDictionaryEvent(NetworkDictionaryEvent<TKey, TValue> dicti
m_DirtyEvents.Add(dictionaryEvent);
}
}

public ushort RemoteTick
{
get
{
// todo: implement proper network tick for NetworkDictionary
return NetworkTickSystem.NoTick;
}
}
}

/// <summary>
Expand Down Expand Up @@ -586,4 +595,4 @@ public enum EventType
/// </summary>
public TValue Value;
}
}
}
11 changes: 10 additions & 1 deletion com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,15 @@ private void HandleAddListEvent(NetworkListEvent<T> listEvent)
m_DirtyEvents.Add(listEvent);
}
}

public ushort RemoteTick
{
get
{
// todo: implement proper network tick for NetworkList
return NetworkTickSystem.NoTick;
}
}
}

/// <summary>
Expand Down Expand Up @@ -590,4 +599,4 @@ public enum EventType
/// </summary>
public int Index;
}
}
}
11 changes: 10 additions & 1 deletion com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,15 @@ public bool Remove(T item)

/// <inheritdoc />
public bool IsReadOnly => m_Set.IsReadOnly;

public ushort RemoteTick
{
get
{
// todo: implement proper network tick for NetworkSet
return NetworkTickSystem.NoTick;
}
}
}

/// <summary>
Expand Down Expand Up @@ -536,4 +545,4 @@ public enum EventType
public T Value;
}
}
#endif
#endif
7 changes: 6 additions & 1 deletion com.unity.multiplayer.mlapi/Runtime/NetworkVariable/INetworkVariable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,10 @@ public interface INetworkVariable
/// </summary>
/// <param name="behaviour">The behaviour the container behaves to</param>
void SetNetworkBehaviour(NetworkBehaviour behaviour);

/// <summary>
/// Accessor for the RemoteTick stored in the networkVariable, list, set or dictionary
/// </summary>
ushort RemoteTick { get; }
Comment on lines +77 to +80

@0xFA11 0xFA11 Mar 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mind describing the problem and the proposed fix in this PR?
I have very little to zero context, what I'm reviewing? :)

@jeffreyrainy jeffreyrainy Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See below comment

}
}
}
8 changes: 0 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 @@ -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)

@0xFA11 0xFA11 Mar 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@jeffreyrainy jeffreyrainy Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 ?

{
using (var writer = PooledNetworkWriter.Get(stream))
{
// write the network tick at which this NetworkVariable was modified remotely
// this will allow lag-compensation
// todo: this is currently only done on delta updates. Consider whether it should be done in WriteField
writer.WriteUInt16Packed(RemoteTick);
}

WriteField(stream);
}

Expand Down
99 changes: 85 additions & 14 deletions testproject/Assets/Scripts/Testing/ManualNetworkVariableTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using System;
using System.Collections.Generic;
using UnityEngine;
using MLAPI.NetworkVariable;
using MLAPI.NetworkVariable.Collections;

namespace MLAPI
{
Expand All @@ -10,7 +12,20 @@ namespace MLAPI
[AddComponentMenu("MLAPI/ManualNetworkVariableTest")]
public class ManualNetworkVariableTest : NetworkBehaviour
{
private NetworkVariable<int> m_TestVar;
// testing NetworkList
private NetworkList<string> m_TestList = new NetworkList<string>();
private bool m_GotNetworkList = false;

// testing NetworkSet
private NetworkSet<string> m_TestSet = new NetworkSet<string>();
private bool m_GotNetworkSet = false;

// testing NetworkDictionary
private NetworkDictionary<int, string> m_TestDictionary = new NetworkDictionary<int, string>();
private bool m_GotNetworkDictionary = false;

// testing NetworkVariable, especially ticks
private NetworkVariable<int> m_TestVar = new NetworkVariable<int>();
private int m_MinDelta = 0;
private int m_MaxDelta = 0;
private int m_LastRemoteTick = 0;
Expand All @@ -22,25 +37,65 @@ public class ManualNetworkVariableTest : NetworkBehaviour

void Start()
{
m_TestVar.OnValueChanged = ValueChanged;
m_TestVar.OnValueChanged += ValueChanged;
m_TestVar.Settings.WritePermission = NetworkVariablePermission.Everyone;

m_TestList.OnListChanged += ListChanged;
m_TestList.Settings.WritePermission = NetworkVariablePermission.OwnerOnly;

m_TestSet.OnSetChanged += SetChanged;
m_TestSet.Settings.WritePermission = NetworkVariablePermission.OwnerOnly;

m_TestDictionary.OnDictionaryChanged += DictionaryChanged;
m_TestDictionary.Settings.WritePermission = NetworkVariablePermission.OwnerOnly;

if (IsOwner)
{
m_TestVar.Value = 0;
Debug.Log("We'll be sending " + MyMessage());
}
}

void Awake()
{
Debug.Log("Awake");
}

private void FixedUpdate()
{
if (IsOwner)
{
m_TestVar.Value = m_TestVar.Value + 1;
m_TestList.Add(MyMessage());
((ICollection<string>)m_TestSet).Add(MyMessage());
m_TestDictionary[0] = MyMessage();
}
}

private string MyMessage()
{
return "Message from " + NetworkObjectId;
}

private void ListChanged(NetworkListEvent<string> listEvent)
{
if (!IsOwner && !m_GotNetworkList)
{
Debug.Log("Received: " + listEvent.Value);
m_GotNetworkList = true;
}
}

private void SetChanged(NetworkSetEvent<string> setEvent)
{
if (!IsOwner && !m_GotNetworkSet)
{
Debug.Log("Received: " + setEvent.Value);
m_GotNetworkSet = true;
}
}

private void DictionaryChanged(NetworkDictionaryEvent<int, string> dictionaryEvent)
{
if (!IsOwner && !m_GotNetworkSet)
{
Debug.Log("Received: " + dictionaryEvent.Key + ":" + dictionaryEvent.Value);
m_GotNetworkDictionary = true;
}
}

Expand Down Expand Up @@ -84,19 +139,35 @@ private void ValueChanged(int before, int after)
{
// Let's be reasonable and allow a 5 tick difference
// that could be due to timing difference, lag, queueing
if (m_Problems == "" && Math.Abs(m_MaxDelta - m_MinDelta) < 5)

if (!m_GotNetworkList)
{
m_Problems += "Didn't receive any NetworkList updates from other machines";
}

if (!m_GotNetworkSet)
{
m_Problems += "Didn't receive any NetworkSet updates from other machines";
}

if (!m_GotNetworkDictionary)
{
m_Problems += "Didn't receive any NetworkDictionary updates from other machines";
}

if (Math.Abs(m_MaxDelta - m_MinDelta) > 5)
{
m_Problems += "Delta range: " + m_MinDelta + " + " + m_MaxDelta + "\n";
}

if (m_Problems == "")
{
Debug.Log("**** TEST PASSED ****");
}
else
{
Debug.Log("**** TEST FAILED ****");
Debug.Log($"Delta range: {m_MinDelta}, {m_MaxDelta}");

if (m_Problems != "")
{
Debug.Log(m_Problems);
}
Debug.Log(m_Problems);
}
enabled = false;
}
Expand Down