Refactor Polyline
#339
Refactor Polyline
#339
Conversation
Updated some tests. GHDebug updated.
sonomirco
added
the
refactor 🧰
src/GShark/Geometry/Polyline.cs
Outdated
| @@ -49,7 +48,7 @@ public Polyline(IList<Point3> vertices) | |||
| /// <summary> | |||
| /// Gets the middle point of the polyline. | |||
| /// </summary> | |||
| public Point3 MidPoint => PointAt(0.5); | |||
| public Point3 MidPoint => PointAtLength(0.5, true); | |||
There was a problem hiding this comment.
Doesn't make sense to be point at length and use a value between 0 and 1.
Maybe would be more clear to name it PointAtNormalizedLength
There was a problem hiding this comment.
So NormalizedLengthAt answers this comment.
There was a problem hiding this comment.
Effectively you are returning a point at a normalized length or a point at a "geometrical" length.
From a method called NormalizedLengthAt, I am expecting a length value given a normalized parameter.
There was a problem hiding this comment.
Not a problem to change the name in normalizedLengthAt
| /// <summary> | ||
| /// Evaluates the point on the polyline at the given parameter. The integer part of the parameter indicates the index of the segment. | ||
| /// </summary> | ||
| /// <param name="t">Evaluate parameter. Parameter should be between 0.0 and segments count.</param> |
There was a problem hiding this comment.
To me using the parameter this way doesn't really make sense. I know is mathematically or geometrically correct but I would prefer to see always a length or a normalized length (0..1), regardless of the type of curve we are working on.
There was a problem hiding this comment.
NormalizedLengthAt solve this problem.
What type of PR is this? (check all applicable)
Description
This PR refactors
Polylineremoving unnecessary methods, and implementing some required byICurve.Related Tickets & Documents
This PR closes #302
Added tests?
Added to documentation?