Add origin Pose to CollisionObject

[felixvd]

This is part of a PR implementing moveit/moveit#2025.

[v4hn]

Suggested change

	# A PoseStamped defining the object's origin. The shapes and subframe poses are defined in this frame
	geometry_msgs/PoseStamped origin
	# The object's origin. The shapes and subframes are interpreted relative to this frame
	geometry_msgs/PoseStamped pose

[v4hn]

object.pose reads much better than object.origin in my opinion. We already have a number of *_poses fields, so I would keep the terminology.

We might want to debate the If set. I would like to keep a fallback behavior for this in place to ease the transition.
My idea is to check for a valid quaternion or frame and fall back to using the first pose of a shape instead.

Secondly, I would propose to allow leaving the respective *_poses vector empty if only one shape is described.
It seems stupid to enforce users have to write down identities for all their objects.

[felixvd]

Agreed on the pose name and being allowed to leave the pose vectors empty

During the transition, the frame that was previously defined in header will need to be defined in the PoseStamped. This step is required either way.

Rather than maintaining logic where the first shape can replace the object's pose though, I would ask users to just switch the shape's pose with the object pose when porting to Noetic. For most people it shouldn't matter anyway, and going forward it will be a lot cleaner.

[v4hn]

I agree with the proposal to ask users to move the pose, in case a manual transition is really required.
I would propose to avoid that though.

For most people it shouldn't matter

It sure does if all your code breaks because the header is suddenly gone. I'm sure I will still find code in 5 years I'll have to migrate. 😕
It also makes it really hard to write code compatible with MoveIt melodic and noetic.
This is a really annoying API change!

Thinking about this some more, I propose to retain the Header and make the object pose a Pose only.
The header should still refer to the object pose only. I understand why you made it a PoseStamped to begin with, but the only difference between that and the alternative is where you store a header. A comment, such as the one you already give, easily suffices to explain this.

The "only" use-cases for this new feature are multi-shape objects (very rare and somewhat broken in MoveIt for a long time) and subframes (which are very useful but not currently used a lot). I don't see this justifying breaking an 8 year old API that is the common API to communicate with the framework.

The additional logic you talk about can be encapsulated in a single function in PlanningScene that rewrites a single pose if there is only one in a message.

[felixvd]

I can get behind that. It will actually make this change very benign. I will update on the weekend.

In this case the pose might better be named offset. What do you think?

[v4hn]

In this case the pose might better be named `offset`. What do you think?

I would still prefer `pose`. To me, `offset` seems to emphasize the less important semantics. I am interested in other opinions here though.

[v4hn]

Looks good to me now.

[mlautman]

LGTM

[rhaschke]

The current message somehow obstructs the fact that both, header and pose are determining the pose of the object. What about replacing header + pose with a PoseStamped?
Alternatively, the comment should explicitly state that the pose is interpreted w.r.t. to the header's frame_id.

EDIT:
Just now followed read the whole conversation. Obviously, I have a different opinion than @v4hn.
But, I can understand that maintaining the old API is a good goal as well. In this case, it should be - more strongly - emphasized that both, header and pose are determining the object frame.

[v4hn]

In this case, it should be - more strongly - emphasized that both, header and pose are determining the object frame.

As you blocked the merge, please make a concrete suggestion for the comment you would like.
I am fine with the current state.

[v4hn]

Merging as this has three approvals now.

[pbeeson]

# The object's pose relative to the header frame.
# The shapes and subframe poses are defined relative to this pose.
geometry_msgs/Pose pose

The above is in moveit_msgs master, but it is simply false. This is NOT used in MoveIt. It might be used in a Moveit PR, but MoveIt PR #2037 still "has a lot of work" needed as one reviewer states. So why are you pushing this into moveit_msgs master as if its already supported in moveit?

Not good practice IMO. I realize that there's an issue with continuous builds on PRs with multiple repos, etc. etc., but then use a continuous_build branch or something that you merge into first. Pushing unsupported features (specifically ones that claim to do things that don't actually get done in the comments of ROS messages) is just going to waste people's time.

My two cents after wasting over an hour debugging why this wasn't working as advertised in the ROS message (I started a new feature and went straight to the master message source to see how it worked), then re-coding everything to use the original primitive_poses and mesh_poses fields.

[pbeeson]

Not good practice IMO.

I write this fully aware of the irony that that I had a moveit_msgs PR that was recently merged into master to support a moveit PR that is still not merged. I'm not blaming anyone here, but if this is how things need to be (why moveit_msgs is its own repo is beyond me), then at least merged messages that are not yet supported by Moveit should be commented as such until the related moveit PR is merged.

[v4hn]

Hi @pbeeson , sorry about your frustration.

Reviews do not always go as planned and the object pose request should have been merged months ago after this messages pr got merged. @felixvd fell silent for quite a while and we are obviously understaffed w.r.t. maintainers who actually do sufficiently complete reviews.

We discussed whether to merge the messages package into the main repo, but nobody did it yet. I believe the person who brought this up vanished rather spontaneously from the Open Source community and most of the other maintainers haven't heard from them since.

Of course you are very welcome to support either of these with a review/pull-request.
I just filed #90 to mitigate your problems for other users.

[pbeeson]

Thanks @v4hn. I fully understand the fact that having messages in a different repo than code can lead to issues like this (in my previous job, we have 10+ interrelated repos in the same workspace). At first I thought this genuinely was a bug where the PlanningSceneInterface wasn't properly handling this. Once I figured out it was just a random addition to the message, I was frustrated because generally the source code (and it's comments) should be the definitive document of what a program can and can't do. I think your change was sufficient and might probably be what's needed moving forward for similar scenarios. In any case, I'm over it. Gotta keep moving ;)

[v4hn]

in my previous job, we have 10+ interrelated repos in the same workspace

You missed the time where we had more than 10 too. We consolidated a lot because of issues like that.

I think your change was sufficient and might probably be what's needed moving forward for similar scenarios

Ideally, moving forward should mean we get the respective patches merged at roughly the same time... But between family, jobs, paper writing and robot experiments there's not enough time for everything else. Please keep your patches and issues coming. We can't fix what we don't know about. Of course patches are always more welcome than just complains ;)

[felixvd]

Sorry for causing wasted time. Insufficient documentation is frustrating enough, so I understand that misleading documentation can be aggravating. If moveit_msgs is not merged into the main repository, a CI branch does sound like a reasonable alternative.

@felixvd fell silent for quite a while

In my defense, it was 〜corona time〜 and I did update the PR every month or more, I was just stuck on bugs after the scope of the changes became clear. But it's my bad and I should have updated this PR to avoid wasting people's time!

but MoveIt PR #2037 still "has a lot of work" needed as one reviewer states.

That was a comment about the version from April, not the latest state. It should run fine now, and is only missing tests/tutorials. I would love if you could test the PR with the setup that got you to start debugging this in the first place. Sorry again for the time loss.

[pbeeson]

If this is indeed working, the. I may try it later this week. It was hard to tell the state from the reviews as there were a lot of changes and fixes early on. Thanks.

…

On Wed, Sep 9, 2020 at 10:22 PM Felix von Drigalski < ***@***.***> wrote: Sorry for causing wasted time. Insufficient documentation is frustrating enough, so I understand that misleading documentation can be aggravating. If moveit_msgs is not merged into the main repository, a CI branch does sound like a reasonable alternative. @felixvd <https://github.com/felixvd> fell silent for quite a while In my defense, it was 〜corona time〜 and I did update the PR every month or more, I was just stuck on bugs after the scope of the changes became clear. But it's my bad and I should have updated this PR to avoid wasting people's time! but MoveIt PR #2037 still "has a lot of work" needed as one reviewer states. That was a comment about the version from April, not the latest state. It should run fine now, and is only missing tests/tutorials. I would love if you could test the PR with the setup that got you to start debugging this in the first place. Sorry again for the time loss. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5N7RROVXSVW2R5QNMDNKLSFBBA5ANCNFSM4MSB7BAQ> .

[felixvd]

There were a lot of changes overall, so every review is appreciated :)