-
Notifications
You must be signed in to change notification settings - Fork 523
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
MoveIt Setup Assistant - Merge the Feature branch #1254
Conversation
This pull request is in conflict. Could you fix it @DLu? |
Codecov Report
@@ Coverage Diff @@
## main #1254 +/- ##
==========================================
+ Coverage 61.56% 61.56% +0.01%
==========================================
Files 274 274
Lines 24977 24977
==========================================
+ Hits 15374 15375 +1
+ Misses 9603 9602 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much more than I had thought initially. Great work, everything looks really well structured and clean. This review is rather superficial, I still want to understand the architecture a little better with a second pass. Running the MSA and generating a config package worked without any issues for me. I just got stuck launching MoveGroup with it, primarily because moveit_configs_utils still seems to look for all capital yaml parameters here. With that fixed, I'm still running into some issues but will investigate them further before throwing them at you. For the remaining TODOs, it would be great if you could document them in one or multiple issues.
I assume that your are planning to preserve most of the commit history and don't want the branch to be squashed, right?
If yes, please squash some of the fix-up commits into bigger chunks, ideally the branch would not have much more than 10 commits or so. If you're fine with squashing, ignore this.
moveit_setup_assistant/moveit_setup_app_plugins/src/perception_config.cpp
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_app_plugins/src/perception_config.cpp
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_app_plugins/src/perception_widget.cpp
Outdated
Show resolved
Hide resolved
...it_setup_assistant/moveit_setup_srdf_plugins/include/moveit_setup_srdf_plugins/srdf_step.hpp
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_srdf_plugins/src/end_effectors_widget.cpp
Outdated
Show resolved
Hide resolved
return true; | ||
} | ||
|
||
void GroupMetaConfig::collectVariables(std::vector<moveit_setup_framework::TemplateVariable>& variables) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't look like it's of much use in a ROS 2 setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to an example of a MoveIt config that uses additional kinematic parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BioIK and TracIk use additional parameters, but they are not supported in MoveIt 2 yet
moveit_setup_assistant/moveit_setup_srdf_plugins/src/robot_poses_widget.cpp
Outdated
Show resolved
Hide resolved
ea483b8
to
2d18fd7
Compare
Alright team, what do we need to get this baby merged? |
I'm happy merging it as soon as we get 2 approvals. We should continue the TODO issues once this branch is merged imo. Humble release is today. |
@DLu I'm running into some issues with generated configs. I just made a plain Panda config which went well without any issues. However, I can't launch anything without crashing. The move_group.launch.py results in this:
I get pretty much the same error when launching |
I can confirm what Henning is seeing, I went down a bit, and the crashes seems to cause from this bit. Although the launch file don't use controllers |
How would you run anything without controller settings? |
These are the issue I had while testing:
|
moveit_setup_assistant/moveit_setup_srdf_plugins/moveit_setup_framework_plugins.xml
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_simulation/moveit_setup_framework_plugins.xml
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_framework/moveit_setup_framework_plugins.xml
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_core_plugins/moveit_setup_framework_plugins.xml
Outdated
Show resolved
Hide resolved
moveit_setup_assistant/moveit_setup_controllers/moveit_setup_framework_plugins.xml
Outdated
Show resolved
Hide resolved
2d18fd7
to
7dc3305
Compare
This pull request is in conflict. Could you fix it @DLu? |
ae8e5e9
to
db8d79d
Compare
This pull request is in conflict. Could you fix it @DLu? |
3545fcc
to
db1f154
Compare
This pull request is in conflict. Could you fix it @DLu? |
91e6f17
to
5fabe99
Compare
moveit_setup_assistant/moveit_setup_srdf_plugins/src/compute_default_collisions.cpp
Outdated
Show resolved
Hide resolved
This pull request is in conflict. Could you fix it @DLu? |
640af1e
to
1557db8
Compare
This pull request is in conflict. Could you fix it @DLu? |
1557db8
to
bd49e18
Compare
I'm sick of rebasing this. Can we merge now @henningkayser @JafarAbdi @tylerjw |
This pull request is in conflict. Could you fix it @DLu? |
There is really nothing missing from my end and I'd like to get it merged as soon as possible. I've still not been able to get the generated launch files to work, and I've sunk several hours this week into it and I'm trill consulting with @vatanaksoytezer trying to figure out what's wrong. Call me nit-picky, but without me being able to launch a simple MoveGroup, I can't approve :/ |
I can't fix what I don't know about. Please make a ticket with diagnostic info, like any error messages you're seeing (and your moveit_config folder). |
One note for anyone switching between branches - I had to delete install/moveit_setup* and build/moveit_setup* to get this to build (otherwise I got an error since there is no top-level CMakeLists.txt in the moveit_setup_assistant folder anymore) |
Some testing notes on my end: was able to load my robot URDF, generate a package, and run the demo: I'm sure there are still some bugs in here, but I will note this has gotten me farther in MoveIt2 than any of my attempts in the past two years - so I think we should merge this and then fix bugs as they arise. The only bug I encountered (but, again shouldn't block merging since this probably works for 95% of use cases right now): is that adding a planning group by "links" or "joints" seems to be broken? If I add by "links", the links themselves show up as joints, along with a series of blank joints. If I add by joints, those same blank joints show up. Adding by "chain", which is probably the most common workflow, works fine. |
Another update: I've managed to make this run on the real robot (simply launching the move_group.launch.py and then running moveit_rviz.launch.py). In the process I found a recent regression in the RVIZ plugins - PR opened here: #1384 I've basically got the full setup I had in ROS1 working, minus perception (which I haven't attempted to test yet - but also the README indicates that might not be functional yet due to moveit_config_utils) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used this to generate a config and get my robot on MoveIt2 - let's merge it
### Notes | ||
* `PerceptionWidget` is ported, except `moveit_configs_utils` needs to be modified to load `sensors_3d.yaml`. This may require adjustments to the format of `sensors_3d.yaml` | ||
* Possible bug in `PlanningGroupsWidget`: Creating a new group does not properly create a new JointModelGroup | ||
* There are additional templates that have not been ported in the `unported_templates` folder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this once #1387 is merged
@henningkayser Please find some time to upload your generated MoveIt config |
@DLu were you gonna fix conflicts and merge this? I think we can deal with any issues @henningkayser is running into in other PRs - this PR is a HUGE step up from what is in the mainline codebase right now and should work for 95% of use cases (as opposed to unported stuff that works for 0%) |
I would second this. Provided fixup PRs have a short turn-around time of course. Many users are basically stuck without the MSA. MoveIt is too complex to not have a wizard guide them when creating new config packages (even if those config packages have been reduced in complexity (at least in the nr of files)). |
I am all too happy to merge it, but I lack the permissions to do so. I can't even push to this branch on my own, which is why @vatanaksoytezer is graciously cherry-picking commits and resolving conflicts for me. |
This, I did not know. |
🎉 🎉 🎉 🎉 🎉 |
Hooray!! Nice work @DLu |
I am so glad to see this merged in! Sorry @DLu this took so long, and thank you for your efforts. The MSA has a special place in my Willow Garage intern heart. Shout out to @JafarAbdi @vatanaksoytezer @henningkayser for the many sub-PR reviews to get here. Let's now button up any remaining issues :-) |
Description
MSA!
Checklist