Remove plan_with_sensing

[stephanie-eng]

Description

Remove unused plan_with_sensing code as mentioned on #1038

[codecov]

Codecov Report

Merging #1142 (d3d9735) into main (54b1a98) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
- Coverage   61.56%   61.55%   -0.01%     
==========================================
  Files         274      274              
  Lines       24982    24982              
==========================================
- Hits        15377    15374       -3     
- Misses       9605     9608       +3     

Impacted Files	Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	75.36% <0.00%> (-1.07%)	⬇️

---

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 54b1a98...d3d9735. Read the comment docs.

[v4hn]

plan with sensing is an advanced feature to deal with noisy octomap data by pointing a mobile camera at potential collisions.
It's a prominent example in our code base that shows that advanced behavior can be done inside the MoveGroup interface with the normal pipeline interface instead of always relying on external control loops.

I absolutely agree that the ROS implementation is not ideal and reworking it would be great, but the policy to just throw away functionality because your non-academical robots (without movable camera) do not need them (for now) is a big minus from my side.
If you strip the codebase like this without providing replacements you are just removing years of developer effort from the repositories for literally no merit. And I believe PickNik advertises quite a lot with all the work that went into the code base over the years.

👎

[v4hn]

Please reconsider

[henningkayser]

plan with sensing is an advanced feature to deal with noisy octomap data by pointing a mobile camera at potential collisions. It's a prominent example in our code base that shows that advanced behavior can be done inside the MoveGroup interface with the normal pipeline interface instead of always relying on external control loops.

I absolutely agree that the ROS implementation is not ideal and reworking it would be great, but the policy to just throw away functionality because your non-academical robots (without movable camera) do not need them (for now) is a big minus from my side. If you strip the codebase like this without providing replacements you are just removing years of developer effort from the repositories for literally no merit. And I believe PickNik advertises quite a lot with all the work that went into the code base over the years.

-1

I think the feature provides a very narrow use case in a somewhat complicated way. You're right, it's an example of advanced behavior, but does it really need to be a default feature that we have to support with MoveIt? IMO, it could really use some re-implementation, but even with that, I think it would much better fit into a tutorial or examples package rather than the main code base. No-one is using this realistically, and just because many years of development effort have been spent on this feature (I doubt it), it doesn't mean that it has to stay in the main code base forever.

[JafarAbdi]

No-one is using this realistically, and just because many years of development effort have been spent on this feature (I doubt it), it doesn't mean that it has to stay in the main code base forever.

I have tried to use it once for a client and it didn't work at all, I also spent some time trying to debug and fix it until I gave up

[v4hn]

If you want to remove a feature because it is broken / badly implemented, then provide a more general (and thus more useful) framework that can implement the same functionality (e.g. behavior-tree controlled planning&execution?) instead: - plan trajectory - the generated plan is too close to collision bodies/the octomap - extract points of highest cost - direct a movable camera sensor at the location and wait for planning scene updates - try to plan again with the updated scene The whole logic is currently implemented to also integrate with the old PickPlace pipeline and thus can handle multi-trajectory manipulation plans. We do not yet have an alternative system that can do that anywhere else. I perfectly agree to remove unused *code* as described in most points of the meta-issue, but not *functionality* - even if possibly broken. I simply oppose removing features from the code-base without providing an alternative way to achieve the same thing. That's plain regression.

I have tried to use it once for a client and it didn't work at all, I also spent some time trying to debug and fix it until I gave up

If you believe it is broken add an issue for a tutorial/test that demonstrates it can work (and possibly fix it). Your description literally describes the state of most of the MoveIt codebase back in 2016 when we started to get things up and running again. And a lot more works again by now.

[AndyZe]

Another vote from me towards removing this functionality.

[mergify]

This pull request is in conflict. Could you fix it @stephanie-eng?

[AndyZe]

@v4hn have you given this any more thought yet? Doesn't it seem like the functionality here could be replicated with hybrid planning?

[v4hn]

have you given this any more thought yet? Doesn't it seem like the functionality here could be replicated with hybrid planning?

Sorry for not replying here for so long.

I do think that in theory this is possible to implement with the hybrid planner so go ahead and remove the old code here.
It's far from easy to implement it though and it would be really great to see the use-case as an example for (very) advanced use of the hybrid planner :-)

[vatanaksoytezer]

@stephanie-eng do you mind resolving the conflicts here when you have time so we can get this merged?