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

Add planEndEffectorTwist #545

Closed
wants to merge 16 commits into from
Closed

Add planEndEffectorTwist #545

wants to merge 16 commits into from

Conversation

nansongyi
Copy link
Member

@nansongyi nansongyi commented Aug 27, 2019 •
edited by aditya-vk
Loading

[Describe this pull request. Link to relevant GitHub issues, if any.]

[Explain how this pull request was tested.]

Resolves #465.

Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@brianhou brianhou mentioned this pull request Aug 31, 2019
Closed
5 tasks
brianhou
brianhou previously requested changes Sep 4, 2019
View reviewed changes
src/planner/vectorfield/VectorFieldPlanner.cpp Outdated Show resolved Hide resolved
brianhou
brianhou reviewed Oct 8, 2019
View reviewed changes
Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

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

@aditya-vk can you review when you get a chance?

include/aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp Outdated
#ifndef AIKIDO_PLANNER_VECTORFIELD_MOVEENDEFFECTORTWISTVECTORFIELD_HPP_
#define AIKIDO_PLANNER_VECTORFIELD_MOVEENDEFFECTORTWISTVECTORFIELD_HPP_

#include <aikido/planner/vectorfield/BodyNodePoseVectorField.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <aikido/planner/vectorfield/BodyNodePoseVectorField.hpp>
#include "aikido/planner/vectorfield/BodyNodePoseVectorField.hpp"

include/aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp Outdated
#define AIKIDO_PLANNER_VECTORFIELD_MOVEENDEFFECTORTWISTVECTORFIELD_HPP_

#include <aikido/planner/vectorfield/BodyNodePoseVectorField.hpp>
#include <aikido/trajectory/Interpolated.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <aikido/trajectory/Interpolated.hpp>
#include "aikido/trajectory/Interpolated.hpp"

src/planner/vectorfield/MoveEndEffectorTwistVectorField.cpp Outdated
#include <dart/math/MathTypes.hpp>
#include <dart/optimizer/Function.hpp>
#include <dart/optimizer/Problem.hpp>
#include <aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp>
#include "aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp"

src/planner/vectorfield/MoveEndEffectorTwistVectorField.cpp Outdated
#include <dart/optimizer/Function.hpp>
#include <dart/optimizer/Problem.hpp>
#include <aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp>
#include <aikido/planner/vectorfield/VectorFieldUtil.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <aikido/planner/vectorfield/VectorFieldUtil.hpp>
#include "aikido/planner/vectorfield/VectorFieldUtil.hpp"

@brianhou brianhou changed the title new PR for planEndEffectorTwist Add planEndEffectorTwist Oct 8, 2019
@egordon egordon added this to the Aikido 0.3.0 milestone Oct 30, 2019
@egordon egordon dismissed brianhou’s stale review October 30, 2019 00:08

Changes already addressed.

aditya-vk
aditya-vk suggested changes Nov 3, 2019
View reviewed changes
Copy link
Contributor

@aditya-vk aditya-vk 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.

Thanks for taking this up! A little bit of work is left to close this.

The PR description needs to be updated. Some of the formatting changes seem to have nothing to do with this PR. If this PR is behind master, I suggest merging master in first. Looks like the PR is not complete, especially the core logic is not "complete" with appropriate error/deviation handling. The other VF planners are a good place to see how deviation is handled and when to report failure. I would also strongly suggest either having tests to prove correctness or implement this on the robot and attach a video to the PR.
Note: I un-ticked (a) tests written for the PR and (b) added documentation for classes introduced since that was not the case.

include/aikido/robot/util.hpp
/// \param[in] collisionTestable Collision constraint to check. Self-collision
/// is checked by default.
/// \param[in] timelimit Timelimit for the plan to be generated
/// \param[in] positionTolerance Tolerance in position // do I need this?
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the units?

Copy link
Contributor

Choose a reason for hiding this comment

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

"do I need this?"

include/aikido/robot/util.hpp
/// is checked by default.
/// \param[in] timelimit Timelimit for the plan to be generated
/// \param[in] positionTolerance Tolerance in position // do I need this?
/// \param[in] angularTolerance Tolerance in angle // do I need this?
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the units?

Copy link
Contributor

Choose a reason for hiding this comment

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

"do I need this?"

include/aikido/robot/util.hpp
/// \param[in] metaSkeleton Metaskeleton to plan with
/// \param[in] body Bodynode for the end-effector
/// \param[in] twistSeq Twist for the end-effector
/// \param[in] durationSeq Time to plan with the desired twist
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reasoning behind naming twistSeq and durationSeq? Why not twist and timeout?

include/aikido/robot/util.hpp
/// \param[in] durationSeq Time to plan with the desired twist
/// \param[in] collisionTestable Collision constraint to check. Self-collision
/// is checked by default.
/// \param[in] timelimit Timelimit for the plan to be generated
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this different from the docstring for durationSeq? Both seem to suggest planning timeout

include/aikido/robot/util.hpp
const Eigen::Vector6d& twistSeq,
double durationSeq,
const constraint::TestablePtr& collisionTestable,
double timelimit,
Copy link
Contributor

Choose a reason for hiding this comment

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

Take this as const

include/aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp
/// Tolerance of linear deviation error.
double mPositionTolerance;

/// Tolerance of angular error.
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the units?

include/aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp
/// Do we need this at this point?
aikido::trajectory::InterpolatedPtr mTwistTraj;

/// Tolerance of linear deviation error.
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the units?

include/aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp
::dart::dynamics::MetaSkeletonPtr metaskeleton,
::dart::dynamics::ConstBodyNodePtr bn,
const Eigen::Vector6d& twistSeq,
double durationSeq,
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable can be renamed better

include/aikido/planner/vectorfield/MoveEndEffectorTwistVectorField.hpp
aikido::statespace::dart::ConstMetaSkeletonStateSpacePtr stateSpace,
::dart::dynamics::MetaSkeletonPtr metaskeleton,
::dart::dynamics::ConstBodyNodePtr bn,
const Eigen::Vector6d& twistSeq,
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable can be renamed better

src/planner/vectorfield/MoveEndEffectorTwistVectorField.cpp
mCount += 1;
DART_UNUSED(pose);

// (avk): Not computing the error/deviation for now
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm? Can we address these TODOs? They are important.

@aditya-vk aditya-vk modified the milestones: Aikido 0.3.0, Aikido 0.4.0 May 10, 2020
@brianhou brianhou modified the milestones: Aikido 0.4.0, Aikido 0.5.0 Aug 27, 2020
@nansongyi nansongyi closed this Nov 10, 2020
@nansongyi nansongyi deleted the squirrel_pr branch November 10, 2020 15:26
@brianhou
Copy link
Contributor

@nansongyi Why did we close this PR? My impression was that it was fairly close to being done. I'm ok with closing this if it's just in preparation for reapplying those changes on top of the formatting changes in the current master branch or something, but I don't want to waste your efforts!

@nansongyi
Copy link
Member Author

oh I am sorry. I think I misunderstood something.
When Ethan closed this pr, libada#32, I thought it's the one for aikido, so I ended up with ignoring it cuz I thought it's already closed.
Today, when I clean up my workspace, I deleted this branch squirrel_pr cuz I thought it's already closed.
I will talk to Ethan whether we still need it or not, then I will get back to you.
Thanks.

@brianhou brianhou restored the squirrel_pr branch November 16, 2020 19:13
@brianhou brianhou deleted the squirrel_pr branch November 16, 2020 19:13
@brianhou brianhou restored the squirrel_pr branch November 16, 2020 19:13
@brianhou brianhou deleted the squirrel_pr branch November 16, 2020 19:13
@brianhou brianhou restored the squirrel_pr branch November 16, 2020 19:13
@brianhou
Copy link
Contributor

Reopening for now, just to make sure we don't forget/lose this 🙂

@brianhou brianhou reopened this Nov 16, 2020
@egordon
Copy link
Contributor

egordon commented Sep 17, 2021 •
edited
Loading

WONTFIX for now. From an application standpoint, planToEndEffector<Offset/Twist> only adds collision checking compared to the new JacobianVelocityController (adding in #605).

We can re-open if requested, added as an issue #611

@egordon egordon closed this Sep 17, 2021
This was referenced Sep 23, 2021
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianhou brianhou brianhou left review comments

@aditya-vk aditya-vk aditya-vk requested changes

@gilwoolee gilwoolee Awaiting requested review from gilwoolee

@egordon egordon Awaiting requested review from egordon egordon is a code owner

@yskim041 yskim041 Awaiting requested review from yskim041

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Aikido 0.5.0
Development

Successfully merging this pull request may close these issues.

Plan trajectories for end-effector twists
4 participants
@nansongyi @brianhou @egordon @aditya-vk