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

Port #3464 from MoveIt1 #2456

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Port #3464 from MoveIt1 #2456

merged 4 commits into from
Oct 18, 2023

Conversation

rhaschke
Copy link
Contributor

moveit/moveit#3464 fixed an issue in the numbering of added path indices.
This PR ports this fix to MoveIt2. As the added_path_index vector was moved into the MotionPlanResponse struct in #2285, I had to choose a slightly different approach.
The first commit ports the (failing) unit test.
The second commit fixes the bug (and the test).

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (c0c6baf) 50.46% compared to head (bc3893e) 50.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2456      +/-   ##
==========================================
+ Coverage   50.46%   50.81%   +0.35%     
==========================================
  Files         385      391       +6     
  Lines       31831    32125     +294     
==========================================
+ Hits        16061    16321     +260     
- Misses      15770    15804      +34     
Files Coverage Δ
...g_request_adapter/src/planning_request_adapter.cpp 78.85% <100.00%> (ø)

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hugal31 and others added 3 commits October 17, 2023 07:13
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks for porting this over! Just a small comment not blocking comment

#include <moveit/utils/robot_model_test_utils.h>
#include <moveit/planning_request_adapter/planning_request_adapter.h>

class AppendingPlanningRequestAdapter final : public planning_request_adapter::PlanningRequestAdapter
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding brief documentation for the classes and the test in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have time to do this. Maybe, @Hugal31 can do so?
On the other hand, these classes are rather trivial and the names already state their semantics quite well.

@sjahr sjahr enabled auto-merge (squash) October 18, 2023 13:17
@sjahr sjahr merged commit 9249e44 into moveit:main Oct 18, 2023
10 of 11 checks passed
@rhaschke rhaschke deleted the fix-add-path-index branch October 18, 2023 17:54
m-elwin pushed a commit to m-elwin/moveit2 that referenced this pull request Dec 4, 2023
* Port unit test

cherry-pick of moveit/moveit#3464

* Increment added_path_index in callAdapter

Doesn't work because previous=0 for all recursively called functions.

* Pass individual add_path_index vectors to callAdapter

---------

Co-authored-by: Hugal31 <hla@lescompanions.com>
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