hypar-io / Elements

The smallest useful BIM.
https://www.hypar.io
MIT License
349 stars 74 forks source link

Arc.Polyline doesn't work if its domain has negative length. #994

Closed DmytroMuravskyi closed 1 year ago

DmytroMuravskyi commented 1 year ago

It Arc is created with start angle bigger than end angle - it's domain is negative. Start is bigger than End. Arc.ToPolyline tried to iterates from Min to Max and immediately leaves the loop with result always consist of only 1 point.

To Reproduce Steps to reproduce the behavior: Run following code:

var arc1 = new Arc(Vector3.Origin, 2.0, 0.0, -90.0);
var p = arc1.ToPolyline();

Expected behavior Polyline with 11 points created.

Desktop (please complete the following information):

Additional context Negative domain is considered valid in Elements. There are to options here:

wynged commented 1 year ago

There is a third option.
When a decreasing angle is provided, we should flip the basis circle (e.g. -Z normal) and make the angle increasing (but preserve the user's thought about "start" "end" in the parameterization — IOW, second parameter angle corresponds to "end", always.)

So:
in Arc constructors, if the user supplies a decreasing angle range, flip the order to be increasing and flip the basis curve direction to preserve the user's intended direction

Resulting behavior should follow this diagram. image

@andrewheumann is the real expert though, and should review the solution. :)

DmytroMuravskyi commented 1 year ago

After some tries, it's not possible to implement what is shown because Arc parameter space is not 0 => Length but tied to the circle: first one is 0 to pi/2, second is 0 to -pi/2 or indeed 0 to pi/2 if normal is reversed. But third one is pi/2 to 0 in original direction or 3pi/2 to 2pi in reverse. Flipping Z or Transform is also have side effect of flipping X or Y axis as well to preserve correct hand rule I think this is too confusing. It's either consistent domain and simple maintenance or more flexibility but some risks. I did a PR with support of negative domain and fixes for Arc functions that were not consistent: https://github.com/hypar-io/Elements/pull/998