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

Velodyne pcl #335

Merged
merged 9 commits into from
Jun 9, 2020
Merged

Velodyne pcl #335

merged 9 commits into from
Jun 9, 2020

Conversation

spuetz
Copy link
Contributor

@spuetz spuetz commented Mar 24, 2020

Regrading #287 The velodyne_pcl ROS package restores the conversion file point_types.h.
The README explains how to use it.

The following has been done:

  • Fixed assignment of the time value in the organized point cloud container
  • Added velodyne_pcl ROS package with the point_types.h header file.
  • Added a RAEDME.md file to explain how to convert velodyne ROS point clouds to PCL
  • Renamed both containers to cover the previously added time property.

@spuetz
Copy link
Contributor Author

spuetz commented Mar 24, 2020

@JWhitleyWork Could you test the conversion? Cheers.

@JWhitleyWork
Copy link
Contributor

@spuetz Just two requests if you could:

  1. Please add a nodelet which simply does the conversion and a launch file that runs the nodelet with parameters for a nodelet manager and input and output topics.
  2. Please add roslint to the package and the associated test.

@JWhitleyWork
Copy link
Contributor

Thanks again for working on this.

@spuetz
Copy link
Contributor Author

spuetz commented Mar 26, 2020

The conversion is also automatically done by the subscriber and the publisher. See http://wiki.ros.org/pcl/Overview#Publishing_and_subscribing_to_point_cloud_messages
I think it would make sense to just provide a test where we know which data is in the could and then compare it after conversion.
Would be good if someone could write the test, I don't have much time... and I think you are more into the testing of your laser scan data with the pcap data.

@JWhitleyWork
Copy link
Contributor

Thanks for the clarification, @spuetz.

@JWhitleyWork
Copy link
Contributor

@spuetz I'll add roslint to the package. However, I've also just realized that this MR breaks API in another way by doing away with the PointCloudXYZIR type. While I agree that the type name should match the contents, this name change breaks existing use of the type by many, many examples on Github. This change can be introduced in Noetic, but not in the Melodic LTS. Please revert this commit.

@spuetz
Copy link
Contributor Author

spuetz commented Apr 3, 2020

Someone introduced the time field in that container, instead of creating a new one. That mainly caused the problem. Cleanest solution would be to add another container without the time field and to add the corresponding point type here.

These depending packages, you mentioned, they also need to change the include file to velodyne_pcl/point_types.h. Additionally they need to install the velodyne_pcl package.

@spuetz
Copy link
Contributor Author

spuetz commented Apr 26, 2020

@JWhitleyWork Any news here? See my comment above.

@JWhitleyWork
Copy link
Contributor

@spuetz Sorry I haven't responded lately. Yes, I understand that users will need to change the include path and depend on velodyne_pcl. Since these changes are necessary already, changing the point type doesn't seem like that big of a deal.

I'm creating a PR to your branch right now to add roslint.

@JWhitleyWork
Copy link
Contributor

PR Created for roslint: spuetz#1

@YoshuaNava
Copy link

@JWhitleyWork @spuetz I'm looking forward to this PR being merged and would be glad to help test if needed. I have a VLP-16 that I can use for tests.

@JWhitleyWork
Copy link
Contributor

@YoshuaNava I would really appreciate that! Could you possibly test this prior to merge? If you clone from @spuetz fork of the repository and check out his velodyne_pcl branch, you should be able to build and test it. Please let me know how it goes and I'll merge it immediately after!

@YoshuaNava
Copy link

@JWhitleyWork Can you send me a short list of the functionality I should text, and what is expected from each test?

Copy link
Contributor

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

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

See comments.

velodyne_pcl/CMakeLists.txt Outdated Show resolved Hide resolved
velodyne_pcl/package.xml Outdated Show resolved Hide resolved
velodyne_pcl/package.xml Outdated Show resolved Hide resolved
@JWhitleyWork
Copy link
Contributor

Last item: can you rebase on master so it uses Noetic CI?

@JWhitleyWork JWhitleyWork reopened this Jun 9, 2020
@JWhitleyWork JWhitleyWork merged commit 3237b8a into ros-drivers:master Jun 9, 2020
wep21 pushed a commit to wep21/velodyne that referenced this pull request Dec 28, 2021
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Jan 4, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Jan 5, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Jan 6, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Feb 2, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Feb 2, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 mentioned this pull request Feb 2, 2022
wep21 pushed a commit to wep21/velodyne that referenced this pull request Mar 3, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Mar 3, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Mar 5, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
wep21 pushed a commit to wep21/velodyne that referenced this pull request Dec 8, 2022
* fix time assignment in organized cloud container

* add velodyne_pcl package with point_types.h

* add README.md with infos for conversion

* rename containers to cover the added time property

* Adding roslint to velodyne_pcl. (#1)

* Update CMake version to 3.5

* Update package.xml to Format2 and package version to 1.5.2

* Update ros package format

Co-authored-by: Joshua Whitley <josh.whitley@autoware.org>
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.

None yet

3 participants