jbuckmccready / CavalierContours

2D polyline library for offsetting, combining, etc.
MIT License
427 stars 79 forks source link

incorrect bulge value in some cases #48

Open tredecimguttatus opened 2 years ago

tredecimguttatus commented 2 years ago

Hello! While performing union of given vectors:

" V1: 0:{3, 7, 0}, 1:{3, 4, 1}, 2:{5, 4, 0}, 3:{5, 7, 1}, CCW" " V2: 0:{4.31623, 3.05132, 0}, 1:{10.3162, 5.05132, 1}, 2:{9.68377, 6.94868, 0}, 3:{3.68377, 4.94868, 1}, CCW" Found that we have some troubles for cases, when 2 counturs has arcs with bulge == 1 and same center.

Function "splitAtPoint", called from"sortAndjoinCoincidentSlices" handles case, when v1 equal to point. Now bulge just copied from v1, but if this slice will be cut later, bulge will be changed.

I used this update locally:

SplitResult<Real> splitAtPoint(PlineVertex<Real> const &v1,
                               PlineVertex<Real> const &v2,
                               Vector2<Real> const &point,
                               Vector2<Real> const pointNext = Vector2<Real>())
........
  } else if (fuzzyEqual(v1.pos(), v2.pos(), utils::realPrecision<Real>())) {
    result.updatedStart = PlineVertex<Real>(point, Real(0));
    result.splitVertex = PlineVertex<Real>(point, v1.bulge());
  } else if (fuzzyEqual(v1.pos(), point, utils::realPrecision<Real>())) {
    result.updatedStart = PlineVertex<Real>(point, Real(0));
    auto radiusAndCenter = arcRadiusAndCenter(v1, v2);
    Vector2<Real> arcCenter = radiusAndCenter.center;
    Real a = angle(arcCenter, pointNext);
    Real arcStartAngle = angle(arcCenter, v1.pos());
    Real theta1 = utils::deltaAngle(arcStartAngle, a);
    Real bulge1 = std::tan(theta1 / Real(4));
    result.splitVertex = PlineVertex<Real>(point, bulge1);
  }

So, we use new parameter "pointNext" to calculate new bulge, if needed. sortAndjoinCoincidentSlices was updated in this way: auto split1 = splitAtPoint(v1, v2, intr.point1, intr.point2);

Not sure if this update it totally correct for all cases, it was not properly checked, because now i'm fighting with union of: " V1: 0:{3, 7, 0}, 1:{3, 4, 1}, 2:{5, 4, 0}, 3:{5, 7, 1}, CCW" " V2: 0:{4, 3, 0}, 1:{9, 3, 1}, 2:{9, 5, 0}, 3:{4, 5, 1}, CCW"

Could you please check update above and use it, if needed.

jbuckmccready commented 2 years ago

Thanks for the bug report.

I made some changes in the Rust code to fix issues around overlapping cases, but it looks like this case maybe still has some problems in the Rust code as well - I'll look into it more in the Rust code and see if I can get everything working properly.

I don't know if changing the splitAtPoint function is the best way to solve it - in the rest of the code the calling code checks to handle odd cases if it's relevant and I think in this case it's just a matter of wanting to split at multiple points on the same segment so it probably makes sense to just create a new function for doing so for clarity. I think the bug is more involved than just handling this split at point cases.

For reference the Rust code is here:

jbuckmccready commented 2 years ago

OK looks like the issue (at least for the Rust code) is the fuzzy comparison epsilon value used in the circle-circle intersect function is too small/not matching the position equal epsilon value used in the rest of the algorithm (uses 1e-8 for intersect functions, 1e-5 for position equal in rest of algorithm). So the overlapping arcs are not detected when it goes to find intersects between polylines. Changing the circle-circle intersect function to accept an epsilon value parameter and passing in 1e-5 fixes this particular problem. I am changing the Rust code to have all intersect functions accept an epsilon value parameter and passing in the position equal epsilon value when those functions are called.

I have not looked at the C++ code to determine if there are other problems involved and I do not plan to work on the C++ code, but hopefully this information helps yourself or someone else if they wish to fix the issue in C++.

jbuckmccready commented 2 years ago

In addition to changing the intersect epsilon values I also needed to make the polyline segment intersect function be more consistent across cases - specifically for the line-arc intersect case it checks if the start or end of the line segment lies on the arc and if it does then use that point as the intersect so it is consistent with the arc overlap intersection end points (it is specifically here in the code for this change).

Commit with changes to fix the issue for reference: https://github.com/jbuckmccready/cavalier_contours/commit/e6a598ff5e0ef7ef418754b17c39cb96ba0c1b85

I also added the two test cases you posed to the Rust test suite.

jbuckmccready commented 2 years ago

Note the commit above wasn't implemented quite right, I fixed a bug introduced by it with this commit: https://github.com/jbuckmccready/cavalier_contours/commit/1a40c08a2a23f18969e9b77c2976e244fea473d1 (see commit changes for details).