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

Modify README for the port to MoveIt! 2.0 #1

Merged
merged 8 commits into from
Feb 20, 2019

Conversation

vmayoral
Copy link
Contributor

No description provided.

@rhaschke
Copy link
Contributor

Dear @vmayoral and @davetcoleman,

I'm not sure who decided to perform MoveIt ROS2 development within a new repository clone. This was not discussed in a maintainer meeting and I don't like the idea, because this makes it much harder to cherry-pick changes back and forth between ROS1 and ROS2 development branches.
I'm not sure, @davetcoleman, you really want to decouple MoveIt ROS2 development completely from ROS1 progress.

@davetcoleman
Copy link
Member

@rhaschke,

Several reasons led us to this decision:

  • Keep PRs related to ROS2 separate from ROS1 so as to not clutter the already large list of pull requests
  • We can still rebase and sync the two projects as they have the same git history still, for now. My goal is to keep syncing it for the next few months but eventually it will become more and more challenging and there are some big refactoring changes i want to do that i think will break the ability to sync in the future
  • PRs move really slowly in moveit1 and we can't have that with moveit2

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

A few suggestions

README.md Outdated
- [Overview of MoveIt! for ROS 2.0](#)
- [Installation Instructions](#)
- [Documentation](#)
- [Get Involved](#)
Copy link
Member

Choose a reason for hiding this comment

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

these are all dead links, i think they should still point to the previous locations. our intent is not to kill moveit.ros.org

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that's a leftover 8b730c8

README.md Outdated
- [Documentation](http://moveit.ros.org/documentation/)
- [Get Involved](http://moveit.ros.org/documentation/contributing/)
## Legal disclaimer
**WORK IN PROGRESS**
Copy link
Member

Choose a reason for hiding this comment

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

why is this work in progress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved in the commit described below.

README.md Outdated

## Continuous Integration Status
*The material and information contained in this repository is published in good faith and is for the benefit of the general ROS 2.0 community only. You should not rely upon the material or information in this repository as a basis for making any business, legal or any other decisions. Acutronic Robotics makes no representations, claims, promises, guarantees or warranties of any kind, express or implied about the completeness, accuracy, reliability, suitability or adequacy with respect to the software, information or related graphics contained in this repository for any purpose. Acutronic Robotics is not liable for any loss, damage or injury that may arise in connection with this repository. Any action or reliance you take or place based upon the information in this repository is strictly at your own risk.*
Copy link
Member

Choose a reason for hiding this comment

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

the BSD license covers these issues already, this disclaimer is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

[![Build Status](https://travis-ci.org/AcutronicRobotics/moveit2.svg?branch=ros2)](https://travis-ci.org/AcutronicRobotics/moveit2)
Copy link
Member

Choose a reason for hiding this comment

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

I've just turned on Travis for moveit2:
https://travis-ci.org/ros-planning/moveit2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f925d83, please review. Note I pointed to master branch.

@vmayoral
Copy link
Contributor Author

Hello @rhaschke,

This was not discussed in a maintainer meeting and I don't like the idea, because this makes it much harder to cherry-pick changes back and forth between ROS1 and ROS2 development branches.

I'm sorry if we stepped on someone's toes, our intention is to contribute. Our team will be happy to participate in the next developers' session. We started working at https://github.com/AcutronicRobotics/moveit2-original (which should disappear as soon as we stabilize things here, or somewhere else) for the same reasons @davetcoleman provides above, in particular and mainly, to have a clean, easy to manage and accesible (to our team) repository. Overall, to advance faster.

this makes it much harder to cherry-pick changes back and forth between ROS1 and ROS2 development branches

True, it will become harder but arguably, and as far as I can see for now, the difficulty of cherry-picking is somewhat manageable. Changes from MoveIt! in ROS 1 (for now and while we keep things coherent) shouldn't imply much more than:

git remote add moveit https://github.com/ros-planning/moveit
git fetch moveit
git cherry-pick ...

@rhaschke
Copy link
Contributor

@davetcoleman

  • Keep PRs related to ROS2 separate from ROS1 so as to not clutter the already large list of pull requests

That one valid argument.

  • We can still rebase and sync the two projects as they have the same git history still, for now.

So you started the new repo from the current melodic-branch including all history?
I'm wondering why the latest commits on moveit2's master already contain some unreviewed PRs of mine against ROS1's melodic-devel...

There are some big refactoring changes i want to do that i think will break the ability to sync in the future

Would be great if you could share your ideas in one of the next maintainer meetings 😉
Potentially these refactoring changes also might apply to ROS1' MoveIt.

  • PRs move really slowly in moveit1 and we can't have that with moveit2

Do you want to relax the reviewing process to improve on this? If not, I don't see how you can progress faster on MoveIt2 than on MoveIt1. The group of people doing reviews stays the same, doesn't it?

vmayoral and others added 3 commits February 19, 2019 20:47
Point to official MoveIt! repos
Co-Authored-By: vmayoral <victor@erlerobotics.com>
@rhaschke
Copy link
Contributor

Dear @vmayoral

I appreciate very much your and your team's involvement in migrating MoveIt to ROS2. Thank you very much for this effort in advance! However, I was surprised and confused that we have chosen the approach of a new repo, because we discussed to keep things in sync as long as possible. Now, I'm afraid that the two repos will quickly diverge.

@vmayoral
Copy link
Contributor Author

@rhaschke, thanks for your quick response.

Now, I'm afraid that the two repos will quickly diverge.

Well this is certainly concerning. Our team is certainly open to consider other possibilities.
I personally don't see much of an improvement between this approach and having a ros2 branch within ros-planning/moveit. Quite the opposite, but this is only the feeling of a newcomer.

Maybe having weekly sync-ups with moveit main branch would help @rhaschke?

davetcoleman and others added 2 commits February 19, 2019 20:59
Point to the issue for further discussion.

Co-Authored-By: vmayoral <victor@erlerobotics.com>
@rhaschke
Copy link
Contributor

As you started from MoveIt1's branch, syncing should be indeed feasible.

@davetcoleman
Copy link
Member

I'm wondering why the latest commits on moveit2's master already contain some unreviewed PRs of mine against ROS1's melodic-devel...

You already found the source of this issue being a mistaken git push: moveit/moveit#1356

Overall, your concerns @rhaschke are all valid, I think we clarified some in an email exchange and we can discuss further at next Thu's maintainer meeting

@davetcoleman davetcoleman merged commit 9f20d5f into moveit:master Feb 20, 2019
mlautman pushed a commit that referenced this pull request Mar 8, 2019
Convert geometry_msgs/Transform to geometry_msgs/TransformStamped
@anasarrak anasarrak deleted the readme branch April 12, 2019 15:50
seanyen referenced this pull request in seanyen/moveit2 Jul 21, 2020
AndyZe pushed a commit that referenced this pull request Dec 8, 2021
Refactor Global and Local Planner Components into NodeClasses

Add a simple launch test (#1)

Try to fix plugin export; add helpful debug when stuck

Error if global planning fails

READY and AWAIT_TRAJ states are redundant

Lock the planning scene as briefly as possible

Specify joint group when checking waypoint distance

Implement a reset() function in the local planner

Detect when the local planner gets stuck
MikeWrock referenced this pull request in MikeWrock/moveit2 Aug 15, 2022
* adding rosdep install and catkin config to instructions
* updating visualization tutorial
MikeWrock referenced this pull request in MikeWrock/moveit2 Aug 15, 2022
@AndyZe AndyZe mentioned this pull request Jul 28, 2023
7 tasks
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.

None yet

4 participants