collision world: check for empty shapes vector before access

[v4hn]

See #2025 (comment)

[felixvd]

LGTM. There is another unguarded check like this in moveShapeInObject, but I suppose users who want to move empty shapes deserve the error.

I also think that method can be deleted, but that's just me.

[felixvd]

Suggested change

	// we need a persisting isometry for the API
	// Return an identity transform so there is a persisting isometry object for the API

nit

[v4hn]

Don't like the suggestion because we return a reference, not the transform.

[v4hn]

I believe you are talking about this access operator?

This is actually guarded by ensuring there exists a shape for this index.
I believe the structure holds the invariant shapes_.size() == shape_poses_.size().

Edit:

but I suppose users who want to move empty shapes deserve the error.

No they don't. A framework should not segfault for bad input (Eigen possibly excluded....)

[rhaschke]

Thanks.

[rhaschke]

We should check for size == 1, because of this:
https://github.com/ros-planning/moveit/blob/4d3062164bf03ebb74c57d5eba0e3e50438481ba/moveit_core/collision_detection/src/world.cpp#L146

[v4hn]

Actually back when we last discussed this, we kept the case with multiple shape_poses_ a gray area on purpose. If you ask for the frame you get the first one, if you ask whether the frame exists, it says no.
I don't have a use-case for objects with multiple shapes, so I don't care too much.

It does seem uncalled for to change this semantic in a segfault fix though. ;)

The proper way to fix this is @felixvd proposal to have a global pose for the object...

[rhaschke]

I cannot believe we agreed on inconsistent behavior on purpose. As both fixes are strongly related, I'm fine to have them in a single PR. IMHO it doesn't make sense to split this tiny PR up into even smaller ones.

[codecov-io]

Codecov Report

Merging #2026 into master will increase coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2026      +/-   ##
==========================================
+ Coverage   54.04%   54.08%   +0.03%     
==========================================
  Files         319      319              
  Lines       24997    24997              
==========================================
+ Hits        13509    13519      +10     
+ Misses      11488    11478      -10     

Impacted Files	Coverage Δ
moveit_core/collision_detection/src/world.cpp	90.47% <0.00%> (ø)
...anning_scene_monitor/src/current_state_monitor.cpp	51.28% <0.00%> (-1.54%)	⬇️
...meterization/work_space/pose_model_state_space.cpp	82.31% <0.00%> (-0.69%)	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	70.50% <0.00%> (-0.45%)	⬇️
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp	75.00% <0.00%> (+5.37%)	⬆️

---

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 4d30621...64d6789. Read the comment docs.

[v4hn]

I cannot believe we agreed on inconsistent behavior on purpose.

I would be very surprised if you were involved in that discussion. As far as I remember the rationale was to avoid breaking code that silently relied on the Transforms of objects with multiple shapes and never checked `knowsTransform` - which is quite a common thing to do if you just added the object a few lines above. After all, it is an arbitrary and restrictive decision to stop treating the first transform of an object as its frame.

[rhaschke]

After all, it is an arbitrary and restrictive decision to stop treating the first transform of an object as its frame.

I agree. We should change both checks to !it->second->shape_poses_.empty() then.
My key point was to make it consistent 😉

[v4hn]

I agree. We should change both checks to `!it->second->shape_poses_.empty()` then. My key point was to make it consistent 😉

I believe the current behavior was present in the `PlanningScene` at least since 2013 (fc1a7e6). There has been a warning since then if you access the transform of an object with multiple shapes, but it still works. We discussed changing it at some point, but did not back then. I'm fine with clearing up the semantics to define the pose as the pose *of the first shape* altogether. One reason why I did not do that before is that the user does not always have full control over which shape is the first, e.g., shapes are generated in the planning scene by type array, so one shape type will always go first.

[rhaschke]

If @felixvd is going to change the semantics anyway, I think we don't need to further discuss this topic here...

[v4hn]

If @felixvd is going to change the semantics anyway, I think we don't need to further discuss this topic here...

I adjusted knowsTransform (which by the way makes it consistent with `RobotState`s code. Feel free to merge once CI finished.