godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.13k stars 92 forks source link

Implement sending custom structs as RPC parameters #6216

Open TheYellowArchitect opened 1 year ago

TheYellowArchitect commented 1 year ago

Describe the project you are working on

Literally any serious game, which will need custom structs (Resources) made of primitive types

Describe the problem or limitation you are having in your project

Basically, I have custom resources which exist on both server and client, yet the network cannot parse them. Cannot convert from Object to Object even though the type and its variables is obvious on both sides. Look at this code sample from the MRP (slightly tweaked):

class_name DeckData
extends Resource

@export var mainDeck: PackedInt32Array
@export var dungeonDeck: PackedInt32Array
@export var equipmentDeck: PackedInt32Array
func _process(delta: float) -> void:
    if (multiplayer.is_server() == false):
        return  

    if (Input.is_action_just_pressed("ui_accept")):
        var new_card_data = DeckData.new()
        new_card_data.dungeonDeck = PackedInt32Array([0,2,6,8])
        new_card_data.equipmentDeck = PackedInt32Array([2,3,6,10])
        new_card_data.mainDeck = PackedInt32Array([0,1,1,5])
        rpc("receive_deck", new_card_data)

@rpc func receive_deck(received_deck: DeckData) -> void:
    print(received_deck.mainDeck)

Since both server and client have the same resources, encoding/decoding should happen automatically.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Basically makes netcoding structs easier. No manual encoding/decoding. No boilerplate.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

See MRP and code sample above. It should work as it is.

If this enhancement will not be used often, can it be worked around with a few lines of script?

Example for the above sample code

func _process(delta: float) -> void:
    if (multiplayer.is_server() == false):
        return  

    if (Input.is_action_just_pressed("ui_accept")):
        var new_card_data = DeckData.new()
        new_card_data.dungeonDeck = PackedInt32Array([0,2,6,8])
        new_card_data.equipmentDeck = PackedInt32Array([2,3,6,10])
        new_card_data.mainDeck = PackedInt32Array([0,1,1,5])
        rpc("receive_deck", new_card_data.dungeonDeck, new_card_data.equipmentDeck, new_card_data.mainDeck)

@rpc func receive_deck(received_deck1: PackedInt32Array, received_deck2: PackedInt32Array, received_deck3: PackedInt32Array) -> void:
    print(received_deck1)
        print(received_deck2)
        print(received_deck3)

The problem is on nested structs/resources. Nested == using structs/resources inside, instead of purely primitive types (PackedInt32Array on above example).

Is there a reason why this should be core and not an add-on in the asset library?

Convenience, since much of netcoding is automated thus far anyway (e.g. MultiplayerSpawner)

TheYellowArchitect commented 1 year ago

MRP: rpc_invalid.tar.gz rpc_invalid.tar.gz

((If there is a better way to make structs and pass them into RPCs, please share))

Flavelius commented 1 year ago

I find the usage of the word struct problematic, Resources in godot are references and not really lightweight (like actual structs such as Vector2/3/4, Color etc). Which i assume might even be the reason they're not supported by default in this case. I wish godot had support for lightweight custom structs though. If there were value type (/blittable) structs, sending them over the network would be much easier and likely supported by default. Btw. it's not uncommon that custom types sent over the network have custom serialization logic anyway, and godot has a few options for this like var to byte conversion and json serialization

TheYellowArchitect commented 1 year ago

I find the usage of the word struct problematic

I agree, but I cannot find a proper replacement for Structs in Godot. The closest thing is Resources, and that is what I use (as demonstrated in MRP above)

Resources in godot are references and not really lightweight (like actual structs such as Vector2/3/4, Color etc). Which i assume might even be the reason they're not supported by default in this case.

True, they are surely heavy to be transferred, though I think it would be possible to pass them through the network, since their fields are determined at compilation, and they are the same in both server and client

I wish godot had support for lightweight custom structs though.

Me too. But resources do the job at least.

Btw. it's not uncommon that custom types sent over the network have custom serialization logic anyway

When you want to send a custom type over the network, and it consists only of primitive types, there shouldn't be any custom serialization imo. Like, even if you make your own packet manager, an int is always 4 bytes, or if you send an array, you can use the first byte of the packet to get data-type and the array length (first bit could be the indication that its an array)

and json serialization

I haven't tried this, can you send JSON through the network? (though that is bloat and the worse solution of all, I am curious)

Flavelius commented 1 year ago

You're probably using gdscript and the built-in networking, but i would suggest you try c# (and a network library like litenetlib), it'll atleast enable you to get everything you intend with this proposal (my multiplayer project has all communication based on actual structs and it's very lightweight, fast and bandwidth efficient). Json, as you correctly mention is not very efficient, but it atleast makes it possible for you to serialize your resources in one go. If you want to keep using godot internal functionality, i would suggest just packing everything into a byte array (https://docs.godotengine.org/en/stable/tutorials/io/binary_serialization_api.html) and unpacking it on the other end with a resource instance that you just recreate, it's likely the closest to what you intend. There look to be methods in packetpeer that even do this for you by serializing whole objects https://docs.godotengine.org/en/stable/classes/class_packetpeer.html#class-packetpeer you should probably even try put_var(..) with full_objects=true, maybe it does what you want without the ceremony (not sure though, haven't used any of the built-in networking yet)

Dragoncraft89 commented 1 year ago

TL;DR: This proposal would benefit from the ability to add classes that only have properties (no functions). These could be sent over the network without issue. Everything else is just a security nightmare

@Flavelius The approach with binary serialization only works with full_objects=true for custom classes and this is a security nightmare, as a malicious client could send any code attached to the object.

This is instant code execution on your server / the client(s). And you cannot even do something against it, because as soon as the object is deserialized, code could potentially be executed. This is the ultimate hacker dream.

What one would actually want is serialization of (only) the class members. Maybe a custom annotation could help here, e.g.:

@serializable
class Data:
  var foo: String

@rpc
func request(data: Data):
  pass # do stuff

Such a class should not support any script properties, unlike Object, so that using it as a rpc argument is not a security nightmare. Furthermore, such a class should not have a constructor, because running code automatically during deserialization is a good way to shoot yourself in the foot when it can be done over the network by any client.

Having any other kind of functions should be fine from a security standpoint, but not really necessary. If you need any kind of functionality from this class, then this can be easily achieved by composition without too much boilerplate code. Borrowing from the deck building example:

@serializable
class NetworkCardDeck:
  var mainDeck: Array[int]
  var dungeonDeck: Array[int]
  var equipmentDeck: Array[int]

class CardDeck:
  var inner: NetworkCardDeck

  func _init(deck: NetworkCardDeck):
    self.inner = deck

  func check_valid():
    return len(inner.mainDeck) > 10

@rpc
func select_deck(data: NetworkCardDeck):
  var deck = CardDeck.new(data)
  if deck.check_valid():
    pass # your stuff here

What is especially nice about this proposal is that all the burden of deserializing and checking can be offloaded to the engine. Checking client input is the most annoying part of it, because any missing check could result in a bug or security issue.

Having this built into the engine would make networking in Godot way more pleasant to use, especially since runtime type checking of variables is not even possible AFAIK, whereas the engine does type checking of variables all the time already

Flavelius commented 1 year ago

The approach with binary serialization only works with full_objects=true

You're mixing two parts of my answer here. You can binary serialize whatever is supported by the api. Only if you don't (want to) care how things you throw into it are serialized you'd use put_var with full objects, but then the docs clearly educate about the caveats. But just serializing your class fields into a provided buffer is as secure as you want it to be. It's also what litenetlib, that i mentioned, encourages as pattern (using the INetSerializable interface). This also gives you full control over what gets de/serialized. It's slightly more verbose than automagic (attributes/annotations), but if that's needed, there's c#/.net with everything already solved.

TheYellowArchitect commented 1 year ago

The main problem is when you have Resources which contain Resources (nested resources), and send them over the network. Writing the serializer/deserializer is very time-consuming, and should be done by the engine as explained in the OP.

But as @Dragoncraft89 explained, if this suggestion was implemented, it would backfire unless there is a special kind of Object which stores exclusively member variables (basically oldschool C structs)

Daylily-Zeleen commented 1 year ago

In this case (auto serialize/deserializ pure data Object), I recommend you to create utility methods like this:

static func serialize(obj: Object) -> PackedByteArray:
    # Maybe some valid check here === start
    # For example, if "obj" is a node, we should not serialize a not data object,
    # and push a error here for debugging.

    var buffer:= StreamPeerBuffer.new()

    # Serialize construt message.
    var script_objtct : bool
    var create_msg : String
    if is_instance_valid(obj.get_script()):
        script_objtct = true
        create_msg = (obj.get_script() as Resource).resource_path
    else:
        script_objtct = false
        create_msg = obj.get_class()
    # Here use a very stupid but simple way to serialize data,
    # your should use a better way to do this.
    buffer.put_var(script_objtct)
    buffer.put_var(create_msg)

    for prop in obj.get_property_list():
        if prop.type == TYPE_NIL:
            # In this implement, all properties which want to be serizlized should be specified its type.
            continue
        # Some rules to filter properties.
        if prop.usage == PROPERTY_USAGE_STORAGE and\
                prop.name.begins_with("sync_"): 
            # Here I only serialize properties which begins with "sync_", so trim this prefix
            buffer.put_var(prop.name.trim_prefix("sync_"))

            var value = obj.get(prop.name)
            if value is Object:
                buffer.put_var(serialize(value))
            elif typeof(value) in [TYPE_ARRAY, TYPE_DICTIONARY]:
                # Because Array and Dictionary maybe contain objects, here should do mo work to realze.
                # But here just a demostrate, let's assume they are not contain object.
                buffer.put_var(var_to_bytes(value))
            else:
                buffer.put_var(value)

    return buffer.data_array

static func desrizalize(bytes: PackedByteArray) -> Object:
    var buffer:= StreamPeerBuffer.new()
    buffer.data_array = bytes
    var obj: Object
    if buffer.get_var():
        obj = load(buffer.get_var()).new()
    else:
        obj = ClassDB.instantiate(buffer.get_var())

    var prop_types := {} 
    for prop in obj.get_property_list():
        prop_types[prop.name] = prop.type

    while buffer.get_available_bytes() > 0:
        var prop_name = "sync_" + buffer.get_var()
        match prop_types[prop_name]:
            TYPE_OBJECT:
                obj.set(prop_name, desrizalize(buffer.get_var()))
            TYPE_ARRAY, TYPE_DICTIONARY:
                # Refer to "serialize()"
                obj.set(prop_name, buffer.get_var())
            _:
                assert(prop_types[prop_name] != TYPE_NIL)
                obj.set(prop_name, buffer.get_var())
    return obj

Then pass a serialized object PackedByteArray to rpc method and deserialize it in rpc method.

TheYellowArchitect commented 1 year ago

Related issue/proposal: https://github.com/godotengine/godot-proposals/issues/4925

maximkulkin commented 1 year ago

+1 for this. It is kind of awkward thing that you can create a function with parameters of any type and add @rpc annotation to it, but you can't actually send data, because default serializer just can't handle it. Writing custom serialize/deserialize static methods and actually accepting PackedByteArray. Although this will work, it requires a lot of boilerplate. IMO, it would be easier if internally serializer/deserializer FOR NETWORKING specifically would check if given type has certain methods and will call them to serialize/deserialize. E.g.

class PlayerState:
  var health: int
  var ammo: int

  func _rpc_serialize(stream: StreamPeerBuffer) -> void:
    stream.put_u32(health)
    stream.put_u32(ammo)

  func _rpc_deserialize(stream: StreamPeerBuffer) -> void:
    health = stream.get_u32()
    ammo = stream.get_u32()

@rpc
func report_player_state(state: PlayerState) -> void:
  pass

IMO, such interface makes it more efficient for serializing/deserializing complex data (instead of returning PackedByteArray and then concatenating them together, one can just pass same StreamPeerBuffer around, reading/writing from/to it. This serialization is specific to RPC, so it won't affect other subsystems (compared to modifying how various types serialize by a default var2bytes() method). Implementations will usually be pretty compact. The serialization interface is duck-typed, you do not need to subclass anything to implement it. Both serialization and deserialization works on the object itself (no static factories etc), on serialization you just pass an object and it calls it's method. On deserialization it first creates and object (with a default constructor, then calls _rpc_deserialize() method to populate fields from a stream). The API is symmetrical and simple.

Disadvantages: