Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Apr 11, 2020

Conversation

ptasev
Copy link
Contributor

@ptasev ptasev commented Apr 11, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

ptasev commented Apr 11, 2020

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
Copy link
Owner

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.

@vpenades vpenades merged commit 958b5e4 into vpenades:master Apr 11, 2020
@ptasev ptasev deleted the imeshbuilder-usemorphtarget branch April 11, 2020 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants