Progress simplifying CPointsMap API

Author: jlblancoc

libs/maps/include/mrpt/maps/CColouredPointsMap.h

@@ -34,7 +34,7 @@ class CColouredPointsMap : public CPointsMap
  public:
   CColouredPointsMap() = default;
 
-  CColouredPointsMap(const CPointsMap& o) { CPointsMap::operator=(o); }
+  explicit CColouredPointsMap(const CPointsMap& o) { CPointsMap::operator=(o); }
   CColouredPointsMap(const CColouredPointsMap& o) : CPointsMap() { impl_copyFrom(o); }
   CColouredPointsMap& operator=(const CPointsMap& o)
   {
@@ -102,11 +102,6 @@ class CColouredPointsMap : public CPointsMap
       const std::optional<const mrpt::poses::CPose3D>& robotPose = std::nullopt) override;
 
  protected:
-  void addFrom_classSpecific(
-      const CPointsMap& anotherMap,
-      size_t nPreviousPoints,
-      const bool filterOutPointsAtZero) override;
-
   // Friend methods:
   template <class Derived>
   friend struct detail::loadFromRangeImpl;

libs/maps/include/mrpt/maps/CGenericPointsMap.h

@@ -49,17 +49,11 @@ class CGenericPointsMap : public CPointsMap
   /** @name Register/unregister custom data fields
     @{ */
 
-  /** Registers a new data channel of type `float`.
-   * If the map is not empty, the new channel is filled with default values (0)
-   * to match the current point count.
-   */
-  void registerField_float(const std::string& fieldName);
+  // see docs in parent class
+  bool registerField_float(const std::string& fieldName) override;
 
-  /** Registers a new data channel of type `uint16_t`.
-   * If the map is not empty, the new channel is filled with default values (0)
-   * to match the current point count.
-   */
-  void registerField_uint16(const std::string& fieldName);
+  // see docs in parent class
+  bool registerField_uint16(const std::string& fieldName) override;
 
   /** Removes a data channel.
    * \return True if the field existed and was removed, false otherwise.
@@ -199,12 +193,6 @@ class CGenericPointsMap : public CPointsMap
   /** Map from field name to data vector */
   std::map<std::string, mrpt::aligned_std_vector<uint16_t>> m_uint16_fields;
 
-  // See base class
-  void addFrom_classSpecific(
-      const CPointsMap& anotherMap,
-      size_t nPreviousPoints,
-      const bool filterOutPointsAtZero) override;
-
   /** Clear the map, erasing all the points and all fields */
   void internal_clear() override;
 

libs/maps/include/mrpt/maps/CPointsMap.h

@@ -199,14 +199,13 @@ class CPointsMap :
     const size_t N = obj.size();
     this->clear();
     this->reserve(N);
-    for (size_t i = 0; i < N; i++) insertPointFrom(obj, i);
+    this->registerPointFieldsFrom(obj);
+    for (size_t i = 0; i < N; i++)
+    {
+      insertPointFrom(obj, i);
+    }
   }
 
-  /** Auxiliary method called from within \a addFrom() automatically, to
-   * finish the copying of class-specific data  */
-  virtual void addFrom_classSpecific(
-      const CPointsMap& anotherMap, size_t nPreviousPoints, const bool filterOutPointsAtZero) = 0;
-
  public:
   /** @} */
   // --------------------------------------------
@@ -357,6 +356,30 @@ class CPointsMap :
     insertAnotherMap(&anotherMap, mrpt::poses::CPose3D::Identity());
   }
 
+  /** @name Register/unregister custom data fields
+    @{ */
+
+  /** Registers a new data channel of type `float`.
+   * If the map is not empty, the new channel is filled with default values (0)
+   * to match the current point count.
+   * \return true if the field could effectively be added to the underlying point map class.
+   * \sa hasPointField(), getPointFieldNames_float()
+   */
+  virtual bool registerField_float(const std::string& fieldName)
+  {
+    return fieldName == "x" || fieldName == "y" || fieldName == "z";
+  }
+
+  /** Registers a new data channel of type `uint16_t`.
+   * If the map is not empty, the new channel is filled with default values (0)
+   * to match the current point count.
+   * \return true if the field could effectively be added to the underlying point map class.
+   * \sa hasPointField(), getPointFieldNames_uint16()
+   */
+  virtual bool registerField_uint16(const std::string& fieldName) { return false; }
+
+  /** @} */
+
   // --------------------------------------------------
   /** @name File input/output methods
     @{ */
@@ -745,16 +768,34 @@ class CPointsMap :
   }
 
   /** Must be called before insertPointFrom() to make sure we have the required fields.
+   *  \return true if ALL fields could be added, false if some would be missing because the
+   *               underlying point cloud class cannot hold them.
    */
-  void copyPointFieldsFrom(const mrpt::maps::CPointsMap& source)
+  bool registerPointFieldsFrom(const mrpt::maps::CPointsMap& source)
   {
-    //
-    xx;
+    bool allAdded = true;
+    for (const auto& f : source.getPointFieldNames_float())
+    {
+      if (!this->hasPointField(f))
+      {
+        const bool added = registerField_float(f);
+        allAdded = allAdded && added;
+      }
+    }
+    for (const auto& f : source.getPointFieldNames_uint16())
+    {
+      if (!this->hasPointField(f))
+      {
+        const bool added = this->registerField_uint16(f);
+        allAdded = allAdded && added;
+      }
+    }
+    return allAdded;
   }
 
   /** Generic method to copy *all* applicable point properties from
    *  one map to another, e.g. timestamp, intensity, etc.
-   *  \note Before calling this in a loop, make sure of calling copyPointFieldsFrom()
+   *  \note Before calling this in a loop, make sure of calling registerPointFieldsFrom()
    */
   void insertPointFrom(const mrpt::maps::CPointsMap& source, size_t sourcePointIndex)
   {
@@ -768,11 +809,11 @@ class CPointsMap :
     // Optional fields: only if they already exist on both maps:
     for (const auto& f : this->getPointFieldNames_float())
     {
-      insertPointField_float(f, this->getPointField_float(i, f));
+      insertPointField_float(f, source.getPointField_float(i, f));
     }
     for (const auto& f : this->getPointFieldNames_uint16())
     {
-      insertPointField_uint16(f, this->getPointField_uint16(i, f));
+      insertPointField_uint16(f, source.getPointField_uint16(i, f));
     }
     mark_as_modified();
   }
@@ -1009,22 +1050,6 @@ class CPointsMap :
    */
   void getVisualizationInto(mrpt::opengl::CSetOfObjects& outObj) const override;
 
-  /** This method returns the largest distance from the origin to any of the
-   * points, such as a sphere centered at the origin with this radius cover
-   * ALL the points in the map (the results are buffered, such as, if the map
-   * is not modified, the second call will be much faster than the first one).
-   */
-  float getLargestDistanceFromOrigin() const;
-
-  /** Like \a getLargestDistanceFromOrigin() but returns in \a output_is_valid
-   * = false if the distance was not already computed, skipping its
-   * computation then, unlike getLargestDistanceFromOrigin() */
-  float getLargestDistanceFromOriginNoRecompute(bool& output_is_valid) const
-  {
-    output_is_valid = m_largestDistanceFromOriginIsUpdated;
-    return m_largestDistanceFromOrigin;
-  }
-
   /** Computes the bounding box of all the points, or (0,0 ,0,0, 0,0) if there
    * are no points.
    *  Results are cached unless the map is somehow modified to avoid repeated
@@ -1212,7 +1237,6 @@ class CPointsMap :
    * kd-tree cache, and such. */
   inline void mark_as_modified() const
   {
-    m_largestDistanceFromOriginIsUpdated = false;
     m_boundingBoxIsUpdated = false;
     kdtree_mark_as_outdated();
   }
@@ -1277,16 +1301,6 @@ class CPointsMap :
   /** Cache of sin/cos values for the latest 2D scan geometries. */
   mrpt::obs::CSinCosLookUpTableFor2DScans m_scans_sincos_cache;
 
-  /** Auxiliary variables used in "getLargestDistanceFromOrigin"
-   * \sa getLargestDistanceFromOrigin
-   */
-  mutable float m_largestDistanceFromOrigin{0};
-
-  /** Auxiliary variables used in "getLargestDistanceFromOrigin"
-   * \sa getLargestDistanceFromOrigin
-   */
-  mutable bool m_largestDistanceFromOriginIsUpdated;
-
   mutable bool m_boundingBoxIsUpdated;
   mutable mrpt::math::TBoundingBoxf m_boundingBox;
 
@@ -1300,9 +1314,6 @@ class CPointsMap :
       const mrpt::obs::CObservation& obs,
       const std::optional<const mrpt::poses::CPose3D>& robotPose = std::nullopt) override;
 
-  /** Helper method for ::copyFrom() */
-  void base_copyFrom(const CPointsMap& obj);
-
   /** @name PLY Import virtual methods to implement in base classes
     @{ */
   /** In a base class, reserve memory to prepare subsequent calls to

libs/maps/include/mrpt/maps/CPointsMapXYZI.h

@@ -99,12 +99,6 @@ class CPointsMapXYZI : public CPointsMap
       const std::optional<const mrpt::poses::CPose3D>& robotPose = std::nullopt) override;
 
  protected:
-  // See base class
-  void addFrom_classSpecific(
-      const CPointsMap& anotherMap,
-      size_t nPreviousPoints,
-      const bool filterOutPointsAtZero) override;
-
   // Friend methods:
   template <class Derived>
   friend struct detail::loadFromRangeImpl;

libs/maps/include/mrpt/maps/CPointsMapXYZIRT.h

@@ -15,8 +15,6 @@
 
 namespace mrpt::maps
 {
-class CPointsMapXYZI;
-
 /** A map of 3D points with channels: X,Y,Z,I (intensity), R (ring), T (time).
  *
  * - `ring` (`uint16_t`) holds the "ring number", or the "row index" for
@@ -44,10 +42,8 @@ class CPointsMapXYZIRT : public CPointsMap
 
   explicit CPointsMapXYZIRT(const CPointsMap& o) { CPointsMap::operator=(o); }
   CPointsMapXYZIRT(const CPointsMapXYZIRT& o);
-  explicit CPointsMapXYZIRT(const CPointsMapXYZI& o);
   CPointsMapXYZIRT& operator=(const CPointsMap& o);
   CPointsMapXYZIRT& operator=(const CPointsMapXYZIRT& o);
-  CPointsMapXYZIRT& operator=(const CPointsMapXYZI& o);
 
   /** @name Pure virtual interfaces to be implemented by any class derived from CPointsMap
   @{ */
@@ -118,12 +114,6 @@ class CPointsMapXYZIRT : public CPointsMap
       const std::optional<const mrpt::poses::CPose3D>& robotPose = std::nullopt) override;
 
  protected:
-  // See base class
-  void addFrom_classSpecific(
-      const CPointsMap& anotherMap,
-      size_t nPreviousPoints,
-      const bool filterOutPointsAtZero) override;
-
   // Friend methods:
   template <class Derived>
   friend struct detail::loadFromRangeImpl;

libs/maps/include/mrpt/maps/CSimplePointsMap.h

@@ -34,7 +34,7 @@ class CSimplePointsMap : public CPointsMap
  public:
   /** Default constructor */
   CSimplePointsMap() = default;
-  CSimplePointsMap(const CPointsMap& o) { CPointsMap::operator=(o); }
+  explicit CSimplePointsMap(const CPointsMap& o) { CPointsMap::operator=(o); }
   CSimplePointsMap(const CSimplePointsMap& o) : CPointsMap() { CPointsMap::operator=(o); }
   CSimplePointsMap& operator=(const CPointsMap& o)
   {
@@ -92,14 +92,6 @@ class CSimplePointsMap : public CPointsMap
       const std::optional<const mrpt::poses::CPose3D>& robotPose = std::nullopt) override;
 
  protected:
-  void addFrom_classSpecific(
-      [[maybe_unused]] const CPointsMap& anotherMap,
-      [[maybe_unused]] size_t nPreviousPoints,
-      [[maybe_unused]] const bool filterOutPointsAtZero) override
-  {
-    // No extra data.
-  }
-
   // Friend methods:
   template <class Derived>
   friend struct detail::loadFromRangeImpl;

libs/maps/include/mrpt/maps/CWeightedPointsMap.h

@@ -33,7 +33,7 @@ class CWeightedPointsMap : public CPointsMap
  public:
   /** Default constructor */
   CWeightedPointsMap() = default;
-  CWeightedPointsMap(const CPointsMap& o) { impl_copyFrom(o); }
+  explicit CWeightedPointsMap(const CPointsMap& o) { impl_copyFrom(o); }
   CWeightedPointsMap(const CWeightedPointsMap& o) : CPointsMap() { impl_copyFrom(o); }
   CWeightedPointsMap& operator=(const CPointsMap& o)
   {
@@ -97,11 +97,6 @@ class CWeightedPointsMap : public CPointsMap
       const std::optional<const mrpt::poses::CPose3D>& robotPose = std::nullopt) override;
 
  protected:
-  void addFrom_classSpecific(
-      const CPointsMap& anotherMap,
-      size_t nPreviousPoints,
-      const bool filterOutPointsAtZero) override;
-
   // Friend methods:
   template <class Derived>
   friend struct detail::loadFromRangeImpl;

libs/maps/src/maps/CColouredPointsMap.cpp

@@ -577,33 +577,6 @@ void CColouredPointsMap::PLY_export_get_vertex(
   pt_color.B = m_color_B[idx];
 }
 
-/*---------------------------------------------------------------
-addFrom_classSpecific
----------------------------------------------------------------*/
-void CColouredPointsMap::addFrom_classSpecific(
-    const CPointsMap& anotherMap, size_t nPreviousPoints, const bool filterOutPointsAtZero)
-{
-  const size_t nOther = anotherMap.size();
-
-  // Specific data for this class:
-  const auto* anotheMap_col = dynamic_cast<const CColouredPointsMap*>(&anotherMap);
-
-  if (anotheMap_col)
-  {
-    for (size_t i = 0, j = nPreviousPoints; i < nOther; i++)
-    {
-      if (filterOutPointsAtZero && anotheMap_col->m_x[i] == 0 && anotheMap_col->m_y[i] == 0 &&
-          anotheMap_col->m_z[i] == 0)
-        continue;  // skip
-
-      m_color_R[j] = anotheMap_col->m_color_R[i];
-      m_color_G[j] = anotheMap_col->m_color_G[i];
-      m_color_B[j] = anotheMap_col->m_color_B[i];
-      j++;
-    }
-  }
-}
-
 namespace mrpt::maps::detail
 {
 using mrpt::maps::CColouredPointsMap;