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

Allow RobotState::setFromIK to work with subframes #3077

Merged
merged 8 commits into from
Nov 13, 2024
Merged

Allow RobotState::setFromIK to work with subframes #3077

merged 8 commits into from
Nov 13, 2024

Conversation

TSNoble
Copy link
Contributor

@TSNoble TSNoble commented Nov 10, 2024 •
edited
Loading

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

@TSNoble TSNoble changed the title (WIP) Allow RobotState::setFromIK to work with subframes Allow RobotState::setFromIK to work with subframes Nov 10, 2024
@TSNoble TSNoble marked this pull request as ready for review November 10, 2024 15:21
This was referenced Nov 10, 2024
Closed
Closed
@TSNoble
Copy link
Contributor Author

TSNoble commented Nov 10, 2024

@sjahr I believe this is ready for review (just the usual tutorial image failures)

sea-bass
sea-bass reviewed Nov 10, 2024
View reviewed changes
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.

This is great! Especially appreciate the tests.

Mostly small comments from my side.

...lanners/pilz_industrial_motion_planner/test/unit_tests/src/unittest_trajectory_functions.cpp Show resolved Hide resolved
...lanners/pilz_industrial_motion_planner/test/unit_tests/src/unittest_trajectory_functions.cpp Show resolved Hide resolved
...lanners/pilz_industrial_motion_planner/test/unit_tests/src/unittest_trajectory_functions.cpp Show resolved Hide resolved
...lanners/pilz_industrial_motion_planner/test/unit_tests/src/unittest_trajectory_functions.cpp Outdated Show resolved Hide resolved
@rr-tom-noble
Copy link
Contributor

@sea-bass Should be ready for review again.

Additionally, I'm always a little unsure of what types of changes require extending the tutorials & api notes. In theory the interface hasn't changed (the tip parameter still accepts a string and is therefore backwards-compatible, but the semantics are a little different in that it now accepts subframe names)

@rr-tom-noble
Copy link
Contributor

Ah, also if this could be marked for backport that'd be much appreciated. I'll get on fixing those PRs ASAP

@sea-bass
Copy link
Contributor

sea-bass commented Nov 11, 2024 •
edited
Loading

@sea-bass Should be ready for review again.

Additionally, I'm always a little unsure of what types of changes require extending the tutorials & api notes. In theory the interface hasn't changed (the tip parameter still accepts a string and is therefore backwards-compatible, but the semantics are a little different in that it now accepts subframe names)

Thank you!

Your assessment is right; API wise, you're fine.

That said, i think subframes as a whole are not generally super well documented in the tutorials. So if you're a big user of this feature, you should in futire consider contributing a tutorial! That would be great.

sea-bass
sea-bass approved these changes Nov 11, 2024
View reviewed changes
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.

LGTM, will await a 2nd review from someone else as well.

@sea-bass sea-bass added backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron labels Nov 11, 2024
@rr-tom-noble
Copy link
Contributor

@sjahr Would you mind providing a second review when you have time?

@rr-tom-noble rr-tom-noble mentioned this pull request Nov 12, 2024
Closed
sjahr
sjahr approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

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

Looks good thanks!

@sjahr sjahr enabled auto-merge November 13, 2024 10:53
@sjahr sjahr added this pull request to the merge queue Nov 13, 2024
Merged via the queue into moveit:main with commit ab34495 Nov 13, 2024
11 checks passed
mergify bot pushed a commit that referenced this pull request Nov 13, 2024
@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 bot pushed a commit that referenced this pull request Nov 13, 2024
@TSNoble @mergify
Allow RobotState::setFromIK to work with subframes (#3077)
93195d6 
* 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 mentioned this pull request Nov 13, 2024
Merged
6 tasks
@mergify mergify bot mentioned this pull request Nov 13, 2024
Closed
6 tasks
@TSNoble TSNoble deleted the bugfix/set-state-from-ik-with-subframes branch November 13, 2024 17:15
This was referenced Nov 14, 2024
Closed
Merged
Merged
sea-bass added a commit that referenced this pull request Nov 16, 2024
@mergify @TSNoble
@rr-tom-noble @sea-bass
Allow RobotState::setFromIK to work with subframes (backport #3077) (#…
8c5cc4a 
…3085)

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

* 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

* Fixes merge conflicts (humble backport 3077) (#3087)

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

---------

Co-authored-by: Tom Noble <53005340+TSNoble@users.noreply.github.com>
Co-authored-by: Tom Noble <tom@rivelinrobotics.com>
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
@TSNoble TSNoble mentioned this pull request Dec 3, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rr-tom-noble rr-tom-noble rr-tom-noble left review comments

@sjahr sjahr sjahr approved these changes

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

Assignees
No one assigned
Labels
backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RobotState::setFromIK is subframe unaware
4 participants
@TSNoble @rr-tom-noble @sea-bass @sjahr