fix(statebag): check for replication state on SendAll

[tens0rfl0w]

Goal of this PR

The current state bag implementation does not keep track of the replication state of its key/value pairs. This causes inconsistent sync behavior when we previously had replication enabled and then switched it off, as clients that enter the scope of an entity/a player or connect to the server will still get sent all sbag key/value pairs regardless of their current replication state.

How is this PR achieving the goal

We now store the replication state of each key/value pair and check if we should replicate its data when a client requests a state bag's data, which occurs when a client connects to the server (global state) or enters the scope of an entity/a player (entity/player state bags).

I opted for the separate object approach to maintain simplicity. If you prefer storing the replication state directly within the data object, please let me know.

This PR applies to the following area(s)

Server

Successfully tested on

Game builds: 2699

Platforms: Windows

Checklist

• Code compiles and has been tested successfully.
• Code explains itself well and/or is documented.
• My commit message explains what the changes do and what they are for.
• No extra compilation warnings are added by these changes.

Fixes issues

fixes #2438

[AvarianKnight]

Would it not be better performance-wise to only keep track of what we dont want to replicate? We're far more likely to want to replicate something over not.

[tens0rfl0w]

While I agree on the assumption that an enabled replication state is much more likely, other than some memory overhead this shouldn't really have a performance impact as std::string's hashing function should make any insert/delete/find operation on std::unordered_set take O(1) in this use case. (Hash collision seems unlikely here? Might be wrong..)

Can be changed ofc.

[thorium-cfx]

I'm not 100% sure about the goal here, is this to privatize key/value pairs?

[tens0rfl0w]

This PR intends to fix a bug in the current statebag implementation that causes non-replicated player and entity statebags to still get synced to clients when a client enters the scope of an entity/a player. See the mentioned issue for an actual reproduction.

Basically, we never store the replication state of a statebag key, so if we sync data after setting a key/value this will always get send to clients.

[thorium-cfx]

I understand, but in this case wouldn't you expect the old value – that was replicated – to be sent instead before we set another value that shouldn't be replicated? The current design or implementation seems flawed either way IMO.

So I'm trying to figure out if we still want to consider the replicate boolean here. You see there's currently a proposal going through the last stages that will allow you define if key/value pairs are synced at all (among more configurability), overruling this replicate state, hence why I asked if it's meant to privatize key/value pairs.

[tens0rfl0w]

Thank you for clarifying.

I personally see switching the replication state on an existing key as something that will not happen that often; that's why I didn't really consider this when implementing. For me, disabling the replication of a key means 'ignoring' the sync-state, which implies we don't sync any data and 'forget' this was replicated to clients, which would mean no syncing of the previous (replicated) value.

So this isn't meant to privatize key/value pairs but rather to actually respect replication flags, so we don't overwrite non-replicated client keys or put additional load on the network thread when someone would expect key-data isn't synced.

[jxckUK]

Sorry to jump in, but as I raised the GitHub issue thought I'd throw my comments in as well.

As @tens0rfl0w mentions and as I see this is to ensure that replication state is honoured as it was intended or at least documented here.

The specific use case that caused me to open this (after finding AvarianKnights original repro) did not change the replication flag state on an existing key, instead a new key with replication set to false. I can say from my experience that I've not yet changed the replication state of an existing key once, so would agree that's uncommon.

Future proposals and extensions of the solution shouldn't necessarily change the need to fix the existing behaviour I guess?

[thorium-cfx]

Interesting, wouldn't you instead want to write non-sync data to a non-synced key? And synced data to a key you do enable to sync? That with a lot less overhead than a std::unordered_set lookup? Mind you, O(1) being the complexity, not the clock cycles.

At least it's obvious that replicate means different things to different people and its function differs per the type of state bag. To me the global state bag works as intended (you keep the old value) but those rules/expectations are no longer applicable to range based state bags, I would then still expect to get the old replicated state, as it was requested to replicate. Though moving away from this design, to me, sounds like the solution.

[AvarianKnight]

Interesting, wouldn't you instead want to write non-sync data to a non-synced key? And synced data to a key you do enable to sync? That with a lot less overhead than a std::unordered_set lookup? Mind you, O(1) being the complexity, not the clock cycles.

I'm kind of confused what you mean by this, but this shouldn't be considered non-sync data, its useful to be able to use state bags so you can use the state across resource restart & in other resources

[tens0rfl0w]

Moving away from implementation details and looking at what "replication" state actually means, I have to disagree. Difficult to say what the "majority" of feature-users think the intended behavior should be, but having separate values for the same key depending on the replication state, as in:

Player(source).state:set("key", "value1", true)

and

Player(source).state:set("key", "value2", false)

not targeting the same key seems confusing for me. (If that's what you meant, sorry If I got you wrong here)
This gets extra confusing when we count in value getters: The server still replicates a value that is currently not retrievable by any statebag getter.

I would rather propose sending a delete operation to all routing targets and removing the key/value from the client statebag when replication is switched off.

[thorium-cfx]

Reading this, I wonder if we're thinking about the current set up and not the actual goal we're trying to achieve (see X/Y problem). What is the real goal we want to achieve? Make the replicate parameter do X or Y, or do we want to prevent synchronization of certain keys (maybe whole statebags) to keep data private/local and/or reduce network load and unnecessary storage?

Tell me why would I want to use:

Player(source).state:set("shared_key", "show everyone", true)
Player(source).state:set("shared_key", "don't sync this data", false)

and not just:

Player(source).state:set("shared_key", "show everyone", true)
Player(source).state:set("local_key", "don't sync this data", false)

@tens0rfl0w correct, though taking the definition, it doesn't define when, how, or for how long it will replicate, it could just be a single time, two?, or 1 hour for all I know. Also the docs doesn't define the behavior as well, merely "By default, state set from the server is replicated, and state set from clients is not replicated.", where "replicated" is not defined explicitly, hence the confusion.

Though what I meant is not having 2 states for a single key, just that the previous value that we do wanted to replicate will be known by clients and still replicate (if server -> client), it'll effectively act the same as the global state bag, but with removal/addition when going out/in scope.

[tens0rfl0w]

I think my confusion came from the fact that global state bags are also affected by this flawed implementation. If you're setting a global state bag key like:

GlobalState:set("key", "value", true)
GlobalState:set("key", "value2", false)

this works for already connected clients, but any client that connects after this set operation will still have "value2" synced as the key/value (SendAll due to entering sbag scope). So sync-state is inconsistent on all sbag types.

What should be the change here?

We could store the last replicated value and still replicate it when a client requests the full sbag data, or we could nullify all synced clients and stop replication altogether. I'm unsure what the best approach is here.

I'd still be fine with just letting the sync-state get 'lazy' when replication is switched off (like this PR currently does), but this is probably not something you'd consider a proper solution. Also, I'm still strongly leaning towards the idea that a disabled replication state means no sync at all, not even replicating previously replicated values, but if you'd prefer the other approach, I will just go with it.

[tens0rfl0w]

Discussion seems to have ended(?) and this shouldn't be needed with the proposed new state bag API.