Skip to content

Cleanup gpu fpfh and remove unnecessary headers#6311

Merged
larshg merged 2 commits intoPointCloudLibrary:masterfrom
jmackay2:gpu_fpfh_cleanup
Aug 3, 2025
Merged

Cleanup gpu fpfh and remove unnecessary headers#6311
larshg merged 2 commits intoPointCloudLibrary:masterfrom
jmackay2:gpu_fpfh_cleanup

Conversation

@jmackay2
Copy link
Contributor

This cleans up some unnecessary headers.

This was tested on Ubuntu 24.04, successfully building and running.

Comment on lines -51 to -53
#ifndef M_PI
#define M_PI 3.14159265358979323846f
#endif
Copy link
Member

Choose a reason for hiding this comment

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

M_PI is used in this file further down, and I assume there is a reason why these lines have been added in the past, so I would rather keep them. Or is there a reason why you removed them? Did they cause any problem for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be required on windows - alternatively we could include #include <pcl/pcl_macros.h> as it has the definition as well. Same for vfh.cu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, and gpu aren't build on windows CI, so we have to verify explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

I think there have been problems when including pcl_macros.h in a .cu file? I would rather just keep these three lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I originally removed this because I saw that PI was defined in internal.hpp which was being included, but I didn't notice M_PI was named different. On my machine this works because it is using the definition in /usr/include/math.h, which probably won't work on windows. Should I put it back in or switch the use of M_PI to PI defined in internal.hpp?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer putting the three lines back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks

@larshg larshg merged commit 5b2da87 into PointCloudLibrary:master Aug 3, 2025
13 checks passed
@larshg larshg added this to the pcl-1.15.1 milestone Aug 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants