use compose for disjoint union #217
Conversation
| // this kMaxUnionSize is a heuristic to avoid the pairwise disjoint check | ||
| // with O(n^2) complexity to take too long. | ||
| // If the number of children exceeded this limit, we will operate on chunks | ||
| // with size kMaxUnionSize. |
There was a problem hiding this comment.
If performance becomes an issue here, this is exactly what collider is for. It does bounding box overlap checks in nlogn, in parallel.
There was a problem hiding this comment.
Yes, I think we talked about it in the previous PR: #131 (comment)
There was a problem hiding this comment.
Ah, right. Is there a reason you need to append boxes? Generally the purpose of the collider is to take your everything vector and find all the collisions at once (hence the parallelism).
There was a problem hiding this comment.
The problem is that we need to partition the set of boxes into sets of boxes such that each set contains disjoint boxes. The target is to minimize the number of boxes. Boxes that collided means that they have to be in different sets. Maybe I can think about how to do it efficiently later.
|
If the tests pass, I'll be happy to merge this. If we get some complicated failures, agreed that we shouldn't hold the release for it. If you're now recomputing the bounding box, is there any source of rounding error? I wouldn't expect this to be a place where tolerance would be required. |
Say we have ε = 0.1, one cube with two corners in (0, 0, 0) and (1, 1, 1), another cube in (1.05, 1, 1), (2.05, 2, 2). If we don't consider tolerance, the bounding box are not overlapping, but I guess we still need union to merge them into one cube? |
Actually, no. There is no tolerance in the Boolean ops (they only use symbolic perturbation), so those would remain disjoint regardless. Only in the triangulation and degenerate removal does the tolerance play a role. I think you're safer to do compose without tolerance. |
|
OK, I will remove that tolerance check. |
Last mystery in elalish#114. It turns out that the problem is due to misunderstanding of the bounding box API: Box::Transform is *not* a safe over-approximation of the bounding box of the manifold after the transform. In fact it might be smaller than that if the transform is not axis aligned. The simple fix is to apply the transform first and then compute the bounding box, this will also make sure that the bounding box is tight.
pca006132
force-pushed
the
union-compose
branch
from
683970d
to
8dcab8b
Compare
Codecov ReportBase: 92.31% // Head: 92.36% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #217 +/- ##
==========================================
+ Coverage 92.31% 92.36% +0.05%
==========================================
Files 32 32
Lines 3395 3419 +24
==========================================
+ Hits 3134 3158 +24
Misses 261 261
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
* use compose for disjoint union Last mystery in elalish#114. It turns out that the problem is due to misunderstanding of the bounding box API: Box::Transform is *not* a safe over-approximation of the bounding box of the manifold after the transform. In fact it might be smaller than that if the transform is not axis aligned. The simple fix is to apply the transform first and then compute the bounding box, this will also make sure that the bounding box is tight. * fix batch union Co-authored-by: Emmett Lalish <elalish@gmail.com>
Last mystery in #114. It turns out that the problem is due to misunderstanding of the bounding box API:
Box::Transformis not a safe over-approximation of the bounding box of the manifold after the transform. In fact it might be smaller than that if the transform is not axis aligned (I should have just RTFM at that time). The simple fix is to apply the transform first and then compute the bounding box, this will also make sure that the bounding box is tight.In the disjointness check, I enlarged the bounding box by the tolerance of the object, which can hopefully overapproximate the behavior of a perturbation of the vertices by at most the tolerance. However, I did not really add tests that can check this, so I am not sure if this is useful.
Note: This patch is not very important right now, maybe we can merge this after the next release, if we are not certain about this. I think the most important part of this patch is the potential performance improvement it brings and shows that our compose implementation should probably be fine.