Disallow nested custom multiplayers in SceneTree

[AThousandShips]

Enables clearing the custom multiplayer

Considering further restricting to only absolute paths (the only thing that makes sense imo)

• Fixes: The 'get_multiplayer' method cannot obtain the value obtained by the 'set_multiplayer' method #77817 (resolves the issue raised)

[Faless]

I understand the issue, and I see how this can be confusing, but this PR heavily de-optimize the most common case (having few custom multiplayer instances and deep path names).

You can test this with the following script (full project):

extends Node

const MULTIPLAYERS = 4
const PATH_DEPTH = 10
const CALLS = 10000

var nodes = []

func build():
	for i in range(MULTIPLAYERS):
		var s = Control.new()
		var cur = s
		for j in range(PATH_DEPTH):
			var step = Control.new()
			cur.add_child(step)
			cur = step
		add_child(s)
		nodes.append(cur)
	for i in range(MULTIPLAYERS):
		get_tree().set_multiplayer(SceneMultiplayer.new(), get_child(i).get_path())

func _ready():
	build()
	test()
	var args := OS.get_cmdline_user_args()
	if args.size() and args[0] == "q":
		get_tree().quit.call_deferred()


# Tests a single node, searching for its custom multiplayer parent
func _test_call(node: Node):
	var mp : MultiplayerAPI
	var time = Time.get_ticks_usec()
	for i in range(CALLS):
		mp = node.multiplayer
	return Time.get_ticks_usec() - time


# Tests each node for its custom multiplayer parent
func test():
	var time = Time.get_ticks_usec()
	var avg = 0
	for node in nodes:
		avg += _test_call(node)
	return avg / nodes.size()

Benchmarks for optimized editor builds (using hyperfine)

$ ~/Downloads/hyperfine --warmup 3 --runs 10 '~/godot-a --path ~/Downloads/mp_test/ --headless -- q' '~/godot-b --path ~/Downloads/mp_test/ --headless -- q'
Benchmark 1: ~/godot-a --path ~/Downloads/mp_test/ --headless -- q
  Time (mean ± σ):     161.2 ms ±   9.4 ms    [User: 121.9 ms, System: 16.5 ms]
  Range (min … max):   151.4 ms … 174.4 ms    10 runs
 
Benchmark 2: ~/godot-b --path ~/Downloads/mp_test/ --headless -- q
  Time (mean ± σ):     294.0 ms ±   3.4 ms    [User: 183.3 ms, System: 24.1 ms]
  Range (min … max):   288.6 ms … 300.0 ms    10 runs
 
Summary
  ~/godot-a --path ~/Downloads/mp_test/ --headless -- q ran
    1.82 ± 0.11 times faster than ~/godot-b --path ~/Downloads/mp_test/ --headless -- q

As per the original issue, I think we should simply disallow having nested custom multiplayer APIs.

[AThousandShips]

Gotcha! Will investigate and restructure to disallow nested custom APIs, will see about methodology for testing if already configured for the path on setting

[AThousandShips]

This doesn't handle the case when a custom API is configured for a descendant of the new path, unsure how to deal with that case

[Faless]

This change was approved in the 2023-07-06 networking meeting (subject to further code review).
If someone has good reasons for using nested custom multiplayer implementations a proposal should be open outlining the use case(s).

[Faless]

Sorry it took me a while.
This looks great, I've realized it still allows setting the child first and the parent after it.
But that actually works as expected during retrieval (as we use a HashSet which preserves ordering, and not an RBSet which doesn't).
I think this is okay so if one really really wants nested custom multiplayers, there is a way at least, as hacky as it is.

In case, we should probably update the commit message to something like:

Disallow set_multiplayer if there is one configured for a parent path.

I guess we'll have to re-discuss it this week. Sorry 😿

[AThousandShips]

No worries! And agreed, noticed the same limitation/quirk before but now you're right about that part

I think unless I come up with a clean way to identify and handle those cases I'll leave it as is in that sense, to not complicate the process, but I will consider how to approach it, I think that it's the least chaotic approach to just prevent the strictly nested cases, whereas removing or blocking on older nested (i.e. preventing adding an API to a parent) would be more disruptive

Could for example add a warning if setting a nested path, should be trivial to identify it (i.e. if a path is a prefix of another path), but will ponder

But my suggestion would be to go with this approach, possibly adding a debug build warning about nested cases

[AThousandShips]

In that case I will update the wording in the docs too to something like:
"Will error if an API is already configured for a parent" etc.

Edit: realised that the wording is sufficient, save for "no nested" part, so will reword that in that case

[Faless]

LGTM! Great work 🏆 !

Thanks for bearing with the many back and forth 🙏

[AThousandShips]

Thank you! It's been a great journey figuring out exactly how to do it, complicated thing

[akien-mga]

Thanks!

[AThousandShips]

Thank you!