Open peci1 opened 4 years ago
Reading this, my first impression was: What a weird kind of padding... Do we actually want to have/keep this definition of padding? Would, e.g., padding only convex vertices, be a reasonable alternative definition?
Does the "optimization" you mention above achieve such an alternative definition consistently?
What do you mean by padding only convex vertices? You mean only those that are not in the middle of a larger plane? That would probably be more of what one expects, but the computations would be even more complicated...
The optimization I suggested would follow the current "weird" padding definition. Maybe the current behavior e.g. in isPointInPlanes()
is closer to your alternative definition (if I understood it correctly) in the fact that it doesn't change normals of the scaled planes. However, it is then not true that the scaled vertices lie on their scaled plane, which is kind of weird. So if you'd want to change the meaning of padding, it would need a more thorough debate on what would be all the consequences and how to compute it.
What do you mean by padding only convex vertices? You mean only those that are not in the middle of a larger plane?
This is what I had in mind, yes.
The optimization I suggested would follow the current "weird" padding definition
That is why I wanted to raise the question of whether the current definition is even what we want. I guess the decision boils down to:
ConvexMesh
objects to be constructed from minimal convex meshes only (i.e., no vertex can be dropped without changing the convex hull).Do you have time to join the MoveIt maintainer meeting tomorrow to discuss this (and your other issues) in person?
Okay, let't talk about it tomorrow, I'll join.
Here's a wrap-up from the WG meeting.
Any changes we make, we should want their behavior to be synchronized between shapes and bodies. I only see a problem that shapes have a (general) Mesh, while bodies have ConvexMesh.
There are multiple alternative ways of implementing padding:
Pros and cons:
One of the comments was that we should keep runtime overhead as low as possible. This can be tested e.g. on PR2 robot meshes.
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...).
This might already be resolved by #238?
Planes are still "bent" by the padding. But I don't see a solution to the problem either. We cannot easily keep planes planar with padding.
Ah, you're right.
In
ConvexMesh::useDimensions()
, there is this code that processes planes (halfspaces) generated by qhull:https://github.com/ros-planning/geometric_shapes/blob/07a5254b698ecc89e511dc5b3182ecb6c7d2258e/src/bodies.cpp#L903-L904
It checks if the plane for the currently processed triangle/facet isn't too similar to the plane for the previous triangle. If it is, it merges these planes into one and makes both triangles reference the same plane.
However, according to the definition of padding for convex meshes in this library, this optimization goes wrong when non-zero padding is used.
I made some visualizations to make this argument clearly visible:
Here, the red line is original (unscaled) "mesh" (well...). The cyan one is the line after applying only scaling. And the green one is after padding is applied. You can see that the line stops being a line and any planes that would be optimized by the previous algorithm would no longer be parallel.
Maybe even more obvious.
This has also consequences for more code paths. It seems to me that two things are required.
mesh_data_->scaled_planes_
which would contain the correctly scaled and padded planes which could be used for collision checking. For example this code tries to subtract padding from the data, but fails to do it correctly (a part of it is fixed in #109):https://github.com/ros-planning/geometric_shapes/blob/07a5254b698ecc89e511dc5b3182ecb6c7d2258e/src/bodies.cpp#L1091-L1100
If there were an already precomputed field containing the scaled planes, all computations of this kind could be done both efficiently and without fear if everything is correctly accounted for. My approach to recomputing the scaled planes would be to just take the scaled vertices and reconstruct the scaled planes from them. It has just one drawback - so far,
mesh_data_
are not touched inupdateInternalData()
, however if we wanted to keep a set of scaled planes, these would obviously need to be updated inupdateInternalData()
. But this function anyways has a loop over all vertices to recompute the scaled vertices, so I think another loop for scaled planes is not a problem. It might well be compensated by the possible speedups from #126.