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

State deltas for game updates of players and projectiles #888

Merged
merged 22 commits into from
Sep 3, 2024

Conversation

AminArria
Copy link
Contributor

@AminArria AminArria commented Aug 28, 2024

Motivation

Part of #494

Summary of changes

  • Update protobuf
    • Mark fields that we are going to delta as optional, this allows to check if there are set or not. Otherwise default value for the type is implied (e.g. 0 for integers)
    • Modify vertices to support deltas
    • Add UNDEFINED enums to use as null value
  • Created diff/2 to receive any 2 terms and output the resulting delta, check the code for a more thorough explanation and edge cases
  • Modify creation of update message to handle all cases where something could be nil due to the result of diffing
  • Send deltas for obstacles, bushes, and crates. This 3 things represented more than 80% of the delta reduction that we could expect to achieve
  • game_client is broken due to this change. Fixing it should be a follow up to this

How to test it?

Test with this client branch https://github.com/lambdaclass/champions_of_mirra/pull/2084

To test play as usual

We should be doing some load tests and profiling in apps to compare against not doing deltas

Checklist

  • Tested the changes locally.
  • Reviewed the changes on GitHub, line by line.
  • This change requires new documentation.
    • Documentation has been added/updated.

@AminArria AminArria force-pushed the handle-diff-game-updates-v2 branch from e143d64 to d2e2a11 Compare August 30, 2024 15:45
@AminArria AminArria force-pushed the handle-diff-game-updates-v2 branch from 22ff26e to fad6e0b Compare August 30, 2024 18:00
float radius = 6;
repeated Position vertices = 7;
optional float radius = 6;
optional ListPositionPB vertices = 7;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting change that is needed for diffing lists under a specific scenario, otherwise its a near impossible task. So the scenarios

  • If new_list is different than old_list (meaning not an exact match) the resulting delta is new_list
  • If new_list is exactly the same as old_list the resulting delta is nil (no delta)

To allow for that second scenario we would have needed a sort of optional repeated field in protobuf, but this is not supported. So the second best thing is this solution where we wrap the list in another message. This wrapper becomes optional and the inner only have the repeated field

ACTIVE = 0;
EXPLODED = 1;
CONSUMED = 2;
PROJECTILE_STATUS_UNDEFINED = 0;
Copy link
Contributor Author

@AminArria AminArria Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new enum value PROJECTILE_STATUS_UNDEFINED and the similar one in CrateStatus is a protobuf best practice cause it been the 0 value will serve as the default for the enum. You are not supposed to actually have business logic tied to it, for example I only use it in the client as a null check

Additionally you probably noticed that its called PROJECTILE_STATUS_UNDEFINED rather than UNDEFINED that's cause the protobuf C# compiler didn't like 2 enums with a value named the same 🤷‍♂️

Copy link
Contributor

@agustinesco agustinesco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Nico-Sanchez Nico-Sanchez merged commit e18a904 into main Sep 3, 2024
6 checks passed
@Nico-Sanchez Nico-Sanchez deleted the handle-diff-game-updates-v2 branch September 3, 2024 22:26
@AminArria AminArria mentioned this pull request Sep 9, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants