From 1159435d47600f5a58a5aa6db8bea1295fadddd7 Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Wed, 28 Oct 2020 11:12:41 -0700 Subject: [PATCH] gltfio: add safety checks to getAnimator. --- libs/gltfio/src/Animator.cpp | 1 + libs/gltfio/src/FFilamentAsset.h | 1 + libs/gltfio/src/FFilamentInstance.h | 10 ++-------- libs/gltfio/src/FilamentAsset.cpp | 10 ++++++++++ libs/gltfio/src/FilamentInstance.cpp | 18 ++++++++++++++++++ libs/gltfio/src/ResourceLoader.cpp | 6 +++--- libs/viewer/src/SimpleViewer.cpp | 3 ++- 7 files changed, 37 insertions(+), 12 deletions(-) diff --git a/libs/gltfio/src/Animator.cpp b/libs/gltfio/src/Animator.cpp index e2004cb334..92de0a7798 100644 --- a/libs/gltfio/src/Animator.cpp +++ b/libs/gltfio/src/Animator.cpp @@ -140,6 +140,7 @@ static void setTransformType(const cgltf_animation_channel& src, Channel& dst) { } Animator::Animator(FFilamentAsset* asset, FFilamentInstance* instance) { + assert(asset->mResourcesLoaded && !asset->mIsReleased); mImpl = new AnimatorImpl(); mImpl->asset = asset; mImpl->instance = instance; diff --git a/libs/gltfio/src/FFilamentAsset.h b/libs/gltfio/src/FFilamentAsset.h index 0ffec28f91..c73b56f913 100644 --- a/libs/gltfio/src/FFilamentAsset.h +++ b/libs/gltfio/src/FFilamentAsset.h @@ -249,6 +249,7 @@ struct FFilamentAsset : public FilamentAsset { std::vector > mPrimitives; MatInstanceCache mMatInstanceCache; MeshCache mMeshCache; + bool mIsReleased = false; }; FILAMENT_UPCAST(FilamentAsset) diff --git a/libs/gltfio/src/FFilamentInstance.h b/libs/gltfio/src/FFilamentInstance.h index 3fa6cd9b82..6487f6d1d9 100644 --- a/libs/gltfio/src/FFilamentInstance.h +++ b/libs/gltfio/src/FFilamentInstance.h @@ -18,7 +18,6 @@ #define GLTFIO_FFILAMENTINSTANCE_H #include -#include #include @@ -36,6 +35,7 @@ struct cgltf_node; namespace gltfio { struct FFilamentAsset; +class Animator; struct Skin { std::string name; @@ -54,13 +54,7 @@ struct FFilamentInstance : public FilamentInstance { FFilamentAsset* owner; SkinVector skins; NodeMap nodeMap; - - Animator* getAnimator() noexcept { - if (!animator) { - animator = new Animator(owner, this); - } - return animator; - } + Animator* getAnimator() noexcept; }; FILAMENT_UPCAST(FilamentInstance) diff --git a/libs/gltfio/src/FilamentAsset.cpp b/libs/gltfio/src/FilamentAsset.cpp index 4dc3dfa608..1b4a2e4d82 100644 --- a/libs/gltfio/src/FilamentAsset.cpp +++ b/libs/gltfio/src/FilamentAsset.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include "Wireframe.h" @@ -70,6 +71,14 @@ FFilamentAsset::~FFilamentAsset() { Animator* FFilamentAsset::getAnimator() noexcept { if (!mAnimator) { + if (!mResourcesLoaded) { + slog.e << "Cannot create animator before resource loading." << io::endl; + return nullptr; + } + if (mIsReleased) { + slog.e << "Cannot create animator from frozen asset." << io::endl; + return nullptr; + } mAnimator = new Animator(this, nullptr); } return mAnimator; @@ -83,6 +92,7 @@ Entity FFilamentAsset::getWireframe() noexcept { } void FFilamentAsset::releaseSourceData() noexcept { + mIsReleased = true; // To ensure that all possible memory is freed, we reassign to new containers rather than // calling clear(). With many container types (such as robin_map), clearing is a fast // operation that merely frees the storage for the items. diff --git a/libs/gltfio/src/FilamentInstance.cpp b/libs/gltfio/src/FilamentInstance.cpp index 9effbacf89..de91a4dcf5 100644 --- a/libs/gltfio/src/FilamentInstance.cpp +++ b/libs/gltfio/src/FilamentInstance.cpp @@ -15,14 +15,32 @@ */ #include "FFilamentInstance.h" +#include "FFilamentAsset.h" #include +#include + using namespace filament; using namespace utils; namespace gltfio { +Animator* FFilamentInstance::getAnimator() noexcept { + if (!animator) { + if (!owner->mResourcesLoaded) { + slog.e << "Cannot create animator before resource loading." << io::endl; + return nullptr; + } + if (owner->mIsReleased) { + slog.e << "Cannot create animator from frozen asset." << io::endl; + return nullptr; + } + animator = new Animator(owner, this); + } + return animator; +} + size_t FilamentInstance::getEntityCount() const noexcept { return upcast(this)->entities.size(); } diff --git a/libs/gltfio/src/ResourceLoader.cpp b/libs/gltfio/src/ResourceLoader.cpp index 4a54a6e9aa..16b4364a4d 100644 --- a/libs/gltfio/src/ResourceLoader.cpp +++ b/libs/gltfio/src/ResourceLoader.cpp @@ -296,7 +296,6 @@ bool ResourceLoader::loadResources(FFilamentAsset* asset, bool async) { if (asset->mResourcesLoaded) { return false; } - asset->mResourcesLoaded = true; mPool->addAsset(asset); const cgltf_data* gltf = asset->mSourceAsset; cgltf_options options {}; @@ -444,8 +443,9 @@ bool ResourceLoader::loadResources(FFilamentAsset* asset, bool async) { asset->mDependencyGraph.finalize(); pImpl->mCurrentAsset = asset; - // Finally, load image files and create Filament Textures. - return pImpl->createTextures(async); + // Finally, create Filament Textures and begin loading image files. + asset->mResourcesLoaded = pImpl->createTextures(async); + return asset->mResourcesLoaded; } bool ResourceLoader::asyncBeginLoad(FilamentAsset* asset) { diff --git a/libs/viewer/src/SimpleViewer.cpp b/libs/viewer/src/SimpleViewer.cpp index 051a328edc..69362c4fc1 100644 --- a/libs/viewer/src/SimpleViewer.cpp +++ b/libs/viewer/src/SimpleViewer.cpp @@ -410,7 +410,8 @@ void SimpleViewer::updateUserInterface() { ImGui::Unindent(); } - if (mAnimator->getAnimationCount() > 0 && ImGui::CollapsingHeader("Animation")) { + if (mAnimator && mAnimator->getAnimationCount() > 0 && + ImGui::CollapsingHeader("Animation")) { ImGui::Indent(); int selectedAnimation = mCurrentAnimation; ImGui::RadioButton("Disable", &selectedAnimation, 0);