MultiplayerSynchronizer doesn't update Texture property

[henriiquecampos]

Godot version

4.0 official

System information

SolusOS

Issue description

When setting up the MultiplayerSynchronizer to update TextureRect:Texture or Sprite2D:Texture it fails and may even cause issues.

When testing with two MultiplayerSynchronizers, one for a Label:text and one for a TextureRect:Texture it overwrote the settings IN THE EDITOR that was veeeery weird, I'm going to report this in another issue. You can see the tests in the video below:

https://youtu.be/JOtoQWPdjRU

Steps to reproduce

1. Setup a server/client using ENetMultiplayerPeer
2. Create a scene that is dynamically spawned across peers using the MultiplayerSpawner
3. When spawning the above scene, change its texture
4. Make sure the above scene has a MultiplayerSynchronizer trying to sync the "Texture" property

It's complex, but that's the gist of it.

Minimal reproduction project

1. Toggle at least 2 Debug Instances in the Debug Menu
2. Open the 03.making-lobby-to-gather-players/MainMenu.tscn scene
3. On one instance, press the "Server" button
4. On another instance, press the "Client" button
5. Look at the Server instance as it has a scene with a different texture than the Client instance
6. Look at the Client instance and notice it has none or the default texture

multiplayer-synchronizer-bug.zip

[AThousandShips]

I don't know the details but I'm not sure the multiplier system can be expected to send texture data like that, it can't just figure out that you loaded the file, but I might be mistaken

[henriiquecampos]

I don't know the details but I'm not sure the multiplier system can be expected to send texture data like that, it can't just figure out that you loaded the file, but I might be mistaken

Maybe we could filter the properties selection then to only allow "allowed" properties?

[AThousandShips]

I don't know if it's intended, a bug, or a limitation, I'd defer to someone more knowledgeable with the internals of multiplayer, just guessed on a possible reason

[AThousandShips]

Cc @Faless maybe

[AThousandShips]

Are there any errors or warnings in the debugger?

[henriiquecampos]

Are there any errors or warnings in the debugger?

No errors on the debugger, no :/

[Faless]

Synchronizing "object type properties" (like resources and node references) is indeed not supported, we should add an error or warning in the debugger (and possibly the editor) to clarify that.

A texture is also pretty big in the context of network packet size, so you should not use a synchronizer anyway (even if it was supported, the "sync" would have thrown an error about the packet size and dropped the state).

Using an RPC to send the texture data (get_image().get_data()) might work assuming the avatar is small enough (e.g. 64x64) but keep in mind the Multiplayer API is not suited to transfer assets, which should be done using dedicated protocols like HTTP instead.

[AThousandShips]

Yes digging through the code I noticed the error and was confused at first why it didn't show up but I assume it's because the property is just encoded as an object id and not as actual data

It seems to fail silently when decoding objects, there is an option in the API to encode objects fully but not used, which makes sense, but there should be checks there for data that obviously can't be managed correctly as objects aren't going to have matching IDs

[AThousandShips]

Looking at some ideas for documenting and fixing this

[Faless]

Looking at some ideas for documenting and fixing this

I think we should see if we can test the properties of a given node in the editor (like we do with the type to show the icon) and add a warning sign for that property if it's of type Variant::OBJECT.

We could possibly also go the hardcore route and add a test_replication_config to MultiplayerSynchronizer which test the encoding for encoded objects and we only call in tool builds. Though this seems a bit to much for me and I'd lean towards the editor-only solution (which I known, still leave the issue for not @exported variables in scripts but it would still be a great UX improvement).

Maybe though there's a third and better way?

[AThousandShips]

Part of the solution would be to restrict the types in the property selector in the editor, but yeah keeping the silent failure doesn't seem like too big a problem.

Also Variant::RID should probably be excluded as well at least in the editor.

Will look at some ideas for editor-only checks, but maybe I'll just start by adding a basic editor-only solution restricting the types that can be selected and adding to documentation? Got some basic things working already.

[AThousandShips]

My working approach for the moment:

• Restrict properties in the editor picker, preventing accidentally picking the wrong types
• Add to documentation

To investigate:

• Checking properties in the editor and warning on unsupported types
• Warn on types that are not guaranteed to work (like arrays and dictionaries), not sure about this one

[henriiquecampos]

Alright, thanks a lot for the fast replies and clarifications! @AThousandShips the current approach sounds good. Hiding properties that can't be synced would be ideal. Maybe filtering anything that inherits from Object could work.

Could you guys help me with a workaround for that? I'm thinking about turning the add_avatar() method into an @rpc function so clients create their own AvatarCard based on data provided to the server from other clients.

It's not necessary to pass the whole Texture through the network as each client will have all the files necessary to build other players' avatars. They just need the instructions.

[AThousandShips]

PR open #74443

[tateorrtot]

Why not sync the path of the texture? That way we just pass a string over the network instead of a texture.

[AThousandShips]

Because a texture isn't guaranteed to have a path, this shouldn't be done automatically

Further this kind of property isn't really something that makes sense being synced like this, it'd be better to use RPC to send that information to a dedicated function that loads the texture

Syncing is for properties that change rapidly, or unpredictably, neither of which applies here

[ariefield]

I am working on a multiplayer project in which I am dynamically adjusting hitboxes (RectangleShape2D), and I want to sync this data across every client. This didn't appear to be working, at which point I stumbled across this thread. I wouldn't expect a shape's size (2 floats) to be too much data to sync, so I'm curious about why this isn't feasible.

Is the only way to achieve this behaviour just updating the shapes directly with an RPC?

[AThousandShips]

It isn't about the size, it's about the type, a shape isn't simple data, it's a resource, can you try syncing the actual property on the shape instead of the shape itself?

Otherwise you should use an RPC, resources are shared so they are unlikely to be efficient in synchronizing, and objects can't be synchronized because they're not simple data, like a Vector2, they are a whole object that has references etc.

[ariefield]

I see, to clarify I was attempting to sync the size property on the shape of a CollisionShape2D:

disabled and position sync properly without errors, but I get the following error when attempting to sync the shape's size:

E 0:00:03:0082   get_state: Property 'Hitbox/CollisionShape2D:shape:size' not found.
  <C++ Error>    Condition "!valid" is true. Returning: ERR_INVALID_DATA
  <C++ Source>   modules/multiplayer/multiplayer_synchronizer.cpp:162 @ get_state()

I validated that this is the correct path, and even compared to the path stored in an Animation player which modifies this same property (Hitbox/CollisionShape2D:shape:size) successfully.

[Faless]

This is a different issue than the OP. The problem is that CollisionShape2D is a "fake" node, that doesn't exists at runtime, which is why it doesn't work. I expect the same to happen when trying to modify it with an animation player.

You probably want to use a custom property, which uses setter/getter to operate on the shape, and sync those, e.g.:

extends PhysicsBody2D

var shape_size : Vector2 :
  set(value):
    var shape := shape_owner_get_shape(0, 0)
    shape.size = value

  get:
    var shape := shape_owner_get_shape(0, 0)
    return shape.size

[ariefield]

Yes, sorry to hijack the original topic - it seemed at least semi-related and I wasn't sure where else to post.

Interesting what you're saying about CollisionShape2D - I actually am able to modify it properly in an AnimationPlayer though. A difference between the two though is that I'm able to add CollisionShape2D:shape:size in the animation player by keyframing it from the Inspector tab, whereas in the MultiplayerSynchronizer, I have to specify the path directly.

It still seems a bit odd to me (from a UX perspective) that the exact same path (Hitbox/CollisionShape2D:shape:size in my case) works in an AnimationPlayer, but not in a MultiplayerSynchronizer.

Thanks for your recommendation about using a custom getter/setter on the property - it works perfectly. I'm using C#, so I'll post the C# equivalent I'm using in case anyone stumbles across this thread similar to me:

[Export]
    public Vector2 ShapeSize
    {
        get { return ((RectangleShape2D)ShapeOwnerGetShape(0, 0)).Size; }
        set { ((RectangleShape2D)ShapeOwnerGetShape(0, 0)).Size = value; }
    }