Multiple MultiplayerSynchronizer's under MultiplayerSpawner with different public_visibility states prints error

[DragonAxe]

Godot version

v4.0.beta.custom_build [f814e15] (built after alpha 4)

System information

Linux 5.15.76-1-MANJARO x86_64 GNU/Linux --- Vulkan API 1.2.0 - Using Vulkan Device #1: Intel - Intel(R) UHD Graphics 620 (KBL GT2)

Issue description

I am setting up an authoritative server/client real-time strategy game using MultiplayerSpawner and MultiplayerSynchronizer. My player scene has several properties that need synchronized to the clients. My player scene also has a 'module' system for easily bolting on functionality to a player. These player modules also have several properties that need synchronized with clients. My first thought was to add a MultiplayerSynchronizer to the player module, so I wouldn't have to pass a reference to the player's synchronizer into the module. That's when I came across these errors.

With a player structure as shown in the images below.

These errors are printed to Godot's console after a client connects:

E 0:00:00:0483   get_node: Node not found: "World/MultiplayerSpawner/Player/Nested/PublicVisibilityOn" (relative to "/root/Main/Client").
  <C++ Error>    Method/function failed. Returning: nullptr
  <C++ Source>   scene/main/node.cpp:1360 @ get_node()

E 0:00:00:0483   process_simplify_path: Condition "node == nullptr" is true.
  <C++ Source>   modules/multiplayer/scene_cache_interface.cpp:77 @ process_simplify_path()

Also, when swapping out the "Nested" node with a "Gun" node containing a MultiplayerSpawner which spawns bullets containing a MultiplayerSynchronizer, I get the errors below.

E 0:00:00:0450   get_node: Node not found: "World/MultiplayerSpawner/Player/Gun/BulletSpawner" (relative to "/root/Main/Client").
  <C++ Error>    Method/function failed. Returning: nullptr
  <C++ Source>   scene/main/node.cpp:1360 @ get_node()

E 0:00:00:0450   process_simplify_path: Condition "node == nullptr" is true.
  <C++ Source>   modules/multiplayer/scene_cache_interface.cpp:77 @ process_simplify_path()

E 0:00:00:0450   get_cached_object: ID 1 not found in cache of peer 1.
  <C++ Error>    Condition "!F" is true. Returning: nullptr
  <C++ Source>   modules/multiplayer/scene_cache_interface.cpp:249 @ get_cached_object()

E 0:00:00:0450   on_spawn_receive: Condition "!spawner" is true. Returning: ERR_DOES_NOT_EXIST
  <C++ Source>   modules/multiplayer/scene_replication_interface.cpp:484 @ on_spawn_receive()

I get the feeling that I am using MultiplayerSpawner and MultiplayerSynchronizer incorrectly. That I should not nest MultiplayerSpawners and not have more than one MultiplayerSynchronizer in a spawned scene.

Perhaps the error messages could be improved to help users better understand what they are doing wrong? Or add documentation to suggest the intended node structure usage?

Steps to reproduce

See minimal reproduction project for details, but here are screenshots of the final remote node structure after running the attached demos.

Final running scene tree:

Final running scene tree with nested bullet spawner:

Minimal reproduction project

problem_demo_multiplayer_spawn_sync_gd4.zip

problem_demo_multiplayer_spawn_sync_gd4_bullet_spawner.zip

[clayjohn]

This isn't really the place to ask for user support. You may have better luck asking around one of the community channels for help narrowing down whether this is an engine bug or you are using the engine incorrectly.

[RafaelVidaurre]

Dis you end up figuring out the issue @DragonAxe? I'm stuck with this too

[DragonAxe]

No unfortunately.

[ismailivanov]

No unfortunately.

Im also having the same problem.

[zibetnu]

I found a workaround, tested to work with Godot 4.2.1.

1. Add another synchronizer like this:
   

2. Add any property to the synchronizer (yes, it doesn't seem to matter what you pick):
   

3. Done! The public_visibility settings will now work as expected:

2024-01-28.10-21-03.mp4

You don't actually need to sync the property you added. This still works:

You can even remove the property completely:

2024-01-28.10-29-29.mp4

This last part makes me confident that there's a bug somewhere. I don't see any good reason why adding and then removing a property should end up with behavior different from not adding the property at all. See #68508 (comment)

[graydoubt]

If I'm interpreting this correctly, the observed behavior works as intended.

In @DragonAxe's example, the PublicVisibilityOff MultiplayerSynchronizer also drives the MultiplayerSpawner behavior to not replicate the respective nodes to the client. As a result, the nested PublicVisibilityOn MultiplayerSynchronizer node isn't present on the client when the server attempts to sync values, hence the "Node not found" error message.

@zibetnu's workaround of having a second DummySynchronizer forces the MultiplayerSpawner to replicate the node, which addresses the issue of the nested node not being present on the client, but this goes against the design intent of the MultiplayerSynchronizer and MultiplayerSpawner's collective behavior of not replicating nodes that aren't visible to a specific peer.

I believe @clayjohn is correct in suggesting that this node setup is an incorrect engine use.

[zibetnu]

(Hidden because it's not directly related to this discussion.)

This last part makes me confident that there's a bug somewhere. I don't see any good reason why adding and then removing a property should end up with behavior different from not adding the property at all.

I went back to this and found that the issue is a lack of GUI feedback instead of a bug. Here is how a user could encounter it and why it is a problem:

1. User adds a MultiplayerSynchronizer to the scene.

[node name="MultiplayerSynchronizer" type="MultiplayerSynchronizer" parent="."]

2. User adds a property to be replicated.

[sub_resource type="SceneReplicationConfig" id="SceneReplicationConfig_0mb4t"]
properties/0/path = NodePath(".:property")
properties/0/spawn = true
properties/0/replication_mode = 1

[node name="MultiplayerSynchronizer" type="MultiplayerSynchronizer" parent="."]
replication_config = SubResource("SceneReplicationConfig_0mb4t")

3. User removes all properties from the SceneReplicationConfig.

[sub_resource type="SceneReplicationConfig" id="SceneReplicationConfig_0mb4t"]

[node name="MultiplayerSynchronizer" type="MultiplayerSynchronizer" parent="."]
replication_config = SubResource("SceneReplicationConfig_0mb4t")

The GUI at the end of part 3 is identical to the GUI at the end of part 1, meaning a user could reasonably conclude that the two are equivalent. However, an empty SceneReplicationConfig is not treated the same way as the lack of one (see my workaround for an example).

I think a good solution would be to export replication_config, similar to how libraries is exported in AnimationPlayer. This way the user can tell something is different without needing to look at the scene file.

[zibetnu]

I believe @clayjohn is correct in suggesting that this node setup is an incorrect engine use.

I'm curious about the correct way to handle the nested synchronizers issue, so I've asked for advice in a forum post:
https://forum.godotengine.org/t/nested-multiplayersynchronizers-with-differing-public-visibilities/59220?u=zibetnu

[graydoubt]

I'm curious about the correct way to handle the nested synchronizers issue, so I've asked for advice in a forum post: https://forum.godotengine.org/t/nested-multiplayersynchronizers-with-differing-public-visibilities/59220?u=zibetnu

In @DragonAxe's example, it should spawn bullets as siblings to the player, not as child nodes of the gun, so that bullets aren't dependent on the player's visibility.

I have a nested multiplayer synchronizer setup where the player character visibility is limited by range, and characters can have equipment attachments (tools and weapons) that each have their own synchronizers (because they can be swapped out). If a character leaves the range of another character, the nested equipment scenes also need to have their visibility disabled for the respective client.

The way I achieve that is by using a separate "visibility synchronizer" node that monitors the visibility of the main synchronizer and applies it to any secondary synchronizers to ensure the settings are consistent between them. While that functionally works great, it does result in an error message when a character leaves the visibility range while having an item equipped:

E 0:00:24:0411   on_despawn_receive: Condition "!pinfo.recv_nodes.has(net_id)" is true. Returning: ERR_UNAUTHORIZED
  <C++ Source>   modules/multiplayer/scene_replication_interface.cpp:663 @ on_despawn_receive()

The challenge is that the synchronizers' visibility setting handles two aspects: whether properties are synced, and whether the scene managed by the corresponding MultiplayerSpawner is visible (or exists, rather) on the client. This results in a potential sequencing issue: If the nested node spawn/sync logic is handled after the parent's node logic, that nested node no longer exists on the client, and the setting can't be applied, hence the error. It should be safe to ignore, but it feels "unclean".

Off the top of my head, I think solving that may require separating spawner and synchronizers, to allow developers to control whether visibility settings should drive spawner behavior. Then I can tell the parent MultiplayerSpawner not to replicate the scene to the client, and tell the parent and nested synchronizers to stop syncing properties. That way the nested MultiplayerSpawner doesn't try to remove a node that's already gone. But that probably messes with the nested spawner's internal state of tracking what scene each client has. Another approach may be that MultiplayerSpawners must internally be aware of any nested spawners, so they don't step on each other. That's probably a larger architectural discussion, though.

Be aware of the MultiplayerSynchronizer visibility regression in 4.2.2 (per #90908), which could result in a wild goose chase if you're testing visibility-related things. It's still present in 4.3-dev6. For now, 4.2.1 is safer when experimenting with visibility syncing.

[nicobatty]

I may have discovered the solution to the problem of nested spawner visibility (working on 4.3.stable). It does fix the error you mentioned @graydoubt.

The trick is to make sure that the child synchronizer visibility is set to True after its parent synchronizer and set to False before its parent synchronizer.

So this end up looking to something like this which is thankfully not too difficult or hacky (can be implemented properly with a proxy pattern):

func add_visibility_for(peer: int) -> void:
    multiplayer_synchronizer.set_visibility_for(peer, true) # Spawn the parent first
    for item in items.get_children(): # Then the items
		item.multiplayer_synchronizer.set_visibility_for(peer, true)

func remove_visibility_for(peer: int) -> void:
    for item in items.get_children(): # Despawn the items first
		item.multiplayer_synchronizer.set_visibility_for(peer, false)
    multiplayer_synchronizer.set_visibility_for(peer, false) # Then the parent

Regarding how it should behave by default, I guess child synchronizer should probably be aware of their parent synchronizer visibility to avoid this sort of confusion.

Hope that helps someone!

[graydoubt]

Thanks, @nicobatty! I went with a similar solution. My original approach was to hook a listener to the primary MultiplayerSynchronizer and apply the visibility changes to any secondary synchronizers. Unfortunately, that results in a sequencing issue I described way back when.

The proxy pattern approach works better. The downside is that the developer experience suffers a bit since you can't use @export to wire up the proxy in place of the synchronizer while maintaining type hinting. That could be solved with interfaces, ... alas. :)

[pennyloafers]

Im finally getting around to using sync visibility in 4.4.1, and i think this is still an open issue for nested spawner nodes. The inner workings of scene replication should be able to handle this, or the documentation should specify nested spawners will not be aware of parent visibility.

I do think in some fashion that this is an implicit conclusion of this statement in the docs.

Since a nested publicly synchronized root node, spawned by a child spawner, is not watched by the parents spawner, the child spawner will attempt a spawn to a broken path on the client.

As far as im aware, a new client will receive spawn packets in the correct order for a fully public tree, there must be a graph of some sort within the scene replication.

If there are two synchronizers in a scene, one public and one private, this should cause a spawn packet. Also because we can trust that the NodePaths are unique they can be leveraged to maintaining a visibility graph of nested scenes and how things should replicate.