Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add midpoint to circular and elliptical arcs #115

Merged
merged 4 commits into from
Jan 5, 2020
Merged

Add midpoint to circular and elliptical arcs #115

merged 4 commits into from
Jan 5, 2020

Conversation

w0rm
Copy link
Contributor

@w0rm w0rm commented Jan 4, 2020

Closes #51

  • midpoint for Arc2d and Arc3d take their respective types, because parametrisation for those doesn't exist.
  • The test for EllipticalArc2d was flaky due to precision, I reduced it to 1.0e-9 and ran quite a few times, seems to always pass for me.

@ianmackenzie
Copy link
Owner

Looks good! I think the EllipticalArc2d test was getting occasional failures because the arc length parameterization is only guaranteed to be accurate to within the maxError specified when constructing it (in this case, meters 1.0e-3).

I think it would be most correct for the point2dWithin tolerance to be equal to the arc length parameterization tolerance; maybe both 1e-3 or perhaps both 1e-6 (some reasonably small tolerance that still lets the tests run quickly - the arc length parameterization is more expensive to construct if the maxError is smaller).

@w0rm
Copy link
Contributor Author

w0rm commented Jan 4, 2020

Changed the precision to be 1e-3. 1e-6 didn't work because of FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

@ianmackenzie
Copy link
Owner

Huh, that's a little bit concerning that we run out of memory when trying to get an arc length parameterization accurate to 1e-6 meters on what's probably a several-meter-long elliptical arc...I mean that's a pretty high precision, but I would have thought it would be possible. Something to look into, but not worth worrying about as part of this PR.

One very minor thing I forgot to mention earlier: I think Quantity 1.0e-3 in the test could just be meters 1.0e-3, which would be more consistent, more type safe and would avoid needing the import line change.

@ianmackenzie
Copy link
Owner

Looks good! Just one more thing I just remembered - add yourself to the AUTHORS file =)

@ianmackenzie ianmackenzie merged commit 72db006 into ianmackenzie:master Jan 5, 2020
@w0rm w0rm deleted the midpoint branch January 5, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'midpoint' functions to curve types
2 participants