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

Adds a constructor for regular polygons #148

Merged
merged 5 commits into from
Apr 8, 2021

Conversation

gampleman
Copy link
Contributor

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

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

Couple small comments above but I think this looks good!

Have start point be right end of bottom edge and end point be left end of bottom edge (assuming Y-up)
@ianmackenzie
Copy link
Owner

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
Copy link
Contributor Author

gampleman commented Mar 31, 2021

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
Copy link
Owner

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
Copy link
Owner

Looks good! OK to merge?

@gampleman
Copy link
Contributor Author

Sure, go ahead.

@ianmackenzie ianmackenzie merged commit fdb7f10 into ianmackenzie:master Apr 8, 2021
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