Some explanation with a diagram:

In the existing code, the point P is projected on to the tu and tv vectors at points 1 and 4. Normally that's just a dot product and division by the magnitude of the vector, but that's in world space not u,v so we need to divide by the magnitude again which is where the magnitude squared came from (yay no square roots!). And is fine if u and v are orthogonal. In this picture they are not.

What we need is p projected on V at point 3 and on U at point 5. Lets just worry about V for now. By constructing W in the plane and orthogonal to U we get this picture. Point 2 is just P projected on W, but it's the same fraction of W that point 3 is of V but that projection requires the division by |W| and we still need to divide by |v|. The later patch calculates the sin of the angle so we can just divide by |V| squared and the sin.

Finding the fraction of U looks like more work, but since there isn't really a distinction between U and V, we can compute that in exactly the same way using another vector perpendicular to V. So I renamed W -> Y in the code and replicated it all with X for the U calculations. The 3rd patch is just optimization.

This is a really short explanation that may not be very clear. Maybe it will help.

I believe I still made a mistake in all that. If the numbered point are P1-P5, what we need is the ratio |P3| / |V| which is the same as |P2| / |W| I'll think about that for a bit as see if anyone else can weigh in.

We will definitely want to get this right. It looks like similar issue might exist in 3 more functions:

PointIntersectingLine()
ClosestPointOnThisAndSurface()
PointOnSurfaces()

And SSurface::ClosestPointTo() special cases a first order NURBS where the control points for a parallelogram - which would be a bug if it's actually skewed.

I wish this were a major find for NURBS issues, but the way SolveSpace usually creates NURBS surfaces (straight extrusion or lathe) the U and V derivatives should always be orthogonal with the exception of doing lathe or revolve around an out-of-plane axis. Helix will always be affected, as will some of the possible future things.

Pushed another update with a math fix. Also used the new math in 2 more functions. Helical extrusions work dramatically better now. I'd still like to update the other couple places.

There should be 5 commits here. I'm not sure why sometimes it shows 3. AFAIK this is done. I don't want to add the U,V range limiting in other places until I have more test cases.

The math here is over my head, so I'd like @jwesthues to review this.

@phkahler Do I understand correctly that f961a71, 0de8618, c76d373 all fix the same bug in different functions?

@whitequark The initial "fix" worked better than the original but was still not right. Then I "optimized" it prematurely. Then I found the right formula, and the then I fixed it in other functions. If you look at the overall effect of the last 4 commits, it's just the same few lines changed in different places.

At no point was the code apparently worse than the original. Even the wrong fix was equivalent to the original in the typical non-skewed case.

If @jwesthues is going weigh in, I'd like your thoughts on changing the implementation of the function Vector::DivPivoting(). It's used in about 16 places, but I noticed it in ratpoly where trying to find a particular point on a curve. Very similar to what we're doing here finding a point on a surface.

The implementation seem to be avoiding the square root of a projection by selecting an axis to compute the ratio of a vector and "this" vector projected on it. The whole function can be replaced with:

return this->Dot(delta) / delta.Dot(delta);

This is mathematically nicer than what's in there and seems to work fine. I suspect it's faster too because straight-line code vs. branching. I would probably want to expand the dot products to:

`

return (x*delta.x + y*delta.y + z*delta.z)

     / (delta.x*delta.x + delta.y*delta.y +delta.z * delta.z);

`

Which seems more likely to be inlined by the compiler. But then there is the issue of the name. It's not a bad name, but it wouldn't fit the intent with this change. Maybe DivProjDistByLengthOf?

As a math guy and a bit of a micro-optimizer this function kinda triggers me ;-)

Sorry for the delay, traveling. I agree that your DivProjecting() is more pleasing than DivPivoting(). I have no objection to that change, though I'd be surprised/concerned if that changed any result. I haven't deeply reviewed the u-and-v-not-orthogonal change but it seems generally plausible to me, thanks.

I think I found another example that only works when forcing NURBS surfaces to triangle meshes.
I tested it with master and this version and both don't work. Is maybe my approach wrong or do you think its due to bugs?

fillet.zip

@maz3max that seems to be the same problem as issue #315. This PR does not address that.

@whitequark Ping. It looks like there may be question about the first change (limit U,V range)??. If that causes any problems it would likely be with planar surfaces, and we can put conditions around it to only apply to higher order surfaces (or remove it, it's independent of the others). I haven't seen any issues with it though, and neither has @ruevs as far as I know. This is really critical to helix booleans.

@phkahler The reason I haven't merged this yet is that, in a project with long history and many contributors, I expect the commits to be (a) self-contained and complete and (b) have messages that provide a detailed explanation of the change: why was it needed, what was changed, what problem does it solve, what is the full context. This allows anyone in the future who encounters any sort of issue with the changeset to easily see what happened and why. You can go through the git history to see what sort of goal I aim towards.

Now, not everyone tends to make their changesets the same way, and that's fine--normally I carefully review the commit, ask the author if necessary, and add all the missing details as well as merging or splitting commits as necessary. Unfortunately it takes a lot of time and lately I've had less time than I wish for maintenance, which results in delays. Perhaps I simply expect too much.

@whitequark that's fair. I didn't realize you were waiting on me. The ideal is probably to split out the first commit into its own PR to fix #471. The other 4 could be squashed into one with a single message "Change the math for projecting a point onto a plane to work better with non-orthogonal U,V derivatives. Fixes #472.". I'll try to make those changes soonish.