vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
470 stars 75 forks source link

Made it possible to call UseMorphTarget on IMeshBuilder<TMaterial> #38

Closed ptasev closed 4 years ago

ptasev commented 4 years ago

I made these changes because I had an IMeshBuilder<> variable that couldn't call UseMorphTarget, and I didn't see a valid reason for why this shouldn't be possible.

IMeshBuilder<MaterialBuilder> mb = CreateMeshBuilder();
var pb = mb.UsePrimitive(material); // This worked fine
var mt = mb.UseMorphTarget(0); // This didn't work, but does with this pull request

Feel free to reject/close this pull request if you have a better way to do it.

vpenades commented 4 years ago

Hmmm... you've removed the generic methods, which will break API compatibility with some users already using the generic API.

My recomendation would be to add a new IMorphTargetBuilder interface, implemented over the MorphTargetBuilder class, in the same way that IMeshBuilder is used in MeshBuilder.

Something like this:

public interface IMeshBuilder<TMaterial>
{
    string Name { get; set; }
    IEnumerable<TMaterial> Materials { get; }
    IReadOnlyCollection<IPrimitiveReader<TMaterial>> Primitives { get; }
    IPrimitiveBuilder UsePrimitive(TMaterial material, int primitiveVertexCount = 3);
    IMeshBuilder<TMaterial> Clone(Func<TMaterial, TMaterial> materialCloneCallback = null);

    IMorphTargetBuilder UseMorphTarget(int index);

    void Validate();
}

public interface IMorphTargetBuilder
{
IReadOnlyCollection<Vector3> Positions {get;}
IReadOnlyCollection<IVertexGeometry> Vertices {get;}
...
}
ptasev commented 4 years ago

I figured since the library version is in alpha, people would expect breaking changes. What you said makes sense though, and I've made the changes.

vpenades commented 4 years ago

The latest changes look nice!

Although the library has the alpha tag, you can really consider it a fully functional library, that gives me the freedom to change APIs when it's required, but it doesn't mean changing an API for no reason. And I believe it's better if new features can be added without breaking the existing API.