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

Short-circuit planning adapters #1694

Merged
merged 9 commits into from
Nov 9, 2022

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Nov 4, 2022

This is a simpler implementation of #1605, which is forward-ported from moveit/moveit#3262.
In contrast to #1605 we can keep the existing boolean return types as the error code can be propagated in res.error_code_.
There is no need to additionally use the return value to propagate an error code. By the way, eventually the propagated return value wasn't used at all, but simply cast back to a boolean:
https://github.com/ros-planning/moveit2/blob/abbbb9d906d21cb7fe2d3ee7c2a2a31799b9d0aa/moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp#L270-L271

As this PR first reverts #1605, the diff is primarily cluttered by this revert. For review, one ideally compares directly to the original main branch: 749c5ae...rhaschke:moveit2:short-circuit-planning-adapters

@rhaschke rhaschke requested a review from AndyZe November 4, 2022 22:04
@rhaschke rhaschke marked this pull request as draft November 4, 2022 22:21
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 51.03% // Head: 50.98% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (8efb971) compared to base (72e29b4).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1694      +/-   ##
==========================================
- Coverage   51.03%   50.98%   -0.04%     
==========================================
  Files         378      378              
  Lines       31649    31649              
==========================================
- Hits        16148    16134      -14     
- Misses      15501    15515      +14     
Impacted Files Coverage Δ
moveit_ros/moveit_servo/src/pose_tracking.cpp 76.78% <0.00%> (-1.89%) ⬇️
moveit_ros/moveit_servo/src/servo_calcs.cpp 66.45% <0.00%> (-1.49%) ⬇️
...e/collision_detection_fcl/src/collision_common.cpp 73.76% <0.00%> (-0.22%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rhaschke rhaschke marked this pull request as ready for review November 4, 2022 23:03
- Move default code to moveit_core/utils
- Override defaults in existing getActionResultString()
- Provide translations for all error codes defined in moveit_msgs
@rhaschke rhaschke force-pushed the short-circuit-planning-adapters branch from 658451e to 44221bd Compare November 4, 2022 23:03
@AndyZe
Copy link
Member

AndyZe commented Nov 8, 2022

I'm fine with the general idea of returning bool. We went back and forth in the original PR between bool, void, and MoveItErrorCode. I think void or bool is the best option so it's not redundant with res. Testing this now.

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Generally looks good, I verified functionality

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

This actually looks simpler. I somewhat like the idea of returning the bool-convertible error code more (which could work just the same way), but either way is fine to me.

@rhaschke
Copy link
Contributor Author

rhaschke commented Nov 8, 2022

This actually looks simpler. I somewhat like the idea of returning the bool-convertible error code more (which could work just the same way).

It is simpler and it is removing redundancy (returning the error code by return value and in response).

rhaschke and others added 3 commits November 8, 2022 20:29
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
@AndyZe AndyZe merged commit 8660258 into moveit:main Nov 9, 2022
@rhaschke rhaschke deleted the short-circuit-planning-adapters branch November 9, 2022 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants