Skip to content

Commit

Permalink
Multiple memory cleanup fixes (#571)
Browse files Browse the repository at this point in the history 
Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
  • Loading branch information
@mjcarroll @ahcorde
mjcarroll and ahcorde authored Jan 29, 2024
1 parent e9dbca7 commit bc24ece
Show file tree
Hide file tree
Showing 10 changed files with 100 additions and 15 deletions.
26 changes: 25 additions & 1 deletion graphics/src/AssimpLoader_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ TEST_F(AssimpLoader, LoadBox)

// Make sure we can read a submesh name
EXPECT_STREQ("Cube", mesh->SubMeshByIndex(0).lock()->Name().c_str());
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -104,6 +105,8 @@ TEST_F(AssimpLoader, Material)
matOpaque->BlendFactors(srcFactor, dstFactor);
EXPECT_DOUBLE_EQ(1.0, srcFactor);
EXPECT_DOUBLE_EQ(0.0, dstFactor);
delete mesh;
delete meshOpaque;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -155,6 +158,7 @@ TEST_F(AssimpLoader, ShareVertices)
}
}
}
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -164,6 +168,7 @@ TEST_F(AssimpLoader, LoadZeroCount)
common::Mesh *mesh = loader.Load(
common::testing::TestFile("data", "zero_count.dae"));
ASSERT_TRUE(mesh);
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -251,6 +256,7 @@ TEST_F(AssimpLoader, TexCoordSets)

subMeshB->SetTexCoordBySet(2u, math::Vector2d(0.1, 0.2), 1u);
EXPECT_EQ(math::Vector2d(0.1, 0.2), subMeshB->TexCoordBySet(2u, 1u));
delete mesh;
}

// Test fails for assimp below 5.2.0
Expand Down Expand Up @@ -289,6 +295,7 @@ TEST_F(AssimpLoader, LoadBoxWithAnimationOutsideSkeleton)
0, 0, 1, 0,
0, 0, 0, 1);
EXPECT_EQ(expectedTrans, poseEnd.at("Armature"));
delete mesh;
}
#endif

Expand All @@ -308,6 +315,7 @@ TEST_F(AssimpLoader, LoadBoxInstControllerWithoutSkeleton)
common::SkeletonPtr skeleton = mesh->MeshSkeleton();
EXPECT_LT(0u, skeleton->NodeCount());
EXPECT_NE(nullptr, skeleton->NodeById("Armature_Bone"));
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -335,6 +343,7 @@ TEST_F(AssimpLoader, LoadBoxMultipleInstControllers)
common::SkeletonPtr skeleton = mesh->MeshSkeleton();
EXPECT_LT(0u, skeleton->NodeCount());
EXPECT_NE(nullptr, skeleton->NodeById("Armature_Bone"));
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -374,6 +383,7 @@ TEST_F(AssimpLoader, LoadBoxNestedAnimation)
0, 0, 1, 0,
0, 0, 0, 1);
EXPECT_EQ(expectedTrans, poseEnd.at("Armature_Bone"));
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -392,6 +402,7 @@ TEST_F(AssimpLoader, LoadBoxWithDefaultStride)
ASSERT_NE(mesh->MeshSkeleton(), nullptr);
// TODO(luca) not working, investigate
// ASSERT_EQ(1u, mesh->MeshSkeleton()->AnimationCount());
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -409,6 +420,7 @@ TEST_F(AssimpLoader, LoadBoxWithMultipleGeoms)
ASSERT_EQ(2u, mesh->SubMeshCount());
EXPECT_EQ(24u, mesh->SubMeshByIndex(0).lock()->NodeAssignmentsCount());
EXPECT_EQ(0u, mesh->SubMeshByIndex(1).lock()->NodeAssignmentsCount());
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -435,6 +447,7 @@ TEST_F(AssimpLoader, LoadBoxWithHierarchicalNodes)

// nested node with name
EXPECT_EQ("StaticCubeNested", mesh->SubMeshByIndex(4).lock()->Name());
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -448,6 +461,7 @@ TEST_F(AssimpLoader, MergeBoxWithDoubleSkeleton)
// The two skeletons have been joined and their root is the
// animation root, called Scene
EXPECT_EQ(skeleton_ptr->RootNode()->Name(), std::string("Scene"));
delete mesh;
}

// For assimp below 5.2.0 mesh loading fails because of
Expand Down Expand Up @@ -487,6 +501,7 @@ TEST_F(AssimpLoader, LoadCylinderAnimatedFrom3dsMax)
// EXPECT_EQ("Bone02", anim->Name());
EXPECT_EQ(1u, anim->NodeCount());
EXPECT_TRUE(anim->HasNode("Bone02"));
delete mesh;
}
#endif

Expand Down Expand Up @@ -521,6 +536,7 @@ TEST_F(AssimpLoader, LoadObjBox)
EXPECT_EQ(mat->Diffuse(), math::Color(0.512f, 0.512f, 0.512f, 1.0f));
EXPECT_EQ(mat->Specular(), math::Color(0.25, 0.25, 0.25, 1.0));
EXPECT_DOUBLE_EQ(mat->Transparency(), 0.0);
delete mesh;
}


Expand All @@ -536,6 +552,7 @@ TEST_F(AssimpLoader, ObjInvalidMaterial)
common::Mesh *mesh = loader.Load(meshFilename);

EXPECT_TRUE(mesh != nullptr);
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -550,6 +567,7 @@ TEST_F(AssimpLoader, NonExistingMesh)
common::Mesh *mesh = loader.Load(meshFilename);

EXPECT_EQ(mesh->SubMeshCount(), 0);
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -584,6 +602,7 @@ TEST_F(AssimpLoader, LoadFbxBox)
EXPECT_EQ(mat->Diffuse(), math::Color(0.8f, 0.8f, 0.8f, 1.0f));
EXPECT_EQ(mat->Specular(), math::Color(0.8f, 0.8f, 0.8f, 1.0f));
EXPECT_DOUBLE_EQ(mat->Transparency(), 0.0);
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -618,6 +637,7 @@ TEST_F(AssimpLoader, LoadGlTF2Box)
EXPECT_EQ(mat->Diffuse(), math::Color(0.8f, 0.8f, 0.8f, 1.0f));
EXPECT_EQ(mat->Specular(), math::Color(0.0f, 0.0f, 0.0f, 1.0f));
EXPECT_DOUBLE_EQ(mat->Transparency(), 0.0);
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -642,6 +662,7 @@ TEST_F(AssimpLoader, LoadGlTF2BoxExternalTexture)
auto testTextureFile =
common::testing::TestFile("data/gltf", "PurpleCube_Diffuse.png");
EXPECT_EQ(testTextureFile, mat->TextureImage());
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -681,6 +702,7 @@ TEST_F(AssimpLoader, LoadGlTF2BoxWithJPEGTexture)
EXPECT_EQ("Scene_Material_Diffuse", mat->TextureImage());
#endif
EXPECT_NE(nullptr, mat->TextureData());
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -733,7 +755,6 @@ TEST_F(AssimpLoader, LoadGlbPbrAsset)
EXPECT_EQ(img->Pixel(0, 0), math::Color(0.0f, 0.0f, 0.0f, 1.0f));
EXPECT_EQ(img->Pixel(100, 100), math::Color(1.0f, 1.0f, 1.0f, 1.0f));


EXPECT_NE(pbr->NormalMapData(), nullptr);
// Metallic roughness and alpha from textures only works in assimp > 5.2.0
#ifndef GZ_ASSIMP_PRE_5_2_0
Expand Down Expand Up @@ -765,6 +786,7 @@ TEST_F(AssimpLoader, LoadGlbPbrAsset)
EXPECT_STREQ("Action1", skel->Animation(0)->Name().c_str());
EXPECT_STREQ("Action2", skel->Animation(1)->Name().c_str());
EXPECT_STREQ("Action3", skel->Animation(2)->Name().c_str());
delete mesh;
}

/////////////////////////////////////////////////
Expand All @@ -782,6 +804,7 @@ TEST_F(AssimpLoader, CheckNonRootDisplacement)
EXPECT_EQ(nullptr, rootNode);
auto xDisplacement = skelAnim->XDisplacement();
ASSERT_TRUE(xDisplacement);
delete mesh;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -825,4 +848,5 @@ TEST_F(AssimpLoader, LoadGLTF2Triangle)
EXPECT_EQ(math::Vector2d(0, 1), subMeshB->TexCoord(0u));
EXPECT_EQ(math::Vector2d(0, 1), subMeshB->TexCoord(1u));
EXPECT_EQ(math::Vector2d(0, 1), subMeshB->TexCoord(2u));
delete mesh;
}
9 changes: 9 additions & 0 deletions graphics/src/ColladaExporter_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ TEST_F(ColladaExporter, ExportBox)
meshReloaded->SubMeshByIndex(i).lock()->TexCoord(j));
}
}
delete meshOriginal;
delete meshReloaded;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -206,6 +208,8 @@ TEST_F(ColladaExporter, ExportCordlessDrill)
meshReloaded->SubMeshByIndex(i).lock()->TexCoord(j));
}
}
delete meshOriginal;
delete meshReloaded;
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -319,6 +323,10 @@ TEST_F(ColladaExporter, ExportMeshWithSubmeshes)
EXPECT_EQ(outMesh.NormalCount(), meshReloaded->NormalCount());
EXPECT_EQ(outMesh.TexCoordCount(),
meshReloaded->TexCoordCount());

delete boxMesh;
delete drillMesh;
delete meshReloaded;
}

TEST_F(ColladaExporter, ExportLights)
Expand Down Expand Up @@ -448,4 +456,5 @@ TEST_F(ColladaExporter, ExportLights)
}
}
EXPECT_EQ(node_with_light_count, 3);
delete meshOriginal;
}
35 changes: 28 additions & 7 deletions graphics/src/ColladaLoader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,9 +366,7 @@ ColladaLoader::ColladaLoader()
}

//////////////////////////////////////////////////
ColladaLoader::~ColladaLoader()
{
}
ColladaLoader::~ColladaLoader() = default;

//////////////////////////////////////////////////
Mesh *ColladaLoader::Load(const std::string &_filename)
Expand Down Expand Up @@ -772,13 +770,16 @@ void ColladaLoader::Implementation::LoadController(
{
SkeletonNode *rootSkelNode =
this->LoadSkeletonNodes(rootNodeXml, nullptr);

if (nullptr == skeleton)
{
skeleton = SkeletonPtr(new Skeleton(rootSkelNode));
skeleton = std::make_shared<Skeleton>(rootSkelNode);
_mesh->SetSkeleton(skeleton);
}
else if (nullptr != rootSkelNode)
{
this->MergeSkeleton(skeleton, rootSkelNode);
}
}
if (nullptr == skeleton)
{
Expand Down Expand Up @@ -1191,7 +1192,7 @@ SkeletonNode *ColladaLoader::Implementation::LoadSkeletonNodes(
return nullptr;
}

auto node = this->LoadSingleSkeletonNode(_xml, _parent);
auto *node = this->LoadSingleSkeletonNode(_xml, _parent);
this->SetSkeletonNodeTransform(_xml, node);

auto childXml = _xml->FirstChildElement("node");
Expand Down Expand Up @@ -2800,8 +2801,14 @@ void ColladaLoader::Implementation::MergeSkeleton(SkeletonPtr _skeleton,
if (currentRoot->Id() == _mergeNode->Id())
return;

if (_mergeNode->ChildById(currentRoot->Id()))
auto *rootInMergeNode = _mergeNode->ChildById(currentRoot->Id());

if (rootInMergeNode != nullptr)
{
if (rootInMergeNode != currentRoot)
{
delete currentRoot;
}
_skeleton->RootNode(_mergeNode);
return;
}
Expand All @@ -2822,8 +2829,21 @@ void ColladaLoader::Implementation::MergeSkeleton(SkeletonPtr _skeleton,
}
if (mergeNodeContainsRoot)
{
std::function<void(SkeletonNode*)> delete_children;
delete_children =
[&delete_children](SkeletonNode *node)-> void {
for (size_t ii = 0; ii < node->ChildCount(); ++ii)
{
auto *child = node->Child(ii);
delete_children(child);
delete child;
}
};

// Since we are replacing the whole tree, recursively clean up
// the existing skeleton nodes
_skeleton->RootNode(_mergeNode);
// TODO(anyone) since we are replacing the whole tree delete the old one
delete_children(currentRoot);
delete currentRoot;
return;
}
Expand All @@ -2834,6 +2854,7 @@ void ColladaLoader::Implementation::MergeSkeleton(SkeletonPtr _skeleton,
dummyRoot =
new SkeletonNode(nullptr, "dummy-root", "dummy-root");
}

if (dummyRoot != currentRoot)
{
dummyRoot->AddChild(currentRoot);
Expand Down
Loading

0 comments on commit bc24ece

Please sign in to comment.