Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Suggestion: Custom MDReplicatedMember for ICollection (and also Dictionary) #23

Open
Beider opened this issue Jun 27, 2020 · 18 comments
Open
Assignees
Labels
new feature Brand new functionality to the framework

Comments

@Beider
Copy link
Collaborator

Beider commented Jun 27, 2020

Now that we split the MDReplicatedMember into a class that we can subclass it would be possible to add a member that would allow for easy replication of ICollection. That way we could replicate generic lists across the network without much overhead.

I would suggest we introduce a new replicated member called MDReplicatedICollection, this replicated member handles all the logic regarding checking for added / removed stuff from the list. Then we add a MDReplicatedICollectionContentHandler that will handle the actual replication, insertion, etc..

The reason I want the handler is because if we introduce the parameters mention in this suggestion we could allow people to pass in their own handlers if they have a list of some custom type.

We could also introduce a handler that default works on KeyValuePair<object, object> which would then work for dictionaries.

@Beider Beider added the new feature Brand new functionality to the framework label Jun 27, 2020
@Beider
Copy link
Collaborator Author

Beider commented Jun 30, 2020

So I was thinking a bit more about this one, I think the only way this would be doable without excessive amounts of processing to check if the list has been updated would be to provide a MDReplicatedList and MDReplicatedDictionary where we override all methods that modify the list with tracking (or directly calling the replicator).

Unless of course there is some way to extend List and Dictionary without subclassing them? We could also implement IList and wrap the list behind that, but if there are other ways as well I wouldn't know.

That being said, I don't think this is urgent but I think it would be very useful, particularly if you allow for a handler so you can have lists of custom types replicated over the network. Maybe we can do this as a custom type and not using the replicator. I also now added another replication method to the MDReplicator for custom handlers.

    [Remote]
    public void ReplicateClockedValue(uint ID, uint Tick, object Value)
    {
        String key = NetworkIdKeyMap.GetValue(ID);
        if (key == null || !KeyToMemberMap.ContainsKey(key))
        {
            // We got no key so add it to our buffer
            NetworkIdKeyMap.AddToBuffer(ID, Tick, Value);
            return;
        }

        KeyToMemberMap[key].SetValue(Tick, Value);
    }

    [Remote]
    public void ReplicateClockedValues(uint ID, uint Tick, params object[] Parameters)
    {
        String key = NetworkIdKeyMap.GetValue(ID);
        if (key == null || !KeyToMemberMap.ContainsKey(key))
        {
            // We got no key so add it to our buffer
            NetworkIdKeyMap.AddToBuffer(ID, Tick, Parameters);
            return;
        }

        KeyToMemberMap[key].SetValues(Tick, Parameters);
    }

Using the second method you would be able to send multiple values at once in case you got a custom subclass that requires multiple values to be replicated at the same time. I could probably have only had the second one but I felt it was cleaner to have both.

@DoubleDeez
Copy link
Owner

However we do end up doing this, we should make sure Adding To/Removing From/Modifying the list shouldn't have a performance impact, so for example it shouldn't send RPCs right when adding, so that you can continue your usual game logic without worrying about the performance and don't have to treat the List/Dictionary than you would any other container.

That said, we could do an extension to List/Dictionary that calls into the Replicator marking it dirty, letting it know it needs to be updated, but that wouldn't fix normal Add/Remove methods:

public static class ListExtensions {
    public static void ReplicatedAdd<T>(this List<T> list, T item, Node nodeContext)
    {
        list.Add(item);
        // Marking dirty should be lightweight, replication will happen when the replicator ticks
        nodeContext.GetReplicator().MarkContainerDirty(list, nodeContext);
    }
}

@Beider
Copy link
Collaborator Author

Beider commented Jun 30, 2020

My plan was to actually track changes on the list so we never have to iterate over it. Perhaps the simplest solution to this would be to just provide a few methods either as extension or as overrides

  • ReplicatedAdd
  • ReplicatedRemove
  • ReplicatedModify
  • ReplicatedClear
  • GetChanges (if override it would be in the class, if not it would be in the replicator)

If we go with override the methods would track changes internally, if not we would track them in the replicator. Then the replicator could just query GetChanges to see if anything has changed since the last check. GetChanges could simply return an ordered list of commands to perform on the list.

  • [Command#] Add [item]
  • [Command#] Add [item] at [index]
  • [Command#] Remove [index]
  • [Command#] Modify [index] new value [item]

etc.. So we just send that and clients can ensure that the commands are executed in the same order as the owner of the list did. On the owner side we can throw away the commands after they are sent across the network. On new player join we just need to make sure we send which command we are at right when we send them the state of the list.

If we want we could even add the game tick with the command to ensure it is executed at the correct time. Of course this may cause problems if some command is delayed as you may hit a game tick for a command but the command before it has not yet arrived.

@DoubleDeez
Copy link
Owner

That sounds good, I do want to avoid having to use custom container types (MDDictionary, MDList, etc) as it makes it more work to maintain in the future. For example, Godot 4.0 will likely have automatic member replication so we would want to make it easier for projects to update the framework and Godot.

@Beider Beider self-assigned this Jul 2, 2020
@Beider
Copy link
Collaborator Author

Beider commented Jul 3, 2020

One thing I have been thinking about but don't have a solution for is if we make this an extension and not it's own class how do we track which list the changes apply to?

The best would be if you didn't have to pass in any identifier but as it stands I don't see how we would manage that.

For instance if you got something like:

myList.ReplicatedAdd(item);

How would we then know which list / node this was for? That means we would most likely have to do something like,

// Pass in the node, but then we still got to resolve the member, which could be troublesome as well
myList.ReplicatedAdd(item, node);
// Pass in some unique key but then you got to track that in the class
myList.ReplicatedAdd(item, key)

Any good ideas?

@DoubleDeez
Copy link
Owner

I think we'll have to pass in the node and then internally build the key, similar to how we do it elsewhere. I think we'll also need to the member name? Unless if we can get it via reflection some how.

@Beider
Copy link
Collaborator Author

Beider commented Jul 3, 2020

How about we instead do something like the this.SpawnNetworkNode, we implement a generic wrapper for ICollection as I suggested but hide it behind a this.CreateNetworkedList in theory you wouldn't even need the attributes on it if we do it this way.

It is a bit of a difference from the current solution, but the alternative would be to require the user to provide node and member name every call to add / remove / etc...

Instead we could just wrap it and hide the wrapping behind the creation method.

@DoubleDeez
Copy link
Owner

I think as long as we make a "NetworkedList" work like a normal list when in standalone it should be okay.

@Beider
Copy link
Collaborator Author

Beider commented Jul 3, 2020

Ok so this is the best solution I could come up with.

The user can do something like this

[MDReplicated]
protected List<String> MyList;

When we pickup the MDReplicated for any List type we initialize the list for you as a MDList, at this point we fill in the member info and everything else in our custom class implementation. We still need a handler for the list to handle the list content replication, we would provide one that would handle anything that you can send as RPC. If the user need a custom handler they would need to add an [MDReplicatedSetting] for this (ie. If they want a List<MyCustomObject> they need to provide handler for replicating MyCustomObject across the network).

Inside the MDList<T> we override every method that modifies the list and if the network is active we create a network command as described above that we can send across the network.

In the replicator we can add a new ReplicatedMember implementation that works on MDLists.

This is the cleanest way I can come up with of doing this, sounds good?

@Beider
Copy link
Collaborator Author

Beider commented Jul 3, 2020

So after discussion on discord this is the solution I think I would go for, I will probably start implementation tomorrow. So if anyone got objections or suggestions please feel free to come with them in the next 8~ hours.

MDList & MDDictionary

New wrapper classes MDList<T> and MDDictionary<T> which simply wraps an internal List<T> and Dictionary<T> inside of them will be introduced. All methods will be wrapped so it behaves just like it's counter part however it will record the actions that are done to the list if the network is active. See this comment above for how recording will work.

We may need some new methods such as ItemModified(int index) and ItemModified(T item), this is in case someone does something like this:

MyCustomObject obj = MyMDList[index];
obj.SetValue(something);

In addition these classes will not be instanced by the user instead the user will simply do

[MDReplicated]
private MDList<String> MyList;

The MDReplicator will instance the list for the user when it picks up the reference. To prevent people from accidentally instancing their own MDList and MDDictionary we will make the constructor private and have a static factory method. In the event that someone still manages to instance one outside of the MDReplicator then the replicator shall detect this when it attempts to send data and log an error.

A new interface will be introduced called IDataConverter which has two methods,

T ConvertToValue(object[] Params)
object[] ConvertFromValue(T value)

This will be used for parsing the list data for sending / adding on the remote side. All MDReplicatedMember classes should implement this so we can reuse these classes for this purpouse. We will also have an [MDReplicatedSetting] that allows you to pick which kind of IDataConverter you use to parse the list content.

MDReplicatedMemberList & MDReplicatedMemberDictionary

A new replicated member type shall be added that takes care of polling the respective list/dictionary for updates, sending them across the network and reciving them on the other end. This new member should also implement IDataConverter, this would allow you to nest other lists or dictionaries inside an MDList. It will probably not be super efficient in regards to the amount of data this would cause to be sent but it would work.

MDReplicator

The replicator will probably not need any changes except for that it needs to instance the MDList & MDDictionaries.

@DoubleDeez
Copy link
Owner

This sounds like a good starting point for container replication. I'm interested to see how ConvertFromValue will be used exactly but I won't have time to discuss before you implement - don't let that hold you back.

@Beider
Copy link
Collaborator Author

Beider commented Jul 4, 2020

This sounds like a good starting point for container replication. I'm interested to see how ConvertFromValue will be used exactly but I won't have time to discuss before you implement - don't let that hold you back.

So you can check out my initial MDList implementation here.

If you look at the methods near the top you can see how I intend to use the convert, in particular these methods:

private void SendAction(ListActionRecord ActionRecord)
{
    MDStatics.GetReplicator().SendListData(ListId, ActionRecord.ActionNumber, ActionRecord.ActionType, ParseParameters(ActionRecord)));
}

private object[] ParseParameters(ListActionRecord ActionRecord)
{
    switch (ActionRecord.ActionType)
    {
        case ListActions.ADD:
            return DataConverter.ConvertToObject((T)ActionRecord.Parameters[0]);
    }

    return null;
}

When we are sending commands the convert does the conversion of type T to a list of object[]. So if you got a custom class like this one that implements the interface itself it can be used to send itself over the network

public class MyClass : IMDListDataConverter<MyClass>
{
    public String Name;
    public String Value;

    public MyClass (String Name, String Value)
    {
        this.Name = Name;
        this.Value = Value;
    }

    public object[] ConvertToObject(MyClass item)
    {
        return new object[] {Name, Value};
    }

    public MyClass ConvertFromObject(object[] Parameters)
    {
        return new MyClass(Parameters[0].ToString(), Parameters[1].ToString());
    }
}

@Beider
Copy link
Collaborator Author

Beider commented Jul 5, 2020

So I have more or less finished the implementation. I need to make some minor adjustment to our buffer for clocked values, which also now is used to buffer for lists if they don't exist yet. Perhaps we should introduce a more general buffer concept that also buffers normal replicated values, but I digress.

MDReplicatedCommandReplicator

The first thing you want to look at is the MDReplicatedCommandReplicator. This is a generic command replicator that can be used for List, Dictionary and anything else that uses commands to replicate. In theory users can now implement their own command replicators that will be picked up by the MDReplicator and replicated correctly as long as they implement the IMDCommandReplicator interface. This interface is found in the MDReplicatedCommandReplicator class.

One thing to notice is this method for initialization, it actually initializes the list for your so you don't have to do that.

    private void InitializeCommandReplicator(MemberInfo Member, Node Node)
    {
        Type MemberType = Member.GetType();
        if (MemberType != null && MemberType.IsAssignableFrom(typeof(IMDCommandReplicator)))
        {
            CommandReplicator = Activator.CreateInstance(MemberType) as IMDCommandReplicator;
        }
        Member.SetValue(Node, CommandReplicator);
    } 

Also notice that it sends the MDReplicatedSetting[] Settings on to the IMDCommandReplicator, this is so you can pickup your own settings if you make custom command replicators.

Currently this replicator does not work with the GameClock, I don't know if we want it to use the GameClock but adding this functionality would not be too difficult.

MDListDataConverters

The MDListDataConverters class contains the interface for data converters, IMDDataConverter. It also contains a default implementation that works on any standard godot base type (string, int, etc..) that can be replicated with RPC.

In addition it contains a List based data converter and examples on how to use it. This is so you can in theory do a replicated MDList<List<List<List<String>>>> if you want. However it will flood the network since any update to any inner list will cause the entire root list for that object to be sent. I don't recommend using this, but I made it possible at least.

MDList

The MDList class implements the default implementation for a networked list. It contains wrappers for all list functions except Sort (outlined why in comment). It contains everything needed to send commands over the network.

I don't have time to finish the example / tests today but I think the implementation is pretty much done. Might have some bugs though.

@Beider
Copy link
Collaborator Author

Beider commented Jul 5, 2020

I now more or less finished the replicated list on my branch, I just got a bit more to do on the example and then I will add replicated dictionary as well.

Here is a video of the example in action and a brief explanation of the code

@DoubleDeez
Copy link
Owner

This is awesome, I love the replicated change network master, that's definitely beneficial.

Currently this replicator does not work with the GameClock, I don't know if we want it to use the GameClock but adding this functionality would not be too difficult.

I think it should support the GameClock, I think it would be a bit weird to have lists replicate out of sync with non-list values, no?

Regarding sorting, I think the best approach would be to manually implement a sort algorithm and we record each change that happens to the list? Sucks having to re-implement sort but I think that might be the best compromise for network bandwidth? There might be a point where it's just better to sort and send the whole list, I guess it depends on how much changes from the sort.

@Beider
Copy link
Collaborator Author

Beider commented Jul 6, 2020

Regarding sorting, I think the best approach would be to manually implement a sort algorithm and we record each change that happens to the list? Sucks having to re-implement sort but I think that might be the best compromise for network bandwidth? There might be a point where it's just better to sort and send the whole list, I guess it depends on how much changes from the sort.

I was thinking a bit about this and I think I will just introduce a new MDReplicatedSetting for the list where you can register comperators with a key. I would just add a comparator enum with like 10 values in the MDList implementation, you can then register up to 10 comparators for your list.

To use a comparator you just need to provide the enum to any of the sort methods instead of the comparator itself. I think this would be the easiest way to save network bandwith, the downside is you would have to provide the type of your comparator on register. So you end up having to implement them as their own class, but I think that is an acceptable tradeoff for the bandwith savings.

This also means that you can't have any randomness in your sort (but why would you?) and you have to provide your comparators when you initialize your list but I think these things are not that big of a deal. If you really need some sort of custom implementation then you could just make a class that inherits from MDList and add your own sort functionality.

@DoubleDeez
Copy link
Owner

This also means that you can't have any randomness in your sort

This bothers me a bit as I could see a randomized replicated list being useful. Might just be best to send the whole list after a sort.

@Beider
Copy link
Collaborator Author

Beider commented Jul 6, 2020

I would like to have a fast network sort that doesn't require excessive bandwith though, how about I just add a Resynchronize() command or something like that. Which would resend the entire list. Then we can still go with that you have to register your sorts before you use them as I suggested, but you could do a random sort and just re-synch the list after you do.

Optionally I could also expose the underlying list that is wrapped inside the MDList and let you do whatever you want to it. Then you can call resynch when you are done.

@DoubleDeez DoubleDeez added this to the Production Ready milestone Jul 17, 2020
@DoubleDeez DoubleDeez removed this from the Production Ready milestone Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new feature Brand new functionality to the framework
Projects
None yet
Development

No branches or pull requests

2 participants