moveit / geometric_shapes

Representation of geometric shapes
57 stars 92 forks source link

Pad using weighted vertex normals #238

Closed kbrameld closed 5 months ago

kbrameld commented 7 months ago

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:

A complex STL file (original) Current padding method
Screenshot from 2024-04-02 15-01-38 Screenshot from 2024-04-02 14-01-02
Fatten method (in Blender) New padding method
Screenshot from 2024-04-02 11-39-09 Screenshot from 2024-04-02 14-27-05
Non-uniform padding x-direction y-direction z-direction
Screenshot from 2024-04-02 15-00-00 Screenshot from 2024-04-02 15-32-50 Screenshot from 2024-04-02 15-32-27

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 commented 7 months ago

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 commented 7 months ago

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
Screenshot from 2024-03-14 14-54-19 Screenshot from 2024-03-14 14-46-05

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).

peci1 commented 7 months ago

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 commented 7 months ago

@kbrameld Do I get it correctly that the unexpected case of enlarging a square would still be present with this PR? https://github.com/ros-planning/geometric_shapes/issues/127#issue-587387573 .

kbrameld commented 7 months ago

@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 commented 6 months ago

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

ijnek commented 5 months ago

Pinging this thread for attention to be reviewed

rhaschke commented 5 months ago

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

kbrameld commented 5 months ago

Thanks for reviewing @rhaschke!

peci1 commented 5 months ago

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 commented 5 months ago

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?