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

[apps] Enable Clang-Format for in_hand_scanner #4804

Merged

Conversation

SunBlack
Copy link
Contributor

No description provided.

@mvieth
Copy link
Member

mvieth commented Jun 23, 2021

I manually ran the documentation CI to see if there are any new doxygen warnings/errors, but it appears that apps are excluded from doxygen. I wonder if that makes sense, because the apps headers are installed and available e.g. via the apt package

@SunBlack
Copy link
Contributor Author

I manually ran the documentation CI to see if there are any new doxygen warnings/errors, but it appears that apps are excluded from doxygen. I wonder if that makes sense, because the apps headers are installed and available e.g. via the apt package

I don't see even any reason to deliver the headers for the apps. Maybe it would be even better to split the PCL package into 3 parts: libpcl (only header), libpcl-dev (libpcl + header), pcl-apps (libpcl + apps). But I think this is not topic of this PR here ;-)

@mvieth
Copy link
Member

mvieth commented Jun 23, 2021

For the parts that are included in the apps library, apparently mainly dominant_plane_segmentation and render_views_tesselated_sphere, it makes sense to deliver the headers and run doxygen. For the point_cloud_editor (and the other apps that are not in apps library) I too currently don't see a reason why the headers should be available. But it could still be good to run doxygen once after these formatting changes to see if the doxygen comments are still valid.
Regarding splitting the PCL package: the current situation is not totally different from what you described: there is a package for each module containing only the module library (e.g. libpcl-filters1.10, libpcl-apps1.10), and there is libpcl-dev containing all the headers (plus some other stuff), and also depending on all individual module library packages.

larshg
larshg previously approved these changes Jun 24, 2021
mvieth
mvieth previously approved these changes Jun 28, 2021
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.

LGTM, but we should discuss sometime whether it is correct that the point_cloud_editor headers are installed

@SunBlack SunBlack dismissed stale reviews from mvieth and larshg via 8e623d8 July 2, 2021 15:04
@SunBlack SunBlack force-pushed the clang-format_in_hand_scanner branch from 42ca63e to 8e623d8 Compare July 2, 2021 15:04
larshg
larshg previously approved these changes Jul 2, 2021
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

11/27 only

@SunBlack
Copy link
Contributor Author

@larshg Since the comments are always related to doxygen: I suggest to apply these changes separately, e.g. if someone has time to create a python script to format them automatically (so it can be also checked on the CI).

@larshg
Copy link
Contributor

larshg commented Oct 28, 2021

I guess it was intended for @kunaltyagi, @SunBlack ?

@SunBlack
Copy link
Contributor Author

Ah well yes, sry^^

@mvieth
Copy link
Member

mvieth commented Jan 1, 2022

Could you try @kunaltyagi 's doxygen_fixer script from pull request #5085 on your changes here?

@SunBlack
Copy link
Contributor Author

SunBlack commented Jan 3, 2022

Could you try @kunaltyagi 's doxygen_fixer script from pull request #5085 on your changes here?

Tried it, but it misses some issues and also add some issue. So I did it now manually by searching for \

PR can be squashed when merging.

@mvieth mvieth requested a review from kunaltyagi January 5, 2022 07:45
@mvieth
Copy link
Member

mvieth commented Feb 7, 2022

@kunaltyagi Would you review this one again?


xyz_mask(r, c) = hsv_mask(r, c) = false;

if (!std::isnan(xyzrgb.x) && !std::isnan(normal.normal_x) && xyzrgb.x >= x_min &&
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: check for isnan on x and normal_x looks funky

@kunaltyagi kunaltyagi merged commit 2b618da into PointCloudLibrary:master Feb 8, 2022
@SunBlack SunBlack deleted the clang-format_in_hand_scanner branch February 8, 2022 19:29
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.

4 participants