Add `copy()` and `deepCopy()` methods to RobotTrajectory class. #1760

shivaang12 · 2019-11-21T00:21:07Z

Description

This PR points to issue #1660. I have deprecated the "normal" copy constructors and added two new methods, RobotTrajectory::copy() and RobotTrajectory::deepCopy(). Also documented these methods.

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
While waiting for someone to review your request, please help review another open pull request to support the maintainers

welcome · 2019-11-21T00:21:09Z

Thanks for helping in improving MoveIt and open source robotics!

davetcoleman

Would a unit test be relevant here?

davetcoleman · 2019-11-21T01:22:10Z

moveit_core/robot_trajectory/src/robot_trajectory.cpp

+
+void RobotTrajectory::deepCopy(const RobotTrajectory& robot_traj)
+{
+  this->robot_model_ = robot_traj.robot_model_;


Is this really a deep copy of the RobotModel?

Thanks for the review! No, this isn't a deep copy. But I observer that constructor is receiving a shared ptr so I thought I should assigned it to a same shared pointer. But now I get the picture, value should be same not the memory! Sorry, I am not much familiar with this part of the package.

i believe we actually want to keep the same RobotModel in the copied trajectory.

the "deep" is supposed to imply that RobotStates are copied, not the whole underlying moveit class structure.

other proposals for this name are welcome.

i believe we actually want to keep the same RobotModel in the copied trajectory.

I agree

the "deep" is supposed to imply that RobotStates are copied, not the whole underlying moveit class structure.

other proposals for this name are welcome.

I think "deep" could be a bit misleading. I would prefer something more explicit, maybe copyWithRobotStates().

Perhaps RobotTrajectory::copy(const RobotTrajectory& other, bool copy_robot_states = false) would be more explicit. This is the approach used in robot_state/conversions.h for copy_attached_bodies.

moveit_core/robot_trajectory/src/robot_trajectory.cpp

shivaang12 · 2019-11-21T02:13:19Z

@davetcoleman I noticed there wasn't any unit test dir for the robot_trajectory package. Is it okay if I create it?

v4hn · 2019-11-21T08:36:07Z

moveit_core/robot_trajectory/src/robot_trajectory.cpp

+
+void RobotTrajectory::deepCopy(const RobotTrajectory& robot_traj)
+{
+  this->robot_model_ = robot_traj.robot_model_;


i believe we actually want to keep the same RobotModel in the copied trajectory.

the "deep" is supposed to imply that RobotStates are copied, not the whole underlying moveit class structure.

other proposals for this name are welcome.

moveit_core/robot_trajectory/include/moveit/robot_trajectory/robot_trajectory.h

rhaschke · 2019-11-21T09:02:19Z

Sorry for the late comment, but I think we could just implement the copy constructor as a deepcopy operation on the waypoints. Is there any use case for a shallow copy, i.e. keeping the waypoints but modifying the durations only? @v4hn: What do you think?

v4hn · 2019-11-21T12:35:49Z

This is the usual "should we change semantics without changing interfaces" question. There's likely *someone* out there who exploited the shallow copy. In this case I somewhat prefer the implemented approach (that I proposed), but I don't have a strong opinion and it is indeed a rather drastic step to eventually remove it as it breaks, e.g., STL containers... Either way I would like to keep the proposed copy methods around.

rhaschke · 2019-11-21T12:43:43Z

This is the usual "should we change semantics without changing interfaces" question.

I understand this argument in general, but it is rather unlikely in this case that somebody made use of this. In my point of view, it's a simple bug fix to return a deep copy of the waypoints by the copy constructor.

Either way I would like to keep the proposed copy methods around.

IMHO, this just clutters the API in this case.

v4hn · 2019-11-21T12:54:13Z

In my point of view, it's a simple bug fix to return a deep copy of the waypoints by the copy constructor. ... IMHO, this just clutters the API in this case.

We'll need a third opinion then, as I do not agree with you (nor do I strongly disagree though). @henningkayser :-)

davetcoleman · 2019-11-21T21:25:52Z

I noticed there wasn't any unit test dir for the robot_trajectory package. Is it okay if I create it?

Its always ok to add more tests! :-)

We'll need a third opinion then, as I do not agree with you (nor do I strongly disagree though).

I lean towards @rhaschke 's thought that its a simple bug fix that makes the API more correct, and that should not be backported to Melodic but should be in MIGRATION.md

But I also want to hear @henningkayser 's opinion

shivaang12 · 2019-11-25T14:37:46Z

I am curious to know if we have a decision here?

henningkayser · 2019-11-25T18:26:14Z

IMHO, this just clutters the API in this case.

Can't we just change the default copy constructor to shallow copy (here copy()) and just provide one alternative for specifically copying the robot states? I think it wouldn't clutter the API if the added functions don't appear redundant and a single added copy function doesn't seem too much to me.

v4hn · 2019-11-25T21:02:13Z

Can't we just change the default copy constructor to shallow copy (here `copy()`) and just provide one alternative for specifically copying the robot states?

If I understand your comment right, it's the exact opposite of what Robert and Dave proposed. :-) They argue `copy()` is not needed at all (I'm unsure about that) and `copyWithRobotStates` should be the semantics of the copy constructor.

henningkayser · 2019-11-26T11:16:53Z

Can't we just change the default copy constructor to shallow copy (here copy()) and just provide one alternative for specifically copying the robot states?
If I understand your comment right, it's the exact opposite of what Robert and Dave proposed. :-)

Well, you asked for my opinion ;)

They argue copy() is not needed at all (I'm unsure about that)

I think shallow copy is probably the most needed of them all.
If I copy a trajectory I do this in order to modify the waypoint sequence, i.e. by adding or removing robot states. Deep copy is only needed if the actual waypoints are modified. Running different time parametrization algorithms (or similar processing methods) is the only valid use case I see here. In this case the semantics should be clear that you need a deep copy, but I wouldn't expect it as a default.

and copyWithRobotStates should be the semantics of the copy constructor.

This is arguable, I would not expect that. It's fine if properly documented.

rhaschke · 2019-11-26T12:22:58Z

I think shallow copy is probably the most needed of them all.
If I copy a trajectory I do this in order to modify the waypoint sequence, i.e. by adding or removing robot states.

Good point. Adding or removing states doesn't require a deep copy.
So, we keep the shallow copy behavior in the (default) copy constructor and we add a new deepCopy() method to achieve deep copy. Naming it copyWithRobotStates is somewhat too verbose in my opinion.

v4hn · 2019-12-01T12:12:39Z

Good point. Adding or removing states doesn't require a deep copy.
So, we keep the shallow copy behavior in the (default) copy constructor and we add a new deepCopy() method to achieve deep copy. Naming it copyWithRobotStates is somewhat too verbose in my opinion.

The problem with this solution is that users will probably still make the same silly mistake and perform a shallow copy, using the copy constructor - I did so two years ago and a colleague of mine did some months ago.
I believe nobody will read through the API first to discover the deepCopy method and infer that copy construction does something different.
This is the original pitfall I wanted to remove from the API with this patch...

shivaang12 · 2019-12-04T16:43:36Z

I have revived the copy constructor which calls method copy(). Let me know if everyone is fine with it.

- Assign the deprecated attribute to copy constructor which forwards the call to the member method called copy().

codecov-io · 2019-12-16T16:43:42Z

Codecov Report

Merging #1760 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1760      +/-   ##
==========================================
+ Coverage   48.36%   48.44%   +0.07%     
==========================================
  Files         297      297              
  Lines       23261    23278      +17     
==========================================
+ Hits        11250    11276      +26     
+ Misses      12011    12002       -9

Impacted Files	Coverage Δ
...include/moveit/robot_trajectory/robot_trajectory.h	`95.65% <ø> (ø)`	⬆️
...eit_core/robot_trajectory/src/robot_trajectory.cpp	`41.81% <100%> (+3.75%)`	⬆️
...e/collision_detection_fcl/src/collision_common.cpp	`73.53% <0%> (+0.3%)`	⬆️
...bot_state/include/moveit/robot_state/robot_state.h	`90.9% <0%> (+0.69%)`	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	`78.73% <0%> (+1.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76c16e6...0241d77. Read the comment docs.

moveit_core/robot_trajectory/include/moveit/robot_trajectory/robot_trajectory.h

moveit_core/robot_trajectory/src/robot_trajectory.cpp

mlautman

See proposed changes in #1813

- keep assigmentment operator (performing a shallow copy) - add a deepcopy flag to the copy constructor

rhaschke · 2019-12-19T08:16:14Z

Additionally to @mlautman's fixups I restored the assignment operator (performing a shallow copy),
and implemented the copy operation(s) as a copy constructor with an additional deepcopy parameter that is false by default, thus maintaining the old behavior.

welcome · 2019-12-19T18:49:44Z

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

Fixup for 01ff035 (moveit#1760)

Fixup for 01ff035 (#1760)

shivaang12 added 2 commits November 20, 2019 16:08

Add copy and deepCopy methods to RobotTrajectory class.

46131de

Add doxygen documentation to copy and deepCopy methods.

9cef76e

shivaang12 requested a review from mlautman as a code owner November 21, 2019 00:21

davetcoleman reviewed Nov 21, 2019

View reviewed changes

v4hn reviewed Nov 21, 2019

View reviewed changes

rhaschke force-pushed the master branch from 68982c0 to 11c82ec Compare November 24, 2019 14:20

shivaang12 requested a review from v4hn December 4, 2019 16:43

Revive copy constructor.

7053676

- Assign the deprecated attribute to copy constructor which forwards the call to the member method called copy().

shivaang12 force-pushed the copy-deepcopy-robot-traj branch from b319def to 7053676 Compare December 4, 2019 18:55

Address clang-tidy issues.

fe40231

shivaang12 force-pushed the copy-deepcopy-robot-traj branch from 4a8da29 to fe40231 Compare December 16, 2019 16:04

mlautman reviewed Dec 19, 2019

View reviewed changes

mlautman mentioned this pull request Dec 19, 2019

RobotTrajectory copy() method #1813

Closed

mlautman reviewed Dec 19, 2019

View reviewed changes

robot_trajectory tests and optional arg to copy

f5a8b4b

cleanup

448a2f3

- keep assigmentment operator (performing a shallow copy) - add a deepcopy flag to the copy constructor

shivaang12 requested a review from rhaschke as a code owner December 19, 2019 08:04

mlautman approved these changes Dec 19, 2019

View reviewed changes

remove unimplemented copy

0241d77

rhaschke approved these changes Dec 19, 2019

View reviewed changes

rhaschke merged commit 01ff035 into moveit:master Dec 19, 2019

rhaschke mentioned this pull request Dec 19, 2019

robot_trajectory::RobotTrajectory::copy and ::deepcopy #1660

Closed

beiju mentioned this pull request Jan 14, 2020

Since #1760, RobotTrajectory's (shallow) copy constructor deletes all waypoints #1833

Closed

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Jan 15, 2020

Fix RobotTrajectory's copy constructor

8ba71f2

Fixup for 01ff035 (moveit#1760)

rhaschke mentioned this pull request Jan 15, 2020

Fix RobotTrajectory's copy constructor #1834

Merged

rhaschke added a commit that referenced this pull request Jan 15, 2020

Fix RobotTrajectory's copy constructor (#1834)

a2b7b02

Fixup for 01ff035 (#1760)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `copy()` and `deepCopy()` methods to RobotTrajectory class. #1760

Add `copy()` and `deepCopy()` methods to RobotTrajectory class. #1760

shivaang12 commented Nov 21, 2019 •

edited

Loading

welcome bot commented Nov 21, 2019

davetcoleman left a comment

davetcoleman Nov 21, 2019

shivaang12 Nov 21, 2019

v4hn Nov 21, 2019

henningkayser Nov 25, 2019

mlautman Dec 18, 2019 •

edited

Loading

shivaang12 commented Nov 21, 2019 •

edited

Loading

v4hn Nov 21, 2019

rhaschke commented Nov 21, 2019

v4hn commented Nov 21, 2019 via email

rhaschke commented Nov 21, 2019

v4hn commented Nov 21, 2019 via email

davetcoleman commented Nov 21, 2019

shivaang12 commented Nov 25, 2019

henningkayser commented Nov 25, 2019

v4hn commented Nov 25, 2019 via email

henningkayser commented Nov 26, 2019 •

edited

Loading

rhaschke commented Nov 26, 2019

v4hn commented Dec 1, 2019

shivaang12 commented Dec 4, 2019

codecov-io commented Dec 16, 2019 •

edited

Loading

mlautman left a comment

rhaschke commented Dec 19, 2019

welcome bot commented Dec 19, 2019

Add copy() and deepCopy() methods to RobotTrajectory class. #1760

Add copy() and deepCopy() methods to RobotTrajectory class. #1760

Conversation

shivaang12 commented Nov 21, 2019 • edited Loading

Description

Checklist

welcome bot commented Nov 21, 2019

davetcoleman left a comment

Choose a reason for hiding this comment

davetcoleman Nov 21, 2019

Choose a reason for hiding this comment

shivaang12 Nov 21, 2019

Choose a reason for hiding this comment

v4hn Nov 21, 2019

Choose a reason for hiding this comment

henningkayser Nov 25, 2019

Choose a reason for hiding this comment

mlautman Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

shivaang12 commented Nov 21, 2019 • edited Loading

v4hn Nov 21, 2019

Choose a reason for hiding this comment

rhaschke commented Nov 21, 2019

v4hn commented Nov 21, 2019 via email

rhaschke commented Nov 21, 2019

v4hn commented Nov 21, 2019 via email

davetcoleman commented Nov 21, 2019

shivaang12 commented Nov 25, 2019

henningkayser commented Nov 25, 2019

v4hn commented Nov 25, 2019 via email

henningkayser commented Nov 26, 2019 • edited Loading

rhaschke commented Nov 26, 2019

v4hn commented Dec 1, 2019

shivaang12 commented Dec 4, 2019

codecov-io commented Dec 16, 2019 • edited Loading

Codecov Report

mlautman left a comment

Choose a reason for hiding this comment

rhaschke commented Dec 19, 2019

welcome bot commented Dec 19, 2019

Add `copy()` and `deepCopy()` methods to RobotTrajectory class. #1760

Add `copy()` and `deepCopy()` methods to RobotTrajectory class. #1760

shivaang12 commented Nov 21, 2019 •

edited

Loading

mlautman Dec 18, 2019 •

edited

Loading

shivaang12 commented Nov 21, 2019 •

edited

Loading

henningkayser commented Nov 26, 2019 •

edited

Loading

codecov-io commented Dec 16, 2019 •

edited

Loading