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

kinematics tests #1272

Merged
merged 13 commits into from
Jan 23, 2019
Merged

kinematics tests #1272

merged 13 commits into from
Jan 23, 2019

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Dec 16, 2018 •
edited
Loading

This is another attempt for #1147, addressing #4. Similarly to moveit/moveit_kinematics_tests#7, I tried to separate the tests from robot-specific code: The tests as designed by @jrgnicho are robot- and ik-plugin-agnostic and can be run (in an appropriate environment) for any robot and plugin.
Additionally to the initial feasibility tests, I added some real validation to FK tests: RobotState's FK and KinematicsPlugin's one should coincide. A failure there pointed me to #1271.
All original tests are random feasibility tests that turned out to be flaky, which is why I disabled them.

Instead of adding additional robots to moveit_resources, I'm running those tests on Fanuc + Panda.
I'm planning to add a ikfast test for these robots as well.

@rhaschke
Copy link
Contributor Author

Note, that the unit test fails due to #1271.

@rhaschke rhaschke force-pushed the kinematics-tests branch 2 times, most recently from 17b9ca0 to 23526f0 Compare December 19, 2018 20:33
@jrgnicho
Copy link
Contributor

@rhaschke thanks for reviving this PR, do you plan on making further changes or can it now be reviewed?

@rhaschke
Copy link
Contributor Author

Do you plan on making further changes or can it now be reviewed?

I'm still working on it. Changed the title accordingly.

@rhaschke rhaschke changed the title kinematics tests WIP: kinematics tests Dec 27, 2018
@rhaschke rhaschke force-pushed the kinematics-tests branch 10 times, most recently from f91b05a to 36d6a99 Compare December 31, 2018 12:15
@rhaschke rhaschke changed the title WIP: kinematics tests kinematics tests Dec 31, 2018
@rhaschke
Copy link
Contributor Author

@jrgnicho This is ready for reviewing now.

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 2, 2019

@jrgnicho: The test getNearestIKSolution checks that getPositionIK() returns the single closest solution to the seed, compared to the multiple-solution method getPositionIK(). However, as I pointed out in #1285, the semantics of these methods is not yet well defined.

@rhaschke rhaschke mentioned this pull request Jan 3, 2019
Open
@rhaschke rhaschke force-pushed the kinematics-tests branch 3 times, most recently from 61abe24 to bfabb75 Compare January 3, 2019 18:53
@rhaschke
speedup + cleanup
dcfaed9 
- read parameters only once and share them between all tests
- reduce code duplication, introducing expectNear()
- remove timing (done by gtest as well)
- simplify getNearestIKSolution()
@rhaschke
validate getFK
bb5ad82
@rhaschke
randomWalkIK
7cc9eb2
@rhaschke rhaschke mentioned this pull request Jan 14, 2019
Merged
@rhaschke
Copy link
Contributor Author

@jrgnicho @davetcoleman I addressed your comments. Please have another look.

@rhaschke rhaschke mentioned this pull request Jan 14, 2019
Closed
@jrgnicho
Copy link
Contributor

Other than the linking error it looks good to me.

@jrgnicho
Copy link
Contributor

The linking issue seems to be caused in the linking step to libmoveit_test_utils

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 21, 2019 •
edited
Loading

The linking issue has the following underlying reason:

  • libmoveit_test_utils uses gtest functions and thus requires GTEST_LIBRARIES.
  • Since 2525a54 I do explicitly link against GTEST_LIBRARIES. But the gtest lib is only locally available within the current package, i.e. moveit_core.
  • Any binary, here moveit_occupancy_map_server, usually links against catkin_LIBRARIES, which also includes libmoveit_test_utils if that's required for testing.
  • While g++ doesn't actually link libraries that are not needed by the executable, clang attempts to link each and every library listed, which is a waste of resources and causes the issue, because GTEST_LIBRARIES are not found from another package than moveit_core.

I searched for a while how to convince clang to link as lazy as gcc does. No solution so far.
I'm afraid we cannot move the testing functions into libmoveit_test_utils, but we need to revert those changes.

@rhaschke
Copy link
Contributor Author

I reverted the last changes (moving some testing functions to libmoveit_test_utils.

@rhaschke rhaschke merged commit bed7cc2 into moveit:melodic-devel Jan 23, 2019
rhaschke added a commit that referenced this pull request Jan 23, 2019
@rhaschke
Merge pull requests #1272 (kinematics tests) and #1294 (kdl cleanup)
aa42e1e
@rhaschke rhaschke deleted the kinematics-tests branch January 23, 2019 17:17
davetcoleman
davetcoleman reviewed Jan 23, 2019
View reviewed changes
Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

You never officially got an approval for this PR. I understand we were taking longer than you'd prefer to review, but notably I was busy in China representing MoveIt!. Please ping us more next time rather than breaking our review policy

moveit_kinematics/test/test_kinematics_plugin.cpp

public:
testing::AssertionResult isNear(const char* expr1, const char* expr2, const char* abs_error_expr,
const geometry_msgs::Point& val1, const geometry_msgs::Point& val2, double abs_error)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for trying this.

Why can't we install gtest? Testing is important to software and shouldn't be held back due to the desire to reduce dependencies

@rhaschke
Copy link
Contributor Author

I considered Jorge's comment in #1272 (comment) as an approval and I pinged you in #1272 (comment).
Sorry, but I wanted to progress with some pending feature branches that are built on top of this.

@rhaschke
Copy link
Contributor Author

Why can't we install gtest?

Because it's only for testing. This shouldn't be installed. Actually, it's Google's philosophy that each and every software package builds gtest on its own from source. If it would be installed, you could even trigger conflicts with other source packages.

@davetcoleman davetcoleman mentioned this pull request Jan 24, 2019
Open
@rhaschke rhaschke mentioned this pull request Jan 31, 2019
Merged
@davetcoleman davetcoleman mentioned this pull request Mar 5, 2019
Closed
@rhaschke rhaschke mentioned this pull request Mar 29, 2019
Closed
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
[Hybrid Planning] Improve action cancellation (moveit#1272)
7427a8e 
* Small changes to make the action server more performant

* Move the threads to member variables

* Add a simple cancellation demo

* Do not use unique_ptr

* Lint

* Launch file cleanup

* When a new HP goal is received, give the previous some time to cancel

* A unique callback group for each action server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonschmeisser simonschmeisser simonschmeisser approved these changes

@davetcoleman davetcoleman davetcoleman left review comments

@jrgnicho jrgnicho jrgnicho requested changes

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
4 participants
@rhaschke @jrgnicho @simonschmeisser @davetcoleman