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

[3.x] Update NetworkedMultiplayerCustom.xml with more explicit documentation to guide usage #63644

Closed
wants to merge 3 commits into from

Conversation

jarommadsen
Copy link

Some suggestions based on the new NetworkedMultiplayerCustom class introduced in #63163 to be used in 3.x.

@dsnopek I've made some suggestions to the docs based on what would have helped me through my first time. This in conjunction with #63628 and your new additions in #63637 should help new users understand what's going on without having to dig through the source code.

I'd also like some help improving the deliver_packet method docs since I haven't gotten around to really digging through how that gets propagated. I assume the from_peer_id is meant to be the unique network id the sender of that packet was initialized with? And am I correct in assuming the target_id/target_peer is what it uses to deliver it the right place locally? I think some further documentation here would be helpful to know how to use set_target_peer in conjunction with this.

…n to guide usage

Some suggestions based on the new `NetworkedMultiplayerCustom` class introduced in godotengine#63163.

@dsnopek I've made some suggestions to the docs based on what would have helped me through my first time. This in conjunction with godotengine#63628 and your new additions in godotengine#63637 should help new users understand what's going on without having to dig through the source code.

I'd also like some help improving the `deliver_packet` method docs since I haven't gotten around to really digging through how that gets propagated. I assume the `from_peer_id` is meant to be the unique network id the sender of that packet was initialized with? And am I correct in assuming the `target_id`/`target_peer` is what it uses to deliver it the right place locally? I think some further documentation here would be helpful to know how to use `set_target_peer` in conjunction with this.
@jarommadsen jarommadsen requested a review from a team as a code owner July 29, 2022 19:05
@jarommadsen jarommadsen changed the title Update NetworkedMultiplayerCustom.xml with more explicit documentation to guide usage [3.x] Update NetworkedMultiplayerCustom.xml with more explicit documentation to guide usage Jul 29, 2022
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

I assume the from_peer_id is meant to be the unique network id the sender of that packet was initialized with?

Yep!

And am I correct in assuming the target_id/target_peer is what it uses to deliver it the right place locally? I think some further documentation here would be helpful to know how to use set_target_peer() in conjunction with this.

No, the target peer is the remote peer to deliver the packet to. We don't need to say which peer to deliver to in deliver_rpc() because it's "this peer" - the developer should have already ensured that the packet only arrived at a peer that was intended to receive it.

The set_target_peer() method should ONLY be called by Godot internally! Developers should NOT be calling it themselves.

You can only change to [constant NetworkedMultiplayerPeer.CONNECTION_CONNECTING] from [constant NetworkedMultiplayerPeer.CONNECTION_DISCONNECTED] and to [constant NetworkedMultiplayerPeer.CONNECTION_CONNECTED] from [constant NetworkedMultiplayerPeer.CONNECTION_CONNECTING].
Setting this to [constant NetworkedMultiplayerPeer.CONNECTION_DISCONNECTED] will emit the [signal NetworkedMultiplayerPeer.connection_failed] if it was previously set to [constant NetworkedMultiplayerPeer.CONNECTION_CONNECTING] or [signal NetworkedMultiplayerPeer.server_disconnected] if previously [constant NetworkedMultiplayerPeer.CONNECTION_CONNECTED]. Note that this is only true if the peer is not in server mode (has the unique network id of [code]1[/code]), otherwise the connection status will be set to [constant NetworkedMultiplayerPeer.CONNECTION_DISCONNECTED] without emitting any signals.
Copy link
Contributor

Choose a reason for hiding this comment

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

This much detail in the docs for set_connection_status() is quite difficult to follow. I think the main thing that developers need to know about the signals, is that connection_succeeded, connection_failed and server_disconnected will get emitted for them (whereas they need to emit peer_connected and peer_disconnected themselves, although I'm not sure where to put that note).

This all might be better explained by including a short example implementation?

Copy link
Author

Choose a reason for hiding this comment

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

I agree but I think that the problem I'm having is that this bit has some control coupling going on. Extra documentation doesn't solve this issue but it alleviates the user experience somewhat. Like you said, knowing the signals get emitted is the most important bit. I felt in the dark as a user since this is a "custom" implementation knowing when and how those signals actually were emitted. It's not obvious I think from the signal docs since they rely on the implementations to call them on whether or not they're handled here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the goal here is that the developer just calls set_connection_status() and the right signals get emitted (in an effort to reduce the number of things the developer needs to consider).

I haven't tried to write it (so it might not be possible to do briefly), but I think a code snippet in the description might be the clearest way to show developers what they need to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't as brief as I'd like it, but this is possibly a first pass at a code snippet:

var custom_peer := NetworkedMultiplayerCustom.new()
var real_multiplayer_client

func join_multiplayer():
	custom_peer.set_connection_status(NetworkedMultiplayerPeer.CONNECTION_CONNECTING)
	get_tree().network_peer = custom_peer

	# Start connecting via real transport (ex. Steam peer-to-peer, PlayFab, etc).
	real_multiplayer_client = _connect_to_real_multiplayer()
	real_multiplayer_client.connect("connection_succeeded", self, "_on_real_multiplayer_connection_succeeded")
	real_multiplayer_client.connect("connection_failed", self, "_on_real_multiplayer_connection_failed")
	real_multiplayer_client.connect("peer_connected", self, "_on_real_multiplayer_peer_connected")
	real_multiplayer_client.connect("peer_disconnected", self, "_on_real_multiplayer_peer_disconnected")
	real_multiplayer_client.connect("recieved_message", self, "_on_real_multiplayer_received_message")

	# Intercept any RPC's sent locally.
	custom_peer.connect("packet_generated", self, "_on_custom_peer_packet_generated")

func _on_real_multiplayer_connection_succeeded():
	# Once connection is formed, assign a peer id somehow. If the real transport
	# doesn't have a compatible id, you'll need to generate your own, and ensure
	# that all peers use the same mapping.
	var my_id = real_multiplayer_client.get_my_id()
	custom_peer.initialize(my_id)
	custom_peer.set_connection_status(NetworkedMultiplayerCustom.CONNECTION_CONNECTED)

func _on_real_multiplayer_connection_failed():
	custom_peer.set_connection_status(NetworkedMultiplayerCustom.CONNECTION_DISCONNECTED)

func _on_real_multiplayer_peer_connected(peer_id):
	# Let Godot know a peer connected. Again, you may need to map between Godot ids
	# and the ids used by the real multiplayer client.
	custom_peer.emit_signal("peer_connected", peer_id)

func _on_real_multiplayer_peer_disconnected(peer_id):
	# Same notes as above.
	custom_peer.emit_signal("peer_disconnected", peer_id)

func _on_custom_peer_packet_generated(peer_id, buffer, transfer_mode):
	# Send any packets generated locally (ex RPCs) to the requested peer
	# using the real multiplayer client.
	if transfer_mode == NetworkedMultiplayerPeer.TRANSFER_MODE_RELIABLE:
		real_multiplayer_client.send_reliable(peer_id, {
			'from_peer_id': custom_peer.get_unique_id(),
			'data': buffer,
		})
	else:
		real_multiplayer_client.send_unreliable(peer_id, {
			'from_peer_id': custom_peer.get_unique_id(),
			'data': buffer,
		})

func _on_real_multiplayer_received_message(message):
	# Take any packets we recieved from other peers, and deliver them locally.
	custom_peer.deliver_packet(message['data'], message['from_peer_id'])

@@ -50,8 +51,9 @@
<argument index="1" name="buffer" type="PoolByteArray" />
<argument index="2" name="transfer_mode" type="int" />
<description>
Emitted when the local [MultiplayerAPI] generates a packet.
Emitted when the local [MultiplayerAPI] generates a packet (e.g. when calling [method Node.rpc]).
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this! Developers will be most familiar with RPC's, so this makes it a little clearer what's going on here.

Your script should take this packet and send it to the requested peer over the network (which should call [method deliver_packet] with the data when it's received).
The [code]peer_id[/code] is the [code]target_peer[/code] set by [method NetworkedMultiplayerPeer.set_target_peer].
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like NetworkedMultiplayerPeer.set_target_peer() is an implementation detail of NetworkedMultiplayerPeer, and this won't be very helpful to developers who are only familiar with the GDScript side of things.

I think it'd be better to just note that this is the target peer id (which could be 0 to denote sending to all peers), without mentioning NetworkedMultiplayerPeer.set_target_peer()

Copy link
Author

Choose a reason for hiding this comment

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

You're probably right but since it's a custom class I felt unsure how much that responsibility was on me to implement if that makes sense.

@dsnopek
Copy link
Contributor

dsnopek commented Aug 18, 2022

Closing in favor of #64585

@dsnopek dsnopek closed this Aug 18, 2022
@jarommadsen jarommadsen deleted the patch-1 branch August 19, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants