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.

Sign up for GitHub

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

[MP] Fix broken RPCs after dictionary keys type change #96915

Merged
merged 1 commit into from
Sep 13, 2024
Merged

[MP] Fix broken RPCs after dictionary keys type change #96915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion modules/multiplayer/scene_rpc_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,24 @@ int get_packet_len(uint32_t p_node_target, int p_packet_len) {
}
}

bool SceneRPCInterface::_sort_rpc_names(const Variant &p_l, const Variant &p_r) {
Copy link
Member

Choose a reason for hiding this comment

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

You could probably use StringName::AlphCompare for this instead of implementing something similar ad hoc?

godot/core/string/string_name.h

Line 167 in 83d54ab

struct AlphCompare {

Copy link
Collaborator Author

@Faless Faless Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I looked into that, the problem is that AlphCompare expects const StringName & because it's meant for internal (templated) data structures (List/HashMap/etc) which implements sort_custom as a template function while Array.sort_custom expects a callable because it's a bound data type (and the callable takes const Variant & arguments).

So I don't think there's a way to reuse it directly. Maybe, we could use it inside _sort_rpc_names when we detect StringName and get (maybe?) a performance optimization? (I don't think the compiler is smart enough to optimize away the string conversion otherwise)

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. BTW I wondered why we're using Variant for the rpc config and the names instead of explicitly Dictionary/String(Name)?

Copy link
Collaborator Author

@Faless Faless Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good question, IIRC originally that was to allow migrating from a dictionary to a refcounted/struct and handle compatibility internally without changing the function signature (that was before we had compatibility methods and so on).

I think at this point we could switch to dictionary (but then, the config of each method would still be composed of "variants" since that's what dictionary values are).

We could even switch to something like HashMap<StringName,InternalStruct> but that involves doing copies every time we cross the boundaries (e.g. gdextension ScriptExtension._get_rpc_config), and I'd rather do that in the multiplayer module itself where they can be properly cached.

EDIT: Or... we could expose RPCConfig as a resource/refcounted, but do we want yet another multiplayer-related class in core?

if (likely(p_l.is_string() && p_r.is_string())) {
return p_l.operator String() < p_r.operator String();
}
bool valid = false;
Variant res;
Variant::evaluate(Variant::OP_LESS, p_l, p_r, res, valid);
return valid ? res.operator bool() : false;
}

void SceneRPCInterface::_parse_rpc_config(const Variant &p_config, bool p_for_node, RPCConfigCache &r_cache) {
if (p_config.get_type() == Variant::NIL) {
return;
}
ERR_FAIL_COND(p_config.get_type() != Variant::DICTIONARY);
const Dictionary config = p_config;
Array names = config.keys();
names.sort(); // Ensure ID order
names.sort_custom(callable_mp_static(&SceneRPCInterface::_sort_rpc_names)); // Ensure ID order
for (int i = 0; i < names.size(); i++) {
ERR_CONTINUE(!names[i].is_string());
String name = names[i].operator String();
Expand Down
2 changes: 2 additions & 0 deletions modules/multiplayer/scene_rpc_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ class SceneRPCInterface : public RefCounted {
#endif

protected:
static bool _sort_rpc_names(const Variant &p_l, const Variant &p_r);

void _process_rpc(Node *p_node, const uint16_t p_rpc_method_id, int p_from, const uint8_t *p_packet, int p_packet_len, int p_offset);

void _send_rpc(Node *p_from, int p_to, uint16_t p_rpc_id, const RPCConfig &p_config, const StringName &p_name, const Variant **p_arg, int p_argcount);
Expand Down
Loading