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

ROS 2 dashing pcl_ros port #255

Closed

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Dec 5, 2019

This is still a work in progress (with a lot more work to do), but the Crop Box and Pass Through filters do work. It has everything from #229 plus changes from @mabelzhang's branch. I'm opening it for visibility.

It should build fine in Eloquent, but requires this rclcpp release to build in Dashing: ros/rosdistro#23240

@sloretz sloretz mentioned this pull request Dec 5, 2019
@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 5, 2019

It seems like there's been alot done and that might not be a realistic to ask to separate this into smaller PRs. As long as someone has tested the basic behavior of each of the parts ported I don't see a reason to block merging it so there's "something" out there, but it will be challenging or near impossible to have too much critical feedback on a 5,000+ line, 200 files touched PR.

Are there areas this PR hasn't touched (yet) that we could rescope for either someone else to work on in parallel or at least separate into another PR so that we can break apart what we can going forward? Looking for a specific set of things that I could give people to work on in a couple bullets. Might also be good to have a ticket filed that we check off what's done in some pending PR or who's working on it

@sloretz
Copy link
Contributor Author

sloretz commented Dec 6, 2019

It seems like there's been alot done and that might not be a realistic to ask to separate this into smaller PRs.

Yeah there are a lot of changes now. I was expecting to split it eventually.

Are there areas this PR hasn't touched (yet) that we could rescope for either someone else to work on in parallel or at least separate into another PR so that we can break apart what we can going forward?

There's nothing untouched, but there's plenty that can be done in parallel. Since there's a lot of progress in this branch, I'd recommend a workflow of branching from dashing-devel then checking out individual files from here as a starting point.

Before that there should a PR for pcl_ros_tf. Afterward the tools/executables can all be done in parallel, plus each plugin category can be done in parallel (once a first PR withpcl_node.h changes is merged). Within a plugin category there needs to be a first example that ports the base class (filter.cpp, feature.cpp, etc) first, but once that's done porting the rest becomes much easier.

I haven't been careful about style or whitespace changes. Can I assume those can be cleaned up later?

@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 6, 2019

Yeah, I'm not a big worrier about linting. Its not like this was clean to begin with so that's an "future improvement". Apparently @JWhitleyAStuff is a saint and enjoys that type of thing, so we might be able to rope him into doing it. If not I'm sure someone will down the line.

There's nothing untouched, but there's plenty that can be done in parallel.

This sounds like merge conflict-central.

I'd prefer if we could merge this PR once its in a reasonable state to do so, then moving forward have a ticket of tasks broken down so that there's a clear separation of what's done, what's being done, and what's open to be done. But with that said, you're the one at the moment "doing the stuff" so by all means I don't want to slow you down.

Signed-off-by: Shane Loretz <[email protected]>
@theseankelly
Copy link
Contributor

theseankelly commented Jul 3, 2020

Hey @sloretz -- @SteveMacenski reached out for general help over on discourse and I signed up to help.

Do you have plans/desires to rebase this work and push it through to Foxy? If so, any specific things you'd like help with? If not, I'll just dig in.

@SteveMacenski
Copy link
Member

@theseankelly my github ID is stevemacenski not smac (side story: I tried desperately to change my discourse name to stevemacenski to fix that exact issue but couldn't figure out how).

I think you may to dig in. All the folks at OR are very busy and if this has been sitting around for this long, I couldn't expect anything to change.

@theseankelly
Copy link
Contributor

@SteveMacenski whoops sorry! Sounds good. I'll do what I can to chunk it up into smaller, more standalone PRs as I go.

@SteveMacenski
Copy link
Member

Please, please, please do

theseankelly added a commit to theseankelly/perception_pcl that referenced this pull request Aug 31, 2020
@sloretz
Copy link
Contributor Author

sloretz commented Jan 25, 2022

I noticed this repo has a ros2 branch and has been released to rolling. I'll close this assuming the ROS 2 port has been completed.

@sloretz sloretz closed this Jan 25, 2022
@vincentrou
Copy link

@sloretz I want to point out that the release on ros2 only concern pcl_ros_tf.
There is a lot of commented library in the CMakeLists : https://github.com/ros-perception/perception_pcl/blob/ros2/pcl_ros/CMakeLists.txt
So this work is still needed.

@vincentrou vincentrou mentioned this pull request Mar 17, 2022
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.

6 participants