Add custom comparision support to NetworkList to better support #3587

[Reag]

Is your feature request related to a problem? Please describe.
We have several places in our code where we relied on the old behavior of NetworkLists from before this change. For example, we would query the status of a value based on a primary key in a more complex NetworkSerializable struct. As such, the changes in #3587 broke considerable portions of our update logic. While the changes to 3587 definitely makes sense for the average user, letting power users override this default logic would be a great addition.

Describe the solution you'd like
Ideally, we would like to have some way of passing in an optional equality check for determining if a value update causes a network event. For example, having it take some kind of optional IComparer to use for checking. This way, we could implement a 'bytewise' equality for set operations as needed.

Describe alternatives you've considered
There are two main alternatives we have considered. One is to simply revert our NGO version back a release, and keep it locked until we have time to do a refactor. The second one is to assign that entry in the list 'default' before reassigning it to its new index.

[EmandM]

Hi, thanks for reporting this issue!

The change we made in #3587 brings the NetworkList closer to the behaviour of the NetworkVariable collections. The intention of the IEquatable constraint on the NetworkList is to be able to avoid unnecessary updates being sent.

To better understand your use-case, what is the constraint on changing your IEquatable implementation in this case?

[Reag]

Basically, we have a generic layer system for displaying various pieces of info on our map. All of these layers inherit from the same base class, shown below:

Code Snip

public interface INetworkLayerData : INetworkSerializeByMemcpy, IEquatable<INetworkLayerData>, IEquatable<string>
{
    public FixedString64Bytes Name { get; init; }
    public bool State { get; set; }
}

[AbsLog]
[GenerateSerializationForGenericParameter(0)]
[GenerateSerializationForGenericParameter(1)]
public abstract class
    LayerServicesNetworkManagerBase<TManager, TStruct, TDefinition> : AbsorberNetworkSingleton<TManager>
    where TManager : SerializedNetworkBehaviour
    where TStruct : unmanaged, INetworkLayerData, IEquatable<TStruct>
    where TDefinition : ServiceDefinition
{
    [NonSerialized] [ShowInInspector] [HideInEditorMode]
    private NetworkList<TStruct> _layerData = new();

    public event Action<NetworkListEvent<TStruct>>? LayerUpdated;
   ...
   ...
   ...
    /// <summary>
    /// Sets the internal network layer list value
    /// </summary>
    /// <param name="layerData"></param>
    private void SetLayerState_ServerOnly(TStruct layerData)
    {
        var state = GetLayerState(layerData.Name.Value);
        if (state == layerData.State)
        {
            // Nothing to update
            return;
        }
    
        int index = _layerData.IndexOf(new TStruct { Name = layerData.Name });
        if (index >= 0)
        {
            _layerData[index] = layerData;
        }
        ...
        ...
     }
}

The way we use this is by doing a '.Contains()' operation to check if a layer exists, while still listening to ValueChanged events for actually changing the state of the layer. This way, when we need to check if a user defined service exists, we can simply check via string, rather than having to do a much more difficult set of comparisons, while still getting the NetworkListEvents for changes to its state. Is this great code? No, not really. Its a bit of a hack that relied on inefficient code. However, the update broke it, leading to a case where we need to do a fairly large refactor for everything related to our user defined services.

[Edit]: Some background: my team has been working with NGO since v1.0.0. As such, we have some very old code and some very old design patterns that are no longer best practices with the newer architecture. NetworkLists are already weird (They lack Enumerators for use with Linq or ForEach, along with weird restrictions that don't apply anywhere else in NGO), so we just accepted this was one of its quirks. For now, we're going to downgrade to 2.4.4 until we have the free time to refactor most of our network data systems.

[EmandM]

Thanks for the extra information!

We absolutely can put back a way to do things the old way. We have a couple of options for doing this.

We can add a public field to the NetworkList that'll ignore the equality check. This has the benefit of globally setting the behaviour and all other code can remain the same.

var myState = new NetworkList<MyType>();
myState.CheckEqualityForUpdate = false;

Otherwise, we can add getter/setter functions alongside the public T this[int index] implementation and the setter can have an override. This has some optimization benefits as the C# compiler doesn't optimize functions inside properties very well. This option is also more inline with NetworkVariable collections.

var myState = new NetworkList<MyType>();
myState.Set(index, value, forceUpdate = true);

Which option would work better for your project?