Fix getTransform()

[rhaschke]

Fix for moveit/moveit_task_constructor#168.

This PR replaces:

• PlanningScene::getTransforms().getTransform() -> PlanningScene::getFrameTransform()
• PlanningScene::getTransforms().canTransform() -> PlanningScene::knowsFrameTransform()

throughout the whole MoveIt code base.
getTransforms() essentially yields the SceneTransforms instance of the parent scene only:
https://github.com/ros-planning/moveit/blob/272eaa01064df0b0184e2ad8766751ea27ef6c92/moveit_core/planning_scene/src/planning_scene.cpp#L630-L635
https://github.com/ros-planning/moveit/blob/272eaa01064df0b0184e2ad8766751ea27ef6c92/moveit_core/planning_scene/include/moveit/planning_scene/planning_scene.h#L172-L181

instead of creating a new layer of transforms as getTransformsNonConst() would do.
Accessing the corresponding frame transforms obviously yields the wrong results.

[v4hn]

Thanks for patching it. Fully makes sense and I will merge it now.

I did not file this request because the downside of not waiting for a user contribution is that now we still don't have tests for diff overlayed frame lookups and much less incentive to write them... sigh

[rhaschke]

This fix was not related to a broken handling of transforms in scene diffs. I'm not sure whether we already have tests for this or not.
The key issue here was that we used the wrong API: First getting the SceneTransforms class (which returned the instance from the parent scene) and then retrieving transforms from this instance, obviously bypasses the overlayed diff scene. In all adapted cases, we didn't actually need the SceneTransforms instance. Actually I was tempted to remove the SceneTransforms class completely (but only keep the base class). This would trigger hard errors if the wrong API is still used somewhere because the Transforms class doesn't know about link, object, or subframe transforms.

[v4hn]

Obviously we did not have a test for it yet, else it would have triggered. The problem was in our own API using "wrong" API and it would have been nice to have a test for this convolved use-case. We have way too few tests for `diff()` usage and I am convinced we will find more issues there in the future.

Actually I was tempted to remove the SceneTransforms class completely (but only keep the base class). This would trigger hard errors if the wrong API is still used somewhere because the `Transforms` class doesn't know about link, object, or subframe transforms.

Do you want to file a pull-request for this with a note in `MIGRATIONS.md`? I would be happy to merge it. The `SceneTransforms` class is an ugly interface wrapper around the planning scene bound to the lifetime of the planning scene by raw pointer. I don't see any merit in keeping it.

[rhaschke]

Obviously we did not have a test for it yet, else it would have triggered.

No. If you use the wrong API, a test will not help.
This would require a test that (obviously) fails when one uses the wrong API. However this would be ridiculous.

[rhaschke]

Ok, reading further, I note that you actually want a test for processCollisionObject() which handles this case. Fair enough.

Do you want to file a pull-request for this with a note in MIGRATIONS.md?

Why should we mention this change in our migration notes? This doesn't affect API nor intended behaviour. It's just a bug fix.

[v4hn]

No. If you use the wrong API, a test will not help.

If we had a test that `diff`'s a scene, modifies the robot state, attaches an object in a robot frame and then check the position of said object via the `getFrameTransform` API, or possibly via both mentioned APIs to check for consistency, it would have helped. Good morning to you too. ;)

[v4hn]

> Do you want to file a pull-request for this with a note in `MIGRATIONS.md`? Why should we mention this change in our migration notes? This doesn't affect API nor intended behaviour.

I mean a pull-request for removing `SceneTransforms`. That one would of course be a breaking change, and not even API breakage.

[rhaschke]

I see. I don't have time for this today (estimated work-load: 1h). I will file an issue instead 😉