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

Pointcloud example #276

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Pointcloud example #276

merged 1 commit into from
Mar 11, 2021

Conversation

flynneva
Copy link
Contributor

@flynneva flynneva commented Jun 1, 2020

added example that publishes an XYZI pointcloud

@flynneva
Copy link
Contributor Author

flynneva commented Jun 6, 2020

the pointfield creation is a little messy. I tried using the old ROS 1 way but it doesnt seem to be supported just yet.

@sloretz
Copy link
Contributor

sloretz commented Jun 25, 2020

@wjwwood friendly ping :)

@flynneva
Copy link
Contributor Author

flynneva commented Aug 8, 2020

would be great to get this example in the next update for foxy.

let me know what else is left to fix/add to be able to get this merged @wjwwood

@flynneva
Copy link
Contributor Author

flynneva commented Nov 9, 2020

@wjwwood, @dirk-thomas, @sloretz....friendly ping 😄

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left a few things inline.

Also, I don't think this belongs in executors. That subdirectory is really for showing off how to use the various executors, and this is not one of them. I think it belongs either in topics, or in a new top-level subdirectory.

@flynneva
Copy link
Contributor Author

flynneva commented Nov 24, 2020

@clalancette thanks for the review! i went ahead and implemented your recommendations, following the similar type architecture for the ROS1 pointcloud publisher example.

looks like this example - if following the similar structure for ROS1 - requires the release of this new sensor_msgs_py package. ive done what i can to try and help further that effort along...but this example wont work without the release of that package.

@flynneva
Copy link
Contributor Author

flynneva commented Nov 24, 2020

Also, I don't think this belongs in executors. That subdirectory is really for showing off how to use the various executors, and this is not one of them. I think it belongs either in topics, or in a new top-level subdirectory.

@clalancette would something like a publishers directory make sense? or were you thinking more like an advanced directory?

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left one more thing to fix inline.

looks like this example - if following the similar structure for ROS1 - requires the release of this new sensor_msgs_py package.

OK, I just did a review of that one, it needs a bit more work to get in. We can do a release once that one is merged.

@clalancette would something like a publishers directory make sense? or were you thinking more like an advanced directory?

I think what we should do here is make a new subdirectory underneath https://github.com/ros2/examples/tree/master/rclpy/topics called pointcloud_publisher, or something like that. Then put this example in there. Note that this would be a new package, so would need package.xml, setup.cfg, setup.py, etc.

@flynneva
Copy link
Contributor Author

@clalancette i moved it to its own package under the topics directory like you requested.

compiled & tested using the common_interfaces package mentioned above. works as expected.

rclpy/executors/setup.py Outdated Show resolved Hide resolved
rclpy/topics/pointcloud_publisher/package.xml Outdated Show resolved Hide resolved
rclpy/topics/pointcloud_publisher/package.xml Outdated Show resolved Hide resolved
@flynneva
Copy link
Contributor Author

flynneva commented Dec 3, 2020

@clalancette CI failing due to missing sensor_msgs_py. i tested it locally now with the new name and it works as expected. also i confirmed no linting/copyright errors and added in backwards compatibility to the flake8 test file with the main_with_errors change.

let me know if there is anything else I can help out with. I noticed there are other linting errors in the other examples, ill see if I can open another PR to fix those sometime soon.

Signed-off-by: flynneva <evanflynn.msu@gmail.com>
@flynneva
Copy link
Contributor Author

friendly ping @wjwwood @clalancette. let me know if you'd like anything else changed on this PR before merging

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I started this review months ago, but for some reason I never posted it... sorry about that.

They may not even apply anymore, but I'll post them just in case.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Looks like all my previous comments were already addressed, so lgtm.

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2021

@clalancette I think your issues have been addressed can I merge?

@clalancette
Copy link
Contributor

@clalancette I think your issues have been addressed can I merge?

Yes, looks good now, please go ahead and merge.

@wjwwood wjwwood merged commit 64bd3a0 into ros2:master Mar 11, 2021
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.

4 participants