BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 13 forks source link

Geometry_Engine: Let Normal() calculate normals for open curves #2184

Closed LMarkowski closed 3 years ago

LMarkowski commented 3 years ago

Broken rules:

Inconsistent handling of checks in Normal() methods. (See #2180)

Suggestions to restore compliance:

Let Normal() calculate normals for open curves.

LMarkowski commented 3 years ago

@pawelbaran, moving your comment here to keep the discussion in one place:

Concerning the open curves, I would opt for being a bit cautious: what does it mean for an open curve to be clockwise or counterclockwise? What does it mean for a line? For a polyline consisting of 2 almost collinear segments? As far as I see it, the best Normal of an open curve we can give to the user is FitPlane()?.Normal, which will only give a false impression that there is some intelligence in it (plus it can be easily done using the existing functionality).

What do others think?

LMarkowski commented 3 years ago

I'll also put my second thoughts here:

As far as I know a normal of a curve doesn't exist mathematically. We calculate them for closed curves but only because we perceive them as potential panels. I'm not sure if there is any practical nor mathematical reason for calculating them for open curves. Especially if a determined user have an easy workaround via FitPlane().

Of course we can allow for it just for a sake of handling edge cases as we did with self-intersecting ones. 🤷‍♂️

@al-fisher @IsakNaslundBh what do you guys think?

al-fisher commented 3 years ago

Hey @LMarkowski - I think the problem here comes from how we refer to a "Single Normal" - an open curve does have mathematically defined normal(s) in the general case. However simply calling it "Normal" is ambiguous. As there a couple of usefully defined vectors.

Two issues:

Good explanation in Struik's Lectures on Classical Differential Geometry pg. 15-19 (which I just plucked off my book shelf to refresh my memory on terminology 😄 ) Also found here a potentially useful link I think https://pages.mtu.edu/~shene/COURSES/cs3621/NOTES/curves/normal.html

So as per discussion here: https://github.com/BHoM/BHoM_Engine/pull/2180 and current code:

Firstly good to continue to return null for Normal() for non-planar curves as an additional argument specifying position would in any case be needed in future to allow this. Not for now though. See below.

Then: If the curve is Planar - then a single normal can be defined. Obviously normal to the plane the curve is lying in. Could be useful, and as you say similar to the FitPlane() result. But good to be precise about terminology. As per above this is the Osculating Plane. And therefore this Normal would be called the BiNormal in the general case I think. With then direction specified by orientation along with the curve tangent direction (for planar could be generalised to vector between start and end points) And the Principle Normal - which points to the centre of curvature.

This obviously points to the other exception that both Normals are undefined for a straight line which we would catch and give a suitable error.

Therefore: Wondering if fine until such a time to return a message if passing an Open ICurve to Normal() to say a single Normal is not unambiguously definable for open curves.

But then would be happy to return say the BiNormal in the future with a note saying that is what is being done.

Wonder if the above is clear?! Worth implementing at a point where we define a need 😄

pawelbaran commented 3 years ago

Thanks @al-fisher for bringing in a scientific point of view 👍 All above makes sense to me, and I would agree that any further development should happen at a point where an actual need is defined. I would say that our current implementation of a normal query seems to be fitting the practical purpose just fine, while loosening the strict requirements (closed, planar curve) could lead to ambiguity that could be defended from mathematical perspective, but confusing for hands-on engineering applications.

LMarkowski commented 3 years ago

Thanks @al-fisher for this extensive reminder 😃 I was so focused on getting a single normal from a curve that I forgot about normals and binormals.

So in general our Normal() returns what mathematically should be called "binormal". 🤔 I agree with @pawelbaran that current functionality is most suitable for engineering needs.

For closing that one I'd go with leaving the implementation as it is today and adding proper descriptions explaining what exactly is being returned. If there will be a need we can later add Normal/Binormal at Point/Parameter in a similar fashion as in TangentAt* methods.