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

Fix remove_attached_object API in Python PSI, add clear() #2609

Merged
merged 3 commits into from Apr 18, 2021

Conversation

felixvd
Copy link
Contributor

@felixvd felixvd commented Apr 16, 2021

Setting the link name in remove_attached_object is not necessary. planning_scene.cpp treats the message as described.

I also added a clear() method because I could have used it today and saw that @mikeferguson also did in moveit_python.

@felixvd felixvd requested a review from rhaschke as a code owner April 16, 2021 08:29
def clear(self):
""" Remove all objects from the planning scene """
self.remove_attached_object()
self.remove_world_object()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a useful abstraction, but it would be nicer to expose this API as sending a single PlanningScene message.
It's just stupid overhead that can even show in Rviz (the attached objects might actually detach in RViz before being removed).
It looks like the asynchronous backend of the class does not even have a publisher to planning_scene at the moment...

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Looks good to me. CI failed for seemingly unrelated reasons, I restarted the job.

Please consider my inline comment though.

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #2609 (c5505b4) into master (3a61f18) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2609      +/-   ##
==========================================
- Coverage   60.49%   60.48%   -0.01%     
==========================================
  Files         402      402              
  Lines       29611    29611              
==========================================
- Hits        17911    17908       -3     
- Misses      11700    11703       +3     
Impacted Files Coverage Δ
.../ompl_interface/src/detail/constrained_sampler.cpp 43.25% <0.00%> (-16.21%) ⬇️
...meterization/work_space/pose_model_state_space.cpp 82.06% <0.00%> (-1.92%) ⬇️
...raint_samplers/src/default_constraint_samplers.cpp 83.89% <0.00%> (-0.32%) ⬇️
...e/collision_detection_fcl/src/collision_common.cpp 78.61% <0.00%> (+0.26%) ⬆️
...nning_scene_monitor/src/planning_scene_monitor.cpp 68.68% <0.00%> (+0.55%) ⬆️
...ipulation/pick_place/src/manipulation_pipeline.cpp 73.79% <0.00%> (+1.95%) ⬆️

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 3a61f18...c5505b4. Read the comment docs.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I can only support Michael's review. +1

@felixvd
Copy link
Contributor Author

felixvd commented Apr 18, 2021

Thanks for the reviews. I agree that there are things to be improved in the backend (a CLEAR_ALL command, a DETACH command separate from REMOVE...), but I prefer not to go that far down the rabbit hole right now.

@felixvd felixvd merged commit 58e1302 into moveit:master Apr 18, 2021
@felixvd felixvd deleted the fix-python-psi branch April 23, 2021 05:32
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request Apr 29, 2021
@tylerjw tylerjw mentioned this pull request Apr 29, 2021
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request Apr 29, 2021
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request May 3, 2021
tylerjw pushed a commit that referenced this pull request May 3, 2021
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