MSA: support loading xacros that use Jade+ extensions on Indigo #540
Conversation
This allows loading xacros that make use of the extensions to xacro that were added in Jade and newer ROS releases on Indigo.
|
Exact text used on the checkbox is obviously something we can discuss. Also: I was unsure whether we would want to add the checkbox to the 'load existing package' UI as well, as technically it's impossible to generate a configuration that needs the extensions without enabling them in the first place -- unless that configuration was modified manually. If we want to support that, I can add the checkbox to that part of the UI as well. |
There was a problem hiding this comment.
@gavanderhoorn what is your use-case for this?
would you say it is useful to have the checkbox for externally modified setup?
| @@ -258,6 +258,7 @@ bool MoveItConfigData::outputSetupAssistantFile(const std::string& file_path) | |||
| emitter << YAML::Value << YAML::BeginMap; | |||
| emitter << YAML::Key << "package" << YAML::Value << urdf_pkg_name_; | |||
| emitter << YAML::Key << "relative_path" << YAML::Value << urdf_pkg_relative_path_; | |||
| emitter << YAML::Key << "use_jade_xacro" << YAML::Value << urdf_requires_jade_xacro_; | |||
There was a problem hiding this comment.
does it make sense to add this only if it is set to true?
There was a problem hiding this comment.
That occured to me as well.
But I couldn't really make up my mind as I don't see any obvious problems with including it even if it is false. Only thing I could come up with was it might 'invite' users to change it to true after the package is generated, expecting the rest of the infrastructure to automagically cope with Jade+ xacros as well - which is obviously not going to happen.
But users aren't supposed to edit .setup_assistant by hand afaik, and something with 'security' and 'obscurity'.
tl;dr: I don't really mind either way, so if anyone has a strong preference for not serialising in case the property is false, I can change it.
There was a problem hiding this comment.
Well, if you include it on false,
you end up with useless config lines (that stay even after the setups move to kinetic+).
I vote for writing it only on true.
|
@v4hn wrote:
with the PR you mean? I have a nr of installations still running Indigo, but do use Jade+ xacro on that. See #511. I'd like those MSAs to be able to load those xacros. See also ros-industrial/fanuc#217.
If you were asking whether I could think of a use-case for having the checkbox on the 'load existing package' UI as well: I can think of one, but I'm not sure whether we want to support that. It would basically be the case where an existing package is modified to make use of some Jade+-features including xacro and now has to be updated. Having the checkbox there would make it possible to do that through the UI. Without it, users would have to edit the I don't mind editing a text file, and if you're this deep into things anyway, it shouldn't be that much effort. |
way to true. I'm fine with leaving it out of the load-existing menu. |
|
I added a tooltip that gives some info on why and when users would want to use this. I'll wait for Travis to finish and then merge. |
|
Thanks for the reviews @v4hn and @davetcoleman. |
* Refactor out velocity limit enforcement with test * Update moveit_ros/moveit_servo/test/enforce_limits_tests.cpp Co-authored-by: AdamPettinger <adam.pettinger@utexas.edu> Co-authored-by: AdamPettinger <adam.pettinger@utexas.edu>
This adds a checkbox to the Setup Assistant urdf loading widget on the start screen that allows users to enable 'Jade+ extensions' in xacro on Indigo installations. This is needed to support loading xacros that make use of those extensions.
The choice is persisted (
.setup_assistant) so loading a config pkg that was created with the option enabled will enable them automatically. Bw-compatibility with existing config pkgs is maintained, as absence of the setting will just make the MSA use the default (false).Extensions are also enabled in
planning_context.launchif needed.Screenshot of UI change:
Ref #511.
As xacro on Jade and newer ROS releases enables those extensions by default, these changes do not need to be cherry-picked to any other MoveIt versions.