Some thoughts on this issue. First, I don't think the small adjustments to u,v are likely to be a problem but I'm still against doing it unless needed. The criteria here are not optimal, you can create a perfectly good curve that has coincident end points (draw bezier with one left click and one right, constrain the end points together). Having said that, this would probably work fine.

I'm leaning toward passing an optional flag "allowZero" defaulting to false (similar to mustConverge). We'd get the tangents and then check if they're within EPS of zero. If so, adjust u or v and call ourself recursively and pass the flag as true so we only try once. This way we can have better control over it. We could also default it to true and try passing it as false in a number of places to see where the bug really is - meaning what part of the boolean code actually fails with a zero tangent vector. Not that I'm excited to do that, but I might just to get a better understanding of the failure. Checking for an actual zero tangent is also the actual condition we're looking for.

What do you think?

You are right that I should have done a for loop and checked all control points along the edge not just the end ones (I debugged and at least for the cone and sphere all three are the same). I'll change that.

As for the "move inwards only if the tangent is zero" approach I tried it here on lines 423-433:
ruevs@6f4412c#diff-9a3c0518824c807f35571bba5775d312f07c78f39e153ef725d3163f47e07bcaR423
It works as well but we should choose carefully what double tol we pass to (tv->Equals(Vector::From(0, 0, 0),1e-9)) and how much we shift u or v by. The 1e-9 I used is too big as it the 0.000000001 or LENGTH_EPS.

Overall I like the approach in this pull request better (caveat - without understanding all affected users of tangents), because we have a "boolen" decision of whether we are on a singularity/degenerate point instead on some arbitrary numerical tolerance. I debugged the failing case (with the cone) and it fails with tv with two of the coordinates 0 and one on the order or 1e10-15. But this is just one isolated case...

As for adding a flag - it is a great idea for debugging, but instinctively I do not like it as a final solution. If we were to add it permanently (defaulted to allowZero=false), and if any place in the code ends up passing true then we should really understand "who wants a tangent but is OK with a zero length vector as a result??!". It does not sound right to me - and therefore the flag should be redundant in "production mode".

@phkahler amended, please take a look.

@ruevs Ouch :-) that's a lot of code, but it does look correct for finding these "pole" point/edges. What if we reduced that first condition with something like:

if (ctrl[0][0].EqualsExactly(ctrl[0][1]) && ctrl[0][degn].EqualsExactly(ctrl[0][degn-1]) {
// move u inward
}
for degree 1 it compares twice, while degree 2 it will compare all 3 points, and degree 3 it kinda works sometimes. It can be evaluated without looping though for any degree. Maybe we limit to 2nd degree curves only? Are we after a general solution, or just checking for the special points produced by Lathe? In the lathe case the curves are always of degree 2 and have exactly equal points due to this fragment in FromRevolutionOf:

        // not sure this is needed
        if(ps.Equals(pf)) {
            ps = c;
            ct = c;
            pf = c;
        }

It seems the answer to the comments question is yes - it makes it easier to find these later.

Another place where tu and tv "matter"
94a3cfd
#623

@ruevs I propose the following change which is less strict but also just less.

    // Check if we are trying to calculate a tengent "along" a degenerate (zero length)
    // edge of a NURBS patch. In this case move the `u` or `v` parameter "inward" a bit.
    if(EXACT(0.0 == u) && (ctrl[0][0].EqualsExactly(ctrl[0][1])
                       || ctrl[0][degn].EqualsExactly(ctrl[0][degn-1])) ) {
        u += LENGTH_EPS;
    }
    else if(EXACT(1.0 == u) && (ctrl[degm][0].EqualsExactly(ctrl[degm][1])
                       || ctrl[degm][degn].EqualsExactly(ctrl[degm][degn-1])) ) {
        u -= LENGTH_EPS;
    }

    if(EXACT(0.0 == v) && (ctrl[0][0].EqualsExactly(ctrl[1][0])
                       || ctrl[degm][0].EqualsExactly(ctrl[degm-1][0])) ) {
        v += LENGTH_EPS;
    }
    else if(EXACT(1.0 == v) && (ctrl[0][0].EqualsExactly(ctrl[1][0])
                       || ctrl[degm][degn].EqualsExactly(ctrl[degm-1][degn])) ) {
        v -= LENGTH_EPS;
    }

Let me think some more...

I don't like mine - too verbose, too much copy paste. For loops (but they don't particularly bother me, even though I have not tried to profile it).
I don't like yours either - as you said on 3rd degree surfaces it kinda ignores the middle point.

I don't like yours either - as you said on 3rd degree surfaces it kinda ignores the middle point.

I believe the tangent will go to zero at U,V = (0,0) if only 2 points are coincident. Meaning if you have a 3rd order curve with 4 control points (A,B,C,D) where A and B are coincident but also C and D are coincident, you will get a line but the tangent will go to zero at both ends but not in the middle.

All of the fixes proposed for tangents so far will work with the degenerate curves SolveSpace can create today via revolve and lathe. I'm still not happy with the verbosity of mine either. I still think checking the result and doing a retry might be the most visually appealing way to go but I haven't written it to verify. It would make explicit in the code what the condition being tested is and probably less ugly too. Performance of a retry should not be a problem, as it is rare.

I don't like yours either - as you said on 3rd degree surfaces it kinda ignores the middle point.

I believe the tangent will go to zero at U,V = (0,0) if only 2 points are coincident. Meaning if you have a 3rd order curve with 4 control points (A,B,C,D) where A and B are coincident but also C and D are coincident, you will get a line but the tangent will go to zero at both ends but not in the middle.

I agree, but in that case if we are at U,V = (0, 0.2) with A = B != C = D we should not have to increase U. It should be necessary only at (0,0) and (0,1). It probably does not matter if we do it anyway but I do not like "imprecise" solutions when there is an efficient precise/discrete one. What I mean is that there is a truth table of what we should do:

Is this correct? If it is then I will try to find a better looking implementation of it. (B=C? can it cause a zero length tangent?)

All of the fixes proposed for tangents so far will work with the degenerate curves SolveSpace can create today via revolve and lathe. I'm still not happy with the verbosity of mine either. I still think checking the result and doing a retry might be the most visually appealing way to go

Visually appealing - yes - I absolutely agree.

but I haven't written it to verify. It would make explicit in the code what the condition being tested is and probably less ugly too. Performance of a retry should not be a problem, as it is rare.

It is rare if there are no such points in a model. If there are... I just implemented the "numerical" approach here: https://github.com/ruevs/solvespace/commits/NURBSTangentsNumerical
And for the simplest possible test model this code is called 1212 times for ONE generation of the NURBS boolean.

Another thing that I do not like (in both solutions) is that the UV space is 0-1 by nudging u or v by constant LENGTH_EPS (1e-6) we are moving the point in XYZ space by an unknown "real" distance based on the size of the patch (on the other hand we are moving by a constant percentage, which may actually be better).

Also depending on the curvature of the patch close to the "singularity" moving u/v by 1e-6 may or may not make the tangent > TANGENT_ZERO_TOLERANCE (1e-9) in five iterations. Therefore how to choose the values of retry (5) TANGENT_ZERO_TOLERANCE (1e-9) and LENGTH_EPS (1e-6) is unclear to me.

I think we should not choose the good looking (compact and simple source) solution when there is an efficient one in this case. TangentsAt is called at 14 places in the code all of them "inner loops" and very often with U,V 0 or 1. So for a model with degenerate NURB patches it may have an impact.

Closing, since #746 is merged in master and achieves the same.