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 intersectionWithSphere function #128

Merged
merged 5 commits into from
Feb 23, 2020

Conversation

SuzzzWood
Copy link
Contributor

Added intersectionWithSphere function
Maths from wikipedia

tv2

@ianmackenzie
Copy link
Owner

Looks good! (And don't worry about the Travis build failure - I checked and it's an unrelated test that's failing, one that fails occasionally and I have to look into at some point but not related to any of this work.)

I was going to make a couple minor tweaks but I'm not sure I have write access to your axis-sphere-intersection branch - is the "Allow edits from maintainers" box checked? The changes are basically:

  • Destructure values using (e.g.) (Types.Vector3d cto) = ... instead of (Vector3d cto) = ... - this is the pattern I've used elsewhere, primarily because it seems weird to both expose the Axis3d type from the Types module and have an Axis3d type alias in the Axis3d module itself
  • Use a couple functions to simplify the code: Vector3d.componentIn instead of dot to avoid the awkward direction/vector conversion, and Point3d.along to simplify the final point construction

@SuzzzWood
Copy link
Contributor Author

@ianmackenzie thanks for the feedback! Yes, 'Allow edits from maintainers' is checked, feel free to edit

@ianmackenzie
Copy link
Owner

Just added a fuzz test for axis/sphere intersection - everything passes, so this looks good!

One last change: could you add yourself to the AUTHORS file? =)

@SuzzzWood
Copy link
Contributor Author

Nice! Have added, thanks

@ianmackenzie ianmackenzie merged commit 6c6158e into ianmackenzie:master Feb 23, 2020
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.

2 participants