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

When a player sends a state RPC, the state is serialized, and on receiving the RPC it is deserialized. #254

Closed

Conversation

TheYellowArchitect
Copy link
Contributor

@TheYellowArchitect TheYellowArchitect commented Aug 26, 2024

Follow up to #251

Its basically the same, but with 1 more commit to add states, see 9205e78
Merge only after the above is merged. This specific commit requires more testing than the above.

I would love to request an update to the network monitor to learn how many bytes are received as ENET header is unknown :/

…d in future godot version and makes the code cleaner (saves 2 typecasts)
…on) and deserialization for received input properties.
if pe.node.is_multiplayer_authority():
_auth_input_props.push_back(pe)
else:
_record_input_props.push_back(pe)
Copy link
Contributor Author

@TheYellowArchitect TheYellowArchitect Aug 26, 2024

Choose a reason for hiding this comment

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

Not critical enough to backport it to #251
From what I understood, _record_state_props records the properties of a node, regardless of authority.
_record_input_props was unused (yet I needed it for that exact purpose, to learn the properties of a node I don't have authority on, as the receiver), so I replicated the same as _record_state_props

But this opens a new issue. Why do _auth_input_props and _auth_state_props even exist?!
They are currently used exclusively as booleans, to get if we have authority or not.
And it's not like properties are added realtime - if they are, they could as well be added onto _record_state_props and _record_input_props without problems.

My suggestion is to straight up remove the variables _auth_input_props and _auth_state_props and replace them with booleans (e.g. node.get_multiplayer_authority()) unless I am missing something

else:
# Broadcast as new state
for picked_peer_id in multiplayer.get_peers():
_submit_state.rpc_id(picked_peer_id, state_to_broadcast, tick)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing as inputs. Locally, not a single RPC should be sent/received. Because we already have the state locally. So we must simply RPC to other peers, because for this tick we are RPCing, we already have set locally the variables latest_state, _states[tick]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpc's call_remote makes the old rpc call work no problems, so this is needless.

@@ -222,8 +236,16 @@ func history_cleanup() -> void:
_inputs.erase(_inputs.keys().min())

if (NetworkRollback.enable_input_serialization):
while _serialized_inputs.size() > NetworkRollback.history_limit:
_serialized_inputs.erase(_serialized_inputs.keys().min())
if (NetworkRollback.serialized_inputs_history_limit > 0):
Copy link
Contributor Author

@TheYellowArchitect TheYellowArchitect Aug 26, 2024

Choose a reason for hiding this comment

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

0 or -1 value is for saving all states/inputs (PackedByteArray format is optimal)
This is required for saving replays (well, the inputs at least, but now states are required instead until that .fire() issue is solved #253)

var state: Dictionary = PropertySnapshot.extract(_props)
_submit_state.rpc(state, tick)
var local_state: Dictionary = PropertySnapshot.extract(_props)
update_last_state_cache(local_state, tick)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tested this class at all, nor am I familiar with it. But it should work since rollback synchronizer works with state serialization. The logic is quite simple, so I doubt I missed something.

@TheYellowArchitect
Copy link
Contributor Author

Closing in favour of #278 because this custom serialization doesn't currently support Strings + PackedTypedArrays (I could do it but it would double the serialization code), and since it is not in C++ (GDExtension) it costs performance.

Godot's serialization isn't that bad for the common use-case, it leads to simply double the bytesize so it's not the end of the world. The bandwidth is saved by diff states, regardless of the technique (custom serialization or not)

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.

1 participant