Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple memory cleanup fixes #571

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Multiple memory cleanup fixes #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
263cdd4
Clean up memory in AssimpLoader_TEST
mjcarroll Jan 12, 2024
b593816
Clean up memory in ColladaLoader_TEST
mjcarroll Jan 16, 2024
faefa9f
Fix leak when rescaling images
mjcarroll Jan 16, 2024
df3a200
More easy fixes
mjcarroll Jan 16, 2024
38f4d23
More easy fixes
mjcarroll Jan 16, 2024
b991f71
Merge branch 'gz-common5' into mjcarroll/asan
ahcorde Jan 16, 2024
751b4b5
Fix skeleton leak in ColladaLoader
mjcarroll Jan 17, 2024
6b79f56
Merge remote-tracking branch 'origin/mjcarroll/asan' into mjcarroll/asan
mjcarroll Jan 17, 2024
abcf273
Merge branch 'gz-common5' into mjcarroll/asan
mjcarroll Jan 18, 2024
eaf8e3e
Merge branch 'gz-common5' into mjcarroll/asan
mjcarroll Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion graphics/src/AssimpLoader_TEST.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading