-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
[Net] MultiplayerReplicator with initial state. #51534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it seems fine. Just one minor qualm, but it’s just coding style/architecture, so not a huge deal.
I wish the move to MultiplayerReplicator would have been done in a separate commit. With everything in one commit, it’s hard to see what exactly changed.
@@ -34,10 +34,15 @@ | |||
#include "core/io/multiplayer_peer.h" | |||
#include "core/io/resource_uid.h" | |||
#include "core/object/ref_counted.h" | |||
#include "core/variant/typed_array.h" | |||
|
|||
class MultiplayerReplicator; | |||
|
|||
class MultiplayerAPI : public RefCounted { | |||
GDCLASS(MultiplayerAPI, RefCounted); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we make MultiplayerReplicator a friend class here and not just use proper encapsulation in MultiplayerReplicator? I tend to find the use if friend classes to be somewhat of a code smell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right, it stank so much 😅 . Fixed.
Yeah, I started working on it based on the other PR, so it kinda stuck. I'll see if I can split it. |
2ad070a
to
1fbd39e
Compare
@jonbonazza splitted it a bit (should be squashed back again pre-merge). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Played around with this a lot, seems like a great first step as discussed :)
Lets get this merged.
Move the former "spawnables" functions to a dedicated MultiplayerReplicator class. Support custom overrides in replicator. Spawn/despawn messages can now contain a state. The state can be automatically encoded/decoded by passing the desired object properties to `spawnable_config`. You can use script properties to optimize the state representation. 2 Callables can be also specified to completely override the default implementation for sending and receiving the spawn/despawn event. (9 bytes overhead, and there's room for improvement here). When using a custom implementation `spawn` and `despawn` can be called with any Object, `send_spawn`/`send_despawn` can receive any Variant as a state, and the path is not required. Two new functions, `spawn` and `despawn`, convey the implementation independent method for requesting a spawn/despawn of an Object, while `send_spawn` and `send_despawn` represent the more low-level send event for a Variant to be used by the custom implementations.
6ab1c1b
to
d4dd859
Compare
Supersedes #51465 . Some documentation is still missing.
Thanks everyone for the feedback on the other PR 🎊 !
I've opened this one to address concerns raised about bloating and customizaion:
MultiplayerReplicator
Simple
Like before, server mode replicates instances automatically from server to client, but spawn/despawn messages can now contain a state.
The state can be automatically encoded/decoded by passing the desired object properties to
spawnable_config
.You can use script properties to optimize the state representation.
Customization
Two new functions,
spawn
anddespawn
, convey the implementation independent method for requesting a spawn/despawn of an Object (and is also used internally by the server mode), whilesend_spawn
andsend_despawn
represent the more low-level send event for a Variant to be used by the custom implementations.2 callables can be specified to completely override the default implementation for sending and receiving the spawn/despawn event, working as callbacks for the new
spawn
/despawn
methods.The spawn command has a 9 bytes overhead (1 for cmd, 8 for scene id), and there's room for improvement here (if we go the route of using the number on a list).
When using a custom implementation
spawn
anddespawn
can be called with any Object,send_spawn
/send_despawn
can receive any Variant as a state, and the path is ignored.