ComponentNetworkGenerator doesn't deeply `Clone` collections

Open
#6,972 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
52/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Quiet
Tech stack
csharp
Domain
game-dev, networking

Research direction

Start at ComponentNetworkGenerator and the generated state-handling code for AutoNetworkedField collections; trace how a Dictionary containing MyDatum values is copied. Reproduce the nested dictionary example and verify that recursive cloning reaches IRobustCloneable values, so repeated predicted ticks do not mutate shared objects before server state arrives.

Written by the indexing model from the issue text.

Description

Consider a situation like

public sealed partial class MyComponent : Component {
  [DataField, AutoNetworkedField]
  public Dictionary<string, MyDatum> MyData = new();
}

[DataDefinition]
public sealed partial class MyDatum {
  public int MyInt = 0;
}

and consider a situation where a shared, predicted system does something like

Entity<MyComponent> ent = ...;
for (var (key, datum) in ent.Comp.MyData) {
  datum.MyInt += 1;
}
Dirty(ent)

The client will repeatedly apply the tick where datum.MyInt += 1, meaning the MyInt value will increment wildly before server state is pushed down and resets the value to a single increment (The same basic issue resolved for SS14 solutions here -- the dictionary itself is copied, but the reference to MyDatums within it are what's copied, meaning the same MyDatum objects are mutated many times).

The fix is to implement IRobustCloneable so that the gamestate is deeply cloned for each tick, meaning the += 1 is applied to the same initial value every time.

HOWEVER, this does not work in our case above -- implementing IRobustCloneable for MyDatum is fruitless as something inside the generated state handling code for MyComponent does a shallow clone of MyData and then doesn't call Clone on the values in the dictionary, meaning our clone implementation isn't used.

Ideally, AutoNetworkedFields in a component which are standard C# collections would invoke clone on their data when doing their copies, or whatever "implementation of IRobustCloneable" that is used for these collections would work recursively because a Dictionary<string, Dictionary<string, List<T : IRobustCloneable<T>>> should still be cloned all the way down to the T value.

Dominant language
C#
Stars
693
Forks
746
Avg merge
3d 9h
Merged PRs (30d)
22

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from space-wizards/RobustToolbox

All issues in space-wizards/RobustToolbox

Similar issues

More C# issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.