Skip to content

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

Merged
sea-bass merged 5 commits intohumblefrom
mergify/bp/humble/pr-3077
Nov 16, 2024
Merged

Allow RobotState::setFromIK to work with subframes (backport #3077)#3085
sea-bass merged 5 commits intohumblefrom
mergify/bp/humble/pr-3077

Conversation

@mergify
Copy link
Copy Markdown

@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).

@TSNoble @mergify
Allow RobotState::setFromIK to work with subframes (#3077)
38299f6 
* 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
@mergify
Copy link
Copy Markdown
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
Copy Markdown
Contributor

sea-bass commented Nov 13, 2024

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

@TSNoble @riv-tnoble
Fixes merge conflicts (humble backport 3077) (#3087)
f07f427 
Co-authored-by: Tom Noble <tom@rivelinrobotics.com>
sea-bass
sea-bass approved these changes Nov 15, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@sea-bass sea-bass left a comment •
edited
Loading

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?

@riv-tnoble
Copy link
Copy Markdown
Contributor

riv-tnoble commented Nov 15, 2024 •
edited
Loading

@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
Copy Markdown
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
@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

Reviewers

1 more reviewer

@sea-bass sea-bass sea-bass approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sea-bass @riv-tnoble @TSNoble