moveit2_tutorials icon indicating copy to clipboard operation
moveit2_tutorials copied to clipboard

Refactored perception pipeline tutorial from ros1 to ros 2

Open CihatAltiparmak opened this issue 1 year ago • 3 comments

Description

I have a PR for refactoring perception pipeline tutorial based on ROS 2. But i have several question about my PR.

  1. I used some external links for not storing big files such as video, bag file which is created by me. Is it okay to give a link to my google drive for bag file and a link to Youtube video for perception pipeline demonstration or should i change these links?

  2. I used Intel's sdf file in the tutorial which the relevant repository has Apache 2.0 License. Does that occur any problem for this organization? Btw, ros2/launch has also Apache 2.0 License. But it is not indicated in this repository. I need a feedback for this situation.

Checklist

  • [ ] Required by CI: Code is auto formatted using clang-format
  • [ ] While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

CihatAltiparmak avatar May 21 '24 22:05 CihatAltiparmak

Just for friendly pinging, @sjahr

CihatAltiparmak avatar Jun 24 '24 07:06 CihatAltiparmak

I used Intel's sdf file in the tutorial which the relevant repository has Apache 2.0 License. Does that occur any problem for this organization?

Good question :thinking: did you use the sdf or the mesh file or both?

sjahr avatar Jun 24 '24 08:06 sjahr

Good question 🤔 did you use the sdf or the mesh file or both?

I had added Intel's license content to mesh file like below. I used their mesh file

Screenshot from 2024-06-24 13-51-39

The most of the sdf is mine however i just took some camera measurement from them. If it is boring in your perspective, I can remove it.

CihatAltiparmak avatar Jun 24 '24 11:06 CihatAltiparmak

@CihatAltiparmak Maybe this is helpful as a reference: https://github.com/moveit/moveit2_tutorials/pull/700/files

sjahr avatar Aug 05 '24 17:08 sjahr

Hello @sjahr , i have added the depth image octomap updater to the perception pipeline tutorial. However i have found some bugs in depth image updater and i have a fix for it. Btw i've also sent PR to moveit_benchmark_resources.

My PR in moveit_benchmark_resources : https://github.com/moveit/moveit_benchmark_resources/pull/3 My PR to fix bug in depth image updater plugin: https://github.com/moveit/moveit2/pull/2963

After merging the above PR's, i believe we can finalize this work.

CihatAltiparmak avatar Aug 10 '24 23:08 CihatAltiparmak

@sjahr I've applied your suggestion in moveit_benchmark_resources and here. I've added redirection to https://github.com/moveit/moveit_benchmark_resources repository.

CihatAltiparmak avatar Aug 14 '24 00:08 CihatAltiparmak

Oh and can you fix the broken link CI error? https://github.com/moveit/moveit2_tutorials/actions/runs/10389968570/job/28769302814?pr=906. Looks like you just need to redirect it to moveit_benchmarking_resources

sjahr avatar Aug 19 '24 08:08 sjahr

Oh and can you fix the broken link CI error? https://github.com/moveit/moveit2_tutorials/actions/runs/10389968570/job/28769302814?pr=906. Looks like you just need to redirect it to moveit_benchmarking_resources

I know, it's searching for bag_files/depth_camera_bag directory. We need to merge my moveit_benchmark_resources pr first to fix CI

CihatAltiparmak avatar Aug 19 '24 08:08 CihatAltiparmak

Oh and can you fix the broken link CI error? https://github.com/moveit/moveit2_tutorials/actions/runs/10389968570/job/28769302814?pr=906. Looks like you just need to redirect it to moveit_benchmarking_resources

I know, it's searching for bag_files/depth_camera_bag directory. We need to merge my moveit_benchmark_resources pr first to fix CI

@CihatAltiparmak I merged it ~1h ago. Can you run formatting and retrigger CI by pushing. Then this should be good to go

sjahr avatar Aug 19 '24 09:08 sjahr

All CIes are passed. Let's roll. Btw thank you so much for your effort :heart:

CihatAltiparmak avatar Aug 19 '24 09:08 CihatAltiparmak

Thank you for porting this!

sjahr avatar Aug 19 '24 10:08 sjahr

@sjahr I'm glad a long-standing ticket https://github.com/moveit/moveit2_tutorials/issues/25 is finally closed by this PR. Then, more than a year ago I made basically the same effort https://github.com/moveit/moveit2_tutorials/pull/700. You also commented you'd start reviewing https://github.com/moveit/moveit2_tutorials/pull/700#issuecomment-1645650394. I admit I hadn't kept asking for review recently though, I'm honestly a bit sad. It'd be great in the future if open PRs aren't discarded without any accolade like in this way.

130s avatar Aug 20 '24 19:08 130s

@130s you're absolutely right and I am very sorry that this happened due to my inattentiveness. As you pointed out I even committed to reviewing your PR last year and then never did it :disappointed: This should not happen and you're right to be frustrated about my (in)action. I think we should re-open your PR as this merged version is a simplified version of the ROS 1 tutorial that doesn't include the Detecting and Adding Object as Collision Object section that you ported as well and use your work to enhance and improve the existing version.

sjahr avatar Aug 21 '24 09:08 sjahr

I want to write as well. Firstly really apologies for this action @130s, You are absolutely right. I already saw your PR before i didn't open this PR. As a newbie of MoveIt2, i strongly needed to learn perception pipeline application in Moveit and i had limited time. I took a look at MoveIt1's tutorial and it didn't be helpful for me as newbie. When i see your PR has no active works, i decided to continue to my work. I have added depth camera octomap updater and i found 2 bugs in MoveIt2.

But i just want to empathize the maintainer as well. When i open the review page of my PR, i saw a lot of complex works for maintainer side and a lot of PR's to review. Btw @sjahr also suggested your PR (https://github.com/moveit/moveit2_tutorials/pull/906#issuecomment-2269539625)

If you still want to help us, my PR can have some lack of information, i become glad for this works. @sjahr 's suggestions are also good.

Best for you

CihatAltiparmak avatar Aug 21 '24 10:08 CihatAltiparmak

@sjahr @CihatAltiparmak You guys made my day. Thanks, thanks. I'll see what I can pick up from #700. Another good thing is now we have a new reviewer for perception pipeline :)

130s avatar Aug 21 '24 16:08 130s