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

Ellipsoid3d #142

Merged
merged 14 commits into from
Nov 6, 2020
Merged

Conversation

g-belmonte
Copy link
Contributor

@g-belmonte g-belmonte commented Oct 18, 2020

Pull request related to #135

Pull request checklist:

  • Is the pull request from a dedicated branch in your fork instead of
    master?
  • Have you added yourself to the AUTHORS file?
  • Is "Allow edits from maintainers" enabled?
  • Is the code formatted using the same version of elm-format as the rest of
    elm-geometry?
  • Is code (mostly) wrapped to 80 columns?
  • BONUS POINTS: Have you added tests for new functionality?
  • EXTRA BONUS POINTS: Have you added documentation for new functionality?

Proposed API:

  • Ellipsoid3d type

Constructors

  • with

Properties

  • centerPoint
  • axes
  • xAxis
  • yAxis
  • zAxis
  • xDirection
  • yDirection
  • zDirection
  • xRadius
  • yRadius
  • zRadius
  • volume
  • surfaceArea
  • boundingBox

Queries

  • contains

Measurement

  • signedDistanceAlong

Transformations

  • scaleAbout
  • rotateAround
  • translateBy
  • translateIn
  • mirrorAcross

Unit conversions

  • at
  • at_

Coordinate conversions

  • relativeTo
  • placeIn

@ianmackenzie
Copy link
Owner

Just did a brief skim but looking pretty good so far!

@g-belmonte
Copy link
Contributor Author

@ianmackenzie take a look at the formula for the surface area: wikipedia or wolfram
The analytical solution seems to require elliptic integrals. Should I try to implement it like this, or should I go for the approximate solutions?

@ianmackenzie
Copy link
Owner

Oof didn't realize that, I would just leave it out then =) I don't think it's a particularly important operation, just for spheres it's so simple it seemed silly not to include it.

@g-belmonte
Copy link
Contributor Author

@ianmackenzie the code is mostly done. We're missing top-level docs and tests.
About the tests, is there anything special you want tested? I was thinking on covering basic usage of all public functions. Wdyt?

@ianmackenzie
Copy link
Owner

Hey @g-belmonte, sorry for the delay! This one took a bit of thought 🙂 Here are some cool and useful tests I can think of:

  • If you apply a random translateBy, rotateAround, scaleAbout, mirrorAcross, placeIn or relativeTo to an Ellipsoid3d, and the matching transformation to a Point3d, do you get the same result from Ellipsoid3d.contains before and after? For example if you have a random ellipsoid and a random point and you both move them the same amount, then if the point is contained in the ellipsoid before the transformation it should also be contained in the ellipsoid after both are transformed, and vice versa.
  • Same for signedDistanceAlong - if you apply a particular transformation to an ellipsoid and the same transformation to an Axis3d then you should get (within roundoff error) the same signed distance range before and after.
  • If you scale an ellipsoid about an arbitrary point by a scale factor k, does the volume increase by a factor of k^3?

For the first couple of tests, you can check out modules like Tests.Generic.Curve3d and tests like Tests.CubicSpline3d which use it - there are a bunch of similar tests like "if I evaluate a curve at t=0.5 and then rotate the point, it should be the same as rotating the curve and then evaluating the rotated curve at t=0.5" (which tests both curve evaluation and rotation).

Let me know if that makes sense!

@g-belmonte
Copy link
Contributor Author

Yes, it does make sense! I'll work on it 😁

@ianmackenzie
Copy link
Owner

One more thought, you could probably make a couple tests based on an Ellipsoid3d with 3 equal radii being equivalent to a Sphere3d with that radius...volume should be the same, point containments results should be the same, etc. But only if you have time and it seems interesting, I don't think those are mandatory 🙂

@g-belmonte
Copy link
Contributor Author

@ianmackenzie this was the very first test I added 😁 I also wanted to compare an Ellipsoid3d having 2 equal radii with an Ellipse3d, but the latter doesn't have a volume function 😅

I had little time the last days, so I'll try to finish the tests today :)

@g-belmonte
Copy link
Contributor Author

I'm getting this message while trying to implement Fuzz.ellipsoid3d:
image

Should I create a new Fuzz.frame3d without local coordinates, or should I tweak the type of Ellipsoid3d?

@g-belmonte
Copy link
Contributor Author

Docs and tests added. The Fuzzer thing I solved by adding the globalFrame3d to the Geometry.Fuzz module, but I haven't exposed it.

@ianmackenzie
Copy link
Owner

Haven't looked through everything in detail but I think a better solution for the frame type issue would be to have Ellipsoid3d.with accept an arbitrary Frame3d units coordinates defines argument instead of a Frame3d units coordinates {}, and then use Frame3d.copy inside the constructor to make the types work out (see the implementation of Rectangle2d.centeredOn as an example). Then I think you should be able to drop the globalFrame3d fuzzer and just use frame3d.

@g-belmonte
Copy link
Contributor Author

@ianmackenzie done! 😄

I think that the PR is now ready for a thorough review. Let me know if anything!

src/Ellipsoid3d.elm Outdated Show resolved Hide resolved
@ianmackenzie
Copy link
Owner

I think this looks good now - thanks for all your work on this!

@ianmackenzie
Copy link
Owner

Anything else you were planning to do before merging? I was about to just merge but then I noticed the title was still set to 'WIP' =)

@g-belmonte g-belmonte changed the title WIP: Ellipsoid3d Ellipsoid3d Nov 6, 2020
@g-belmonte
Copy link
Contributor Author

@ianmackenzie Oh, I forgot about the WIP! You can proceed with merging :)

@ianmackenzie ianmackenzie merged commit 9cb556f into ianmackenzie:master Nov 6, 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