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

Reverts #2985, Ports moveit #3388 #3470 #3539 #3284

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Reverts #2985, Ports moveit #3388 #3470 #3539 #3284

wants to merge 13 commits into from
+395 −246

Conversation

rr-mark
Copy link

@rr-mark rr-mark commented Jan 30, 2025 •
edited
Loading

Description

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

rr-mark and others added 5 commits January 30, 2025 13:49
@rr-mark
Revert "Fix RobotState::getRigidlyConnectedParentLinkModel() (moveit#…
439c619 
…2985)"

This reverts commit 1f23344.
@rhaschke @rr-mark
Merge PR #3388: Generalize RobotState::setFromIK()
c034fae 
So far, setFromIK only accepted target (link) frames that were rigidly connected to a solver's tip frame.
This, for example, excluded the fingertip of an actuated gripper, because that would be separated by an
active joint from the arm's tooltip. However, as long as this joint is not part of the JMG,
the corresponding transform can be considered as fixed as well.

This PR generalizes the functions getRigidlyConnectedParentLinkModel() in
RobotState and RobotModel to receive an optional JMG pointer.
If present, only (active) joints from that group are considered non-fixed.
This PR also enables subframe support for setFromIK - simply by using
getRigidlyConnectedParentLinkModel(), which already supported that.

There is one drawback of this approach: A repeated application of setFromIK
with the same target frame and JMG (as in computeCartesianPath()), will
repeat the search for the common fixed parent link.
Additionally, the passed RobotState needs to be up-to-date.
We could mitigate this by pulling the corresponding code into a separate
function and calling it once in computeCartesianPath().
@rhaschke @rr-mark
Merge PR #3470: Avoid global transforms in getRigidlyConnectedParentL…
b4d00ec 
…inkModel()

Fixes #3388
@v4hn @rr-mark
Merge pull request #3539 from v4hn/find-links-with-slashes-again
f6d5bff 
find links with slashes again
@rr-mark
Ports to ROS2 and fixes problems introduced in merge conflicts.
ce74986
JafarAbdi
JafarAbdi approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

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

Thanks!

@rr-mark rr-mark mentioned this pull request Jan 30, 2025
Closed
6 tasks
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2025 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.

Project coverage is 45.98%. Comparing base (0af5e0f) to head (819c000).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
moveit_core/robot_model/src/robot_model.cpp 75.00% 3 Missing ⚠️
...it_core/robot_state/src/cartesian_interpolator.cpp 0.00% 1 Missing ⚠️
moveit_core/robot_state/test/robot_state_test.cpp 94.12% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3284      +/-   ##
==========================================
+ Coverage   45.60%   45.98%   +0.38%     
==========================================
  Files         716      716              
  Lines       62380    62428      +48     
  Branches     7544     7550       +6     
==========================================
+ Hits        28445    28700     +255     
+ Misses      33769    33561     -208     
- Partials      166      167       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sjahr
sjahr reviewed Jan 30, 2025
View reviewed changes
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks for porting this over! I hate to ask but do you mind adding a bit documentation here? Moveit core is already complicated enough and these changes don't simplify it 😅

moveit_core/robot_model/include/moveit/robot_model/robot_model.hpp Outdated Show resolved Hide resolved
moveit_core/robot_model/include/moveit/robot_model/robot_model.hpp Outdated Show resolved Hide resolved
moveit_core/robot_model/src/robot_model.cpp Show resolved Hide resolved
moveit_core/robot_state/src/robot_state.cpp Show resolved Hide resolved
@rr-mark
Copy link
Author

rr-mark commented Jan 31, 2025

Thanks for porting this over! I hate to ask but do you mind adding a bit documentation here? Moveit core is already complicated enough and these changes don't simplify it 😅

Sure thing. None of this was originally my code, but I'll see what I can do later today.

@sjahr
Copy link
Contributor

sjahr commented Jan 31, 2025

Sure thing. None of this was originally my code, but I'll see what I can do later today.

Yes, I am aware of this. Thanks a lot for doing it and let me know when you need help!

@rr-mark
Copy link
Author

rr-mark commented Jan 31, 2025

It looks like the dubious transform arguments are only there for unit tests. I'm trying to work out how to calculate them separately, so they can be deleted.

@rr-mark
Copy link
Author

rr-mark commented Jan 31, 2025

Actually, scratch that. Any alternate code to calculate the transform would only be used in the tests. I think I'm just going to delete these arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjahr sjahr sjahr left review comments

@JafarAbdi JafarAbdi JafarAbdi approved these changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

RobotState::getRigidlyConnectedParentLinkModel and subframes: PRs not ported from moveit1.
6 participants
@rr-mark @codecov-commenter @sjahr @JafarAbdi @rhaschke @v4hn