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
minor RobotTrajectory patches #2751
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The chain setter support is cool, but I've seen it nowhere else in the codebase.
- I can't comment on the
make_shared
, but the rest LGTM.
On Wed, Jul 07, 2021 at 03:11:44AM -0700, Felix von Drigalski wrote:
1. The chain setter support is cool, but I've seen it nowhere else in the codebase.
Yes, we could use it in many other places I agree :-)
2. I can't comment on the `make_shared`, but the rest LGTM.
Aside from the fact that it doesn't build right now. ;)
I'll look into it.
- trajectory->addSuffixWayPoint(robot_state_, duration_from_previous);
+ trajectory->addSuffixWayPoint(*robot_state_, duration_from_previous);
Why did this API change, even though the function itself did not seem to?
There are two interfaces in the API, one taking a pointer and one taking an actual RobotState.
The latter will *copy* the robot state, but the former will only add the pointer.
I wanted to have a more proper trajectory for these tests where not all the waypoints refer to the same robot state.
|
Support calls like: ``` RobotTrajectory()->append(trajectory, 0.0) ->addSuffixWayPoint(state,0.1) ->addSuffixWayPoint(state2,0.1) ```
5ddc006
to
179e82e
Compare
179e82e
to
9c26aff
Compare
9c26aff
to
7ac6c6b
Compare
Codecov Report
@@ Coverage Diff @@
## master #2751 +/- ##
==========================================
+ Coverage 60.47% 60.54% +0.08%
==========================================
Files 402 402
Lines 29659 29680 +21
==========================================
+ Hits 17932 17968 +36
+ Misses 11727 11712 -15
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @v4hn, please decide for yourself how you want to merge this (squash vs. merge commit).
RobotTrajectory(const moveit::core::RobotModelConstPtr& robot_model, const std::string& group); | ||
|
||
/** @brief construct a trajectory for the JointModelGroup | ||
* If group is nullptr this is equivalent to the first constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, late to the party, could you please explain (in another PR) in the comments what it actually means for a RobotTrajectory
to have a group set (or what happens when you use the default constructor)
👍 for starting with the comments!
Please look at the individual commits for details.