KhronosGroup / glTF-CSharp-Loader

C# Reference Loader for glTF
Other
219 stars 58 forks source link

Deserializing the .Extras perperties? #18

Closed schubboy closed 3 years ago

schubboy commented 6 years ago

What is the intended method of serializing/deserializing to/from the .Extras objects in the various properties of GLTF? Inherit from your base Extras object? I've been attempting to store some custom properties by doing that and writing the GLTF file works, but when calling LoadModel, the .Extras objects have nothing in them. Is there an example or "best practices" for how you intended this to work?

Perhaps these .Extras properties should have been "dynamic" object types so the Json Serializer could at least give us our information back?

bghgary commented 6 years ago

This isn't properly supported right now. Open to suggestions on how to fix it. Dynamic might work, but I'm not sure how Newtonsoft.Json handles it. Another way is expose the Newtonsoft.Json object directly.

schubboy commented 6 years ago

Inheriting from your base class and creating a custom object seemed to work ok, but doesn't solve getting it back out. I think the generic typing system in c# is a pretty good approach, perhaps using .Extras<T>, which would enable you to supply your own objects/types. Serialization should work fine there - its the de-serialization that gets weird. If you lazy-load - defer the deserialization until first property access, then the serializer would know exactly what type to look for from the 'T' in the method call.

schubboy commented 6 years ago

Just thinking out loud here, but using Generics as your main approach is a more elegant solution that exposing JSON.NET directly as your API surface, since you are then tied directly to a third party solution and their API decisions then directly impact this project, which is not the best idea.

bghgary commented 6 years ago

Inheriting from your base class and creating a custom object seemed to work ok, but doesn't solve getting it back out.

Yeah, we do this in the asset generator project as well for extras.

Generics as your main approach is a more elegant solution

I'm not sure I understand exactly what you mean with this. A PR with this change will help a lot if you can.

schubboy commented 6 years ago

If you define the Materials extras property like this:

public class glTFMaterial<T> {
     public T Extras { get; set; }
}

then it makes it real obvious what you're doing. That would be a very "C#" way to handle this.

The JSON.NET library does this same approach with generics in its ToObject methods, so that would sync up pretty nicely with that API as well.

bghgary commented 6 years ago

I'm not sure how this would work. How do you store these objects under the Gltf object?

courgeon commented 6 years ago

Hello there I have the exact same request. if would be perfectly fine to me to expose Json objects, in a similar way to what is done for material extensions. But i really need the "extra" properties to be accessible somehow. thanks

courgeon commented 6 years ago

So, here is what i did, that is enough for me:

modified glTF/specifications/2.0/schema/extras.schema.json to look like "extensions"

 {

"$schema": "http://json-schema.org/draft-04/schema",

"title": "Extras",

"type": "object",

"description": "Application-specific data.",

 "properties": {

},

"additionalProperties": {

    "type": "object"

}

}

Then, the resulting dll allows me to load extras :

            foreach(var ex in aNode.Extras) {

                    Console.WriteLine("Extra : " + ex.Key + " = " + ex.Value);

            }

Hope it helps, I'm sticking to my modified version for now. I don't know if it's compliant to the original glTF extra spec, but it does match the blender export behavior. Example :

"nodes" : [

    {

        "extras" : {

            "movable" : 1.0

        }, 

        "mesh" : 0, 

        "name" : "Cube", 

        "translation" : [

            0.22430461645126343, 

            -0.12129101902246475, 

            -0.2835808992385864

        ]

    }, 

C

Krakean commented 6 years ago

@courgeon pull request, may be? So guys at glTF would review it and merge if it fine enough

bghgary commented 3 years ago

Closing due to inactivity. Feel free to reopen if there are other ideas.