Skip to content

fix!: added plainly-callable Add() method to NetworkSet [MTT-1005] #1022

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 4 commits into from
Aug 5, 2021
Merged

fix!: added plainly-callable Add() method to NetworkSet [MTT-1005] #1022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cbf19e8
oops
mattwalsh-unity Aug 4, 2021
27f3030
fixadded NetworkSet plainly-callable Add() method MTT-1005
mattwalsh-unity Aug 5, 2021
f68626c
removed unneeded package
mattwalsh-unity Aug 5, 2021
c55a2ca
Merge branch 'develop' into feature/improve_set
Aug 5, 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
37 changes: 12 additions & 25 deletions 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 @@ -331,29 +331,6 @@ IEnumerator IEnumerable.GetEnumerator()
return m_Set.GetEnumerator();
}

/// <inheritdoc />
Copy link
Copy Markdown
Contributor Author

@mattwalsh-unity mattwalsh-unity Aug 5, 2021

Choose a reason for hiding this comment

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

Git has made a mess of this diff; I just hoisted the Add functionality out of the interface-qualified definitions into a method and then made those interfaces call that method.

void ICollection<T>.Add(T item)
{
EnsureInitialized();

if (m_NetworkBehaviour.NetworkManager.IsServer)
{
m_Set.Add(item);
}

var setEvent = new NetworkSetEvent<T>()
{
Type = NetworkSetEvent<T>.EventType.Add,
Value = item
};
m_DirtyEvents.Add(setEvent);

if (m_NetworkBehaviour.NetworkManager.IsServer && OnSetChanged != null)
{
OnSetChanged(setEvent);
}
}

/// <inheritdoc />
public void ExceptWith(IEnumerable<T> other)
{
Expand Down Expand Up @@ -478,8 +455,7 @@ public void UnionWith(IEnumerable<T> other)
}
}

/// <inheritdoc />
bool ISet<T>.Add(T item)
public void Add(T item)
Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer Aug 5, 2021

Choose a reason for hiding this comment

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

nit: You don't need 3 methods for this. Just put the implementation in the ISet and call it from the ICollection.

Copy link
Copy Markdown
Contributor Author

@mattwalsh-unity mattwalsh-unity Aug 5, 2021

Choose a reason for hiding this comment

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

I think the wackiness here is that we have ISet<T> which has its own Add method, and then ICollection<T> (which is a parent to ISet<T>) also has its own Add method, which differs only by return type, and so I have to implement both explicit interfaces.

But then if a user wants to call either one, they have to do a non-intuitive cast, e.g. ((ISet<int>)m_ServerComp.TheSet).Add(1234);. That's why I did the 3rd method. But maybe I'm missing another way?

I'm thinking about using a less grand interface than ISet / ICollection anyway vs. today where we promise to do everything those interfaces do, so that if we use native container underlying storage we don't have to make up for make of these calls that the native types don't support.

Copy link
Copy Markdown
Contributor

@0xFA11 0xFA11 Aug 5, 2021

Choose a reason for hiding this comment

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

HashSet<T> is similar and it implements both ISet<T> and ICollection<T> at the same time — and yes, it implements both void returning and bool returning methods and they both call the exact same method behind the scenes. Let me get you some useful links to relevant points.

bool Add:

public bool Add(T item) => AddIfNotPresent(item, out _);

https://github.com/dotnet/runtime/blob/1a4675db08384e4f20cf4481e6eaf03e40f3d754/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L455

void Add:

void ICollection<T>.Add(T item) => AddIfNotPresent(item, out _);

https://github.com/dotnet/runtime/blob/1a4675db08384e4f20cf4481e6eaf03e40f3d754/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L181

underlying AddIfNotPresent (the actual code):

private bool AddIfNotPresent(T value, out int location)
{
    if (_buckets == null)
    {
        Initialize(0);
    }
    Debug.Assert(_buckets != null);

    Entry[]? entries = _entries;
// ...

https://github.com/dotnet/runtime/blob/1a4675db08384e4f20cf4481e6eaf03e40f3d754/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L1084

I think we could follow the same interface pattern closely to make our stuff more .NET-like :)

Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer Aug 5, 2021

Choose a reason for hiding this comment

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

Yeah, I ended up sending Matt much the same thing in a DM & gist. The key here is you don't need explicit implementation on both of them, just enough to disambiguate. Explicit implementation of both is also what causes this required cast in the first place, as explicit implementations always need an explicit cast.

{
EnsureInitialized();

Expand All @@ -499,10 +475,21 @@ bool ISet<T>.Add(T item)
{
OnSetChanged(setEvent);
}
}

/// <inheritdoc />
bool ISet<T>.Add(T item)
{
Add(item);
return true;
}

/// <inheritdoc />
void ICollection<T>.Add(T item)
Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer Aug 5, 2021

Choose a reason for hiding this comment

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

nit: Maybe use an expression-bodied method to make it SUPER clear this is just a remap... e.g.

bool Add(T item)
{
   ..
}

void ICollection<T>.Add(T item) => Add(item);

(you also only need the explicit implementation on one of them)

Copy link
Copy Markdown
Contributor Author

@mattwalsh-unity mattwalsh-unity Aug 5, 2021

Choose a reason for hiding this comment

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

Yeah, cool, I can do that on ICollection, but I'm stuck on doing this for ISet because it has a different return type

Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer Aug 5, 2021

Choose a reason for hiding this comment

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

My code snippet should work. You need to change your implementation to return a bool, and then just ignore it on ICollection, that should meet both ISet and ICollection needs (but is a moot point if you do the suggestion in the other comment thread)

{
Add(item);
}

/// <inheritdoc />
public void Clear()
{
Expand Down
25 changes: 11 additions & 14 deletions com.unity.multiplayer.mlapi/Tests/Runtime/NetworkVariableTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.TestTools;
using NUnit.Framework;
Expand Down Expand Up @@ -101,6 +100,7 @@ public override IEnumerator Setup()

// This is the *SERVER VERSION* of the *CLIENT PLAYER*
var serverClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper<NetworkObject>();

yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation(
x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId,
m_ServerNetworkManager, serverClientPlayerResult));
Expand All @@ -118,14 +118,16 @@ public override IEnumerator Setup()
m_ClientComp = clientSideClientPlayer.GetComponent<NetworkVariableTest>();

m_ServerComp.TheList.Clear();
m_ServerComp.TheSet.Clear();
m_ServerComp.TheDictionary.Clear();

if (m_ServerComp.TheList.Count > 0)
if (m_ServerComp.TheList.Count > 0 || m_ServerComp.TheSet.Count > 0 || m_ServerComp.TheDictionary.Count > 0)
{
throw new Exception("server network list not empty at start");
throw new Exception("at least one server network container not empty at start");
}
if (m_ClientComp.TheList.Count > 0)
if (m_ClientComp.TheList.Count > 0 || m_ClientComp.TheSet.Count > 0 || m_ClientComp.TheDictionary.Count > 0)
{
throw new Exception("client network list not empty at start");
throw new Exception("at least one client network container not empty at start");
}
}

Expand Down Expand Up @@ -178,8 +180,6 @@ public IEnumerator AllNetworkVariableTypes()
[UnityTest]
public IEnumerator NetworkListAdd()
{
var waitResult = new MultiInstanceHelpers.CoroutineResultWrapper<bool>();

yield return MultiInstanceHelpers.RunAndWaitForCondition(
() =>
{
Expand Down Expand Up @@ -245,9 +245,8 @@ public IEnumerator NetworkSetAdd()
yield return MultiInstanceHelpers.RunAndWaitForCondition(
() =>
{
ISet<int> iSet = m_ServerComp.TheSet;
iSet.Add(k_TestVal1);
iSet.Add(k_TestVal2);
m_ServerComp.TheSet.Add(k_TestVal1);
m_ServerComp.TheSet.Add(k_TestVal2);
},
() =>
{
Expand All @@ -272,8 +271,7 @@ public IEnumerator NetworkSetRemove()
yield return MultiInstanceHelpers.RunAndWaitForCondition(
() =>
{
ISet<int> iSet = m_ServerComp.TheSet;
iSet.Remove(k_TestVal1);
m_ServerComp.TheSet.Remove(k_TestVal1);
},
() =>
{
Expand All @@ -296,8 +294,7 @@ public IEnumerator NetworkSetClear()
yield return MultiInstanceHelpers.RunAndWaitForCondition(
() =>
{
ISet<int> iSet = m_ServerComp.TheSet;
iSet.Clear();
m_ServerComp.TheSet.Clear();
},
() =>
{
Expand Down