search

spnda / fastgltf

A modern C++17 glTF 2.0 library focused on speed, correctness, and usability
https://fastgltf.readthedocs.io/v0.8.x/
MIT License
301 stars 44 forks source link

Add: Animation support to Exporter; Fix: Vector source not allowed as embedded buffer #64

Closed Deweh closed 4 months ago

Deweh commented 4 months ago

This PR adds support for animations to the Exporter, and also fixes an issue with the Exporter where a sources::Vector buffer would not pass the check for using an embedded buffer, even though documentation states it should be usable for an embedded buffer.

spnda commented 4 months ago

Did you intend to target this for the 0.7.x branch? I intend to release 0.8 rather soon and only released 0.7.2 recently for bug fixes, since some people don't want to deal with the breaking changes yet. If not I can switch this to go into the main branch instead.

Otherwise, this looks good (I thought the Exporter supported everything already lol) and good catch about the embedded buffer. I reworked Vector into Array, but then re-added Vector since it's a more suitable option for some use cases, and must've forgot re-adding it here. Perhaps there are other places where I missed it...

Deweh commented 4 months ago

Did you intend to target this for the 0.7.x branch? I intend to release 0.8 rather soon and only released 0.7.2 recently for bug fixes, since some people don't want to deal with the breaking changes yet. If not I can switch this to go into the main branch instead.

I don't PR very often, so I'm not familiar with the proper procedure for going about these things, apologies. I just pushed to the 0.7.x branch since that's the branch a project of mine was on when I realized I needed these changes.

I'll push those requested changes soon.

spnda commented 4 months ago

I don't PR very often, so I'm not familiar with the proper procedure for going about these things, apologies. No worries, this is more just about what I have intended for the future, you can't really know that.

Since this is technically a bugfix, we could still PR this onto the 0.7.x branch, and then I'll bring the commit over to the main branch, since there might be some conflicts and then I can just resolve those. I'll wait on the additional changes, test them myself, and then I'll merge.

Deweh commented 4 months ago

I don't PR very often, so I'm not familiar with the proper procedure for going about these things, apologies. No worries, this is more just about what I have intended for the future, you can't really know that.

Since this is technically a bugfix, we could still PR this onto the 0.7.x branch, and then I'll bring the commit over to the main branch, since there might be some conflicts and then I can just resolve those. I'll wait on the additional changes, test them myself, and then I'll merge.

Sounds good. I've pushed the changes, and also added the extrasWriteCallback before the name to match the functions for the other types.

spnda commented 4 months ago

Thank you, I've merged this onto the v0.7.x branch and cherry-picked it onto the main branch as 65e1fec04eccff1939f68f09aaca35152104ea30.