From 46557771cb787b5700fe6552af141134511e6da0 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Tue, 7 Sep 2021 10:35:39 -0400
Subject: [PATCH 01/12] Clone visuals and geometries

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/Geometry.hh        |   6 +
 include/ignition/rendering/Visual.hh          |  11 +-
 .../ignition/rendering/base/BaseGeometry.hh   |   3 +
 include/ignition/rendering/base/BaseVisual.hh |  85 +++++++++-
 .../ignition/rendering/ogre/OgreGeometry.hh   |   5 +
 ogre/src/OgreGeometry.cc                      |  36 +++++
 ogre/src/OgreMeshFactory.cc                   |   2 +
 .../ignition/rendering/ogre2/Ogre2Geometry.hh |   9 ++
 ogre2/src/Ogre2Geometry.cc                    |  36 +++++
 ogre2/src/Ogre2MeshFactory.cc                 |   2 +
 src/Visual_TEST.cc                            | 151 +++++++++++++++++-
 11 files changed, 342 insertions(+), 4 deletions(-)

diff --git a/include/ignition/rendering/Geometry.hh b/include/ignition/rendering/Geometry.hh
index 51839a1a5..bb284745b 100644
--- a/include/ignition/rendering/Geometry.hh
+++ b/include/ignition/rendering/Geometry.hh
@@ -66,6 +66,12 @@ namespace ignition
       /// \brief Get the material of this geometry
       /// \return Material used by this geometry
       public: virtual MaterialPtr Material() const = 0;
+
+      /// \brief Clone the geometry with a new name.
+      /// \param[in] _name The name of the cloned geometry.
+      /// \param[in] _newParent The parent of the cloned geometry.
+      public: virtual GeometryPtr Clone(const std::string &_name,
+                  VisualPtr _newParent) const = 0;
     };
     }
   }
diff --git a/include/ignition/rendering/Visual.hh b/include/ignition/rendering/Visual.hh
index c9ec0b635..7bd83b4d1 100644
--- a/include/ignition/rendering/Visual.hh
+++ b/include/ignition/rendering/Visual.hh
@@ -104,7 +104,7 @@ namespace ignition
       /// \return the Pointer to the material assigned to this visual. If the
       /// material is cloned at the time it is set to this visual, the cloned
       /// material will be returned.
-      public: virtual MaterialPtr Material() = 0;
+      public: virtual MaterialPtr Material() const = 0;
 
       /// \brief Enable or disable wireframe
       /// \param[in] _show True to enable wireframe
@@ -142,6 +142,15 @@ namespace ignition
       /// \return The local bounding box
       public: virtual ignition::math::AxisAlignedBox LocalBoundingBox()
               const = 0;
+
+      /// \brief Clone the visual (and its children) with a new name.
+      /// \param[in] _name Name of the cloned Visual. Set this to an empty
+      /// string to auto-generate a unique name for the cloned visual.
+      /// \param[in] _newParent Parent of the cloned Visual. Set to nullptr if
+      /// the cloned visual should have no parent.
+      /// \return The visual. nullptr is returned if cloning failed.
+      public: virtual VisualPtr Clone(const std::string &_name,
+                  NodePtr _newParent) const = 0;
     };
     }
   }
diff --git a/include/ignition/rendering/base/BaseGeometry.hh b/include/ignition/rendering/base/BaseGeometry.hh
index 78578053f..0bdef4a02 100644
--- a/include/ignition/rendering/base/BaseGeometry.hh
+++ b/include/ignition/rendering/base/BaseGeometry.hh
@@ -46,6 +46,9 @@ namespace ignition
       public: virtual void SetMaterial(MaterialPtr _material,
                   bool unique = true) override = 0;
 
+      public: virtual GeometryPtr Clone(const std::string &_name,
+                  VisualPtr _newParent) const override = 0;
+
       // Documentation inherited
       public: virtual void Destroy() override;
     };
diff --git a/include/ignition/rendering/base/BaseVisual.hh b/include/ignition/rendering/base/BaseVisual.hh
index a7e53d488..bbc792375 100644
--- a/include/ignition/rendering/base/BaseVisual.hh
+++ b/include/ignition/rendering/base/BaseVisual.hh
@@ -77,7 +77,7 @@ namespace ignition
                   bool _unique = true) override;
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() override;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetWireframe(bool _show) override;
@@ -114,6 +114,10 @@ namespace ignition
       public: virtual ignition::math::AxisAlignedBox LocalBoundingBox()
               const override;
 
+      // Documentation inherited.
+      public: virtual VisualPtr Clone(const std::string &_name,
+                  NodePtr _newParent) const override;
+
       protected: virtual void PreRenderChildren() override;
 
       protected: virtual void PreRenderGeometries();
@@ -293,7 +297,7 @@ namespace ignition
 
     //////////////////////////////////////////////////
     template <class T>
-    MaterialPtr BaseVisual<T>::Material()
+    MaterialPtr BaseVisual<T>::Material() const
     {
       return this->material;
     }
@@ -471,6 +475,83 @@ namespace ignition
     {
       return this->visibilityFlags;
     }
+
+    //////////////////////////////////////////////////
+    template <class T>
+    VisualPtr BaseVisual<T>::Clone(const std::string &_name,
+        NodePtr _newParent) const
+    {
+      ScenePtr scene_ = this->Scene();
+      if (nullptr == scene_)
+      {
+        ignerr << "Cloning a visual failed because the visual to be cloned is "
+          << "not attached to a scene." << std::endl;
+        return nullptr;
+      }
+      VisualPtr result;
+      if (_name.empty())
+        result = scene_->CreateVisual();
+      else
+        result = scene_->CreateVisual(_name);
+
+      if (nullptr != _newParent)
+      {
+        auto parentScene = _newParent->Scene();
+        if (nullptr != parentScene && parentScene->Id() != scene_->Id())
+        {
+          ignerr << "Cloning a visual failed because the desired parent of the "
+            << "cloned visual belongs to a different scene." << std::endl;
+          scene_->DestroyVisual(result);
+          return nullptr;
+        }
+        _newParent->AddChild(result);
+      }
+
+      result->SetOrigin(this->Origin());
+      result->SetInheritScale(this->InheritScale());
+      result->SetLocalScale(this->LocalScale());
+      result->SetLocalPose(this->LocalPose());
+      result->SetVisibilityFlags(this->VisibilityFlags());
+      result->SetWireframe(this->Wireframe());
+
+      // if the visual that was cloned has child visuals, clone those as well
+      auto children_ =
+          std::dynamic_pointer_cast<BaseStore<ignition::rendering::Node, T>>(
+          this->Children());
+      if (!children_)
+      {
+        ignerr << "Cast failed in BaseVisual::Clone" << std::endl;
+        scene_->DestroyVisual(result);
+        return nullptr;
+      }
+      for (auto it = children_->Begin(); it != children_->End(); ++it)
+      {
+        NodePtr child = it->second;
+        VisualPtr visual = std::dynamic_pointer_cast<Visual>(child);
+        if (nullptr != visual)
+          visual->Clone("", result);
+      }
+
+      for (unsigned int i = 0; i < this->GeometryCount(); ++i)
+      {
+        // TODO(adlarkin) clone the geometry. I will need to add this
+        // functionality. Once it's added, the code may look something like:
+        /*
+         *auto geometry = this->GeometryByIndex(i);
+         *auto clonedGeometry = geometry->Clone();
+         *result->AddGeometry(clonedGeometry);
+         */
+      }
+
+      result->SetMaterial(this->Material());
+
+      // should userData be cloned, or should that be left out since (I believe)
+      // userData is intended to be custom/unique for each visual
+      for (const auto &[key, val] : this->userData)
+        result->SetUserData(key, val);
+
+      return result;
+    }
     }
   }
 }
diff --git a/ogre/include/ignition/rendering/ogre/OgreGeometry.hh b/ogre/include/ignition/rendering/ogre/OgreGeometry.hh
index 2700d3c97..bc49a82c3 100644
--- a/ogre/include/ignition/rendering/ogre/OgreGeometry.hh
+++ b/ogre/include/ignition/rendering/ogre/OgreGeometry.hh
@@ -46,6 +46,11 @@ namespace ignition
 
       public: virtual Ogre::MovableObject *OgreObject() const = 0;
 
+      public: virtual GeometryPtr Clone(const std::string &_name,
+                  VisualPtr _newParent) const override;
+
+      protected: std::string descriptorName;
+
       protected: virtual void SetParent(OgreVisualPtr _parent);
 
       IGN_COMMON_WARN_IGNORE__DLL_INTERFACE_MISSING
diff --git a/ogre/src/OgreGeometry.cc b/ogre/src/OgreGeometry.cc
index 821967c35..d6ca056aa 100644
--- a/ogre/src/OgreGeometry.cc
+++ b/ogre/src/OgreGeometry.cc
@@ -49,3 +49,39 @@ void OgreGeometry::SetParent(OgreVisualPtr _parent)
 {
   this->parent = _parent;
 }
+
+//////////////////////////////////////////////////
+GeometryPtr OgreGeometry::Clone(const std::string &_name,
+    VisualPtr _newParent) const
+{
+  if (nullptr == _newParent)
+  {
+    ignerr << "Cloning a geometry failed because the parent of the clone is "
+      << "null" << std::endl;
+    return nullptr;
+  }
+
+  GeometryPtr result;
+
+  if (this->descriptorName == "unit_box")
+    result = this->Scene()->CreateBox();
+  else if (this->descriptorName == "unit_cone")
+    result = this->Scene()->CreateCone();
+  else if (this->descriptorName == "unit_cylinder")
+    result = this->Scene()->CreateCylinder();
+  else if (this->descriptorName == "unit_plane")
+    result = this->Scene()->CreatePlane();
+  else if (this->descriptorName == "unit_sphere")
+    result = this->Scene()->CreateSphere();
+  else
+  {
+    ignerr << "Attempted to clone a geometry with a descriptor name of ["
+      << this->descriptorName << "], which is invalid." << std::endl;
+    return nullptr;
+  }
+
+  // TODO(adlarkin) need to also set the name of the cloned geometry, the
+  // parent, and check if anything else needs to be set
+
+  return result;
+}
diff --git a/ogre/src/OgreMeshFactory.cc b/ogre/src/OgreMeshFactory.cc
index 875d8c83a..9578a9d07 100644
--- a/ogre/src/OgreMeshFactory.cc
+++ b/ogre/src/OgreMeshFactory.cc
@@ -64,6 +64,8 @@ OgreMeshPtr OgreMeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
+  mesh->descriptorName = _desc.meshName;
+
   // create sub-mesh store
   OgreSubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreEntity);
   mesh->subMeshes = subMeshFactory.Create();
diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh
index fbea7edf5..191f51404 100644
--- a/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh
+++ b/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh
@@ -47,6 +47,10 @@ namespace ignition
       // Documentation inherited.
       public: virtual VisualPtr Parent() const override;
 
+      // Documentation inherited.
+      public: virtual GeometryPtr Clone(const std::string &_name,
+                  VisualPtr _newParent) const override;
+
       /// \brief Get the ogre object representing this geometry
       /// \return Pointer to an ogre movable object
       public: virtual Ogre::MovableObject *OgreObject() const = 0;
@@ -58,6 +62,11 @@ namespace ignition
       /// \brief Parent visual
       protected: Ogre2VisualPtr parent;
 
+      /// \brief The name from the descriptor object that was used
+      /// to help create this geometry (for something like meshes, this would be
+      /// the mesh name from the MeshDescriptor
+      protected: std::string descriptorName;
+
       /// \brief Make ogre2 visual our friend so it can it can access function
       /// for setting the parent of this geometry
       private: friend class Ogre2Visual;
diff --git a/ogre2/src/Ogre2Geometry.cc b/ogre2/src/Ogre2Geometry.cc
index 007b1f7e4..56a769d2c 100644
--- a/ogre2/src/Ogre2Geometry.cc
+++ b/ogre2/src/Ogre2Geometry.cc
@@ -49,3 +49,39 @@ void Ogre2Geometry::SetParent(Ogre2VisualPtr _parent)
 {
   this->parent = _parent;
 }
+
+//////////////////////////////////////////////////
+GeometryPtr Ogre2Geometry::Clone(const std::string &_name,
+    VisualPtr _newParent) const
+{
+  if (nullptr == _newParent)
+  {
+    ignerr << "Cloning a geometry failed because the parent of the clone is "
+      << "null" << std::endl;
+    return nullptr;
+  }
+
+  GeometryPtr result;
+
+  if (this->descriptorName == "unit_box")
+    result = this->Scene()->CreateBox();
+  else if (this->descriptorName == "unit_cone")
+    result = this->Scene()->CreateCone();
+  else if (this->descriptorName == "unit_cylinder")
+    result = this->Scene()->CreateCylinder();
+  else if (this->descriptorName == "unit_plane")
+    result = this->Scene()->CreatePlane();
+  else if (this->descriptorName == "unit_sphere")
+    result = this->Scene()->CreateSphere();
+  else
+  {
+    ignerr << "Attempted to clone a geometry with a descriptor name of ["
+      << this->descriptorName << "], which is invalid." << std::endl;
+    return nullptr;
+  }
+
+  // TODO(adlarkin) need to also set the name of the cloned geometry, the
+  // parent, and check if anything else needs to be set
+
+  return result;
+}
diff --git a/ogre2/src/Ogre2MeshFactory.cc b/ogre2/src/Ogre2MeshFactory.cc
index a896332b2..02561bc2c 100644
--- a/ogre2/src/Ogre2MeshFactory.cc
+++ b/ogre2/src/Ogre2MeshFactory.cc
@@ -127,6 +127,8 @@ Ogre2MeshPtr Ogre2MeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
+  mesh->descriptorName = _desc.meshName;
+
   // create sub-mesh store
   Ogre2SubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreItem);
   mesh->subMeshes = subMeshFactory.Create();
diff --git a/src/Visual_TEST.cc b/src/Visual_TEST.cc
index 46cba7e86..09e4a1a11 100644
--- a/src/Visual_TEST.cc
+++ b/src/Visual_TEST.cc
@@ -58,6 +58,9 @@ class VisualTest : public testing::Test,
 
   /// \brief Test changing to wireframe
   public: void Wireframe(const std::string &_renderEngine);
+
+  /// \brief Test cloning visuals
+  public: void Clone(const std::string &_renderEngine);
 };
 
 /////////////////////////////////////////////////
@@ -569,7 +572,7 @@ void VisualTest::BoundingBox(const std::string &_renderEngine)
 }
 
 /////////////////////////////////////////////////
-TEST_P(VisualTest, BoundingBox)
+TEST_P(VisualTest, Boundingbox)
 {
   BoundingBox(GetParam());
 }
@@ -603,6 +606,152 @@ TEST_P(VisualTest, Wireframe)
   Wireframe(GetParam());
 }
 
+/////////////////////////////////////////////////
+void VisualTest::Clone(const std::string &_renderEngine)
+{
+  RenderEngine *engine = rendering::engine(_renderEngine);
+  if (!engine)
+  {
+    igndbg << "Engine '" << _renderEngine
+              << "' is not supported" << std::endl;
+    return;
+  }
+
+  ScenePtr scene = engine->CreateScene("scene7");
+  ASSERT_NE(nullptr, scene);
+
+  VisualPtr parent = scene->CreateVisual();
+  ASSERT_NE(nullptr, parent);
+
+  // add descendant visuals (one child, one grandchild)
+  VisualPtr child = scene->CreateVisual();
+  ASSERT_NE(nullptr, child);
+  parent->AddChild(child);
+  VisualPtr grandChild = scene->CreateVisual();
+  ASSERT_NE(nullptr, grandChild);
+  child->AddChild(grandChild);
+  EXPECT_EQ(1u, parent->ChildCount());
+  EXPECT_EQ(1u, child->ChildCount());
+  EXPECT_EQ(0u, grandChild->ChildCount());
+
+  // create geometries
+  GeometryPtr parentBox = scene->CreateBox();
+  parent->AddGeometry(parentBox);
+  GeometryPtr childCylinder = scene->CreateCylinder();
+  child->AddGeometry(childCylinder);
+  GeometryPtr grandChildSphere = scene->CreateSphere();
+  grandChild->AddGeometry(grandChildSphere);
+
+  // create material
+  math::Color ambient(0.5f, 0.2f, 0.4f, 1.0f);
+  math::Color diffuse(0.1f, 0.9f, 0.3f, 1.0f);
+  math::Color specular(0.8f, 0.7f, 0.0f, 1.0f);
+  double transparency = 0.3;
+  MaterialPtr material = scene->CreateMaterial("unique");
+  ASSERT_NE(nullptr, material);
+  EXPECT_TRUE(scene->MaterialRegistered("unique"));
+  material->SetAmbient(ambient);
+  material->SetDiffuse(diffuse);
+  material->SetSpecular(specular);
+  material->SetTransparency(transparency);
+  // this SetMaterial call applies the material to all of the geometries for the
+  // visual (including geometries of child visuals)
+  parent->SetMaterial(material);
+
+  // set scale
+  const math::Vector3d parentLocalScale(1, 2, 3);
+  parent->SetLocalScale(parentLocalScale);
+  const math::Vector3d childLocalScale(4, 5, 6);
+  child->SetLocalScale(childLocalScale);
+  child->SetInheritScale(true);
+  const math::Vector3d grandChildLocalScale(7, 8, 9);
+  grandChild->SetLocalScale(grandChildLocalScale);
+  grandChild->SetInheritScale(false);
+
+  // set user data
+  const auto parentUserData = "parent";
+  parent->SetUserData(parentUserData, parentUserData);
+  const auto childUserData = "child";
+  child->SetUserData(childUserData, childUserData);
+  const auto grandChildUserData = "grandChild";
+  grandChild->SetUserData(grandChildUserData, grandChildUserData);
+
+  // set visibility flags
+  const uint32_t visibilityFlags = 0x00000001u;
+  parent->SetVisibilityFlags(visibilityFlags);
+  const uint32_t grandChildVisibilityFlags = 0x01000000u;
+  grandChild->SetVisibilityFlags(grandChildVisibilityFlags);
+  EXPECT_EQ(visibilityFlags, parent->VisibilityFlags());
+  EXPECT_EQ(visibilityFlags, child->VisibilityFlags());
+  EXPECT_EQ(grandChildVisibilityFlags, grandChild->VisibilityFlags());
+
+  // set pose
+  const math::Pose3d parentPose(1, 1, 1, 0, 0, 0);
+  parent->SetWorldPose(parentPose);
+  const math::Pose3d grandChildPoseOffset(1, 1, 1, 0, 0, 1);
+  grandChild->SetLocalPose(grandChildPoseOffset);
+  EXPECT_EQ(parentPose, parent->WorldPose());
+  EXPECT_EQ(parentPose, child->WorldPose());
+  EXPECT_EQ(parentPose * grandChildPoseOffset, grandChild->WorldPose());
+
+  // set wireframe
+  parent->SetWireframe(true);
+  child->SetWireframe(false);
+  grandChild->SetWireframe(true);
+
+  // clone the parent visual
+  const auto preCloneNodeCount = scene->NodeCount();
+  const auto clonedVisualName = "clonedVisual";
+  auto clonedVisual = parent->Clone(clonedVisualName, parent->Parent());
+  ASSERT_NE(nullptr, clonedVisual);
+  EXPECT_GT(scene->NodeCount(), preCloneNodeCount);
+
+  // check the clone
+  EXPECT_EQ(clonedVisualName, clonedVisual->Name());
+  EXPECT_NE(clonedVisual->Name(), parent->Name());
+  EXPECT_NE(clonedVisual->Id(), parent->Id());
+  EXPECT_EQ(clonedVisual->Scene(), parent->Scene());
+  EXPECT_EQ(clonedVisual->ChildCount(), parent->ChildCount());
+  EXPECT_EQ(clonedVisual->LocalScale(), parent->LocalScale());
+  EXPECT_EQ(clonedVisual->WorldScale(), parent->WorldScale());
+  EXPECT_EQ(clonedVisual->UserData(parentUserData),
+      parent->UserData(parentUserData));
+  EXPECT_EQ(clonedVisual->VisibilityFlags(), parent->VisibilityFlags());
+  EXPECT_EQ(clonedVisual->WorldPose(), parent->WorldPose());
+  EXPECT_EQ(clonedVisual->LocalPose(), parent->LocalPose());
+  EXPECT_EQ(clonedVisual->Wireframe(), parent->Wireframe());
+
+  // TODO(adlarkin) compare geometries once cloning is implemented for those
+
+  // compare materials (the material is cloned, so the name is different but
+  // the properties of the material are the same)
+  auto clonedVisualMaterial = clonedVisual->Material();
+  auto originalVisualMaterial = parent->Material();
+  ASSERT_NE(nullptr, clonedVisualMaterial);
+  ASSERT_NE(nullptr, originalVisualMaterial);
+  EXPECT_NE(clonedVisualMaterial, originalVisualMaterial);
+  EXPECT_NE(clonedVisualMaterial->Name(), originalVisualMaterial->Name());
+  EXPECT_EQ(clonedVisualMaterial->Type(), originalVisualMaterial->Type());
+  EXPECT_EQ(clonedVisualMaterial->Ambient(), originalVisualMaterial->Ambient());
+  EXPECT_EQ(clonedVisualMaterial->Diffuse(), originalVisualMaterial->Diffuse());
+  EXPECT_EQ(clonedVisualMaterial->Specular(),
+      originalVisualMaterial->Specular());
+  EXPECT_DOUBLE_EQ(clonedVisualMaterial->Transparency(),
+      originalVisualMaterial->Transparency());
+
+  // TODO(adlarkin) compare children
+
+  // clean up
+  engine->DestroyScene(scene);
+  rendering::unloadEngine(engine->Name());
+}
+
+/////////////////////////////////////////////////
+TEST_P(VisualTest, Clone)
+{
+  Clone(GetParam());
+}
+
 
 INSTANTIATE_TEST_CASE_P(Visual, VisualTest,
     RENDER_ENGINE_VALUES,

From b44d6e856dd3e4836ca8f6ca4c0d27e2aa7dd552 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Wed, 8 Sep 2021 13:55:53 -0400
Subject: [PATCH 02/12] have visuals clone geometries. Fix segfaults

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/base/BaseVisual.hh | 23 +++++++++++--------
 ogre/src/OgreGeometry.cc                      |  4 +---
 ogre2/src/Ogre2Geometry.cc                    |  4 +---
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/include/ignition/rendering/base/BaseVisual.hh b/include/ignition/rendering/base/BaseVisual.hh
index bbc792375..0d6404da0 100644
--- a/include/ignition/rendering/base/BaseVisual.hh
+++ b/include/ignition/rendering/base/BaseVisual.hh
@@ -534,21 +534,24 @@ namespace ignition
 
       for (unsigned int i = 0; i < this->GeometryCount(); ++i)
       {
-        // TODO(adlarkin) clone the geometry. I will need to add this
-        // functionality. Once it's added, the code may look something like:
-        /*
-         *auto geometry = this->GeometryByIndex(i);
-         *auto clonedGeometry = geometry->Clone();
-         *result->AddGeometry(clonedGeometry);
-         */
+        auto geometry = this->GeometryByIndex(i);
+        // TODO(adlarkin) fix the Clone call below once geometry cloning
+        // approach is cleaned up
+        auto clonedGeometry = geometry->Clone("", nullptr);
+        result->AddGeometry(clonedGeometry);
       }
 
-      result->SetMaterial(this->Material());
+      if (this->Material())
+        result->SetMaterial(this->Material());
 
       // should userData be cloned, or should that be left out since (I believe)
       // userData is intended to be custom/unique for each visual
-      for (const auto &[key, val] : this->userData)
-        result->SetUserData(key, val);
+      //
+      // copying userData seems to segfault, so this is commented out for now
+      /*
+       *for (const auto &[key, val] : this->userData)
+       *  result->SetUserData(key, val);
+       */
 
       return result;
     }
diff --git a/ogre/src/OgreGeometry.cc b/ogre/src/OgreGeometry.cc
index d6ca056aa..b20e50c11 100644
--- a/ogre/src/OgreGeometry.cc
+++ b/ogre/src/OgreGeometry.cc
@@ -56,9 +56,7 @@ GeometryPtr OgreGeometry::Clone(const std::string &_name,
 {
   if (nullptr == _newParent)
   {
-    ignerr << "Cloning a geometry failed because the parent of the clone is "
-      << "null" << std::endl;
-    return nullptr;
+    // TODO(adlarkin) handle this case?
   }
 
   GeometryPtr result;
diff --git a/ogre2/src/Ogre2Geometry.cc b/ogre2/src/Ogre2Geometry.cc
index 56a769d2c..32e781f10 100644
--- a/ogre2/src/Ogre2Geometry.cc
+++ b/ogre2/src/Ogre2Geometry.cc
@@ -56,9 +56,7 @@ GeometryPtr Ogre2Geometry::Clone(const std::string &_name,
 {
   if (nullptr == _newParent)
   {
-    ignerr << "Cloning a geometry failed because the parent of the clone is "
-      << "null" << std::endl;
-    return nullptr;
+    // TODO(adlarkin) handle this case?
   }
 
   GeometryPtr result;

From 0f89be6dec80e74baf32873edf71a8ad33c568a7 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Wed, 8 Sep 2021 22:30:01 -0400
Subject: [PATCH 03/12] update geometry cloning to handle any mesh

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/Geometry.hh        |  9 +++--
 .../ignition/rendering/base/BaseGeometry.hh   | 34 +++++++++++++++++--
 include/ignition/rendering/base/BaseVisual.hh | 10 ++----
 .../ignition/rendering/ogre/OgreGeometry.hh   |  5 ---
 ogre/src/OgreGeometry.cc                      | 34 -------------------
 ogre/src/OgreMeshFactory.cc                   |  2 +-
 .../ignition/rendering/ogre2/Ogre2Geometry.hh |  9 -----
 ogre2/src/Ogre2Capsule.cc                     |  8 ++---
 ogre2/src/Ogre2Geometry.cc                    | 34 -------------------
 ogre2/src/Ogre2MeshFactory.cc                 |  2 +-
 10 files changed, 44 insertions(+), 103 deletions(-)

diff --git a/include/ignition/rendering/Geometry.hh b/include/ignition/rendering/Geometry.hh
index bb284745b..697d4346f 100644
--- a/include/ignition/rendering/Geometry.hh
+++ b/include/ignition/rendering/Geometry.hh
@@ -18,6 +18,7 @@
 #define IGNITION_RENDERING_GEOMETRY_HH_
 
 #include <string>
+
 #include "ignition/rendering/config.hh"
 #include "ignition/rendering/RenderTypes.hh"
 #include "ignition/rendering/Object.hh"
@@ -67,11 +68,9 @@ namespace ignition
       /// \return Material used by this geometry
       public: virtual MaterialPtr Material() const = 0;
 
-      /// \brief Clone the geometry with a new name.
-      /// \param[in] _name The name of the cloned geometry.
-      /// \param[in] _newParent The parent of the cloned geometry.
-      public: virtual GeometryPtr Clone(const std::string &_name,
-                  VisualPtr _newParent) const = 0;
+      /// \brief Clone the geometry.
+      /// \return The cloned geometry.
+      public: virtual GeometryPtr Clone() const = 0;
     };
     }
   }
diff --git a/include/ignition/rendering/base/BaseGeometry.hh b/include/ignition/rendering/base/BaseGeometry.hh
index 0bdef4a02..eec653086 100644
--- a/include/ignition/rendering/base/BaseGeometry.hh
+++ b/include/ignition/rendering/base/BaseGeometry.hh
@@ -18,6 +18,9 @@
 #define IGNITION_RENDERING_BASE_BASEGEOMETRY_HH_
 
 #include <string>
+
+#include <ignition/common/Console.hh>
+
 #include "ignition/rendering/Geometry.hh"
 #include "ignition/rendering/Scene.hh"
 
@@ -46,11 +49,13 @@ namespace ignition
       public: virtual void SetMaterial(MaterialPtr _material,
                   bool unique = true) override = 0;
 
-      public: virtual GeometryPtr Clone(const std::string &_name,
-                  VisualPtr _newParent) const override = 0;
+      public: virtual GeometryPtr Clone() const override;
 
       // Documentation inherited
       public: virtual void Destroy() override;
+
+      /// \brief Name of the mesh for this geometry
+      protected: std::string meshName;
     };
 
     //////////////////////////////////////////////////
@@ -90,12 +95,37 @@ namespace ignition
       if (material) this->SetMaterial(material, _unique);
     }
 
+    //////////////////////////////////////////////////
+    template <class T>
+    GeometryPtr BaseGeometry<T>::Clone() const
+    {
+      if (!this->Scene())
+      {
+        ignerr << "Cloning a geometry failed because the geometry to be "
+          << "cloned does not belong to a scene." << std::endl;
+        return nullptr;
+      }
+      else if (this->meshName.empty())
+      {
+        ignerr << "Cloning a geometry failed because the name of the mesh is "
+          << "missing." << std::endl;
+        return nullptr;
+      }
+
+      auto result = this->Scene()->CreateMesh(this->meshName);
+      if (result && this->Material())
+        result->SetMaterial(this->Material());
+
+      return result;
+    }
+
     //////////////////////////////////////////////////
     template <class T>
     void BaseGeometry<T>::Destroy()
     {
       T::Destroy();
       this->RemoveParent();
+      this->meshName = "";
     }
     }
   }
diff --git a/include/ignition/rendering/base/BaseVisual.hh b/include/ignition/rendering/base/BaseVisual.hh
index 0d6404da0..465da8956 100644
--- a/include/ignition/rendering/base/BaseVisual.hh
+++ b/include/ignition/rendering/base/BaseVisual.hh
@@ -533,18 +533,12 @@ namespace ignition
       }
 
       for (unsigned int i = 0; i < this->GeometryCount(); ++i)
-      {
-        auto geometry = this->GeometryByIndex(i);
-        // TODO(adlarkin) fix the Clone call below once geometry cloning
-        // approach is cleaned up
-        auto clonedGeometry = geometry->Clone("", nullptr);
-        result->AddGeometry(clonedGeometry);
-      }
+        result->AddGeometry(this->GeometryByIndex(i)->Clone());
 
       if (this->Material())
         result->SetMaterial(this->Material());
 
-      // should userData be cloned, or should that be left out since (I believe)
+      // should userData be cloned? Or should that be left out since (I believe)
       // userData is intended to be custom/unique for each visual
       //
       // copying userData seems to segfault, so this is commented out for now
diff --git a/ogre/include/ignition/rendering/ogre/OgreGeometry.hh b/ogre/include/ignition/rendering/ogre/OgreGeometry.hh
index bc49a82c3..2700d3c97 100644
--- a/ogre/include/ignition/rendering/ogre/OgreGeometry.hh
+++ b/ogre/include/ignition/rendering/ogre/OgreGeometry.hh
@@ -46,11 +46,6 @@ namespace ignition
 
       public: virtual Ogre::MovableObject *OgreObject() const = 0;
 
-      public: virtual GeometryPtr Clone(const std::string &_name,
-                  VisualPtr _newParent) const override;
-
-      protected: std::string descriptorName;
-
       protected: virtual void SetParent(OgreVisualPtr _parent);
 
       IGN_COMMON_WARN_IGNORE__DLL_INTERFACE_MISSING
diff --git a/ogre/src/OgreGeometry.cc b/ogre/src/OgreGeometry.cc
index b20e50c11..821967c35 100644
--- a/ogre/src/OgreGeometry.cc
+++ b/ogre/src/OgreGeometry.cc
@@ -49,37 +49,3 @@ void OgreGeometry::SetParent(OgreVisualPtr _parent)
 {
   this->parent = _parent;
 }
-
-//////////////////////////////////////////////////
-GeometryPtr OgreGeometry::Clone(const std::string &_name,
-    VisualPtr _newParent) const
-{
-  if (nullptr == _newParent)
-  {
-    // TODO(adlarkin) handle this case?
-  }
-
-  GeometryPtr result;
-
-  if (this->descriptorName == "unit_box")
-    result = this->Scene()->CreateBox();
-  else if (this->descriptorName == "unit_cone")
-    result = this->Scene()->CreateCone();
-  else if (this->descriptorName == "unit_cylinder")
-    result = this->Scene()->CreateCylinder();
-  else if (this->descriptorName == "unit_plane")
-    result = this->Scene()->CreatePlane();
-  else if (this->descriptorName == "unit_sphere")
-    result = this->Scene()->CreateSphere();
-  else
-  {
-    ignerr << "Attempted to clone a geometry with a descriptor name of ["
-      << this->descriptorName << "], which is invalid." << std::endl;
-    return nullptr;
-  }
-
-  // TODO(adlarkin) need to also set the name of the cloned geometry, the
-  // parent, and check if anything else needs to be set
-
-  return result;
-}
diff --git a/ogre/src/OgreMeshFactory.cc b/ogre/src/OgreMeshFactory.cc
index 9578a9d07..59ca33510 100644
--- a/ogre/src/OgreMeshFactory.cc
+++ b/ogre/src/OgreMeshFactory.cc
@@ -64,7 +64,7 @@ OgreMeshPtr OgreMeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
-  mesh->descriptorName = _desc.meshName;
+  mesh->meshName = _desc.meshName;
 
   // create sub-mesh store
   OgreSubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreEntity);
diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh
index 191f51404..fbea7edf5 100644
--- a/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh
+++ b/ogre2/include/ignition/rendering/ogre2/Ogre2Geometry.hh
@@ -47,10 +47,6 @@ namespace ignition
       // Documentation inherited.
       public: virtual VisualPtr Parent() const override;
 
-      // Documentation inherited.
-      public: virtual GeometryPtr Clone(const std::string &_name,
-                  VisualPtr _newParent) const override;
-
       /// \brief Get the ogre object representing this geometry
       /// \return Pointer to an ogre movable object
       public: virtual Ogre::MovableObject *OgreObject() const = 0;
@@ -62,11 +58,6 @@ namespace ignition
       /// \brief Parent visual
       protected: Ogre2VisualPtr parent;
 
-      /// \brief The name from the descriptor object that was used
-      /// to help create this geometry (for something like meshes, this would be
-      /// the mesh name from the MeshDescriptor
-      protected: std::string descriptorName;
-
       /// \brief Make ogre2 visual our friend so it can it can access function
       /// for setting the parent of this geometry
       private: friend class Ogre2Visual;
diff --git a/ogre2/src/Ogre2Capsule.cc b/ogre2/src/Ogre2Capsule.cc
index cf6560e5f..efaf9d850 100644
--- a/ogre2/src/Ogre2Capsule.cc
+++ b/ogre2/src/Ogre2Capsule.cc
@@ -102,9 +102,9 @@ void Ogre2Capsule::Update()
     meshMgr->CreateCapsule(capsuleMeshName, this->radius, this->length, 32, 32);
   }
 
-  MeshDescriptor meshDescriptor;
-  meshDescriptor.mesh = meshMgr->MeshByName(capsuleMeshName);
-  if (meshDescriptor.mesh == nullptr)
+  MeshDescriptor descriptor;
+  descriptor.mesh = meshMgr->MeshByName(capsuleMeshName);
+  if (descriptor.mesh == nullptr)
   {
     ignerr << "Capsule mesh is unavailable in the Mesh Manager" << std::endl;
     return;
@@ -123,7 +123,7 @@ void Ogre2Capsule::Update()
     this->dataPtr->ogreMesh->Destroy();
   }
   this->dataPtr->ogreMesh = std::dynamic_pointer_cast<Ogre2Mesh>(
-      this->Scene()->CreateMesh(meshDescriptor));
+      this->Scene()->CreateMesh(descriptor));
   if (this->dataPtr->material != nullptr)
   {
     this->dataPtr->ogreMesh->SetMaterial(this->dataPtr->material, false);
diff --git a/ogre2/src/Ogre2Geometry.cc b/ogre2/src/Ogre2Geometry.cc
index 32e781f10..007b1f7e4 100644
--- a/ogre2/src/Ogre2Geometry.cc
+++ b/ogre2/src/Ogre2Geometry.cc
@@ -49,37 +49,3 @@ void Ogre2Geometry::SetParent(Ogre2VisualPtr _parent)
 {
   this->parent = _parent;
 }
-
-//////////////////////////////////////////////////
-GeometryPtr Ogre2Geometry::Clone(const std::string &_name,
-    VisualPtr _newParent) const
-{
-  if (nullptr == _newParent)
-  {
-    // TODO(adlarkin) handle this case?
-  }
-
-  GeometryPtr result;
-
-  if (this->descriptorName == "unit_box")
-    result = this->Scene()->CreateBox();
-  else if (this->descriptorName == "unit_cone")
-    result = this->Scene()->CreateCone();
-  else if (this->descriptorName == "unit_cylinder")
-    result = this->Scene()->CreateCylinder();
-  else if (this->descriptorName == "unit_plane")
-    result = this->Scene()->CreatePlane();
-  else if (this->descriptorName == "unit_sphere")
-    result = this->Scene()->CreateSphere();
-  else
-  {
-    ignerr << "Attempted to clone a geometry with a descriptor name of ["
-      << this->descriptorName << "], which is invalid." << std::endl;
-    return nullptr;
-  }
-
-  // TODO(adlarkin) need to also set the name of the cloned geometry, the
-  // parent, and check if anything else needs to be set
-
-  return result;
-}
diff --git a/ogre2/src/Ogre2MeshFactory.cc b/ogre2/src/Ogre2MeshFactory.cc
index 02561bc2c..f2d856b7b 100644
--- a/ogre2/src/Ogre2MeshFactory.cc
+++ b/ogre2/src/Ogre2MeshFactory.cc
@@ -127,7 +127,7 @@ Ogre2MeshPtr Ogre2MeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
-  mesh->descriptorName = _desc.meshName;
+  mesh->meshName = _desc.meshName;
 
   // create sub-mesh store
   Ogre2SubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreItem);

From cbce76f9ca8d5490eaa378dbd7fe94b4bd9eb756 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Tue, 14 Sep 2021 12:35:27 -0400
Subject: [PATCH 04/12] review feedback

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/base/BaseGeometry.hh | 4 ++--
 include/ignition/rendering/base/BaseVisual.hh   | 6 +++---
 ogre2/src/Ogre2Capsule.cc                       | 8 ++++----
 src/Visual_TEST.cc                              | 4 ++--
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/ignition/rendering/base/BaseGeometry.hh b/include/ignition/rendering/base/BaseGeometry.hh
index eec653086..7ad30ae70 100644
--- a/include/ignition/rendering/base/BaseGeometry.hh
+++ b/include/ignition/rendering/base/BaseGeometry.hh
@@ -102,13 +102,13 @@ namespace ignition
       if (!this->Scene())
       {
         ignerr << "Cloning a geometry failed because the geometry to be "
-          << "cloned does not belong to a scene." << std::endl;
+          << "cloned does not belong to a scene.\n";
         return nullptr;
       }
       else if (this->meshName.empty())
       {
         ignerr << "Cloning a geometry failed because the name of the mesh is "
-          << "missing." << std::endl;
+          << "missing.\n";
         return nullptr;
       }
 
diff --git a/include/ignition/rendering/base/BaseVisual.hh b/include/ignition/rendering/base/BaseVisual.hh
index 465da8956..121fa89ef 100644
--- a/include/ignition/rendering/base/BaseVisual.hh
+++ b/include/ignition/rendering/base/BaseVisual.hh
@@ -485,7 +485,7 @@ namespace ignition
       if (nullptr == scene_)
       {
         ignerr << "Cloning a visual failed because the visual to be cloned is "
-          << "not attached to a scene." << std::endl;
+          << "not attached to a scene.\n";
         return nullptr;
       }
       VisualPtr result;
@@ -500,7 +500,7 @@ namespace ignition
         if (nullptr != parentScene && parentScene->Id() != scene_->Id())
         {
           ignerr << "Cloning a visual failed because the desired parent of the "
-            << "cloned visual belongs to a different scene." << std::endl;
+            << "cloned visual belongs to a different scene.\n";
           scene_->DestroyVisual(result);
           return nullptr;
         }
@@ -520,7 +520,7 @@ namespace ignition
           this->Children());
       if (!children_)
       {
-        ignerr << "Cast failed in BaseVisual::Clone" << std::endl;
+        ignerr << "Cast failed in BaseVisual::Clone\n";
         scene_->DestroyVisual(result);
         return nullptr;
       }
diff --git a/ogre2/src/Ogre2Capsule.cc b/ogre2/src/Ogre2Capsule.cc
index efaf9d850..cf6560e5f 100644
--- a/ogre2/src/Ogre2Capsule.cc
+++ b/ogre2/src/Ogre2Capsule.cc
@@ -102,9 +102,9 @@ void Ogre2Capsule::Update()
     meshMgr->CreateCapsule(capsuleMeshName, this->radius, this->length, 32, 32);
   }
 
-  MeshDescriptor descriptor;
-  descriptor.mesh = meshMgr->MeshByName(capsuleMeshName);
-  if (descriptor.mesh == nullptr)
+  MeshDescriptor meshDescriptor;
+  meshDescriptor.mesh = meshMgr->MeshByName(capsuleMeshName);
+  if (meshDescriptor.mesh == nullptr)
   {
     ignerr << "Capsule mesh is unavailable in the Mesh Manager" << std::endl;
     return;
@@ -123,7 +123,7 @@ void Ogre2Capsule::Update()
     this->dataPtr->ogreMesh->Destroy();
   }
   this->dataPtr->ogreMesh = std::dynamic_pointer_cast<Ogre2Mesh>(
-      this->Scene()->CreateMesh(descriptor));
+      this->Scene()->CreateMesh(meshDescriptor));
   if (this->dataPtr->material != nullptr)
   {
     this->dataPtr->ogreMesh->SetMaterial(this->dataPtr->material, false);
diff --git a/src/Visual_TEST.cc b/src/Visual_TEST.cc
index 09e4a1a11..16bf027db 100644
--- a/src/Visual_TEST.cc
+++ b/src/Visual_TEST.cc
@@ -572,7 +572,7 @@ void VisualTest::BoundingBox(const std::string &_renderEngine)
 }
 
 /////////////////////////////////////////////////
-TEST_P(VisualTest, Boundingbox)
+TEST_P(VisualTest, BoundingBox)
 {
   BoundingBox(GetParam());
 }
@@ -584,7 +584,7 @@ void VisualTest::Wireframe(const std::string &_renderEngine)
   if (!engine)
   {
     igndbg << "Engine '" << _renderEngine
-              << "' is not supported" << std::endl;
+              << "' is not supported\n";
     return;
   }
 

From 7a8d57ae0139e8ee04c54c256fa003b876eddda7 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Tue, 14 Sep 2021 13:54:05 -0400
Subject: [PATCH 05/12] use unique scene name for Visual clone unit test

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 src/Visual_TEST.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/Visual_TEST.cc b/src/Visual_TEST.cc
index 16bf027db..a7fbf09dd 100644
--- a/src/Visual_TEST.cc
+++ b/src/Visual_TEST.cc
@@ -617,7 +617,7 @@ void VisualTest::Clone(const std::string &_renderEngine)
     return;
   }
 
-  ScenePtr scene = engine->CreateScene("scene7");
+  ScenePtr scene = engine->CreateScene("scene8");
   ASSERT_NE(nullptr, scene);
 
   VisualPtr parent = scene->CreateVisual();

From 0a3f104a324b6d5871c600c269a55a7928395b5b Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Tue, 14 Sep 2021 15:58:40 -0400
Subject: [PATCH 06/12] clone userData

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/base/BaseVisual.hh | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/include/ignition/rendering/base/BaseVisual.hh b/include/ignition/rendering/base/BaseVisual.hh
index 121fa89ef..a234d82b9 100644
--- a/include/ignition/rendering/base/BaseVisual.hh
+++ b/include/ignition/rendering/base/BaseVisual.hh
@@ -538,14 +538,8 @@ namespace ignition
       if (this->Material())
         result->SetMaterial(this->Material());
 
-      // should userData be cloned? Or should that be left out since (I believe)
-      // userData is intended to be custom/unique for each visual
-      //
-      // copying userData seems to segfault, so this is commented out for now
-      /*
-       *for (const auto &[key, val] : this->userData)
-       *  result->SetUserData(key, val);
-       */
+      for (const auto &[key, val] : this->userData)
+        result->SetUserData(key, val);
 
       return result;
     }

From f5b07ea19707d9f2ba97565f5d7d5140d496f238 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Thu, 16 Sep 2021 15:05:33 -0400
Subject: [PATCH 07/12] copy MeshDescriptor and move Clone implementation to
 Mesh class

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/Mesh.hh            |  7 +++
 .../ignition/rendering/base/BaseGeometry.hh   | 23 +------
 include/ignition/rendering/base/BaseMesh.hh   | 61 +++++++++++++++++++
 ogre/src/OgreMeshFactory.cc                   |  2 +-
 ogre2/src/Ogre2MeshFactory.cc                 |  2 +-
 5 files changed, 71 insertions(+), 24 deletions(-)

diff --git a/include/ignition/rendering/Mesh.hh b/include/ignition/rendering/Mesh.hh
index cd9b7bdb5..2fb7644a7 100644
--- a/include/ignition/rendering/Mesh.hh
+++ b/include/ignition/rendering/Mesh.hh
@@ -23,6 +23,7 @@
 #include <ignition/math/Matrix4.hh>
 #include "ignition/rendering/config.hh"
 #include "ignition/rendering/Geometry.hh"
+#include "ignition/rendering/MeshDescriptor.hh"
 #include "ignition/rendering/Object.hh"
 
 namespace ignition
@@ -119,6 +120,12 @@ namespace ignition
       /// \return The sub-mesh at the given index
       public: virtual SubMeshPtr SubMeshByIndex(
                   unsigned int _index) const = 0;
+
+      /// \brief Copy an existing MeshDescriptor. Once copying is complete,
+      /// MeshDescriptor::Load needs to be called on the mesh's MeshDescriptor
+      /// before using it
+      /// \param[in] _copy The MeshDescriptor to copy
+      public: virtual void CopyMeshDescriptor(const MeshDescriptor &_copy) = 0;
     };
 
     /// \class SubMesh Mesh.hh ignition/rendering/Mesh.hh
diff --git a/include/ignition/rendering/base/BaseGeometry.hh b/include/ignition/rendering/base/BaseGeometry.hh
index d0bb7af1f..8d5cf685e 100644
--- a/include/ignition/rendering/base/BaseGeometry.hh
+++ b/include/ignition/rendering/base/BaseGeometry.hh
@@ -55,9 +55,6 @@ namespace ignition
 
       // Documentation inherited
       public: virtual void Destroy() override;
-
-      /// \brief Name of the mesh for this geometry
-      protected: std::string meshName;
     };
 
     //////////////////////////////////////////////////
@@ -101,24 +98,7 @@ namespace ignition
     template <class T>
     GeometryPtr BaseGeometry<T>::Clone() const
     {
-      if (!this->Scene())
-      {
-        ignerr << "Cloning a geometry failed because the geometry to be "
-          << "cloned does not belong to a scene.\n";
-        return nullptr;
-      }
-      else if (this->meshName.empty())
-      {
-        ignerr << "Cloning a geometry failed because the name of the mesh is "
-          << "missing.\n";
-        return nullptr;
-      }
-
-      auto result = this->Scene()->CreateMesh(this->meshName);
-      if (result && this->Material())
-        result->SetMaterial(this->Material());
-
-      return result;
+      return nullptr;
     }
 
     //////////////////////////////////////////////////
@@ -127,7 +107,6 @@ namespace ignition
     {
       T::Destroy();
       this->RemoveParent();
-      this->meshName = "";
     }
     }
   }
diff --git a/include/ignition/rendering/base/BaseMesh.hh b/include/ignition/rendering/base/BaseMesh.hh
index 4f55794db..ca0858a5f 100644
--- a/include/ignition/rendering/base/BaseMesh.hh
+++ b/include/ignition/rendering/base/BaseMesh.hh
@@ -100,6 +100,11 @@ namespace ignition
 
       public: virtual void PreRender() override;
 
+      public: virtual GeometryPtr Clone() const override;
+
+      public: virtual void CopyMeshDescriptor(
+                  const MeshDescriptor &_copy) override;
+
       // Documentation inherited
       public: virtual void Destroy() override;
 
@@ -111,6 +116,9 @@ namespace ignition
 
       /// \brief Pointer to currently assigned material
       protected: MaterialPtr material;
+
+      /// \brief MeshDescriptor for this mesh
+      protected: MeshDescriptor meshDescriptor;
     };
 
     //////////////////////////////////////////////////
@@ -315,6 +323,58 @@ namespace ignition
       T::PreRender();
     }
 
+    //////////////////////////////////////////////////
+    template <class T>
+    GeometryPtr BaseMesh<T>::Clone() const
+    {
+      if (!this->Scene())
+      {
+        ignerr << "Cloning a geometry failed because the geometry to be "
+          << "cloned does not belong to a scene.\n";
+        return nullptr;
+      }
+      else if (this->meshDescriptor.meshName.empty())
+      {
+        ignerr << "Cloning a geometry failed because the name of the mesh is "
+          << "missing.\n";
+        return nullptr;
+      }
+
+      auto result = this->Scene()->CreateMesh(this->meshDescriptor.meshName);
+      if (result)
+      {
+        result->CopyMeshDescriptor(this->meshDescriptor);
+
+        if (this->Material())
+        {
+          // this call will set the material for the mesh and its submeshes
+          result->SetMaterial(this->Material());
+        }
+        else
+        {
+          // if the mesh doesn't have a material, clone any existing submesh
+          // materials
+          for (unsigned int i = 0; i < this->SubMeshCount(); ++i)
+          {
+            auto existingSubMeshMaterial = this->SubMeshByIndex(i)->Material();
+            if (existingSubMeshMaterial)
+              result->SubMeshByIndex(i)->SetMaterial(existingSubMeshMaterial);
+          }
+        }
+      }
+
+      return result;
+    }
+
+    //////////////////////////////////////////////////
+    template <class T>
+    void BaseMesh<T>::CopyMeshDescriptor(const MeshDescriptor &_copy)
+    {
+      this->meshDescriptor.meshName = _copy.meshName;
+      this->meshDescriptor.subMeshName = _copy.subMeshName;
+      this->meshDescriptor.centerSubMesh = _copy.centerSubMesh;
+    }
+
     //////////////////////////////////////////////////
     template <class T>
     void BaseMesh<T>::Destroy()
@@ -324,6 +384,7 @@ namespace ignition
       if (this->material && this->ownsMaterial)
         this->Scene()->DestroyMaterial(this->material);
       this->material.reset();
+      this->meshDescriptor = MeshDescriptor();
     }
 
     //////////////////////////////////////////////////
diff --git a/ogre/src/OgreMeshFactory.cc b/ogre/src/OgreMeshFactory.cc
index 2954bf27f..09ae5f1d4 100644
--- a/ogre/src/OgreMeshFactory.cc
+++ b/ogre/src/OgreMeshFactory.cc
@@ -64,7 +64,7 @@ OgreMeshPtr OgreMeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
-  mesh->meshName = _desc.meshName;
+  mesh->CopyMeshDescriptor(normDesc);
 
   // create sub-mesh store
   OgreSubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreEntity);
diff --git a/ogre2/src/Ogre2MeshFactory.cc b/ogre2/src/Ogre2MeshFactory.cc
index 02489cf97..966fd7c93 100644
--- a/ogre2/src/Ogre2MeshFactory.cc
+++ b/ogre2/src/Ogre2MeshFactory.cc
@@ -127,7 +127,7 @@ Ogre2MeshPtr Ogre2MeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
-  mesh->meshName = _desc.meshName;
+  mesh->CopyMeshDescriptor(normDesc);
 
   // create sub-mesh store
   Ogre2SubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreItem);

From 200be225867510616312089e9a702481f5549ba3 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Mon, 20 Sep 2021 17:10:26 -0400
Subject: [PATCH 08/12] trying to clone capsule geometries

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 .../ignition/rendering/base/BaseCapsule.hh    | 27 +++++++++++++++++++
 .../ignition/rendering/base/BaseGeometry.hh   |  2 ++
 include/ignition/rendering/base/BaseMesh.hh   |  2 +-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/ignition/rendering/base/BaseCapsule.hh b/include/ignition/rendering/base/BaseCapsule.hh
index 463c1a61c..634025ca9 100644
--- a/include/ignition/rendering/base/BaseCapsule.hh
+++ b/include/ignition/rendering/base/BaseCapsule.hh
@@ -18,7 +18,10 @@
 #ifndef IGNITION_RENDERING_BASECAPSULE_HH_
 #define IGNITION_RENDERING_BASECAPSULE_HH_
 
+#include <ignition/common/Console.hh>
+
 #include "ignition/rendering/Capsule.hh"
+#include "ignition/rendering/Scene.hh"
 #include "ignition/rendering/base/BaseObject.hh"
 
 namespace ignition
@@ -50,6 +53,9 @@ namespace ignition
       // Documentation inherited
       public: virtual double Length() const override;
 
+      // Documentation inherited
+      public: virtual GeometryPtr Clone() const override;
+
       /// \brief Radius of the capsule
       protected: double radius = 0.5;
 
@@ -103,6 +109,27 @@ namespace ignition
     {
       return this->length;
     }
+
+    /////////////////////////////////////////////////
+    template <class T>
+    GeometryPtr BaseCapsule<T>::Clone() const
+    {
+      if (!this->Scene())
+      {
+        ignerr << "Cloning a Capsule failed because the capsule to be "
+          << "cloned does not belong to a scene.\n";
+        return nullptr;
+      }
+
+      auto result = this->Scene()->CreateCapsule();
+      if (result)
+      {
+        result->SetRadius(this->Radius());
+        result->SetLength(this->Length());
+      }
+
+      return result;
+    }
     }
   }
 }
diff --git a/include/ignition/rendering/base/BaseGeometry.hh b/include/ignition/rendering/base/BaseGeometry.hh
index 8d5cf685e..337262037 100644
--- a/include/ignition/rendering/base/BaseGeometry.hh
+++ b/include/ignition/rendering/base/BaseGeometry.hh
@@ -51,6 +51,7 @@ namespace ignition
       public: virtual void SetMaterial(MaterialPtr _material,
                   bool _unique = true) override = 0;
 
+      // Documentation inherited
       public: virtual GeometryPtr Clone() const override;
 
       // Documentation inherited
@@ -98,6 +99,7 @@ namespace ignition
     template <class T>
     GeometryPtr BaseGeometry<T>::Clone() const
     {
+      ignwarn << "Clone functionality for Geometry does not exist yet.\n";
       return nullptr;
     }
 
diff --git a/include/ignition/rendering/base/BaseMesh.hh b/include/ignition/rendering/base/BaseMesh.hh
index ca0858a5f..b3c0b9f2a 100644
--- a/include/ignition/rendering/base/BaseMesh.hh
+++ b/include/ignition/rendering/base/BaseMesh.hh
@@ -329,7 +329,7 @@ namespace ignition
     {
       if (!this->Scene())
       {
-        ignerr << "Cloning a geometry failed because the geometry to be "
+        ignerr << "Cloning a mesh failed because the mesh to be "
           << "cloned does not belong to a scene.\n";
         return nullptr;
       }

From 4e8bea85df4b0084824ebe1db54360a3d95be242 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Mon, 20 Sep 2021 20:26:24 -0400
Subject: [PATCH 09/12] clone capsule material

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/base/BaseCapsule.hh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/ignition/rendering/base/BaseCapsule.hh b/include/ignition/rendering/base/BaseCapsule.hh
index 634025ca9..653be69b5 100644
--- a/include/ignition/rendering/base/BaseCapsule.hh
+++ b/include/ignition/rendering/base/BaseCapsule.hh
@@ -126,6 +126,9 @@ namespace ignition
       {
         result->SetRadius(this->Radius());
         result->SetLength(this->Length());
+
+        if (this->Material())
+          result->SetMaterial(this->Material());
       }
 
       return result;

From 42f048482d522db6fcfe5ef5da66ff6a9efabd95 Mon Sep 17 00:00:00 2001
From: Ashton Larkin <ashton@openrobotics.org>
Date: Mon, 20 Sep 2021 20:26:41 -0400
Subject: [PATCH 10/12] add tests for cloning meshes and capsules

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
---
 include/ignition/rendering/Mesh.hh          |  4 +
 include/ignition/rendering/base/BaseMesh.hh |  9 +++
 src/Capsule_TEST.cc                         | 25 ++++++
 src/Mesh_TEST.cc                            | 89 +++++++++++++++++++++
 src/Visual_TEST.cc                          |  4 +-
 5 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/include/ignition/rendering/Mesh.hh b/include/ignition/rendering/Mesh.hh
index 2fb7644a7..10246d6f8 100644
--- a/include/ignition/rendering/Mesh.hh
+++ b/include/ignition/rendering/Mesh.hh
@@ -121,6 +121,10 @@ namespace ignition
       public: virtual SubMeshPtr SubMeshByIndex(
                   unsigned int _index) const = 0;
 
+      /// \brief Get the mesh's mesh descriptor
+      /// \return The mesh's mesh descriptor
+      public: virtual const MeshDescriptor &GetMeshDescriptor() const = 0;
+
       /// \brief Copy an existing MeshDescriptor. Once copying is complete,
       /// MeshDescriptor::Load needs to be called on the mesh's MeshDescriptor
       /// before using it
diff --git a/include/ignition/rendering/base/BaseMesh.hh b/include/ignition/rendering/base/BaseMesh.hh
index b3c0b9f2a..f425b6aac 100644
--- a/include/ignition/rendering/base/BaseMesh.hh
+++ b/include/ignition/rendering/base/BaseMesh.hh
@@ -102,6 +102,8 @@ namespace ignition
 
       public: virtual GeometryPtr Clone() const override;
 
+      public: const MeshDescriptor &GetMeshDescriptor() const override;
+
       public: virtual void CopyMeshDescriptor(
                   const MeshDescriptor &_copy) override;
 
@@ -366,6 +368,13 @@ namespace ignition
       return result;
     }
 
+    //////////////////////////////////////////////////
+    template <class T>
+    const MeshDescriptor &BaseMesh<T>::GetMeshDescriptor() const
+    {
+      return this->meshDescriptor;
+    }
+
     //////////////////////////////////////////////////
     template <class T>
     void BaseMesh<T>::CopyMeshDescriptor(const MeshDescriptor &_copy)
diff --git a/src/Capsule_TEST.cc b/src/Capsule_TEST.cc
index 99e2b7ad9..d59fbe811 100644
--- a/src/Capsule_TEST.cc
+++ b/src/Capsule_TEST.cc
@@ -17,6 +17,8 @@
 
 #include <gtest/gtest.h>
 
+#include <memory>
+
 #include <ignition/common/Console.hh>
 
 #include "test_config.h"  // NOLINT(build/include)
@@ -71,6 +73,7 @@ void CapsuleTest::Capsule(const std::string &_renderEngine)
   mat->SetAmbient(0.6, 0.7, 0.8);
   mat->SetDiffuse(0.3, 0.8, 0.2);
   mat->SetSpecular(0.4, 0.9, 1.0);
+  mat->SetTransparency(0.3);
 
   capsule->SetMaterial(mat);
   MaterialPtr capsuleMat = capsule->Material();
@@ -79,6 +82,28 @@ void CapsuleTest::Capsule(const std::string &_renderEngine)
   EXPECT_EQ(math::Color(0.3f, 0.8f, 0.2f), capsuleMat->Diffuse());
   EXPECT_EQ(math::Color(0.4f, 0.9f, 1.0f), capsuleMat->Specular());
 
+  // test cloning a capsule
+  auto clonedCapsule =
+    std::dynamic_pointer_cast<rendering::Capsule>(capsule->Clone());
+  ASSERT_NE(nullptr, clonedCapsule);
+  EXPECT_DOUBLE_EQ(clonedCapsule->Radius(), capsule->Radius());
+  EXPECT_DOUBLE_EQ(clonedCapsule->Length(), capsule->Length());
+
+  // compare materials (the material is cloned, so the name is different but
+  // the properties of the material are the same)
+  auto clonedMaterial = clonedCapsule->Material();
+  ASSERT_NE(nullptr, clonedMaterial);
+  auto originalMaterial = capsule->Material();
+  ASSERT_NE(nullptr, originalMaterial);
+  EXPECT_NE(clonedMaterial, originalMaterial);
+  EXPECT_NE(clonedMaterial->Name(), originalMaterial->Name());
+  EXPECT_EQ(clonedMaterial->Type(), originalMaterial->Type());
+  EXPECT_EQ(clonedMaterial->Ambient(), originalMaterial->Ambient());
+  EXPECT_EQ(clonedMaterial->Diffuse(), originalMaterial->Diffuse());
+  EXPECT_EQ(clonedMaterial->Specular(), originalMaterial->Specular());
+  EXPECT_DOUBLE_EQ(clonedMaterial->Transparency(),
+      originalMaterial->Transparency());
+
   // Clean up
   engine->DestroyScene(scene);
   rendering::unloadEngine(engine->Name());
diff --git a/src/Mesh_TEST.cc b/src/Mesh_TEST.cc
index 4441ea14a..c82a7f8a5 100644
--- a/src/Mesh_TEST.cc
+++ b/src/Mesh_TEST.cc
@@ -16,6 +16,7 @@
 */
 
 #include <gtest/gtest.h>
+#include <memory>
 #include <string>
 
 #include <ignition/common/Console.hh>
@@ -42,6 +43,9 @@ class MeshTest : public testing::Test,
   /// \brief Test mesh skeleton animation API
   public: void MeshSkeletonAnimation(const std::string &_renderEngine);
 
+  /// \brief Test mesh clone API
+  public: void MeshClone(const std::string &_renderEngine);
+
   public: const std::string TEST_MEDIA_PATH =
         common::joinPaths(std::string(PROJECT_SOURCE_PATH),
         "test", "media", "meshes");
@@ -202,6 +206,91 @@ TEST_P(MeshTest, MeshSkeletonAnimation)
   MeshSkeletonAnimation(GetParam());
 }
 
+/////////////////////////////////////////////////
+void MeshTest::MeshClone(const std::string &_renderEngine)
+{
+  RenderEngine *engine = rendering::engine(_renderEngine);
+  if (!engine)
+  {
+    igndbg << "Engine '" << _renderEngine
+           << "' is not supported" << std::endl;
+    return;
+  }
+
+  ScenePtr scene = engine->CreateScene("scene");
+  ASSERT_TRUE(scene != nullptr);
+
+  // create the mesh using mesh descriptor
+  MeshDescriptor descriptor("unit_box");
+  MeshPtr mesh = scene->CreateMesh(descriptor);
+  ASSERT_NE(nullptr, mesh);
+
+  // clone the mesh
+  auto clonedMesh = std::dynamic_pointer_cast<rendering::Mesh>(mesh->Clone());
+  ASSERT_NE(nullptr, clonedMesh);
+
+  // Compare mesh descriptors. The pointer to the mesh isn't included, but all
+  // other fields should be equal
+  auto clonedMeshDescriptor = clonedMesh->GetMeshDescriptor();
+  auto originalMeshDescriptor = mesh->GetMeshDescriptor();
+  EXPECT_EQ(clonedMeshDescriptor.meshName, originalMeshDescriptor.meshName);
+  EXPECT_EQ(clonedMeshDescriptor.subMeshName,
+      originalMeshDescriptor.subMeshName);
+  EXPECT_EQ(clonedMeshDescriptor.centerSubMesh,
+      originalMeshDescriptor.centerSubMesh);
+  EXPECT_EQ(nullptr, clonedMeshDescriptor.mesh);
+  EXPECT_EQ(nullptr, originalMeshDescriptor.mesh);
+
+  // Helper function for comparing materials.
+  std::function<void(MaterialPtr, MaterialPtr, bool)> compareMaterials =
+    [&](const MaterialPtr _mat1, const MaterialPtr _mat2, bool _unique)
+    {
+      ASSERT_NE(nullptr, _mat1);
+      ASSERT_NE(nullptr, _mat2);
+      if (_unique)
+      {
+        EXPECT_NE(_mat1, _mat2);
+        EXPECT_NE(_mat1->Name(), _mat2->Name());
+      }
+      else
+      {
+        EXPECT_EQ(_mat1, _mat2);
+        EXPECT_EQ(_mat1->Name(), _mat2->Name());
+      }
+      EXPECT_EQ(_mat1->Type(), _mat2->Type());
+      EXPECT_EQ(_mat1->Ambient(), _mat2->Ambient());
+      EXPECT_EQ(_mat1->Diffuse(), _mat2->Diffuse());
+      EXPECT_EQ(_mat1->Specular(),
+          _mat2->Specular());
+      EXPECT_DOUBLE_EQ(_mat1->Transparency(),
+          _mat2->Transparency());
+    };
+
+  // compare materials and submeshes
+  compareMaterials(clonedMesh->Material(), mesh->Material(), true);
+  ASSERT_EQ(clonedMesh->SubMeshCount(), mesh->SubMeshCount());
+  for (unsigned int i = 0; i < clonedMesh->SubMeshCount(); ++i)
+  {
+    // since the "top level mesh" has a material, the submesh materials are not
+    // unique copies:
+    // https://github.com/ignitionrobotics/ign-rendering/blob/8f961d0c4cc755b6a2ca217d5a73de268ef95514/include/ignition/rendering/base/BaseMesh.hh#L293
+    auto clonedSubMesh = clonedMesh->SubMeshByIndex(i);
+    auto originalSubMesh = clonedMesh->SubMeshByIndex(i);
+    compareMaterials(clonedSubMesh->Material(), originalSubMesh->Material(),
+        false);
+  }
+
+  // Clean up
+  engine->DestroyScene(scene);
+  rendering::unloadEngine(engine->Name());
+}
+
+/////////////////////////////////////////////////
+TEST_P(MeshTest, MeshClone)
+{
+  MeshClone(GetParam());
+}
+
 INSTANTIATE_TEST_CASE_P(Mesh, MeshTest,
     RENDER_ENGINE_VALUES,
     ignition::rendering::PrintToStringParam());
diff --git a/src/Visual_TEST.cc b/src/Visual_TEST.cc
index 19f1a92c8..a15d8a467 100644
--- a/src/Visual_TEST.cc
+++ b/src/Visual_TEST.cc
@@ -734,8 +734,6 @@ void VisualTest::Clone(const std::string &_renderEngine)
   EXPECT_EQ(clonedVisual->LocalPose(), parent->LocalPose());
   EXPECT_EQ(clonedVisual->Wireframe(), parent->Wireframe());
 
-  // TODO(adlarkin) compare geometries once cloning is implemented for those
-
   // compare materials (the material is cloned, so the name is different but
   // the properties of the material are the same)
   auto clonedVisualMaterial = clonedVisual->Material();
@@ -752,7 +750,7 @@ void VisualTest::Clone(const std::string &_renderEngine)
   EXPECT_DOUBLE_EQ(clonedVisualMaterial->Transparency(),
       originalVisualMaterial->Transparency());
 
-  // TODO(adlarkin) compare children
+  // TODO(anyone) compare child visuals that were cloned
 
   // clean up
   engine->DestroyScene(scene);

From 6791b41d7b6a74efca1f7132d00589da2753e0ab Mon Sep 17 00:00:00 2001
From: Ian Chen <ichen@osrfoundation.org>
Date: Tue, 21 Sep 2021 18:27:58 -0700
Subject: [PATCH 11/12] style, doc, api tweaks (#418)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
---
 include/ignition/rendering/Mesh.hh          | 12 +++++-------
 include/ignition/rendering/base/BaseMesh.hh | 20 +++++++++-----------
 ogre/src/OgreMeshFactory.cc                 |  2 --
 ogre/src/OgreScene.cc                       |  1 +
 ogre2/src/Ogre2MeshFactory.cc               |  2 --
 ogre2/src/Ogre2Scene.cc                     |  1 +
 src/Mesh_TEST.cc                            |  4 ++--
 7 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/include/ignition/rendering/Mesh.hh b/include/ignition/rendering/Mesh.hh
index 10246d6f8..a98c96790 100644
--- a/include/ignition/rendering/Mesh.hh
+++ b/include/ignition/rendering/Mesh.hh
@@ -121,15 +121,13 @@ namespace ignition
       public: virtual SubMeshPtr SubMeshByIndex(
                   unsigned int _index) const = 0;
 
-      /// \brief Get the mesh's mesh descriptor
+      /// \brief Set the mesh's mesh descriptor
       /// \return The mesh's mesh descriptor
-      public: virtual const MeshDescriptor &GetMeshDescriptor() const = 0;
+      public: virtual void SetDescriptor(const MeshDescriptor &_desc) = 0;
 
-      /// \brief Copy an existing MeshDescriptor. Once copying is complete,
-      /// MeshDescriptor::Load needs to be called on the mesh's MeshDescriptor
-      /// before using it
-      /// \param[in] _copy The MeshDescriptor to copy
-      public: virtual void CopyMeshDescriptor(const MeshDescriptor &_copy) = 0;
+      /// \brief Get the mesh's mesh descriptor
+      /// \return The mesh's mesh descriptor
+      public: virtual const MeshDescriptor &Descriptor() const = 0;
     };
 
     /// \class SubMesh Mesh.hh ignition/rendering/Mesh.hh
diff --git a/include/ignition/rendering/base/BaseMesh.hh b/include/ignition/rendering/base/BaseMesh.hh
index f425b6aac..456787770 100644
--- a/include/ignition/rendering/base/BaseMesh.hh
+++ b/include/ignition/rendering/base/BaseMesh.hh
@@ -100,12 +100,14 @@ namespace ignition
 
       public: virtual void PreRender() override;
 
+      // Documentation inherited.
       public: virtual GeometryPtr Clone() const override;
 
-      public: const MeshDescriptor &GetMeshDescriptor() const override;
+      // Documentation inherited.
+      public: void SetDescriptor(const MeshDescriptor &_desc) override;
 
-      public: virtual void CopyMeshDescriptor(
-                  const MeshDescriptor &_copy) override;
+      // Documentation inherited.
+      public: const MeshDescriptor &Descriptor() const override;
 
       // Documentation inherited
       public: virtual void Destroy() override;
@@ -342,11 +344,9 @@ namespace ignition
         return nullptr;
       }
 
-      auto result = this->Scene()->CreateMesh(this->meshDescriptor.meshName);
+      auto result = this->Scene()->CreateMesh(this->meshDescriptor);
       if (result)
       {
-        result->CopyMeshDescriptor(this->meshDescriptor);
-
         if (this->Material())
         {
           // this call will set the material for the mesh and its submeshes
@@ -370,18 +370,16 @@ namespace ignition
 
     //////////////////////////////////////////////////
     template <class T>
-    const MeshDescriptor &BaseMesh<T>::GetMeshDescriptor() const
+    const MeshDescriptor &BaseMesh<T>::Descriptor() const
     {
       return this->meshDescriptor;
     }
 
     //////////////////////////////////////////////////
     template <class T>
-    void BaseMesh<T>::CopyMeshDescriptor(const MeshDescriptor &_copy)
+    void BaseMesh<T>::SetDescriptor(const MeshDescriptor &_desc)
     {
-      this->meshDescriptor.meshName = _copy.meshName;
-      this->meshDescriptor.subMeshName = _copy.subMeshName;
-      this->meshDescriptor.centerSubMesh = _copy.centerSubMesh;
+      this->meshDescriptor = _desc;
     }
 
     //////////////////////////////////////////////////
diff --git a/ogre/src/OgreMeshFactory.cc b/ogre/src/OgreMeshFactory.cc
index 09ae5f1d4..d30c55a79 100644
--- a/ogre/src/OgreMeshFactory.cc
+++ b/ogre/src/OgreMeshFactory.cc
@@ -64,8 +64,6 @@ OgreMeshPtr OgreMeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
-  mesh->CopyMeshDescriptor(normDesc);
-
   // create sub-mesh store
   OgreSubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreEntity);
   mesh->subMeshes = subMeshFactory.Create();
diff --git a/ogre/src/OgreScene.cc b/ogre/src/OgreScene.cc
index 54e046fde..b6afb8d72 100644
--- a/ogre/src/OgreScene.cc
+++ b/ogre/src/OgreScene.cc
@@ -546,6 +546,7 @@ MeshPtr OgreScene::CreateMeshImpl(unsigned int _id, const std::string &_name,
   if (nullptr == mesh)
     return nullptr;
 
+  mesh->SetDescriptor(_desc);
   bool result = this->InitObject(mesh, _id, _name);
   return (result) ? mesh : nullptr;
 }
diff --git a/ogre2/src/Ogre2MeshFactory.cc b/ogre2/src/Ogre2MeshFactory.cc
index 966fd7c93..82d392abb 100644
--- a/ogre2/src/Ogre2MeshFactory.cc
+++ b/ogre2/src/Ogre2MeshFactory.cc
@@ -127,8 +127,6 @@ Ogre2MeshPtr Ogre2MeshFactory::Create(const MeshDescriptor &_desc)
     return nullptr;
   }
 
-  mesh->CopyMeshDescriptor(normDesc);
-
   // create sub-mesh store
   Ogre2SubMeshStoreFactory subMeshFactory(this->scene, mesh->ogreItem);
   mesh->subMeshes = subMeshFactory.Create();
diff --git a/ogre2/src/Ogre2Scene.cc b/ogre2/src/Ogre2Scene.cc
index 288ae7b85..7571c57e4 100644
--- a/ogre2/src/Ogre2Scene.cc
+++ b/ogre2/src/Ogre2Scene.cc
@@ -1119,6 +1119,7 @@ MeshPtr Ogre2Scene::CreateMeshImpl(unsigned int _id,
   Ogre2MeshPtr mesh = this->meshFactory->Create(_desc);
   if (nullptr == mesh)
     return nullptr;
+  mesh->SetDescriptor(_desc);
 
   bool result = this->InitObject(mesh, _id, _name);
   return (result) ? mesh : nullptr;
diff --git a/src/Mesh_TEST.cc b/src/Mesh_TEST.cc
index c82a7f8a5..6a148bb78 100644
--- a/src/Mesh_TEST.cc
+++ b/src/Mesh_TEST.cc
@@ -231,8 +231,8 @@ void MeshTest::MeshClone(const std::string &_renderEngine)
 
   // Compare mesh descriptors. The pointer to the mesh isn't included, but all
   // other fields should be equal
-  auto clonedMeshDescriptor = clonedMesh->GetMeshDescriptor();
-  auto originalMeshDescriptor = mesh->GetMeshDescriptor();
+  auto clonedMeshDescriptor = clonedMesh->Descriptor();
+  auto originalMeshDescriptor = mesh->Descriptor();
   EXPECT_EQ(clonedMeshDescriptor.meshName, originalMeshDescriptor.meshName);
   EXPECT_EQ(clonedMeshDescriptor.subMeshName,
       originalMeshDescriptor.subMeshName);

From 73381056c6ef9838061c538fcfc3ba9a039ddbff Mon Sep 17 00:00:00 2001
From: Ian Chen <ichen@osrfoundation.org>
Date: Tue, 21 Sep 2021 19:54:14 -0700
Subject: [PATCH 12/12] fix clang warnings

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
---
 ogre/include/ignition/rendering/ogre/OgreCOMVisual.hh        | 2 +-
 ogre/include/ignition/rendering/ogre/OgreInertiaVisual.hh    | 2 +-
 ogre/include/ignition/rendering/ogre/OgreLightVisual.hh      | 2 +-
 ogre2/include/ignition/rendering/ogre2/Ogre2COMVisual.hh     | 2 +-
 ogre2/include/ignition/rendering/ogre2/Ogre2InertiaVisual.hh | 2 +-
 ogre2/include/ignition/rendering/ogre2/Ogre2LightVisual.hh   | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ogre/include/ignition/rendering/ogre/OgreCOMVisual.hh b/ogre/include/ignition/rendering/ogre/OgreCOMVisual.hh
index 3d37d60e9..6dda62d99 100644
--- a/ogre/include/ignition/rendering/ogre/OgreCOMVisual.hh
+++ b/ogre/include/ignition/rendering/ogre/OgreCOMVisual.hh
@@ -64,7 +64,7 @@ namespace ignition
       public: virtual VisualPtr SphereVisual() const override;
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() const;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetMaterial(
diff --git a/ogre/include/ignition/rendering/ogre/OgreInertiaVisual.hh b/ogre/include/ignition/rendering/ogre/OgreInertiaVisual.hh
index 75b0c78eb..a956823d5 100644
--- a/ogre/include/ignition/rendering/ogre/OgreInertiaVisual.hh
+++ b/ogre/include/ignition/rendering/ogre/OgreInertiaVisual.hh
@@ -68,7 +68,7 @@ namespace ignition
       public: VisualPtr BoxVisual() const override;
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() const;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetMaterial(
diff --git a/ogre/include/ignition/rendering/ogre/OgreLightVisual.hh b/ogre/include/ignition/rendering/ogre/OgreLightVisual.hh
index eaca483c0..801049c52 100644
--- a/ogre/include/ignition/rendering/ogre/OgreLightVisual.hh
+++ b/ogre/include/ignition/rendering/ogre/OgreLightVisual.hh
@@ -61,7 +61,7 @@ namespace ignition
       public: void CreateVisual();
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() const;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetMaterial(
diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2COMVisual.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2COMVisual.hh
index 54c8dc92a..d55afb8b7 100644
--- a/ogre2/include/ignition/rendering/ogre2/Ogre2COMVisual.hh
+++ b/ogre2/include/ignition/rendering/ogre2/Ogre2COMVisual.hh
@@ -62,7 +62,7 @@ namespace ignition
       public: virtual VisualPtr SphereVisual() const override;
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() const;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetMaterial(
diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2InertiaVisual.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2InertiaVisual.hh
index d18511c1c..c15a2b112 100644
--- a/ogre2/include/ignition/rendering/ogre2/Ogre2InertiaVisual.hh
+++ b/ogre2/include/ignition/rendering/ogre2/Ogre2InertiaVisual.hh
@@ -66,7 +66,7 @@ namespace ignition
       public: VisualPtr BoxVisual() const override;
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() const;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetMaterial(
diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2LightVisual.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2LightVisual.hh
index b9b9bd472..b1bd2281b 100644
--- a/ogre2/include/ignition/rendering/ogre2/Ogre2LightVisual.hh
+++ b/ogre2/include/ignition/rendering/ogre2/Ogre2LightVisual.hh
@@ -59,7 +59,7 @@ namespace ignition
       public: void CreateVisual();
 
       // Documentation inherited.
-      public: virtual MaterialPtr Material() const;
+      public: virtual MaterialPtr Material() const override;
 
       // Documentation inherited.
       public: virtual void SetMaterial(