-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
re-enabled reordering #171
Conversation
Ah and interestingly the |
It seems that it is stuck in an infinite loop in |
I upgraded our debug mode, so you should be able to run |
Well, I was hoping #179 would solve this problem, but it seems to still exist after merging. It's definitely a related issue though - I'll keeping looking into it. |
Okay, I've got a repro on master now, #183. I must say, this has been a great PR for testing! |
Indeed. After this is merged we can try to enable usinf compose for union which also need some fixes. Also, I can start working on the fuzzing/property-based testing, which will probably expose more subtle bugs. |
It looks like this PR isn't breaking anything, but just exposing some degenerate triangle removal difficulties already present on master. That's fine, but before we start reordering the operations, can you think of a way that we can cache intermediate results that are reused? We don't have any great examples of this in the samples right now, but it's pretty common in design. I really don't want to force users to trigger intermediate evaluation manually. |
I think intermediate results are already cached? Users only need to trigger intermediate evaluation manually if they want to debug or need to do some benchmarking. Btw, should we modify the |
Interesting; I was referring to this, but I guess I'm not quite sure how common that behavior is. Maybe it's not a big deal; I should really write some more samples. Yes, let's go ahead and update the tests and merge this. 👍 |
Oh, because we are not using compose for now, that behavior will not occur. For compose, it may be beneficial because we may be able to get more non-intersecting manifolds, but it is not beneficial when we do boolean directly. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
==========================================
+ Coverage 96.55% 98.15% +1.60%
==========================================
Files 30 30
Lines 2611 2602 -9
==========================================
+ Hits 2521 2554 +33
+ Misses 90 48 -42 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage is looking good 👍
* re-enabled reordering * fix test failures
This reverts ecd9329 and make
BatchUnion
useBatchBoolean
instead ofCompose
, as there are additional issues withCompose
. Will finally close #114 after the triangulation problem is resolved.Currently, the
Samples.Sponge4
test fails with