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

Refactored perception pipeline tutorial from ros1 to ros 2 #906

Merged
merged 22 commits into from
Aug 19, 2024

Conversation

CihatAltiparmak
Copy link
Member

@CihatAltiparmak CihatAltiparmak commented May 21, 2024

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
Copy link
Member Author

Just for friendly pinging, @sjahr

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Nice work, I still need to test it but I left a couple of comments

@sjahr
Copy link
Contributor

sjahr commented Jun 24, 2024

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 🤔 did you use the sdf or the mesh file or both?

- Added new video demo in webm format
- Removed youtube video link
- Renamed moveit2_tutorials instead of ${PROJECT_NAME} in CMakeLists.txt
- Fixed the typo
@CihatAltiparmak
Copy link
Member Author

CihatAltiparmak commented Jun 24, 2024

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 sjahr self-requested a review July 31, 2024 14:51
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

I tested it and it works! I've added a bunch of comments and suggestions and once they are addressed we can finally merge this 🚀

@sjahr
Copy link
Contributor

sjahr commented Aug 5, 2024

@CihatAltiparmak
Copy link
Member Author

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 : moveit/moveit_benchmark_resources#3
My PR to fix bug in depth image updater plugin: moveit/moveit2#2963

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

@CihatAltiparmak
Copy link
Member Author

CihatAltiparmak commented Aug 14, 2024

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

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Just some nitpicks. I am testing the tutorial one last time with the new files and then we'll merge it

@sjahr
Copy link
Contributor

sjahr commented Aug 19, 2024

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

…mo.launch.py

Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
CihatAltiparmak and others added 4 commits August 19, 2024 11:32
…mo.launch.py

Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
…mo.launch.py

Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
…mo.launch.py

Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
@CihatAltiparmak
Copy link
Member Author

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
Copy link
Contributor

sjahr commented Aug 19, 2024

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
Copy link
Member Author

CihatAltiparmak commented Aug 19, 2024

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

@sjahr sjahr merged commit 9e74acd into moveit:main Aug 19, 2024
8 checks passed
@sjahr
Copy link
Contributor

sjahr commented Aug 19, 2024

Thank you for porting this!

@130s
Copy link
Contributor

130s commented Aug 20, 2024

@sjahr I'm glad a long-standing ticket #25 is finally closed by this PR.
Then, more than a year ago I made basically the same effort #700. You also commented you'd start reviewing #700 (comment). 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
Copy link
Contributor

sjahr commented Aug 21, 2024

@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 😞 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
Copy link
Member Author

CihatAltiparmak commented Aug 21, 2024

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 (#906 (comment))

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
Copy link
Contributor

130s commented Aug 21, 2024

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

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.

3 participants