Skip to content

Add missing aligned operators new#5116

Closed
themightyoarfish wants to merge 1 commit intoPointCloudLibrary:masterfrom
themightyoarfish:add-missing-aligned-operators
Closed

Add missing aligned operators new#5116
themightyoarfish wants to merge 1 commit intoPointCloudLibrary:masterfrom
themightyoarfish:add-missing-aligned-operators

Conversation

@themightyoarfish
Copy link
Copy Markdown
Contributor

This PR adds missing PCL_MAKE_ALIGNED_OPERATOR_NEW in some classes with Eigen members. I did not comb through all classes, only those that seemed to be likely candidates to me. This fixes alignment crashes for me when using the parallel plane segmentation, for instance.

There is no consistency across the codebase where to place the macro exactly, so I just put it close to the constructor up top.

I also added a few semicolons; some formatters get confused if it's missing.

getModelType () const override { return (SACMODEL_NORMAL_SPHERE); }

PCL_MAKE_ALIGNED_OPERATOR_NEW
PCL_MAKE_ALIGNED_OPERATOR_NEW
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PCL_MAKE_ALIGNED_OPERATOR_NEW
PCL_MAKE_ALIGNED_OPERATOR_NEW;

@mvieth
Copy link
Copy Markdown
Member

mvieth commented Jan 4, 2022

I am confused why you added the macro to some of these classes (e.g. sac_model_perpendicular_plane.h, sac_model_parallel_plane.h, sac_model_parallel_line.h). The only Eigen member they have is Eigen::Vector3f, but the Eigen documentation says that only fixed-size vectorizable Eigen types are affected, which does not include Eigen::Vector3f?

@themightyoarfish
Copy link
Copy Markdown
Contributor Author

themightyoarfish commented Jan 4, 2022 via email

@larshg
Copy link
Copy Markdown
Contributor

larshg commented Jan 4, 2022

I don't see how Vector3f is not a fixed-sized vectorizable type.

They have to a byte size that is multiple of 16 bytes.
Vector3f has 3 floats of 4 bytes, ie 12 bytes. (Do not require alignment)
Vector4f has 4 floats of 4 bytes, ie 16 bytes. (Do require alignment)

@themightyoarfish
Copy link
Copy Markdown
Contributor Author

themightyoarfish commented Jan 5, 2022 via email

@themightyoarfish
Copy link
Copy Markdown
Contributor Author

I guess that once again means that changing the alignment just randomly hides the problem that is actually somewhere else. It's unfortunate that I seem to be the only person for whom this issue recurs time and again.

@themightyoarfish
Copy link
Copy Markdown
Contributor Author

The problem arose with this code

 	  using Cloud = PointCloud<PointXYZI>;
      Cloud::Ptr input_projected{new Cloud};
      ProjectInliers<PointXYZI> proj;
      proj.setModelType(SACMODEL_PARALLEL_PLANE);
      proj.setInputCloud(input);
      proj.setModelCoefficients(coeffs);
      proj.filter(*input_projected);

when input_projected gets deleted, this crashes with an unaligned free.
But I now reinstalled the same PCL version I used when this occurred and now it's gone again. sigh.

@larshg
Copy link
Copy Markdown
Contributor

larshg commented Jan 5, 2022

The problem arose with this code

 	  using Cloud = PointCloud<PointXYZI>;
      Cloud::Ptr input_projected{new Cloud};
      ProjectInliers<PointXYZI> proj;
      proj.setModelType(SACMODEL_PARALLEL_PLANE);
      proj.setInputCloud(input);
      proj.setModelCoefficients(coeffs);
      proj.filter(*input_projected);

when input_projected gets deleted, this crashes with an unaligned free. But I now reinstalled the same PCL version I used when this occurred and now it's gone again. sigh.

Still sounds a lot like a missing AVX(2) flags that could cause this.

@mvieth
Copy link
Copy Markdown
Member

mvieth commented Jan 6, 2022

@themightyoarfish I went over the classes again, and 3 classes might indeed need the macro: voxel_grid_occlusion_estimation.h, voxel_grid.h, and uniform_sampling.h (I will check again to make sure). So if you are interested, we could continue this pull request with those three classes (and formatting improvements if you like)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants