Fix discussed in #49 for incorrect normals to curves in at least some cases in 2D

[carthurs]

Not sure if this fixes all cases - but it's a start. I don't have tests for the 3D version, so I've just left that as-is. Possibly the wrong sort of derivative was being taken (should be w.r.t. arc length) but I don't know what the derivative code is actually doing. Also second derivatives will never be able to compute the normal of a straight line, so probably a different approach is needed in 3D too. A similar rotation approach is probably possible.

[codecov]

Codecov Report

Merging #50 into devmaster will decrease coverage by 0.03%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##           devmaster      #50      +/-   ##
=============================================
- Coverage      44.98%   44.94%   -0.04%     
=============================================
  Files             36       36              
  Lines           6764     6770       +6     
  Branches        1651     1652       +1     
=============================================
  Hits            3043     3043              
- Misses          3444     3450       +6     
  Partials         277      277

Impacted Files	Coverage Δ
geomdl/_operations.py	32.23% <0%> (-1.69%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8a8b79...74b8a9c. Read the comment docs.

[carthurs]

Actually now I think about it, this method will not follow the curvature as a derivative method should. It's still an improvement on normals that aren't normal, but a more general fix would be better.

[orbingol]

I think the naming convention of the library is a little bit incorrect here.

I guess it seems more logical to add a curvature method to Curve class which returns the curvature vector with the origin point, and change the existing normal method to "normal to the tangent line" (as in your PR). As far as I remember, binormal method is also dependent on these calculations.

The normal method can be limited to 2 and 3 dimensions. I don't think that would be a huge problem.

[orbingol]

I have found a better approach:

1. Find the tangent using t = curve.tangent(u) where u is the parameter value.
2. Set tn = [0.0, 0.0, 1.0]
3. Take cross-product of t and tn, e.g. n = linalg.vector_cross(t[1], tn)

To validate for 2-D, check the angle between t and n: angle = linalg.vector_angle_between(t[1], n[0:-1]). It also validates for 3-D.

This result should work for both 2-D and 3-D.

[carthurs]

I'm surprised that works in 2D, since the cross product isn't defined in 2D. Cross products exist only in 3 and 7 dimensions. I suspect the vector_cross() is doing something hacky here. If not, the fix I suggested will also work for 2D. I agree that your suggestion is fine for 3D.

I think the naming convention of the library is a little bit incorrect here.

"normal" is a perfectly reasonable name for the method - my comment about (in 2D) the normal not following the curvature was that the normal defined in this way might give the negative of the (normalised to unit length) curvature vector. The "normal" method as it previously existed wasn't returning the curvature either - so renaming it to "curvature" shouldn't be done.

[orbingol]

I suspect the vector_cross() is doing something hacky here.

Yes, you are correct :) It is converting 2-D to 3-D by setting z = 0 ref. It may not look mathematically correct but it is good for the programming side (saves the code from additional conditional checks and/or mapping variables).

Considering linalg is mostly designed as a very limited set of internal utility functions but not a perfectly perfect linear algebra library, I guess hacking around a bit is just fine :)

"normal" is a perfectly reasonable name for the method - my comment about (in 2D) the normal not following the curvature was that the normal defined in this way might give the negative of the (normalised to unit length) curvature vector. The "normal" method as it previously existed wasn't returning the curvature either - so renaming it to "curvature" shouldn't be done.

I agree with your statement. The function name will stay as normal but logic is going to be fixed.

How would you like to proceed? Would you like to fix it yourself or if you are busy, I can take care of it. I am planning to release a new version in probably 2 weeks or so, fixing some of the issues I left on my paper notes (and forgot about them). In all cases, I am going to add your name to the list of contributors :)

[carthurs]

Apologies - I've just got back from travelling for a number of conferences. Is this all dealt with now for the next release? I'm happy to help with anything remaining, and of course happy to be included in the contributors - please include me as Chris Arthurs: christopher.arthurs at kcl.ac.uk