[voronoi] pps() is treating site3 as an infinite line #66
Conversation
eadf
changed the title
|
Hi, I will start looking into this and probably will get back with some questions. Thanks for the PR! |
|
The same problem exists for |
Fixed by simply reporting the CE as "not found" when pps() finds a CE outside the segment range.
eadf
force-pushed
the
fix_bad_circle_events
branch
from
444a621
to
ab83894
Compare
|
I've changed the PR so that In addition to the test-cases listed above, this solution also fixes: (this is one of the test cases I thought should be fixed by a future By comparing identical circle events by their age, the But it turns out that cases like this, that works w/o the identical-CE-age-ordering, will break: At least we got a lead on what's causing the problems: the ordering of coordinate-identical circle events. |
eadf
force-pushed
the
fix_bad_circle_events
branch
from
f321238
to
ab83894
Compare
| @@ -1129,6 +1129,27 @@ class voronoi_predicates { | |||
| site1, site2, site3, segment_index, c_event, | |||
| recompute_c_x, recompute_c_y, recompute_lower_x); | |||
| } | |||
| // All points needs to be unique or the dot product calculation will not be valid | |||
| bool unique_endpoints = !(site1.point0() == site2.point0() || | |||
There was a problem hiding this comment.
Do we actually actually need this now that you are not calling ppp anymore?
i.e. don't we only want that site3.is_segment() == true? (which should be required for calling pps anyway)
There was a problem hiding this comment.
without a unique endpoint test the diagram generation will fail on examples like
0
3
-5138 -5149 -5038 -5142
-5042 -5069 -5165 -5162
-5011 -5195 -5404 -5134
or
0
2
-5093 -5402 -5362 -5008
-5200 -5273 -5273 -5265
(I got more examples if you need them) (It's interesting that none of those examples uses shared end-points, and yet they fail)
I'm not exactly sure why, but I guess it ties in to your follow up question:
That can lead to missing voronoi vertices right?
Yes
| @@ -1044,7 +1044,7 @@ class voronoi_predicates { | |||
| } | |||
| } | |||
|
|
|||
| void pps(const site_type& site1, | |||
| bool pps(const site_type& site1, | |||
There was a problem hiding this comment.
Just a suggestion: instead of having pps() (and the other ones) return a bool, could we perhaps check the circle downstream, say where lies_outside_vertical_segment() is called?
Something like:
if (!site_touches_circle(circle, site1) || !site_touches_circle(circle, site2) ||
!site_touches_circle(circle, site3)) {
return false;
}
with:
bool site_touches_circle(const circle_type &c, const site_type &s) {
fpt_type v0c[2] = {c.x() - to_fpt(s.point0().x()), c.y() - to_fpt(s.point0().y())};
if (!s.is_segment()) {
fpt_type square_dist = v0c[0] * v0c[0] + v0c[1] * v0c[1];
return ulp_cmp(square_dist, (c.lower_x() - c.x()) * (c.lower_x() - c.x()), ULPS) == ulp_cmp_type::EQUAL;
}
fpt_type v01[2] = {to_fpt(s.point1().x()) - to_fpt(s.point0().x()),
to_fpt(s.point1().y()) - to_fpt(s.point0().y())};
fpt_type dot = (v0c[0] * v01[0] + v0c[1] * v01[1]) / (v01[0] * v01[0] + v01[1] * v01[1]);
return ulp_cmp(dot, 0.0, ULPS) != ulp_cmp_type::LESS && ulp_cmp(dot, 1.0, ULPS) != ulp_cmp_type::MORE;
}
However, it is probably desirable that if circle_existence_predicate_.pps() returns true then circle_formation_functor_.pps() has to compute a valid circle event (i.e. not a NaN and all three sites touching the circle) and we should then assert it instead of returning false.
There was a problem hiding this comment.
Sure, that is an option. I just added the test in pps() itself so I did not have to repeat the same code 3 times (in code text).
and we should then assert it instead of returning
false.
Yes, this PR does not solve the root issue. It just fixes a few edge cases.
|
Thanks @eadf ! |
Yes. I tried the same fix for The best thing would be if |
This voronoi example:
Will get this result from 1.76.0:
The problem is that
pps()is treating segments as a lines of infinite length.Here is an illustration of the problem:
site1andsite2are the black and blue dots.site3is the green line.site3.point0is the dark green dot.pps()will find the red circle event, and that would have been correct ifsite3were just a little bit longer.pps()assumes thatsite3tangents the CE it is looking for, but that's not always the case.The correct circle event, the one passing through
site1,site2andsite3is the cyan colored circle.My proposed solution to this problem is to take the CE
pps()finds and project the (site3.point0-> CE) vector upon the (site3.point0->site3.point1) vector.If the length of the projection is larger (or negative) than the
site3vector, the real CE is re-calculated usingppp().The 'cost' of this fix is an additional calculation of a dot product for every call to
pps()+ the rare occasion whenppp()actually needs to be called.To be able to call
ppp()with eithersite3.point0orsite3.point1I had to change the method signature ofppp()to accept points instead of sites.Here are more examples of the problem. (this PR fixes 'em all)