search

moveit / moveit_task_constructor

A hierarchical multi-stage manipulation planner
https://moveit.github.io/moveit_task_constructor
BSD 3-Clause "New" or "Revised" License
183 stars 152 forks source link

Set independant start and end states in a Generator stage #546

Closed DaniGarciaLopez closed 8 months ago

DaniGarciaLopez commented 8 months ago

Releated to https://github.com/ros-planning/moveit_task_constructor/issues/543. This PR enables the possibility to set independent start and end states when calling the spawn method of a Generator stage, while maintaining the previous functionality that uses the same state.

I didn't implement this overload as it implies that the trajectory will be empty and it makes no sense to set different start and end states when the robot is not actually moving, right? Or am I missing any specific use case where this could be handy?

I noticed that connect uses const lvalue reference (const InterfaceState&) instead of rvalue (InterfaceState&&). Are there any design preferences?

Is there an easy way to backport this to humble branch or I should cherry-pick another PR?

DaniGarciaLopez commented 8 months ago

Thanks for the review @rhaschke, changes addressed!

rhaschke commented 8 months ago

I noticed that the remaining functions also use the argument names from and to. For this reason, I reverted the requested change. Sorry for the confusion.

As this change should become part of master branch too, could you please (squash)rebase onto and retarget to master? Thanks.

DaniGarciaLopez commented 8 months ago

I also personally believe that using start and end is more intuitive. However, to maintain consistency, IMHO sticking with from and to seems to be the clearer option.

Commits squashed!

DaniGarciaLopez commented 8 months ago

Not sure how to retarget to master with that many commits ahead. Can you retarget the PR or should I open a new one?

rhaschke commented 8 months ago

I retargeted the PR to master, but you need to rebase onto master. I don't have permissions to force-push your branch.

DaniGarciaLopez commented 8 months ago

Ok, thanks. Done!

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 58.82%. Comparing base (55c4b52) to head (faa8486).

Files Patch % Lines
core/src/stage.cpp 78.58% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #546 +/- ## ========================================== - Coverage 58.83% 58.82% -0.00% ========================================== Files 91 91 Lines 8617 8623 +6 ========================================== + Hits 5069 5072 +3 - Misses 3548 3551 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.