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

Allow RobotState::setFromIK to work with subframes (backport #3077) #3085

Merged
merged 5 commits into from
Nov 16, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Nov 13, 2024

Description

Fixes #3072

  • Adds passing regression / sanity tests which verify that a RobotState can setFromIK with a collision object
  • Adds failing tests demonstrating that RobotState cannot setFromIK with a subframe
  • Fixes failing tests by modifying RobotState::setFromIK to consider subframes
  • Adds some helper functions to the pilz tests for attaching objects / subframes and checking joint values

I tried adding the tests directly to the robot state tests, however, I was getting issues with no kinematic solver being associated with the planning group of the OneRobot fixture.

Pilz already had a test which was checking setFromIK, and appears to be setup with a kinematic solver, so I've added the tests there.

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 (no GUI changes)
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

This is an automatic backport of pull request #3077 done by [Mergify](https://mergify.com).

* Adds regression tests for setFromIK with objects. Adds failing tests demonstrating failure with subframes

* Modifies RobotState::setFromIK to account for subframes

* Fixes formatting

* Fixes formatting

* Fixes formatting

* Applies PR suggestions

* Applies PR comments

---------

Co-authored-by: Tom Noble <tom@rivelinrobotics.com>
Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
(cherry picked from commit ab34495)

# Conflicts:
#	moveit_core/robot_state/src/robot_state.cpp
@mergify mergify bot added the conflicts label Nov 13, 2024
Copy link
Author

mergify bot commented Nov 13, 2024

Cherry-pick of ab34495 has failed:

On branch mergify/bp/humble/pr-3077
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit ab34495d2.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   moveit_planners/pilz_industrial_motion_planner/test/unit_tests/src/unittest_trajectory_functions.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   moveit_core/robot_state/src/robot_state.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@sea-bass
Copy link
Contributor

@rr-tom-noble sadly this one has conflicts too -- would you like to take a look? Thanks!

Co-authored-by: Tom Noble <tom@rivelinrobotics.com>
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Seems to work!

Though @rr-tom-noble you said you were running into other issues? Is this just related to mixing source/binary installs but should be good pending that resolution?

@rr-tom-noble
Copy link
Contributor

rr-tom-noble commented Nov 15, 2024

@sea-bass Given the tests, I'm confident that attaching objects & subframes through the RobotState now work with setFromIK.

Our in-practice use case a bit more complicated and involves using planning scene services to load and attach objects. It could be related to our mix of and binary and source, but my gut feel is that getFrameInfo() is working, and it's more related to the difference in how the objects are attached.

Given that issue arises from a somewhat different use-case, I think I'm happy with this PR as is, though I'm happy to leave it up to your discretion. I'll be looking into the next issue shortly.

@sea-bass sea-bass enabled auto-merge (squash) November 15, 2024 18:55
@TSNoble
Copy link
Contributor

TSNoble commented Nov 16, 2024

@sea-bass I believe the failure is unrelated. I've seen it sporadically on a few other PRs

@sea-bass sea-bass disabled auto-merge November 16, 2024 15:42
@sea-bass sea-bass merged commit 8c5cc4a into humble Nov 16, 2024
6 of 7 checks passed
@sea-bass sea-bass deleted the mergify/bp/humble/pr-3077 branch November 16, 2024 16:20
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