Pad using weighted vertex normals

[kbrameld]

Resolves: #76

As discussed in the linked issue, padding behavior for complex meshes are not good. These changes improve upon the padding functionality, and obtain a closer functionality to blender's fatten method. Two main changes:

• padding is performed in direction of vertex normal instead of direction from center of mesh
• calculates better weighted vertex normal by accounting for the angle of a face at a vertex.

A complex STL file (original)	Current padding method

Fatten method (in Blender)	New padding method

Non-uniform padding x-direction	y-direction	z-direction

As discussed in the linked issue, although this is a general improvement, this change may potentially affect behavior in downstream packages if they relied on the bad padding.

[peci1]

Wow, it's been a long time since I was interested in this issue.

In #127, I found:

Another comment was that generally change of the current behavior is okay as long as we can provide some guarantees on the relationship between the old approach and the new one (i.e. all points contained by the old behavior would be contained by the new one, or so...).

Could you sketch up a "proof" of some relationship?

Or, alternatively, wouldn't it be easier to create a function with a new name (fatten?) instead of scaleAndPad to retain the original behavior for existing code?

[kbrameld]

Another comment was that generally change of the current behavior is okay as long as we can provide some guarantees on the relationship between the old approach and the new one (i.e. all points contained by the old behavior would be contained by the new one, or so...).

Not sure if there is a nice relationship. The old padding method was closer to a scaling operation, and the resulting mesh often didn't include the original vertices. Here's an example with the blue being the padded shapes:

Old method	New Method

The only meshes that will look similar between the old and new method will be meshes where the weighted vertex normal direction is close to the direction of the vertex from the center. (eg. cube, sphere meshes) For any other mesh, the padded shapes will look different.

Or, alternatively, wouldn't it be easier to create a function with a new name (fatten?) instead of scaleAndPad to retain the original behavior for existing code?

Yes, this can be considered an alternative. If we go this approach, I advocate for deprecating the "pad" methods (in upcoming ros2 distros).

[rhaschke]

Thanks for this contribution.
Given that the old mesh padding was not working as expected, I don't see the need to introduce the new behavior with a new name. I even suggest replacing the old method computeVertexNormals() with the new one.

[peci1]

Given that the old mesh padding was not working as expected, I don't see the need to introduce the new behavior with a new name.

At least robot_body_filter users actively use padding with its current behavior. Any big changes could break their workflows. But as long as the result is similar for the commonly found meshes (e.g. UR-5), it should be okay. I think the example mesh used in this PR is kind of extreme and very far from what people normally use, so the big difference in the example is okay.

[peci1]

@kbrameld Do I get it correctly that the unexpected case of enlarging a square would still be present with this PR? #127 (comment) .

[kbrameld]

@kbrameld Do I get it correctly that the unexpected case of enlarging a square would still be present with this PR? #127 (comment) .

You're correct, the changes in this PR won't change the behavior of enlarging the square in that issue.

[ijnek]

Once this is merged, I will create a PR against the ros2 branch too. What's required to move forward with this PR?

[ijnek]

Pinging this thread for attention to be reviewed

[rhaschke]

Merging this, w/o a second review as nobody seems to do it...

[kbrameld]

Thanks for reviewing @rhaschke!

[peci1]

I've tested robot_body_filter (which uses geometric_shapes) with a few models from https://github.com/ctu-vras/rosdevday_cloud_filtering and the models seem to be affected only a little by this PR. So it seems this change should be safe for most users! Thanks.

[timonegk]

Once this is merged, I will create a PR against the ros2 branch too.

Now that this is merged, could you create a PR for ROS 2?