vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
457 stars 72 forks source link

Custom extensions? #93

Closed emackey closed 3 years ago

emackey commented 3 years ago

Hi, this is a wonderful project, thank you!

Is there any way I can add my own glTF extensions from a client app, without modifying the library itself?

For example if I wanted a particular node to have an extension AGI_testing with a value "itWorks" : true in my exported glTF, can I do that without modifying SharpGLTF? Or what's the smallest mod I would need to make?

vpenades commented 3 years ago

It's something I have planned, but I've never tested it. Mostly because extensions usually imply changing things in the code.... and data only extensions can easily be simulated with the extras.

Nevertherless, the way to make the library aware of an extension is by registering it here.

Notice the ExtensionsFactory class is still internal, so you'll have to call it by reflection, or fork the library and make it public.

The extension classes are usually generated from the schemas using the builder tool in the project, but it's also possible to write a class by hand.

About the class itself, I would suggest you to look into the KHR_materials_unlit class, which is the closest thing you can look for.

You have to remember to inherit from ExtraProperties class, add the appropiate contructor (even if it's empty, it's called by the deserializer), and add the appropiate read/write serialization methods.

Assuming you've been able to do all that, you can just call UseExtension() on the object you want it to use the given extension.

emackey commented 3 years ago

Thanks for all this detail. I think I have it mostly set up, but I keep getting an exception:

System.MissingMethodException: 'Constructor on type 'SharpGLTF.Schema2.NodeAgiArticulations' not found.'

https://github.com/vpenades/SharpGLTF/blob/2681603bdf70bb9d64746c1085d88b28a090e76c/src/SharpGLTF.Core/Schema2/gltf.ExtensionsFactory.cs#L71-L78

But I have that constructor:

namespace SharpGLTF.Schema2
{
    public sealed partial class NodeAgiArticulations
    {
        public NodeAgiArticulations(Node node) { }

        public Boolean? IsAttachPoint
        {
            get => _isAttachPoint;
            set => _isAttachPoint = value;
        }

You'll notice it's partial, actually I have the code generator hooked up to the AGI_articulations schema now, and it generates the other part (derived from ExtraProperties as you mention).

I'm not sure if it matters, but I'm trying to add this extension after the Model is built:

            var model = scene.ToGltf2();
            var node = model.LogicalNodes[0];
            node.SetExtension(new NodeAgiArticulations(node)
            {
                IsAttachPoint = true,
            });

I don't know if I can edit the node like this after the model has already been built, but I was hoping to avoid going through and hooking this all up to the node builder.

Have you ever seen that exception thrown, and does it mean I forgot something?

vpenades commented 3 years ago

It's the NodeAgiArticulations type registered to the same parent type used by the constructor? The error is giving you seems to be due to a constructor argument type mismatch.

The extension type must be registered at the beginning of the application, so it's available to the parser.

Once registered, the better way to add it to a node is:

node.UseExtension<NodeAgiArticulations>().IsAttachPoint = true;

You'll notice I use the verb Use* extensively along the whole library: it's a sort of lazy constructor: Use if it exist, create if it doesn't.

emackey commented 3 years ago

That code looks very clean. But I must be missing something, as I haven't found UseExtension anywhere, only SetExtension. In any case, I did figure out what was broken for me:

public NodeAgiArticulations(Node node) { }

https://github.com/vpenades/SharpGLTF/blob/2681603bdf70bb9d64746c1085d88b28a090e76c/src/SharpGLTF.Core/Schema2/gltf.ExtensionsFactory.cs#L74

Public classes are disallowed as extensions, that's why the exception was thrown. Can this line be changed to allow them?

vpenades commented 3 years ago

what's disallowed is the constructor being public, not the class itself, so

internal NodeAgiArticulations(Node node) { }

will do the trick for now... but yes, I agree it should allow public methods too.

That code looks very clean. But I must be missing something, as I haven't found UseExtension anywhere, only SetExtension

You're right... I guess at some point removed the UseExtension and my mind tricked me, sorry.

So for now, setting extensions has to be done in exactly the way you suggested, as seen here

vpenades commented 3 years ago

I've updated the code to make the ExtensionsFactorypublic, and now there's a UseExtension<T> along with the Get and Set methods.

I've kept the extensions constructor internal, as it is intended to be called exclusively by ExtensionsFactory.Create and not meant to be called by the user.

What's needed is... more documentation and more tests.

Btw, the extension you're implementing has children objects?

emackey commented 3 years ago

Btw, the extension you're implementing has children objects?

Yes. The AGI_articulations schema is public, under Vendor extensions. I'm working on a local branch where I've got the code generator starting to read it into partial classes. I could open a PR once it gets a little further along.

vpenades commented 3 years ago

If that's the case, I suggest you to take a look at how the PunctualLights extension at root is implemented:

https://github.com/vpenades/SharpGLTF/blob/0b317ffb1b92979f833fee91ac5ab9cf5edb8a89/src/SharpGLTF.Core/Schema2/gltf.Root.PunctualLights.cs#L10-L33

If the children collections are also derived from ExtraProperties, you must make the document aware of them.

Notice that this is only required if the children elements are of type ExtraProperties... anything else doesn't require this.

About the PR, it could be a great contribution!

emackey commented 3 years ago

This is excellent advice, thank you.

I've confirmed that the recent commit to master (along with the internal constructor) fixes the original issue here, so I'll close this.