Skip to content
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.

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

Replace property names with indexes in state sync #159

Open
elementbound opened this issue Nov 28, 2023 · 6 comments
Open

Replace property names with indexes in state sync #159

elementbound opened this issue Nov 28, 2023 · 6 comments
Labels
feature New feature or request

Comments

@elementbound
Copy link
Contributor

Refs #157

@elementbound elementbound added the feature New feature or request label Nov 28, 2023
@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Oct 18, 2024

I can grab this after #278 is merged. After all, the keys often cost more bandwidth than their values lol

Replacing keys with indexes is very easy, but there is only 1 problem once strings are replaced with indexes: Adding new properties on runtime. No idea how that should be handled in detail.

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Nov 28, 2024

When a new state property is added on runtime, the authority sends full states, until a new_property ack is received from the client. When a state property is removed on runtime, the authority sends full states, until a removed_property ack is received from the client. Does it sound good for a PR?

Or want me to just implement indexes as is, but without covering the above edge use-cases, so diff states will be incompatible with adding/removing state properties on runtime?

Also, since this feature will be done, and input has the same property format [tick][property] perhaps input could also be included in the same PR hmmmm.... Input indexes should be simpler than state indexes. And have the exact same logic.

@elementbound
Copy link
Contributor Author

I'd prefer to pick this up after #321, as it might bring some refactors on the internals of RollbackSynchronizer. Can't say exactly how much time it will take for a PR though, so if it takes too long, I'm ok with a PR. Will ping if that happens.

Ensuring that settings are synced ( if they change ) is up to the user. So for that case, I'd just reset the diff state variables, basically restarting the process - sending the initial full state and then continuing from there.

Good point on the inputs, we should update those to use indexes as well.


Also just thinking out loud for a bit - we could technically use hashes instead of indexes. Both would boil down to sending numbers, but properties having the same hashes between changes might make it more resilient, by reducing index mismatches. A drawback that needs to be considered is hash collisions - not sure how common these would be.

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Dec 8, 2024

I'd prefer to pick this up after #321, as it might bring some refactors on the internals of RollbackSynchronizer

Seeing #347 (realtime adding properties), I agree that you have the perfect insight to implement this cleanly and without bugs (and without back and forth :P )

Can't say exactly how much time it will take for a PR though, so if it takes too long, I'm ok with a PR

For my game, I'm in no rush for this as this is basically a feature for production/release to save bandwidth for the rented machine. And I'm moooooooooooooooonths away from that 😅

So for that case, I'd just reset the diff state variables, basically restarting the process - sending the initial full state and then continuing from there.

Sounds simple and clean 👍

Ensuring that settings are synced ( if they change ) is up to the user.

Let's confirm it after #347

Also just thinking out loud for a bit - we could technically use hashes instead of indexes. Both would boil down to sending numbers, but properties having the same hashes between changes might make it more resilient, by reducing index mismatches. A drawback that needs to be considered is hash collisions - not sure how common these would be.

Since a property - independent of its instance - is to have the same index/hash order, there shouldn't be any risk of hash collisions. So for example the brawlers at forest-brawl will always have the same indexes/hashes for their properties, regardless of how many players there are.

That said, the benefit of indexes would be that it's 1 byte per key, but godot sends 4 bytes (int) anyway, so using hash sounds like a better choice 👍

@elementbound
Copy link
Contributor Author

Let's confirm it after #347

Nothing to confirm. This is a design choice.

Since a property - independent of its instance - is to have the same index/hash order, there shouldn't be any risk of hash collisions.

Two property names in the same config have a chance to hash to the same value, resulting in two different properties having the same index.

That said, the benefit of indexes would be that it's 1 byte per key, but godot sends 4 bytes (int) anyway, so using hash sounds like a better choice 👍

An index is just a number, it has no inherent size. FYI, Godot's int is 64 bits, so 8 bytes. Though, an option is to limit the property count to 255, and send the indexes as a separate PackedByteArray followed by an array of values. This is for diff states only - for full states, we can simply send an array of values.

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Dec 16, 2024

FYI, Godot's int is 64 bits, so 8 bytes.

AAAA pov-you-learn-godot-sends-8-bytes-for-an-index

Though, an option is to limit the property count to 255, and send the indexes as a separate PackedByteArray followed by an array of values. This is for diff states only - for full states, we can simply send an array of values.

I love this idea. Literally optimal bandwidth usage, without bloating code with serialization.

Two property names in the same config have a chance to hash to the same value, resulting in two different properties having the same index.

As a use-case, hopefully I understand this correctly:
RollbackSynchronizer.states has two property names, "movement" and "rotation". Let's say the hash for "movement" is 23423474273 and the hash for "rotation" is 234723747

With a hash being 8 bytes (Godot's int), the chances of hashes colliding is astronomically low imo. Especially since these hash collisions don't increase per node/instance, but only for new scenes with RollbackSynchronizers.
That said, since there won't be that many properties for RollbackSynchronizer globally, could keep a list of all the property hashes in a static dictionary inside RollbackSynchronizer, to compare against newly created hashes.

Anyway, once this check is added, the code will be bloated compared to the PackedByteArray solution which has no such problems, and has far better bandwidth. The bandwidth is the goal of this PR, and after this is solved I will never care about bandwidth again, because the submit state/input has the most bandwidth cost in netfox (sending array of strings 🥹 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants