Skip to content

Add demo package for tutorial - #104

Merged
rhaschke merged 18 commits into
moveit:masterfrom
PickNikRobotics:demo_for_tutorial
Oct 8, 2019
Merged

Add demo package for tutorial#104
rhaschke merged 18 commits into
moveit:masterfrom
PickNikRobotics:demo_for_tutorial

Conversation

@simonGoldstein

@simonGoldstein simonGoldstein commented Jul 22, 2019

Copy link
Copy Markdown
Contributor

Moveit Task constructor needs a tutorial. This adds the package moveit_task_constructor_demo which contains a short pick and place example. The goal is to add a new tutorial to the tutorials. This should be added to the moveit_task_constructor repo to keep it consistent with other demos like moveit_grasps.

roslaunch moveit_task_constructor_demo demo.launch

@simonGoldstein
simonGoldstein force-pushed the demo_for_tutorial branch 2 times, most recently from b0531f5 to 4d485fe Compare July 29, 2019 23:23
@simonGoldstein simonGoldstein changed the title [WIP] Add demo package for tutorial Add demo package for tutorial Aug 1, 2019
Comment thread demo/.clang-format
Comment thread demo/CMakeLists.txt Outdated
Comment thread demo/README.md Outdated
Comment thread demo/src/pick_place_task.cpp Outdated
Comment thread demo/src/moveit_task_constructor_demo.cpp Outdated
Comment thread demo/src/pick_place_task.cpp Outdated
Comment thread demo/src/pick_place_task.cpp Outdated
Comment thread demo/src/pick_place_task.cpp Outdated
Comment thread demo/config/panda_config.yaml Outdated
Comment thread demo/src/pick_place_task.cpp Outdated
@simonGoldstein

simonGoldstein commented Aug 5, 2019

Copy link
Copy Markdown
Contributor Author

I am working on addressing all the feedback. I have currently added the children frames and adjusted some parameters but I still need to rename and set better default parameter values

const std::string object = "object";

// Reset ROS introspection before constructing the new object
// TODO(henningkayser): verify this is a bug, fix if possible

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a known problem and we hit it before.
A patch is very welcome, if you have a good idea on how to improve this without too much overhead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not aware of this problem. What is the problem, i.e. what happens, if introspection is not reset?

@v4hn v4hn Aug 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We talked about this before @rhaschke .
If you reset a shared-pointer to a task with a new Task you create in the same line,
the new Task (and its introspection) are set up before the destructor of the old one was called.
This becomes a problem because only one ROS service can be active with the same name in the same node and the new one just does not get set up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this clarification. One option to solve the issue in MTC is to use a global cache of those ROS services (and reuse existing ones).
File an issue: #106.

@v4hn v4hn Aug 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, but then we add static state to the Task and I believe we have avoided it thus far.
Not counting your planner cache which I still plan to remove again.

An alternative would be lazy (or just late) initialization for the ROS interfaces.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't consider a cache as static state. Formally, of course, every cache is.
Late initialization will be brittle on timing. Lazy might work, but requires much more work.
Currently, the existence of the introspection pointer also indicates that introspection is desired...

@rhaschke rhaschke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The demo looks good. Thanks a lot for your effort.
However, I suggest to move this code into moveit_tutorials. Why do you want to separate the example code from the rest of the tutorial?
This would also allow to use sub frames, because moveit_tutorials might depend on MoveIt master.

Comment thread demo/src/moveit_task_constructor_demo.cpp Outdated
Comment thread demo/src/pick_place_task.cpp
const std::string object = "object";

// Reset ROS introspection before constructing the new object
// TODO(henningkayser): verify this is a bug, fix if possible

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not aware of this problem. What is the problem, i.e. what happens, if introspection is not reset?

@v4hn

v4hn commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

However, I suggest to move this code into moveit_tutorials. Why do you want to separate the example code from the rest of the tutorial?

If we move it there, we also create a dependency from the tutorials to MTC.
So do you effectively propose to release MTC as-is and maintain the external tutorial when API changes? (which also calls for regular releases)

I would just keep it in this repository for now, as proposed by @simonGoldstein

@rhaschke

Copy link
Copy Markdown
Contributor

If we move it there, we also create a dependency from the tutorials to MTC.
So do you effectively propose to release MTC as-is and maintain the external tutorial when API changes?

This dependency is natural to me: If we do have an MTC tutorial there, there should be a dependency to MTC. We proceed in the same fashion for all other tutorials: If there are changes in MoveIt, we need a separate PR to adapt the tutorials correspondingly.
I remember that we discussed in the past to shift tutorials into the core MoveIt repo as well, but I don't like this idea "just to facilitate" tutorial modifications.

@v4hn

v4hn commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

This dependency is natural to me: If we do have an MTC tutorial there, there should be a dependency to MTC.

Yes and No.
I do see your point, but I am afraid we'd end up with turning moveit_tutorials into a demos package, that includes dependencies to every possible piece of software you can integrate with MoveIt.
This would make it rather useless for newcomers who want to build some tutorials from source.
This was probably also the reasoning to keep the moveit_grasp tutorial separate.

Still, having the actual tutorial (the text description, not the source package) in the main moveit_tutorials repository allows to collect many tutorials in one place, even if we don't build them together there.
After all we do not have our own tutorials website (and I don't think it is needed at this point).

@rhaschke

Copy link
Copy Markdown
Contributor

Still, having the actual tutorial in the main repo allows to collect many tutorials in one place.

I don't get what you are suggesting. Which tutorial and repo you are referring to? MTC or MoveIt?

@v4hn

v4hn commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

I tried to disambiguate my previous comment.

rhaschke and others added 3 commits September 10, 2019 18:10
* Simplify parameters using rosparam_shortcuts
* Remove example with picknik_ur5_moveit_config
* Remove "hacky" object poses
* Remove unused parameters
@henningkayser

Copy link
Copy Markdown
Member

@v4hn, @rhaschke I just cleaned this up (parameters + remove picknik UR5 example), rebased onto master and ran clang-format. Can we make a decision if we merge this here or not? ;) My opinion is that full demos like this should go inside the package and not inside the tutorial, because they're readily available for a quick runtime test. BTW, I'm already working on fixing-up the tutorial.

@v4hn

v4hn commented Sep 11, 2019

Copy link
Copy Markdown
Contributor

I just cleaned this up

CI fails though.

Can we make a decision if we merge this here or not?

In my opinion the package (and thus the code dependency) can stay in this repository.
The actual tutorial should still be part of MoveIt's tutorials collection.

@henningkayser

Copy link
Copy Markdown
Member

CI fails though.

The remaining errors occur because of changes in the current MoveIt master branch.

@rhaschke

Copy link
Copy Markdown
Contributor

Unfortunately, I was not able to push my cleanup changes here. For this reason, I file them at PickNikRobotics#3. @henningkayser, please have a look and merge them. With those changes, I finally approve.

- fix dependencies
  - moveit_visual_tools is not actually used
  - panda_moveit_config is required in a recent version
- use more specific lib/binary names
@henningkayser

Copy link
Copy Markdown
Member

@rhaschke I merged your changes

@rhaschke
rhaschke merged commit 36e561a into moveit:master Oct 8, 2019
rhaschke added a commit that referenced this pull request Oct 8, 2019
Co-Authored-By: Henning Kayser <henningkayser@picknik.ai>
Co-Authored-By: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
Comment thread demo/package.xml
<depend>moveit_ros_planning_interface</depend>
<depend>moveit_core</depend>
<depend>rosparam_shortcuts</depend>
<exec_depend version_gte="0.7.4">panda_moveit_config</exec_depend>

@progtologist progtologist Oct 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this version actually available anywhere or is this a typo?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, it's not a typo. But the next release of the package is still pending.
The MTC tutorial requires the latest melodic-devel branch, particularly moveit/panda_moveit_config#34. So, if you want to try the tutorial, just use the latest source branch.

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.

5 participants