ayumax / ObjectDeliverer

ObjectDeliverer is a data transmission / reception library for Unreal Engine (C ++, Blueprint).
https://www.unrealengine.com/marketplace/ja/slug/objectdeliverer
MIT License
163 stars 37 forks source link

UObject dynamic class serialization WIP #29

Closed Whiz652 closed 4 years ago

Whiz652 commented 4 years ago

Hello, I was thinking of how to improve my saving system with built-in serialization and after a few days of looking through UE4 serialization and archive code found out that part of your plugin does a big part of the job needed. Although, I see some improvements to be made and I want to contribute code that I came up with.

[ NOT TESTED ] [ BUILT WITH UE4.23.1 ]

Feature to support UObjects with dynamic classes added to Utils/ObjectJsonSerializer

Before: Top level UObject class was set with TargetClass Nested UObject classes were set with higher level UObject/UStruct UObjectProperty->PropertyClass Issue: If serialized UObject inherited from UObjectProperty->PropertyClass and all subclass-specific information will be lost on de-serialization. This means UObjectProperty->PropertyClass UObject will be created, serialized subclass properties will be ignored.

Now: UObject class name is serialized. UObject of serialized class should be created. No serialized properties should be ignored.

Please, tell if this feature is out-of-design and test it.

Whiz652 commented 4 years ago

Had a little test.

JSON from UObject

{
    "Class": "Object_TEST_C",
    "Properties":
    {
        "UberGraphFrame":
        {
        },
        "bool_TEST": true,
        "byte_TEST": 1,
        "int32_TEST": 2,
        "int64_TEST": 3,
        "float_TEST": 4,
        "name_TEST": "5",
        "string_TEST": "6",
        "text_TEST": "7",
        "vector_TEST":
        {
            "x": 8,
            "y": 9,
            "z": 10
        },
        "rotator_TEST":
        {
            "pitch": 12,
            "yaw": 13,
            "roll": 11
        },
        "transform_TEST":
        {
            "rotation":
            {
                "x": -0.11845193803310394,
                "y": -0.17668959498405457,
                "z": 0.13841962814331055,
                "w": 0.96725904941558838
            },
            "translation":
            {
                "x": 14,
                "y": 15,
                "z": 16
            },
            "scale3D":
            {
                "x": 20,
                "y": 21,
                "z": 22
            }
        },
        "subObjectParent_TEST":
        {
            "Class": "subObjectChild_TEST_C",
            "Properties":
            {
                "UberGraphFrame":
                {
                },
                "string_TEST": "I'm a little kiddo!"
            }
        }
    }
}

UObject from JSON CreatedObject

ayumax commented 4 years ago

Thank you for a good suggestion. That ’s a great idea. Give me some time to review.

ayumax commented 4 years ago

I think it's a great implementation. However, the format of the json string has been changed and is not compatible with the old one. This may be a problem for those using existing plugins.

Do you have any other good ideas for introducing a new implementation while keeping the old compatibility?

Whiz652 commented 4 years ago

Do you have any other good ideas for introducing a new implementation while keeping the old compatibility?

I guess we can change 'Class' key to some ridiculous string (perfectly, something that couldn't be used as UProperty name) and put it with 'Properties', without separating 'Class' and 'Properties' in different JSONValues. But I don't know much about UProperty name limitations. Looks like UPROPERTY DisplayName allows pretty mush any name?

ayumax commented 4 years ago

I thought for a while. I felt it was difficult to change the serialization rule of "ObjectDeliveryBoxUsingJson" to this new method.

This is because ObjectDeliverer wants to handle communications with other than UE4, so it doesn't want to change the Json string format as much as possible.

However, since it seems to be a very convenient method for communication using the same plug-in of UE4, let's create a new "DynamicObjectDeliveryBoxUsingJson".

Keep the UObjectJsonSerializer implementation logic as before.

Implement this new serialization implementation (including changes to the UObjectJsonSerializer implementation) in "DynamicObjectDeliveryBoxUsingJson.cpp".

If necessary, use the UObjectJsonSerializer class for serializing non-UObject properties. The UObjectJsonSerializer method is private and can be changed to public.

Is the above change possible?

(To facilitate serialization customization, I will consider changing the UObjectJsonSerializer in a future version)

Whiz652 commented 4 years ago

Is the above change possible?

It is possible, but it will create unnecessary complexity in your plugin code.

The UObjectJsonSerializer method is private and can be changed to public.

You would probably want to make it non-static class (or even make an interface) and inherit UDynamicObjectJsonSerializer from it. The same goes for DynamicObjectDeliveryBoxUsingJson - unnecessary complexity.

In my opinion, this feature could be considered as a bug fix, so that format change could be reasonable as object class information is the major metadata to keep track of.

Just creating DynamicObjectDeliveryBoxUsingJson is not reasonable. For example, UE4 has some similar problems with FArchive and FArchiveUObject as UObject serialization is ambigous and they can not support UObject serialization in the base class. It is also the case why they have call back function in FJsonObjectConverter::UStructToJsonObject, strange things go in FJsonObjectConverter::JsonObjectStringToUStruct as they don't have call back function here to meet serialization requirements of FJsonObjectConverter::UStructToJsonObject.

In our case, we don't really have ambiguity but an issue with serializer invertibility. In terms of inversive operators (that serialized should obviously be) my changes make this: 'UObject to JSON' serialization was hierarchical and dynamic (only in terms of properties). 'JSON to UObject' was hierarchical but static (in terms of properties and class) The fix makes them inverse operators of each other and dynamic in terms of properties and object classes. The other way to make serializer inversive again is to make these methods static in terms of properties and in terms of class.

My point: The only reasonable way to create DynamicObjectJsonSerializer and DynamicObjectDeliveryBoxUsingJson is to also create StaticObjectJsonSerializer and StaticObjectDeliveryBoxUsingJson that will require target class and will cut off all subclass properties and will not serialize class information. This will be correct in terms of serialization as inversive operator.

ayumax commented 4 years ago

Thank you for the helpful opinions. Certainly, as you say, it is not a good idea to behave differently when saving and reading. It can be convinced that it is a problem as a serializer. Your opinion is not wrong.

However, ObjectDeliverer's main purpose is to communicate, so when paying attention to the amount of traffic, it may be felt that the fact that all objects contain type information is an adverse effect. This is especially true if you have an array of objects as a property.

Therefore, at this stage, it is not possible to judge accepting your proposal as it is.

The best thing is that users can customize it as needed.

Thanks to this discussion, I realized that the most important thing to do was remodeling UObjectJsonSerializer

It will take some time, but I would like to replace this class with a customizable structure.

After that, I would like to modify ObjectDeliveryBoxUsingJson.

I'm really sorry, but please let me withdraw this pull request.

After finishing the remodeling of UObjectJsonSerializer, I want to re-implement it as it should be.

Thanks to your opinion, ObjectDeliverer is likely to be even better. Thank you very much.