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

Moved PointXYZLAB to common point types #4706

Merged
merged 8 commits into from
Apr 26, 2021

Conversation

ueqri
Copy link
Contributor

@ueqri ueqri commented Apr 17, 2021

Fixes #4600

I've compiled this branch on Ubuntu 20.10 GCC and passed all unit tests 😆. I'd be grateful if you could have a review on it. Thank you!

@kunaltyagi
Copy link
Member

You should add a git pre-commit hook to run make format or ninja format 😆 for the CI

@ueqri
Copy link
Contributor Author

ueqri commented Apr 18, 2021

You should add a git pre-commit hook to run make format or ninja format for the CI

Thank you for reminding me of the format check 😃 in CI ! I've applied the patch to yesterday's commit, and added pre-commit hook with .dev/format.sh to ensure the behavior is the same as CI.

ueqri and others added 2 commits April 19, 2021 17:02
* added constraint for XYZRGB2XYZLAB template function

Co-authored-by: Kunal Tyagi <tyagi.kunal@live.com>
@ueqri
Copy link
Contributor Author

ueqri commented Apr 19, 2021

I think I've fixed the compiling error of LUT (built by Clang Darwin and MSVC) in fa0782c, by adding the header of std::array which needs to be included explicitly especially for those compilers.

And the CI shows the branch passed {Clang macOS, GCC Ubuntu, MSVC Windows x86} build, but met error in MSVC Windows x64 build due to the lack of space (detailed log) .

Could you give me some advice about solving this error, or just let it go? BTW, I found a relevant issue #3387 and it seems to be a tricky problem, but I'm willing to work on it.:smiley: Is there any plan to tackle this issue and enhance the quality of CI? I'm really glad to do it!

@larshg
Copy link
Contributor

larshg commented Apr 19, 2021

I just restarted the win x64 build.

The space error is quite spurious and its not really space problem. We tried moving the build from the temporary D drive to the C drivce, which has about 78G free space at start.

But what causes the error, is probably a task for the DevOps people at MS. But if you have some good ideas, please go ahead.

@ueqri
Copy link
Contributor Author

ueqri commented Apr 19, 2021

Thank you for rerunning the CI and the helpful explanations:blush:! I would try to find out the causes and solutions if possible.

kunaltyagi
kunaltyagi previously approved these changes Apr 20, 2021
common/include/pcl/point_types_conversion.h Outdated Show resolved Hide resolved
registration/include/pcl/registration/gicp6d.h Outdated Show resolved Hide resolved
common/include/pcl/point_types_conversion.h Outdated Show resolved Hide resolved
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.

Thank you for working on this!

@mvieth mvieth merged commit 842ff67 into PointCloudLibrary:master Apr 26, 2021
mvieth pushed a commit to mvieth/pcl that referenced this pull request Dec 27, 2021
* moved PointXYZLAB to common point types

* fixed SEGFAULT in registration test

* fixed format error from CI patch

* made XYZRGB2XYZLAB templated and added array header

* Update common/include/pcl/point_types_conversion.h

* added constraint for XYZRGB2XYZLAB template function

Co-authored-by: Kunal Tyagi <tyagi.kunal@live.com>

* added constraint for XYZRGB2XYZLAB template function

* removed some redundant codes & tidied headers

* fixed format error in gicp6d.cpp

Co-authored-by: Kunal Tyagi <tyagi.kunal@live.com>
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.

Move PointXYZLAB to common?
4 participants