Merge animations before writing to JSON. #102
Conversation
There was a problem hiding this comment.
I was going to leave a comment about how to handle the memory management but it seems this tool doesn't care it leaks memory everywhere. Since its a command line/single block tool it really doesn't matter much as all the memory will be reclaimed by the OS when the process finishes.
GLTF/src/GLTFAsset.cpp
Outdated
| // according to COLLADA <animation_clip/> nodes. | ||
| for (size_t i = 0; i < animations.size(); i++) { | ||
| GLTF::Animation* animation = animations[i]; | ||
| for (GLTF::Animation::Channel* channel : animation->channels) { |
There was a problem hiding this comment.
I see this pattern in this file already:
for (GLTF::Animation* animation : animations) {
for (GLTF::Animation::Channel* channel : animation->channels) {
|
@lasalvavida are you able to take a quick look at this? |
|
This looks good to me, thanks @donmccurdy! And yeah, don't worry about memory. It's on the roadmap (see #70), but not super high priority since as far as I know people just use this as a command-line tool. If there is interest in using the |
|
Thanks @lasalvavida! Is there any scripting to do bulk updates like KhronosGroup/glTF-Sample-Models#76, or would I do that manually? |
|
@donmccurdy, check out the tools branch. It was never added to master, but I did write a python script for autogenerating the models. |
|
Ok, perfect! 🙂 |
| GLTF::Animation* mergedAnimation = new GLTF::Animation(); | ||
|
|
||
| // Merge all animations. In the future, animations should be grouped | ||
| // according to COLLADA <animation_clip/> nodes. |
There was a problem hiding this comment.
Did a new issue get opened to track this?
Fixes #66.
Related:
Disclaimer: I have no idea how memory management works in C++. Do i need to deallocate the old
animations somehow? 😅