Razzeeyy / godot-networked-instancing-example

An example for semi-automatic instantiation and replication over network for Godot
MIT License
15 stars 2 forks source link

Set instance name does not works #6

Closed T4g1 closed 5 years ago

T4g1 commented 5 years ago

It seems the "@" added by the engine to generate unique names cannot be replicated as it is. I try to generate two node named "AK47" on the server.

The following modification (added print/debug):

remote func rpc_sync_spawn(filename, node_path, master_id):
    if multiplayer.is_network_server():
        return
    var sender = multiplayer.get_rpc_sender_id()
    if sender == 1:
        print("Sync spawn from ", master_id, " ", node_path)
        var node_name = node_path.get_name(node_path.get_name_count()-1)
        var parent_path = str(node_path).rstrip(node_name)
        var scene = load(filename)
        var instance = scene.instance()
        print(node_name)
        instance.name = node_name
        print(instance.name)
        instance.set_network_master(master_id)
        get_node(parent_path).add_child(instance)

Gives the following:

Sync spawn from 1 /root/SyncRoot/@AK47@67
@AK47@67
AK4767

And of course: DeepinScreenshot_select-area_20191025155212

T4g1 commented 5 years ago

Seems like a godot bug, the workaround is this:

(...)
Network.sync_root.add_child(node)

print(node.name)
node.name = node.name
print(node.name)

Print are non-mandatory of course but its fun to see that this is effectively filtering the arobase out of the name...:

@AK47@67
AK4767
T4g1 commented 5 years ago

Reported: https://github.com/godotengine/godot/issues/33068

Edited by @Razzeeyy: fixed link markdown code

Razzeeyy commented 5 years ago

I believe there is no third-party workaround except explicitly naming things.

Also the library was developed under the assumption that objects (who rely on SyncNodes) should be named explicitly. (Probably I should mention it in the documentation to make it clearer)

Also I believe the suggested workaround even those makes things looks like they works but will fail the RPCs because of node path collisions cause there will be 2 nodes with the exact same path and godot's internal rpc system uses node paths to find the rpc target.

T4g1 commented 5 years ago

Assuming you spawn a bullet: If you explicitly name them, there will be no problem as long as you don't use at signs (this should be documented in the engine if it's not a bug). Afaik, if you don't name them explicitly, godot will always use @<name>@<instance id?> so using node.name = node.name will never create two nodes with the same name as it will simply strip the at sign. Now if you do both (some explicitely named and some automaticaly named) then yes, you'll get clash depending on how you named them.

girng commented 5 years ago

Names need to be explicitly set, if not, Godot will do it for you. Give your names a unique id name = "AK47_%s" % idx or something. This way, when you need to access that node with get_node("AK47_%s" % idx), it will work (I usually do has_node before, just in case)

T4g1 commented 5 years ago

Well yes but that implies keeping track of idx and extra work to make it consistent. Why would you want to do that over node.name = node.name or having a node naming that behaves as expected?

Razzeeyy commented 5 years ago

I did some testing on that node.name = node.name fix and yeah it's pretty good. Although I'm not sure if SyncNode should try to auto fix the names?

On the surface it looks like in most cases that fix should be fine (I even tried the case where node is renamed after it's spawned and it works fine as long as it's renamed on both server and client at the same time), but ultimately that namings can break in many different ways... Who knows how other developers approach naming their stuff. Maybe someone stores the node name for later use and if SyncNode's gonna change that node name it's now introducing some interesting bugs into the application. Therefore I decide that SyncNode's won't attempt to autofix the names...

And it looks like there will always be the need to approach naming networked nodes with care, that's somewhat of a feature of an engine's design.

I think I'll mention that easy workaround in readme for now, thanks for the hint about the workaround! I hope that naming issue will be fixed by godot team in next release...

girng commented 5 years ago

Why would you want to do that over node.name = node.name or having a node naming that behaves as expected?

One thing I've learned over half a decade using Godot, is that developers must adapt to Godot's conventions/quirks, features, etc. If not, it's akin to Donald Knuth's famous quote "premature optimization is the root of all evil", but in this case, it's not optimization, it's circumventing ways around Godot. On the surface / first glance, it may seem like a good thing, however, in the end and grand scheme of things, the less the developer adapts, more time is lost.

T4g1 commented 5 years ago

@Razzeeyy I agree with you, I think spawning multiple entities of the same scene is a pretty common use case in networked games and without knowing that you made the assumption of all nodes being manually named, the issue will probably raise for many others. So yes, either document the assumption like you said or the workaround (or even both, that's up to you).

Side note, mclark4386 suggest a more elegant workaround to deal with this, add_child(node, true): It seems to prevent the engine to use at sign in generated names (I dont find the documentation on that one to be 100% clear on that but my tests seems to validate that hypothesis)

Razzeeyy commented 5 years ago

Side note, mclark4386 suggest a more elegant workaround to deal with this, add_child(node, true): It seems to prevent the engine to use at sign in generated names (I dont find the documentation on that one to be 100% clear on that but my tests seems to validate that hypothesis)

Tested it, yeah good one, adding this one to the workaround tips.

I guess I can close the issue now...