bind() them all (not) by v4hn · Pull Request #3106 · moveit/moveit

v4hn · 2022-03-31T22:56:46Z

Fixes #2766. Replaces #2972 .

@Abishalini will hate me for this changeset for its ease of porting to moveit2... 😢
But it was overdue, I managed to do it over a few evenings and I believe it makes the code much more readable.

This PR should get some priority because patches are quite likely to generate conflicts with this due to its size.
I split all non-trivial changes into separate commits with one huge commit in the end addressing all "trivial" (up to identifier naming) cases. During this work I found and reworked a number of internal details, often related to pointer-use where object lifetime was known or APIs that could be reworked with the flexibility of lambdas.

Eventually I added the clang-tidy check that Robert found. It's important to point out that the provided patch will usually work, but ends up with horrible overhead per parameter/capture and no useful identifiers. So users cannot just apply the patch. But then again, nobody will contribute new code that still uses bind() anyway... Is what you would hope.

@tylerjw @rhaschke

That was a long overdue todo.

codecov · 2022-04-01T00:29:10Z

Codecov Report

Merging #3106 (d879aab) into master (5398531) will decrease coverage by 0.06%.
The diff coverage is 57.63%.

❗ Current head d879aab differs from pull request most recent head 9e3d71b. Consider uploading reports for the commit 9e3d71b to get more accurate results

@@            Coverage Diff             @@
##           master    #3106      +/-   ##
==========================================
- Coverage   61.62%   61.57%   -0.05%     
==========================================
  Files         376      376              
  Lines       33275    33310      +35     
==========================================
+ Hits        20504    20508       +4     
- Misses      12771    12802      +31

Impacted Files	Coverage Δ
..._core/collision_detection/src/collision_matrix.cpp	`44.04% <0.00%> (ø)`
...sion_distance_field/collision_env_distance_field.h	`9.10% <ø> (ø)`
...e/moveit/ompl_interface/planning_context_manager.h	`100.00% <ø> (ø)`
.../ompl_interface/src/detail/constraints_library.cpp	`1.31% <0.00%> (-0.01%)`	⬇️
...z_industrial_motion_planner_testutils/async_test.h	`76.00% <ø> (ø)`
...ult_capabilities/kinematics_service_capability.cpp	`8.09% <0.00%> (ø)`
...ccupancy_map_monitor/src/occupancy_map_monitor.cpp	`28.75% <0.00%> (-0.88%)`	⬇️
...ion/include/moveit/plan_execution/plan_execution.h	`81.82% <ø> (ø)`
...anning_scene_monitor/src/current_state_monitor.cpp	`51.45% <0.00%> (ø)`
.../planning_scene_monitor/src/trajectory_monitor.cpp	`31.38% <0.00%> (ø)`
... and 46 more

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 5398531...9e3d71b. Read the comment docs.

rhaschke

Thanks a lot for all this cleanup work. I like all the changes.
However, in a few locations you also changed public API (changing pointer arguments to references to indicate that valid instances are expected.)
I'm fine with these changes as well, but I'm surprised to see them proposed from you. Don't you argue against public API changes usually?

I guess you already noticed that several tests are broken now.

rhaschke · 2022-04-01T08:27:05Z

moveit_core/planning_request_adapter/src/planning_request_adapter.cpp

-{
-// boost bind is not happy with overloading, so we add intermediate function objects
-
-bool callAdapter1(const PlanningRequestAdapter* adapter, const planning_interface::PlannerManagerPtr& planner,


Great cleanup!

moveit_kinematics/test/test_kinematics_plugin.cpp

rhaschke · 2022-04-01T08:55:35Z

moveit_core/planning_scene/test/test_multi_threaded.cpp

 const int TRIALS = 1000;
 const int THREADS = 2;

-bool runCollisionDetection(unsigned int /*id*/, unsigned int trials, const planning_scene::PlanningScene* scene,


Thanks for cleaning up this kind of pointer usage!

rhaschke · 2022-04-01T11:07:30Z

I shortly looked into the first failing test and it is very strange: The ModelBasedStateSpaceFactoryPtr returned from factory_selector:
https://github.com/ros-planning/moveit/blob/779b7c8b019f70898d4de3189f9261c9697d9b9f/moveit_planners/ompl/ompl_interface/src/planning_context_manager.cpp#L316
is NULL although the actually called function returns a valid pointer. Hence, somewhere in the hierarchy of calling this std::function, the return value is screwed! Looks like a gcc or libstdc++ bug.

the trailing 1 and 2 and bind() usage made this harder to parse than it has to be. Technically this breaks API (the methods are protected, not private). But if you are that far into the code base to inherit here and rely on the names of these simple methods, you probably agree that this improves readability.

v4hn · 2022-04-01T12:04:03Z

Thanks for the additional patches! I rebased for a clean history. Yes, I saw the failing tests already, but just finished the patch yesterday and wanted to push it already. I just started to look into it a bit further as well, but could not understand it right away.

I shortly looked into the first failing test and it is very strange: The ModelBasedStateSpaceFactoryPtr returned from factory_selector:

Messy. I see the same issue with clang 13 (did not get to upgrade to 14 yet), so I'm not sure it's a compiler issue. We probably need to find a workaround whether or not we can pinpoint an external bug.
That part of the API is insane anyway. Why pass a function to a method just to call it first thing and ignore it after that...

Speaking about API: All the header-specific changes I made address private (and in one case protected) methods. the mesh helper might be debatable (see my commit message), but I do not consider the rest to be public API.

rhaschke · 2022-04-01T12:21:25Z

I see the same issue with clang 13 (did not get to upgrade to 14 yet), so I'm not sure it's a compiler issue. We probably need to find a workaround whether or not we can pinpoint an external bug. That part of the API is insane anyway. Why pass a function to a method just to call it first thing and ignore it after that...

Yes, probably it is a bug in libstdc++. At some point in the call hierarchy the returned shared_ptr reference is copied (which is wrong in first place) and then released.

Speaking about API: All the header-specific changes I made address private (and in one case protected) methods. the mesh helper might be debatable (see my commit message), but I do not consider the rest to be public API.

Sorry, I was convinced to have seen a public declaration. Checking again, you are right, everything is private or protected.

rhaschke · 2022-04-01T12:52:33Z

That part of the API is insane anyway. Why pass a function to a method just to call it first thing and ignore it after that...

You are right, I didn't noticed this before. I suggested two approaches to fix the issue in v4hn#3 - both changing protected API of OMPL::PlanningContextManager, which is fine I think. Given your comment, I prefer the last one.

v4hn · 2022-04-01T13:05:25Z

Makes sense. I cherry-picked your proposed modification of the protected getPlanningContext method. 👍

rhaschke · 2022-04-01T13:17:01Z

Sorry, I forgot to remove the unused req argument at caller side... The PR was meant to be tested by GHA before merging ;-)

rhaschke · 2022-04-01T14:46:16Z

Closing and reopening to trigger CI...

rhaschke · 2022-04-01T17:10:36Z

I tried to reproduce the shared_ptr issue in a minimal example, but there it works: https://godbolt.org/z/T68jzMYW4.

rhaschke · 2022-04-02T13:17:46Z

@v4hn, I think this is ready to merge after squash-merging the fixups.

It made the code hard to understand in many places and was needed because lambdas did not exist when this was written. Times change.

to ensure people use lambdas. Look anywhere. That's what you should do.

... to improve readability

- Calling the factory_selector lambda causes the returned value to be NULL. There is no value in having this selector function. We could directly pass the correct factory. - MotionPlanRequest isn't actually used.

v4hn · 2022-04-02T21:01:36Z

I think this is ready to merge after squash-merging the fixups.

Thank you for spending the time to find my superfluous ampersand that broke the patch... 🏅
I integrated the fixes and will merge.

Adaption of moveit/moveit#3106

Fixup for moveit#3106

v4hn added 3 commits March 31, 2022 22:33

simplify distance field method binding

0322d63

drop unused arguments not needed for lambda binding

6805b7e

Migrate PRA internals to lambdas

6436597

That was a long overdue todo.

v4hn requested review from AndyZe, felixvd, gavanderhoorn, henningkayser, jrgnicho, jschleicher, mamoll, mikeferguson, mlautman, rhaschke and tylerjw as code owners March 31, 2022 22:56

v4hn force-pushed the pr-master-bind-them-all branch from 269f01e to edac397 Compare April 1, 2022 09:13

rhaschke approved these changes Apr 1, 2022

View reviewed changes

v4hn added 2 commits April 1, 2022 13:48

kinematics test: remove unused argument

ddb68b6

v4hn force-pushed the pr-master-bind-them-all branch from 04eea20 to 681bc2b Compare April 1, 2022 11:52

rhaschke closed this Apr 1, 2022

rhaschke reopened this Apr 1, 2022

v4hn and others added 6 commits April 2, 2022 22:48

various: prefer objects and references over pointers

1a8e571

banish bind()

a2911c8

It made the code hard to understand in many places and was needed because lambdas did not exist when this was written. Times change.

add clang-tidy check for bind()

7f71a6c

to ensure people use lambdas. Look anywhere. That's what you should do.

using namespace collision_detection

9804c19

... to improve readability

Cleanup OMPL's PlanningContextManager's protected API

c07be63

- Calling the factory_selector lambda causes the returned value to be NULL. There is no value in having this selector function. We could directly pass the correct factory. - MotionPlanRequest isn't actually used.

Fix clang-tidy

9e3d71b

v4hn force-pushed the pr-master-bind-them-all branch from d879aab to 9e3d71b Compare April 2, 2022 20:56

v4hn merged commit cce0ffe into moveit:master Apr 2, 2022

v4hn mentioned this pull request Apr 8, 2022

Never Use std::bind moveit/moveit2#872

Closed

peterdavidfagan mentioned this pull request Apr 30, 2022

Remove bind moveit/moveit2#1204

Merged

henningkayser added a commit to moveit/moveit2 that referenced this pull request May 10, 2022

Avoid bind(), use lambdas instead (#1204)

8a5357a

Adaption of moveit/moveit#3106

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Nov 22, 2022

ik_callback_fn: propagate return value

c0b8df5

Fixup for moveit#3106

rhaschke mentioned this pull request Nov 22, 2022

Fix ik_callback_fn: propagate return value #3277

Merged

Conversation

v4hn commented Mar 31, 2022

Uh oh!

codecov bot commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

rhaschke Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhaschke Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Apr 1, 2022

Uh oh!

v4hn commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhaschke commented Apr 1, 2022

Uh oh!

rhaschke commented Apr 1, 2022

Uh oh!

v4hn commented Apr 1, 2022

Uh oh!

rhaschke commented Apr 1, 2022

Uh oh!

rhaschke commented Apr 1, 2022

Uh oh!

rhaschke commented Apr 1, 2022

Uh oh!

rhaschke commented Apr 2, 2022

Uh oh!

v4hn commented Apr 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Apr 1, 2022 •

edited

Loading

v4hn commented Apr 1, 2022 •

edited

Loading