Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

memset for Eigen::Matrix in integral_image2D.cpp #3368

Open
kunaltyagi opened this issue Sep 26, 2019 · 9 comments
Open

memset for Eigen::Matrix in integral_image2D.cpp #3368

kunaltyagi opened this issue Sep 26, 2019 · 9 comments
Labels
effort: medium Rough estimate of time needed to fix/implement/solve kind: todo Type of issue module: features skill: meta-programming Skills/areas of expertise needed to tackle the issue

Comments

@kunaltyagi
Copy link
Member

kunaltyagi commented Sep 26, 2019

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

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

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

@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet module: features labels Sep 26, 2019
@taketwo
Copy link
Member

taketwo commented Sep 26, 2019

Could you please post the warning you are talking about?

@kunaltyagi
Copy link
Member Author

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
Copy link
Member

taketwo commented Sep 28, 2019

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.

@kunaltyagi kunaltyagi added effort: medium Rough estimate of time needed to fix/implement/solve kind: todo Type of issue skill: meta-programming Skills/areas of expertise needed to tackle the issue and removed needs: code review Specify why not closed/merged yet labels Mar 12, 2020
@stale
Copy link

stale bot commented May 19, 2020

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

@gnawme
Copy link
Contributor

gnawme commented Jul 28, 2020

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
Copy link
Member Author

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
Copy link
Contributor

gnawme commented Jul 29, 2020

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
Copy link
Member Author

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
Copy link
Contributor

al-tu commented Aug 17, 2021

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Rough estimate of time needed to fix/implement/solve kind: todo Type of issue module: features skill: meta-programming Skills/areas of expertise needed to tackle the issue
Projects
None yet
Development

No branches or pull requests

4 participants