From 813f1714935f71696e8d6d8d1b398192c3c44c33 Mon Sep 17 00:00:00 2001 From: Natanael Rabello Date: Mon, 16 Nov 2020 21:39:59 -0300 Subject: [PATCH 1/7] Update Android port README.md with ABI doc --- port/AndroidJNI/README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/port/AndroidJNI/README.md b/port/AndroidJNI/README.md index 0b95efd04..336ed8782 100644 --- a/port/AndroidJNI/README.md +++ b/port/AndroidJNI/README.md @@ -14,6 +14,10 @@ To use this module please provide following cmake defines: "SOME_PATH" is a path to your cmake android toolchain script. + +The build script for this port is based on [android-cmake](https://github.com/taka-no-me/android-cmake). +See its documentation for more Android-specific cmake options (e.g. -DANDROID_ABI for the target ABI). + ### Code ### A small example how to wrap assimp for Android: ```cpp From 213a9f9d55249d0fcf09fec32ec8cc87533672b1 Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Tue, 17 Nov 2020 10:39:03 +0000 Subject: [PATCH 2/7] First pass at PotentialNode --- code/AssetLib/FBX/FBXConverter.cpp | 232 +++++++++++++++-------------- code/AssetLib/FBX/FBXConverter.h | 5 +- 2 files changed, 120 insertions(+), 117 deletions(-) diff --git a/code/AssetLib/FBX/FBXConverter.cpp b/code/AssetLib/FBX/FBXConverter.cpp index a7f9e6832..90082172d 100644 --- a/code/AssetLib/FBX/FBXConverter.cpp +++ b/code/AssetLib/FBX/FBXConverter.cpp @@ -185,6 +185,16 @@ std::string FBXConverter::MakeUniqueNodeName(const Model *const model, const aiN return unique_name; } +/// This struct manages nodes which may or may not end up in the node hierarchy. +/// When a node becomes a child of another node, that node becomes its owner and mOwnership should be released. +struct FBXConverter::PotentialNode +{ + PotentialNode(std::unique_ptr uptr) : mNode(uptr.get()), mOwnership(std::move(uptr)) {} + aiNode* operator->() { return mNode; } + aiNode* mNode; + std::unique_ptr mOwnership; +}; + /// todo: pre-build node hierarchy /// todo: get bone from stack /// todo: make map of aiBone* to aiNode* @@ -192,137 +202,129 @@ std::string FBXConverter::MakeUniqueNodeName(const Model *const model, const aiN void FBXConverter::ConvertNodes(uint64_t id, aiNode *parent, aiNode *root_node) { const std::vector &conns = doc.GetConnectionsByDestinationSequenced(id, "Model"); - std::vector nodes; + std::vector nodes; nodes.reserve(conns.size()); - std::vector nodes_chain; - std::vector post_nodes_chain; + std::vector nodes_chain; + std::vector post_nodes_chain; - try { - for (const Connection *con : conns) { - // ignore object-property links - if (con->PropertyName().length()) { - // really important we document why this is ignored. - FBXImporter::LogInfo("ignoring property link - no docs on why this is ignored"); - continue; //? + for (const Connection *con : conns) { + // ignore object-property links + if (con->PropertyName().length()) { + // really important we document why this is ignored. + FBXImporter::LogInfo("ignoring property link - no docs on why this is ignored"); + continue; //? + } + + // convert connection source object into Object base class + const Object *const object = con->SourceObject(); + if (nullptr == object) { + FBXImporter::LogError("failed to convert source object for Model link"); + continue; + } + + // FBX Model::Cube, Model::Bone001, etc elements + // This detects if we can cast the object into this model structure. + const Model *const model = dynamic_cast(object); + + if (nullptr != model) { + nodes_chain.clear(); + post_nodes_chain.clear(); + + aiMatrix4x4 new_abs_transform = parent->mTransformation; + std::string node_name = FixNodeName(model->Name()); + // even though there is only a single input node, the design of + // assimp (or rather: the complicated transformation chain that + // is employed by fbx) means that we may need multiple aiNode's + // to represent a fbx node's transformation. + + // generate node transforms - this includes pivot data + // if need_additional_node is true then you t + const bool need_additional_node = GenerateTransformationNodeChain(*model, node_name, nodes_chain, post_nodes_chain); + + // assert that for the current node we must have at least a single transform + ai_assert(nodes_chain.size()); + + if (need_additional_node) { + nodes_chain.emplace_back(PotentialNode(std::unique_ptr(new aiNode(node_name)))); } - // convert connection source object into Object base class - const Object *const object = con->SourceObject(); - if (nullptr == object) { - FBXImporter::LogError("failed to convert source object for Model link"); - continue; - } + //setup metadata on newest node + SetupNodeMetadata(*model, *nodes_chain.back().mNode); - // FBX Model::Cube, Model::Bone001, etc elements - // This detects if we can cast the object into this model structure. - const Model *const model = dynamic_cast(object); + // link all nodes in a row + aiNode *last_parent = parent; + for (PotentialNode& child : nodes_chain) { + ai_assert(child.mNode); - if (nullptr != model) { - nodes_chain.clear(); - post_nodes_chain.clear(); - - aiMatrix4x4 new_abs_transform = parent->mTransformation; - std::string node_name = FixNodeName(model->Name()); - // even though there is only a single input node, the design of - // assimp (or rather: the complicated transformation chain that - // is employed by fbx) means that we may need multiple aiNode's - // to represent a fbx node's transformation. - - // generate node transforms - this includes pivot data - // if need_additional_node is true then you t - const bool need_additional_node = GenerateTransformationNodeChain(*model, node_name, nodes_chain, post_nodes_chain); - - // assert that for the current node we must have at least a single transform - ai_assert(nodes_chain.size()); - - if (need_additional_node) { - nodes_chain.push_back(new aiNode(node_name)); + if (last_parent != parent) { + last_parent->mNumChildren = 1; + last_parent->mChildren = new aiNode *[1]; + last_parent->mChildren[0] = child.mOwnership.release(); } - //setup metadata on newest node - SetupNodeMetadata(*model, *nodes_chain.back()); + child->mParent = last_parent; + last_parent = child.mNode; - // link all nodes in a row - aiNode *last_parent = parent; - for (aiNode *child : nodes_chain) { - ai_assert(child); + new_abs_transform *= child->mTransformation; + } + + // attach geometry + ConvertModel(*model, nodes_chain.back().mNode, root_node, new_abs_transform); + + // check if there will be any child nodes + const std::vector &child_conns = doc.GetConnectionsByDestinationSequenced(model->ID(), "Model"); + + // if so, link the geometric transform inverse nodes + // before we attach any child nodes + if (child_conns.size()) { + for (PotentialNode& postnode : post_nodes_chain) { + ai_assert(postnode.mNode); if (last_parent != parent) { last_parent->mNumChildren = 1; last_parent->mChildren = new aiNode *[1]; - last_parent->mChildren[0] = child; + last_parent->mChildren[0] = postnode.mOwnership.release(); } - child->mParent = last_parent; - last_parent = child; + postnode->mParent = last_parent; + last_parent = postnode.mNode; - new_abs_transform *= child->mTransformation; + new_abs_transform *= postnode->mTransformation; } - - // attach geometry - ConvertModel(*model, nodes_chain.back(), root_node, new_abs_transform); - - // check if there will be any child nodes - const std::vector &child_conns = doc.GetConnectionsByDestinationSequenced(model->ID(), "Model"); - - // if so, link the geometric transform inverse nodes - // before we attach any child nodes - if (child_conns.size()) { - for (aiNode *postnode : post_nodes_chain) { - ai_assert(postnode); - - if (last_parent != parent) { - last_parent->mNumChildren = 1; - last_parent->mChildren = new aiNode *[1]; - last_parent->mChildren[0] = postnode; - } - - postnode->mParent = last_parent; - last_parent = postnode; - - new_abs_transform *= postnode->mTransformation; - } - } else { - // free the nodes we allocated as we don't need them - Util::delete_fun deleter; - std::for_each( - post_nodes_chain.begin(), - post_nodes_chain.end(), - deleter); - } - - // recursion call - child nodes - ConvertNodes(model->ID(), last_parent, root_node); - - if (doc.Settings().readLights) { - ConvertLights(*model, node_name); - } - - if (doc.Settings().readCameras) { - ConvertCameras(*model, node_name); - } - - nodes.push_back(nodes_chain.front()); - nodes_chain.clear(); + } else { + // free the nodes we allocated as we don't need them + post_nodes_chain.clear(); } + + // recursion call - child nodes + ConvertNodes(model->ID(), last_parent, root_node); + + if (doc.Settings().readLights) { + ConvertLights(*model, node_name); + } + + if (doc.Settings().readCameras) { + ConvertCameras(*model, node_name); + } + + nodes.push_back(std::move(nodes_chain.front())); + nodes_chain.clear(); } + } - if (nodes.size()) { - parent->mChildren = new aiNode *[nodes.size()](); - parent->mNumChildren = static_cast(nodes.size()); + if (nodes.size()) { + parent->mChildren = new aiNode *[nodes.size()](); + parent->mNumChildren = static_cast(nodes.size()); - std::swap_ranges(nodes.begin(), nodes.end(), parent->mChildren); - } else { - parent->mNumChildren = 0; - parent->mChildren = nullptr; + for (int i = 0; i < nodes.size(); ++i) + { + parent->mChildren[i] = nodes[i].mOwnership.release(); } - - } catch (std::exception &) { - Util::delete_fun deleter; - std::for_each(nodes.begin(), nodes.end(), deleter); - std::for_each(nodes_chain.begin(), nodes_chain.end(), deleter); - std::for_each(post_nodes_chain.begin(), post_nodes_chain.end(), deleter); + nodes.clear(); + } else { + parent->mNumChildren = 0; + parent->mChildren = nullptr; } } @@ -681,8 +683,8 @@ std::string FBXConverter::NameTransformationChainNode(const std::string &name, T return name + std::string(MAGIC_NODE_TAG) + "_" + NameTransformationComp(comp); } -bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std::string &name, std::vector &output_nodes, - std::vector &post_output_nodes) { +bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std::string &name, std::vector &output_nodes, + std::vector &post_output_nodes) { const PropertyTable &props = model.Props(); const Model::RotOrder rot = model.RotationOrder(); @@ -828,7 +830,7 @@ bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std chain[i] = chain[i].Inverse(); } - aiNode *nd = new aiNode(); + PotentialNode nd(std::unique_ptr(new aiNode())); nd->mName.Set(NameTransformationChainNode(name, comp)); nd->mTransformation = chain[i]; @@ -836,9 +838,9 @@ bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std if (comp == TransformationComp_GeometricScalingInverse || comp == TransformationComp_GeometricRotationInverse || comp == TransformationComp_GeometricTranslationInverse) { - post_output_nodes.push_back(nd); + post_output_nodes.emplace_back(std::move(nd)); } else { - output_nodes.push_back(nd); + output_nodes.emplace_back(std::move(nd)); } } @@ -847,8 +849,7 @@ bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std } // else, we can just multiply the matrices together - aiNode *nd = new aiNode(); - output_nodes.push_back(nd); + PotentialNode nd(std::unique_ptr(new aiNode())); // name passed to the method is already unique nd->mName.Set(name); @@ -857,6 +858,7 @@ bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std for (unsigned int i = TransformationComp_Translation; i < TransformationComp_MAXIMUM; i++) { nd->mTransformation = nd->mTransformation * chain[i]; } + output_nodes.push_back(std::move(nd)); return false; } diff --git a/code/AssetLib/FBX/FBXConverter.h b/code/AssetLib/FBX/FBXConverter.h index 7db4b795b..5c73663d1 100644 --- a/code/AssetLib/FBX/FBXConverter.h +++ b/code/AssetLib/FBX/FBXConverter.h @@ -171,9 +171,10 @@ private: // ------------------------------------------------------------------------------------------------ /** - * note: memory for output_nodes will be managed by the caller + * note: memory for output_nodes is managed by the caller, via the PotentialNode struct. */ - bool GenerateTransformationNodeChain(const Model& model, const std::string& name, std::vector& output_nodes, std::vector& post_output_nodes); + struct PotentialNode; + bool GenerateTransformationNodeChain(const Model& model, const std::string& name, std::vector& output_nodes, std::vector& post_output_nodes); // ------------------------------------------------------------------------------------------------ void SetupNodeMetadata(const Model& model, aiNode& nd); From c00153089a351ff097735475bf3fe701ba34a3d0 Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Tue, 17 Nov 2020 10:45:10 +0000 Subject: [PATCH 3/7] Neater construction --- code/AssetLib/FBX/FBXConverter.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/code/AssetLib/FBX/FBXConverter.cpp b/code/AssetLib/FBX/FBXConverter.cpp index 90082172d..fb625599d 100644 --- a/code/AssetLib/FBX/FBXConverter.cpp +++ b/code/AssetLib/FBX/FBXConverter.cpp @@ -189,10 +189,11 @@ std::string FBXConverter::MakeUniqueNodeName(const Model *const model, const aiN /// When a node becomes a child of another node, that node becomes its owner and mOwnership should be released. struct FBXConverter::PotentialNode { - PotentialNode(std::unique_ptr uptr) : mNode(uptr.get()), mOwnership(std::move(uptr)) {} + PotentialNode() : mOwnership(new aiNode), mNode(mOwnership.get()) {} + PotentialNode(const std::string& name) : mOwnership(new aiNode(name)), mNode(mOwnership.get()) {} aiNode* operator->() { return mNode; } - aiNode* mNode; std::unique_ptr mOwnership; + aiNode* mNode; }; /// todo: pre-build node hierarchy @@ -246,7 +247,7 @@ void FBXConverter::ConvertNodes(uint64_t id, aiNode *parent, aiNode *root_node) ai_assert(nodes_chain.size()); if (need_additional_node) { - nodes_chain.emplace_back(PotentialNode(std::unique_ptr(new aiNode(node_name)))); + nodes_chain.emplace_back(PotentialNode(node_name)); } //setup metadata on newest node @@ -830,7 +831,7 @@ bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std chain[i] = chain[i].Inverse(); } - PotentialNode nd(std::unique_ptr(new aiNode())); + PotentialNode nd; nd->mName.Set(NameTransformationChainNode(name, comp)); nd->mTransformation = chain[i]; @@ -849,7 +850,7 @@ bool FBXConverter::GenerateTransformationNodeChain(const Model &model, const std } // else, we can just multiply the matrices together - PotentialNode nd(std::unique_ptr(new aiNode())); + PotentialNode nd; // name passed to the method is already unique nd->mName.Set(name); From fc842a0f97d9456f20463c9a7fb3dce6301bee8f Mon Sep 17 00:00:00 2001 From: Neil Clifford Date: Thu, 19 Nov 2020 13:20:43 +0000 Subject: [PATCH 4/7] Sceneprecessor potential memory leak --- code/Common/ScenePreprocessor.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/code/Common/ScenePreprocessor.cpp b/code/Common/ScenePreprocessor.cpp index 55aa04ad2..5850e0ffd 100644 --- a/code/Common/ScenePreprocessor.cpp +++ b/code/Common/ScenePreprocessor.cpp @@ -210,6 +210,10 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { // No rotation keys? Generate a dummy track if (!channel->mNumRotationKeys) { + if (channel->mRotationKeys) { + delete[] channel->mRotationKeys; + channel->mRotationKeys = NULL; + } ai_assert(!channel->mRotationKeys); channel->mNumRotationKeys = 1; channel->mRotationKeys = new aiQuatKey[1]; @@ -225,6 +229,10 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { // No scaling keys? Generate a dummy track if (!channel->mNumScalingKeys) { + if (channel->mScalingKeys) { + delete[] channel->mScalingKeys; + channel->mScalingKeys = NULL; + } ai_assert(!channel->mScalingKeys); channel->mNumScalingKeys = 1; channel->mScalingKeys = new aiVectorKey[1]; @@ -240,6 +248,10 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { // No position keys? Generate a dummy track if (!channel->mNumPositionKeys) { + if (channel->mPositionKeys) { + delete[] channel->mPositionKeys; + channel->mPositionKeys = NULL; + } ai_assert(!channel->mPositionKeys); channel->mNumPositionKeys = 1; channel->mPositionKeys = new aiVectorKey[1]; From 885a196c74cdfac3c0f12f8855270218d3774bde Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Thu, 19 Nov 2020 16:30:44 +0000 Subject: [PATCH 5/7] Unsigned --- code/AssetLib/FBX/FBXConverter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/AssetLib/FBX/FBXConverter.cpp b/code/AssetLib/FBX/FBXConverter.cpp index fb625599d..f8395543f 100644 --- a/code/AssetLib/FBX/FBXConverter.cpp +++ b/code/AssetLib/FBX/FBXConverter.cpp @@ -318,7 +318,7 @@ void FBXConverter::ConvertNodes(uint64_t id, aiNode *parent, aiNode *root_node) parent->mChildren = new aiNode *[nodes.size()](); parent->mNumChildren = static_cast(nodes.size()); - for (int i = 0; i < nodes.size(); ++i) + for (unsigned int i = 0; i < nodes.size(); ++i) { parent->mChildren[i] = nodes[i].mOwnership.release(); } From 75204dfe7f8cdd86e0f53f8811446d961ceb7fe5 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 1 Dec 2020 08:58:45 +0100 Subject: [PATCH 6/7] Update README.md --- port/AndroidJNI/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/port/AndroidJNI/README.md b/port/AndroidJNI/README.md index 336ed8782..003b1da1a 100644 --- a/port/AndroidJNI/README.md +++ b/port/AndroidJNI/README.md @@ -17,6 +17,7 @@ To use this module please provide following cmake defines: The build script for this port is based on [android-cmake](https://github.com/taka-no-me/android-cmake). See its documentation for more Android-specific cmake options (e.g. -DANDROID_ABI for the target ABI). +Check [Asset-Importer-Docs](https://assimp-docs.readthedocs.io/en/latest/) for more information. ### Code ### A small example how to wrap assimp for Android: From c7aeb882e645c81bdb02ea73c247a17ceb1952cd Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Thu, 3 Dec 2020 17:28:51 +0100 Subject: [PATCH 7/7] Update ScenePreprocessor.cpp --- code/Common/ScenePreprocessor.cpp | 34 +++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/code/Common/ScenePreprocessor.cpp b/code/Common/ScenePreprocessor.cpp index 5850e0ffd..2b73d9452 100644 --- a/code/Common/ScenePreprocessor.cpp +++ b/code/Common/ScenePreprocessor.cpp @@ -96,8 +96,9 @@ void ScenePreprocessor::ProcessMesh(aiMesh *mesh) { if (!mesh->mTextureCoords[i]) { mesh->mNumUVComponents[i] = 0; } else { - if (!mesh->mNumUVComponents[i]) + if (!mesh->mNumUVComponents[i]) { mesh->mNumUVComponents[i] = 2; + } aiVector3D *p = mesh->mTextureCoords[i], *end = p + mesh->mNumVertices; @@ -105,16 +106,19 @@ void ScenePreprocessor::ProcessMesh(aiMesh *mesh) { // as if they were 2D channels .. just in case an application doesn't handle // this case if (2 == mesh->mNumUVComponents[i]) { - for (; p != end; ++p) + for (; p != end; ++p) { p->z = 0.f; + } } else if (1 == mesh->mNumUVComponents[i]) { - for (; p != end; ++p) + for (; p != end; ++p) { p->z = p->y = 0.f; + } } else if (3 == mesh->mNumUVComponents[i]) { // Really 3D coordinates? Check whether the third coordinate is != 0 for at least one element for (; p != end; ++p) { - if (p->z != 0) + if (p->z != 0) { break; + } } if (p == end) { ASSIMP_LOG_WARN("ScenePreprocessor: UVs are declared to be 3D but they're obviously not. Reverting to 2D."); @@ -151,7 +155,6 @@ void ScenePreprocessor::ProcessMesh(aiMesh *mesh) { // If tangents and normals are given but no bitangents compute them if (mesh->mTangents && mesh->mNormals && !mesh->mBitangents) { - mesh->mBitangents = new aiVector3D[mesh->mNumVertices]; for (unsigned int i = 0; i < mesh->mNumVertices; ++i) { mesh->mBitangents[i] = mesh->mNormals[i] ^ mesh->mTangents[i]; @@ -165,11 +168,9 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { for (unsigned int i = 0; i < anim->mNumChannels; ++i) { aiNodeAnim *channel = anim->mChannels[i]; - /* If the exact duration of the animation is not given - * compute it now. - */ + // If the exact duration of the animation is not given + // compute it now. if (anim->mDuration == -1.) { - // Position keys for (unsigned int j = 0; j < channel->mNumPositionKeys; ++j) { aiVectorKey &key = channel->mPositionKeys[j]; @@ -192,11 +193,10 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { } } - /* Check whether the animation channel has no rotation - * or position tracks. In this case we generate a dummy - * track from the information we have in the transformation - * matrix of the corresponding node. - */ + // Check whether the animation channel has no rotation + // or position tracks. In this case we generate a dummy + // track from the information we have in the transformation + // matrix of the corresponding node. if (!channel->mNumRotationKeys || !channel->mNumPositionKeys || !channel->mNumScalingKeys) { // Find the node that belongs to this animation aiNode *node = scene->mRootNode->FindNode(channel->mNodeName); @@ -212,7 +212,7 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { if (!channel->mNumRotationKeys) { if (channel->mRotationKeys) { delete[] channel->mRotationKeys; - channel->mRotationKeys = NULL; + channel->mRotationKeys = nullptr; } ai_assert(!channel->mRotationKeys); channel->mNumRotationKeys = 1; @@ -231,7 +231,7 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { if (!channel->mNumScalingKeys) { if (channel->mScalingKeys) { delete[] channel->mScalingKeys; - channel->mScalingKeys = NULL; + channel->mScalingKeys = nullptr; } ai_assert(!channel->mScalingKeys); channel->mNumScalingKeys = 1; @@ -250,7 +250,7 @@ void ScenePreprocessor::ProcessAnimation(aiAnimation *anim) { if (!channel->mNumPositionKeys) { if (channel->mPositionKeys) { delete[] channel->mPositionKeys; - channel->mPositionKeys = NULL; + channel->mPositionKeys = nullptr; } ai_assert(!channel->mPositionKeys); channel->mNumPositionKeys = 1;