ianmackenzie / elm-geometry

2D/3D geometry package for Elm
Mozilla Public License 2.0
183 stars 26 forks source link

Adds a constructor for regular polygons #148

Closed gampleman closed 3 years ago

gampleman commented 3 years ago

Adds a constructor for regular polygons that are aligned with the x axis.

ianmackenzie commented 3 years ago

Couple small comments above but I think this looks good!

ianmackenzie commented 3 years ago

Argh I thought I had left a comment about API design here but it looks like it got lost somehow - I think we talked about switching to a record argument and supporting fromCircumcircle and fromIncircle as well? For the purposes of argument, what do you think about:

Polygon2d.regular :
    { numSides : Int
    , centerPoint : Point2d units coordinates
    , circumradius : Quantity Float units
    }
    -> Polygon2d units coordinates

Polygon2d.fromCircumcircle : 
    Circle2d units coordinates
    -> Int 
    -> Polygon2d units coordinates

Polygon2d.fromIncircle : 
    Circle2d units coordinates
    -> Int 
    -> Polygon2d units coordinates
gampleman commented 3 years ago

I believe we agreed to do the first, then leave the rest for later.

With regard to record vs. positional, I don't have any strong opinion. Since all the arguments have distinct types, I don't think the record particularly adds much value and so perhaps is just unnecessary verbose? But I could definitely swing either way.

fromCircumcircle is super straightforward and is just a wrapper around regular. It kind of makes me think you would want to have only one of these.

fromIncircle is a little trickier, so perhaps we would want to have that?

(Also I wonder if the names should be regularFromIncircle to emphasise that this is construction of regular polygons, rather than say an approximation of circle)

ianmackenzie commented 3 years ago

Oops, forgot to get back to this - I think I kind of like the record argument for explicitness/clarity (you can't mess up the order, but I think when reading the code it would be nice to have that confirmation that for example the Quantity value is in fact the circumradius and not the length of one side or something).

And even fromIncircle isn't that bad, I think you'd just end up doing something like

Polygon2d.regular
    { numSides = n
    , centerPoint = p
    , circumradius = r |> Quantity.divideBy (cos (pi / toFloat n))
    }

So I'm tempted to say let's just switch regular to use a record and then merge this - we can always add regularFromIncircle later if it turns out to be a common use case.

ianmackenzie commented 3 years ago

Looks good! OK to merge?

gampleman commented 3 years ago

Sure, go ahead.