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.

Sign up for GitHub

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

Jump to bottom

add get_active_joint_names #2533

Merged
merged 2 commits into from
Mar 1, 2021
Merged

add get_active_joint_names #2533

merged 2 commits into from
Mar 1, 2021

Conversation

PeterMitrano
Copy link
Contributor

@PeterMitrano PeterMitrano commented Mar 1, 2021 •
edited

Description

Creates new API in C++ and Python for getting active joint names, including the case where no group is specified.

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

@codecov
Copy link

codecov bot commented Mar 1, 2021 •
edited

Codecov Report

Merging #2533 (078eb82) into master (34b2356) will increase coverage by 0.02%.
The diff coverage is 33.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2533      +/-   ##
==========================================
+ Coverage   60.24%   60.25%   +0.02%     
==========================================
  Files         351      351              
  Lines       26473    26483      +10     
==========================================
+ Hits        15945    15954       +9     
- Misses      10528    10529       +1
Impacted Files Coverage Δ
...bot_model/include/moveit/robot_model/robot_model.h 84.22% <ø> (ø)
...obot_interface/src/wrap_python_robot_interface.cpp 26.38% <25.00%> (-0.21%) ⬇️
moveit_core/robot_model/src/robot_model.cpp 79.30% <100.00%> (+0.04%) ⬆️
...ipulation/pick_place/src/manipulation_pipeline.cpp 72.35% <0.00%> (-1.06%) ⬇️
...meterization/work_space/pose_model_state_space.cpp 85.30% <0.00%> (+0.74%) ⬆️
.../ompl_interface/src/detail/constrained_sampler.cpp 60.00% <0.00%> (+17.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34b2356...e26cddb. Read the comment docs.

felixvd
felixvd reviewed Mar 1, 2021
View reviewed changes
Copy link
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

Didn't test, but LGTM. My only concern is that this introduces a string copy and vector in joint_model.cpp, although I doubt that it will have a noticeable effect.

moveit_commander/src/moveit_commander/robot.py Outdated
(mimic joints and fixed joints are excluded). If no group name is
specified, all joints in the robot model are returned, including
fixed and mimic joints.
If no group name is specified, all joints in the robot model are returned
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If no group name is specified, all joints in the robot model are returned
If no group name is specified, all joints in the robot model are returned.

Missing punctuation. Same for the line above and the other doc string. I would also keep the "(including mimic joints and fixed joints)".

moveit_core/robot_model/src/robot_model.cpp Show resolved Hide resolved
moveit_core/robot_model/include/moveit/robot_model/robot_model.h
@@ -163,6 +163,12 @@ class RobotModel
return active_joint_model_vector_const_;
}

/** \brief Get the array of active joint names, in the order they appear in the robot state. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** \brief Get the array of active joint names, in the order they appear in the robot state. */
/** \brief Get the array of active joint names, in the order that they appear in the robot state. */

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is wrong in the rest of the file, but let's not perpetuate mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd leave it as is and literature backs me up :-)

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 also think it's fine as is

@felixvd
Copy link
Contributor

felixvd commented Mar 1, 2021

Fixes #2505 (please give your PRs a description)

felixvd
felixvd approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

I'm good with this after punctuation is added. Sadly I can't push to this branch.

@PeterMitrano
Copy link
Contributor Author

Ok should be good now, thanks everyone!

@v4hn v4hn merged commit f6fd34b into moveit:master Mar 1, 2021
@tylerjw tylerjw mentioned this pull request Apr 9, 2021
Closed
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request Apr 29, 2021
@PeterMitrano @tylerjw
add get_active_joint_names (moveit#2533)
3b57500 
add get_active_joint_names and getActiveJointModelNames to Python and C++, respectively.

To keep calling costs constant we add the additional vector<string> to the RobotModel class.

Co-authored-by: Peter Mitrano <pmitrano@armstorm>
@tylerjw tylerjw mentioned this pull request Apr 29, 2021
Merged
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request May 3, 2021
@PeterMitrano @tylerjw
add get_active_joint_names (moveit#2533)
e56084a 
add get_active_joint_names and getActiveJointModelNames to Python and C++, respectively.

To keep calling costs constant we add the additional vector<string> to the RobotModel class.

Co-authored-by: Peter Mitrano <pmitrano@armstorm>
tylerjw pushed a commit that referenced this pull request May 3, 2021
@PeterMitrano @tylerjw
add get_active_joint_names (#2533)
5781a59 
add get_active_joint_names and getActiveJointModelNames to Python and C++, respectively.

To keep calling costs constant we add the additional vector<string> to the RobotModel class.

Co-authored-by: Peter Mitrano <pmitrano@armstorm>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v4hn v4hn v4hn approved these changes

@felixvd felixvd felixvd approved these changes

@rhaschke rhaschke Awaiting requested review from rhaschke rhaschke is a code owner

@tylerjw tylerjw Awaiting requested review from tylerjw tylerjw is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@PeterMitrano @felixvd @v4hn