Tangents don't give intersection points reliably - BUG! #356

Closed
atorrey opened this Issue Jan 13, 2013 · 5 comments

Comments

Projects
None yet
3 participants

atorrey commented Jan 13, 2013

Even though by definition, a line that is tangent to a circle, or two circles tangent to each other MUST intersect, I find the program does not reliably recognize the intersection point when you have a line tangential to a circle, or two tangential circles, especially the latter.

In the cases I'm encountering this bug, the entities that are supposed to be intersecting were constructed using one of the tangent construction features, so there should absolutely be a point of intersection, and the program should "know" where it is, since it drew the entities in question.

Where this matters is when trying to divide an entity for subsequent trimming, or defining endpoints for measuring, etc. I select the "snap to intersection" mode, and it will often find one of two points, but not the other. Sometimes it works fine...

Contributor

TNick commented Mar 17, 2013

The problem, as discussed with Dongxu Li on IRC is related to the way math regarding intersection points is done by the program. In this particular case, in file /lib/math/rs_math.cpp, on line 427 quadraticSolver() function checks the determinant for a positive or zero number. However, the number may be negative and very small. Real life test shows that qFuzzyIsNull() is too strict. Comparing agains an arbitrary small value, say qAbs(discriminant) < 0.0000000001 may work but is not fesable. Dongxu Li contributed the following idea:

std::vector<double> RS_Math::quadraticSolver(const std::vector<double>& ce)
//quadratic solver for
// x^2 + ce[0] x + ce[1] =0
{
    std::vector<double> ans(0,0.);
    if(ce.size() != 2) return ans;
    double d1 = 0.25*ce[0]*ce[0];
    double discriminant=d1-ce[1];
    if ( discriminant > -(DBL_EPSILON)*qMax(d1, qAbs(ce[1]) ) ) {
        ans.push_back(-0.5*ce[0] + sqrt(discriminant));
        ans.push_back(-ce[0] - ans[0]);
    }
    return ans;
}

That solves current bug (the two circles are shown to intersect) but may not solve the problem in the long run. For example a circle of radius 1 placed in origin and a line from (-1,1.000001) to (1,1.000001) will not count as an intersection in above implementation.

Now in my view (that may not be shared by the rest of the users, so that may need to be investigated) the snap feature should be dependent on the zoom factor. This is what Autocad does and feels natural to me (probably because I only use Autocad these days). This means that the snap is shown to be active only when (in some tolerances) the two objects appear to "touch" on the screen. To do that, the code would need to take into account current scale factor when comparing distances / values. since quadraticSolver() may be used in other parts of the code or by future plug-ins to do just that - solve the equation, maybe a new set of functions that take the zoom factor into account should be implemented.

Will submit a pull request tomorrow with Dongxu Li's solution.

atorrey commented Mar 18, 2013

Thanks for investigating this. The math / code is a bit over my head, but I could see possibly doing it the way I think I understand your description...

I'm not totally sure about the zoom issue though, as at least on my screen, the objects appear to touch at any zoom factor where they can be seen easily... OTOH if I tried to approximate a solution by zooming way in and manually placing a point, I find that the two objects appear to overlap for a considerable distance, so I end up trying to guesstimate the midpoint of the overlapping region (which is close enough for my needs, but isn't "right")

Contributor

TNick commented Mar 18, 2013

Hello, atorrey. The above post is meant as an investigation, not specifically regarding your problem. This is a symptom of a more larger problem that will need to be addressed.

For your specific problem I will submit a pull request that fixes the issue as described above.

Contributor

TNick commented Mar 18, 2013

Ideally, the fix should have been inserted in this thread. However, I'm not sure how and I was too lazy to research.
Temporary fix

Owner

dxli commented Jul 14, 2014

I suppose this bug has been fixed for a while.

dxli closed this Jul 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment