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

Parse xacro args from .setup_assistant config in MoveIt Configs Builder #2172

Merged
merged 6 commits into from
May 15, 2023

Conversation

abake48
Copy link
Contributor

@abake48 abake48 commented May 12, 2023

Description

Update MoveIt Config Builder to parse xacro_args parameter from MSA generated MoveIt 2 configs that use xacro generated robot descriptions.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@abake48 abake48 requested a review from MarqRazz May 12, 2023 00:17
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (226f81c) 50.49% compared to head (8be6e23) 50.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2172      +/-   ##
==========================================
- Coverage   50.49%   50.47%   -0.02%     
==========================================
  Files         387      387              
  Lines       31790    31790              
==========================================
- Hits        16050    16043       -7     
- Misses      15740    15747       +7     

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JafarAbdi
Copy link
Member

Servo tests are flaky, I just retriggered CI

@abake48
Copy link
Contributor Author

abake48 commented May 12, 2023

Servo tests are flaky, I just retriggered CI

Gotcha, thanks!

if (mappings is None) or all(
(isinstance(key, str) and isinstance(value, str))
for key, value in mappings.items()
):
try:
self.__moveit_configs.robot_description = {
self.__robot_description: load_xacro(
robot_description_file_path, mappings=mappings
robot_description_file_path,
mappings=mappings or self.__urdf_xacro_args,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this or when you assigned mappings on L226 (does 226 need to be removed like Jafar suggested)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that line needs to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assignment should be removed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I cut it out of there but I must have accidentally undo'd that change. Thank you @JafarAbdi for cleaning that up for me

Copy link
Contributor

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

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

I was not able to test these changes but they look good to me.

@JafarAbdi JafarAbdi merged commit 79a2fa5 into moveit:main May 15, 2023
@abake48 abake48 deleted the pr-config-builder-xacro-args branch May 15, 2023 17:50
sjahr pushed a commit that referenced this pull request Apr 3, 2024
* Parse xacro args from .setup_assistant config in MoveIt Configs Builder (#2172)

Co-authored-by: Jafar <jafar.uruc@gmail.com>
(cherry picked from commit 79a2fa5)

* Fix xacro args loading issue (#2684)

* Fixed xacro args loading issue

* Formatting fixes with pre-commit action

---------
(cherry picked from commit cdb20ae)

---------

Co-authored-by: Anthony Baker <abake48@users.noreply.github.com>
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