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

Update high_level_multiplayer.rst #8697

Merged
merged 6 commits into from
Jan 13, 2024
Merged

Conversation

scotmcp
Copy link
Contributor

@scotmcp scotmcp commented Jan 1, 2024

Clarifying RPC checksum, and how all RPCs on a script must have matching partners on the target peer's script as well.

Clarifying RPC checksum, and how all RPCs on a script must have matching partners on the target peer's script as well.
@AThousandShips AThousandShips requested a review from a team January 1, 2024 18:17
@AThousandShips AThousandShips added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Jan 1, 2024
@mhilbrunner
Copy link
Member

Thanks for contributing! Not sure we should be calling it a checksum - we're just sending an ID based on the function name and path. Functionally, I guess it is a checksum, but that makes me think of something more complex than a simple numerical ID.

We should also maybe mention here that both path and function name must match.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 2, 2024

It being a checksum, if it is, could be an implementation detail and shouldn't be mentioned unless it's considered a stable detail, we avoid specifying things like that unless it really matters to leave the option to modify things at need

This is specific for checking the interfaces match, I think saying the names match should be sufficient:

String SceneRPCInterface::get_rpc_md5(const Object *p_obj) {
	const Node *node = Object::cast_to<Node>(p_obj);
	ERR_FAIL_NULL_V(node, "");
	const RPCConfigCache cache = _get_node_config(node);
	String rpc_list;
	for (const KeyValue<uint16_t, RPCConfig> &config : cache.configs) {
		rpc_list += String(config.value.name);
	}
	return rpc_list.md5_text();
}

@scotmcp
Copy link
Contributor Author

scotmcp commented Jan 2, 2024

Yeah it is a checksum, and I think due to what I consider why, that it would remain a stable solution. Sending a checksum rather than the strings is much more efficient use of the network. Additionally, we are speaking to network developers, or at least people who want to become. Regarding whether we should mentioned checksum or not, the ERROR message that results specifically says it's a checksum error, so I think it makes sense to be clear about that in the warning box, it would make searching the MultiplayerAPI web page for "checksum", much more direct and get the developer right to the warning message!

I will look at my patch again and see if I can word smith it a little better.

Second update to improve wording in RPC Checksum warning block.
@scotmcp
Copy link
Contributor Author

scotmcp commented Jan 2, 2024

I made a new commit.

Further improvements to Warning Box about RPC signature matching.
More refinements to RPC signature Warning box.
@scotmcp
Copy link
Contributor Author

scotmcp commented Jan 2, 2024

Ok I promise I am done making changes now.

@scotmcp
Copy link
Contributor Author

scotmcp commented Jan 9, 2024

Checking in here, is there anything else I need to do?

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@mhilbrunner mhilbrunner merged commit 9d7cbfd into godotengine:master Jan 13, 2024
1 check passed
@mhilbrunner
Copy link
Member

Committed the above minor styling improvements and squashed and merged. Thanks and congrats on your first merged contribution! 🎉 Thanks for contributing!

@scotmcp
Copy link
Contributor Author

scotmcp commented Jan 13, 2024

Thank you M !!!

mhilbrunner added a commit that referenced this pull request Jan 25, 2024
* Update high_level_multiplayer.rst

Clarifying RPC checksum, and how all RPCs on a script must have matching partners on the target peer's script as well.

---------

Co-authored-by: Max Hilbrunner <mhilbrunner@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@mhilbrunner
Copy link
Member

Cherry-picked to 4.2.

@scotmcp
Copy link
Contributor Author

scotmcp commented Jan 25, 2024

w00t

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants