moveit / moveit2_tutorials

A sphinx-based centralized documentation repo for MoveIt 2
https://moveit.picknik.ai
BSD 3-Clause "New" or "Revised" License
162 stars 196 forks source link

Refactored perception pipeline tutorial from ros1 to ros 2 #906

Closed CihatAltiparmak closed 3 months ago

CihatAltiparmak commented 6 months ago

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

CihatAltiparmak commented 5 months ago

Just for friendly pinging, @sjahr

sjahr commented 5 months ago

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?

CihatAltiparmak commented 5 months ago

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.

sjahr commented 3 months ago

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

CihatAltiparmak commented 3 months ago

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 commented 3 months ago

@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.

sjahr commented 3 months ago

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

CihatAltiparmak commented 3 months ago

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

sjahr commented 3 months ago

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

CihatAltiparmak commented 3 months ago

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

sjahr commented 3 months ago

Thank you for porting this!

130s commented 3 months ago

@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.

sjahr commented 3 months ago

@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.

CihatAltiparmak commented 3 months ago

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

130s commented 3 months ago

@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 :)