memset for Eigen::Matrix in integral_image2D.cpp

[kunaltyagi]

Lines 159, 190 in file features/include/pcl/features/impl/integral_image2D.hpp:

pcl/features/include/pcl/features/impl/integral_image2D.hpp

Line 159 in 8ab5ae4

memset (previous_row, 0, sizeof (ElementType) * (width_ + 1));

pcl/features/include/pcl/features/impl/integral_image2D.hpp

Line 190 in 8ab5ae4

memset (so_previous_row, 0, sizeof (SecondOrderType) * (width_ + 1));

memset is being called upon ElementType and SecondOrderType which are template parameter. As such I don't see a way out of the warning, unless we are guaranteed that types have data field if it has no trivial copy-assignment

[taketwo]

Could you please post the warning you are talking about?

[kunaltyagi]

In file included from ../features/include/pcl/features/integral_image2D.h:346,
                 from ../features/include/pcl/features/integral_image_normal.h:45,
                 from ../features/include/pcl/features/impl/integral_image_normal.hpp:42,
                 from ../features/src/integral_image_normal.cpp:39:
../features/include/pcl/features/impl/integral_image2D.hpp: In instantiation of ‘void
pcl::IntegralImage2D<DataType, Dimension>::computeIntegralImages(const DataType*,
unsigned int, unsigned int) [with DataType = float; unsigned int Dimension = 3]’:
../features/include/pcl/features/impl/integral_image2D.hpp:65:3:   required from ‘void
pcl::IntegralImage2D<DataType, Dimension>::setInput(const DataType*, unsigned int,
unsigned int, unsigned int, unsigned int) [with DataType = float; unsigned int Dimension
= 3]’
../features/include/pcl/features/impl/integral_image_normal.hpp:133:23:   required from
‘void pcl::IntegralImageNormalEstimation<PointInT, PointOutT>::initCovarianceMatrixMethod()
[with PointInT = pcl::PointXYZ; PointOutT = pcl::Normal]’
../features/src/integral_image_normal.cpp:48:3:   required from here
../features/include/pcl/features/impl/integral_image2D.hpp:159:10: warning: ‘void*
memset(void*, int, size_t)’ clearing an object of type ‘using ElementType = class
Eigen::Matrix<double, 3, 1>’ {aka ‘class Eigen::Matrix<double, 3, 1>’} with no trivial
copy-assignment; use assignment or value-initialization instead [-Wclass-memaccess]
  159 |   memset (previous_row, 0, sizeof (ElementType) * (width_ + 1));
      |   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/eigen3/Eigen/Core:458,
                 from ../common/include/pcl/pcl_macros.h:73,
                 from ../features/include/pcl/features/integral_image_normal.h:41,
                 from ../features/include/pcl/features/impl/integral_image_normal.hpp:42,
                 from ../features/src/integral_image_normal.cpp:39:
/usr/include/eigen3/Eigen/src/Core/Matrix.h:178:7: note: ‘using ElementType = class
Eigen::Matrix<double, 3, 1>’ {aka ‘class Eigen::Matrix<double, 3, 1>’} declared here
  178 | class Matrix
      |       ^~~~~~
In file included from ../features/include/pcl/features/integral_image2D.h:346,
                 from ../features/include/pcl/features/integral_image_normal.h:45,
                 from ../features/include/pcl/features/impl/integral_image_normal.hpp:42,
                 from ../features/src/integral_image_normal.cpp:39:
../features/include/pcl/features/impl/integral_image2D.hpp:190:12: warning: ‘void*
memset(void*, int, size_t)’ clearing an object of type ‘using SecondOrderType = class
Eigen::Matrix<double, 6, 1, 0, 6, 1>’ {aka ‘class Eigen::Matrix<double, 6, 1, 0, 6, 1>’} with
no trivial copy-assignment; use assignment or value-initialization instead
[-Wclass-memaccess]
  190 |     memset (so_previous_row, 0, sizeof (SecondOrderType) * (width_ + 1));
         |  ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/eigen3/Eigen/Core:458,
                 from ../common/include/pcl/pcl_macros.h:73,
                 from ../features/include/pcl/features/integral_image_normal.h:41,
                 from ../features/include/pcl/features/impl/integral_image_normal.hpp:42,
                 from ../features/src/integral_image_normal.cpp:39:
/usr/include/eigen3/Eigen/src/Core/Matrix.h:178:7: note: ‘using SecondOrderType =
class Eigen::Matrix<double, 6, 1, 0, 6, 1>’ {aka ‘class Eigen::Matrix<double, 6, 1, 0, 6, 1>’}
declared here
  178 | class Matrix
      |       ^~~~~~

[taketwo]

I don't see a way out of this without at least minor refactoring of the code.

first_order_integral_image_ may be converted from an std::vector of Eigen::Vectors into a two-dimensional Eigen::Matrix. Then memset can be replaced with selection of certain rows (or columns) and calling setZero() on them.

[stale]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

[gnawme]

In io.hpp, line 139:

  if (isSamePointType<PointInT, PointOutT> ()) {
    // Copy the whole memory block
    std::copy(cloud_in.begin(), cloud_in.end(), cloud_out.begin());
  }

If I replace the memcpy with a std::copy, I get this compile error:

no match for ‘operator=’ (operand types are ‘pcl::PointXYZL’ and ‘const pcl::PointXYZRGBA’)

which means that isSamePointType<PointXYZL, PointXYZRGBA> passed.

Why does the compiler think that PointXYZL and PointXYZRGBA are the same type?

=====
EDIT: I found this snippet in the typeid doc:

const std::type_info& ti1 = typeid(A);
const std::type_info& ti2 = typeid(A);
 
assert(&ti1 == &ti2); // **not guaranteed**
assert(ti1.hash_code() == ti2.hash_code()); // guaranteed
assert(std::type_index(ti1) == std::type_index(ti2)); // guaranteed

=====
EDIT:

typeid(PointXYZL).hash_code() == typeid(PointXYZRGBA).hash_code()

passes as well.

[kunaltyagi]

Yeah, I have been meaning to change it to use std::is_same<PointA, PointB> but it was breaking a few local tests and I never bothered to check more. Could you try editing the function at L372 in common/include/pcl/common/io.h to not use RTTI instead rely on compile time info?

[gnawme]

if constexpr combined with std::is_same solves it -- it prevents the if clause from being compiled at all if std::is_same fails -- but if constexpr is a C++17 construct.

Are you targeting C++17 support in 1.12?

[kunaltyagi]

Are you targeting C++17 support in 1.12?

No. Maybe template specialization can help (or we can add a function to abstract the issue)

[al-tu]

hi! came across this code and decided to make a quick fix, please take a look
#4901
there should be some tests for point selection functions, but i think that should be covered by another PR implementing correspondence estimation for different point types #4902
(related issues are #329 and #1476)