[MP] Fix broken RPCs after dictionary keys type change #96915
Conversation
As part of RPCs processing, they need to be sorted reliably across all peers, so that unique IDs can be assigned to greatly optimize the network layer. The RPC configuration nodes are stored in dictionaries which, until recently, always casted StringName keys to String. Since method names (keys) in the RPC configuration were StringName, a side effect of the above change is that sorting the dictionary keys no longer sort them alphabetically by default (StringName are compared using their pointers). This commit changes the RPC processing logic to use sort_custom to provide a function that can handle the StringName comparison.
| @@ -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) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Makes sense. BTW I wondered why we're using Variant for the rpc config and the names instead of explicitly Dictionary/String(Name)?
There was a problem hiding this comment.
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?
|
Thanks! |
As part of RPCs processing, they need to be sorted reliably across all peers, so that unique IDs can be assigned to greatly optimize the network layer.
The RPC configuration nodes are stored in dictionaries which, until recently (#70096), always casted StringName keys to String.
Since method names (keys) in the RPC configuration were StringName, a side effect of the above change is that sorting the dictionary keys no longer sort them alphabetically by default (StringName are compared using their pointers).
This commit changes the RPC processing logic to use sort_custom to provide a function that can handle the StringName comparison.
Fixes #96889
Regression from #70096