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

robot_trajectory::RobotTrajectory as standard container and tests #1162

Closed
wants to merge 2 commits into from

Conversation

bhomberg
Copy link
Contributor

Description

This is a WIP -- still working on completing the tests.

-Adds iterator to RobotTrajectory class.
-Adds .size() method to RobotTrajectory class.
-Adds basic tests for RobotTrajectory class.
-Adds (in progress) equality operator for RobotState class in order to complete RobotTrajectory tests.

Addresses #1121

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

@bhomberg bhomberg changed the title Bh traj container WIP robot_trajectory::RobotTrajectory as standard container and tests Oct 26, 2018
for (auto pair : traj)
{
// Compare waypoints
EXPECT_EQ(*pair.first, traj_robot_state);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you compare pointers instead of values here, so you don't need to implement an equality operator for class?

"<group name=\"base_with_bad_subgroups\">"
"<group name=\"error\"/>"
"</group>"
"</robot>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this URDF taken from another moveit file? Alternatively, you could use one of the URDFs from moveit_resources and load it from the file, to keep this test file concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@henningkayser
Copy link
Member

@bhomberg what's the current state of this PR? I agree with @jonbinney that you don't need to implement an equality operator just for the test. Since pointers won't work (copy-construct) just compare if distance is 0.0 or smaller than EPS.

@bhomberg
Copy link
Contributor Author

@henningkayser -- I haven't had time to look at this since world MoveIt day. I don't know that I'll have a chance to work on it for a while.

@v4hn v4hn changed the title WIP robot_trajectory::RobotTrajectory as standard container and tests robot_trajectory::RobotTrajectory as standard container and tests Aug 12, 2019
@v4hn v4hn added help wanted please consider improving this request! wip labels Aug 12, 2019
@davetcoleman
Copy link
Member

@bhomberg its the next WMD - think you could finish this PR up?

@bhomberg
Copy link
Contributor Author

@davetcoleman -- I only had time for a couple hours at WMD today, I don't think I'll get to this until maybe next year's WMD...

@davetcoleman
Copy link
Member

That's too bad - this looks like such an improvement of test coverage that is close to being ready for merging!


std::size_t generateTestTraj(robot_trajectory::RobotTrajectory& traj,
const moveit::core::RobotModelConstPtr& robot_model,
const robot_model::JointModelGroup* joint_model_group)
Copy link
Contributor

Choose a reason for hiding this comment

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

The last parameter is unused.

@davetcoleman
Copy link
Member

@bhomberg this PR is 3 years old, seems we should close it. Or could you finish it? Would be a shame to not add this test coverage!

@bhomberg
Copy link
Contributor Author

I don't have time to work on this right now, but I'll close + reopen if I have a chance to pick it back up!

@bhomberg bhomberg closed this Mar 10, 2021
DLu added a commit to DLu/moveit2 that referenced this pull request Oct 4, 2021
DLu added a commit to DLu/moveit2 that referenced this pull request Oct 29, 2021
henningkayser pushed a commit to moveit/moveit2 that referenced this pull request Nov 4, 2021
* Based on initial size/iterator implementations from moveit/moveit#1162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted please consider improving this request! wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants