Polygon2d.centroid

[w0rm]

Implemented polygon centroid according to the formula from here. Closes #64.

I wonder if the approach taken here is correct?

I haven't yet implemented the calculation relative to the first vertex. If the polygon is far from the origin, the precision may be bad.

[w0rm]

Tests pass for me locally, I wonder if there is a problem with ci.

[ianmackenzie]

Yeah, that was unlucky timing, I broke CI briefly today - I'm doing some vacation hacking on a laptop that doesn't have Node (and therefore doesn't have elm-test) so I've been just kind of committing the test files blindly and letting Travis tell me a few minutes later if there are any issues 😬

I think things should be fixed now, so maybe merge master into your PR and see if things work?

[w0rm]

@ianmackenzie I rebased with master so it works now!

Wonder if this approach looks good to you? Then I can add an offset relative to the first point to improve precision.

[ianmackenzie]

On my phone so I haven't fully looked through in detail, but I think there might be an issue in centroidHelp if there's a degenerate loop (0 or 1 points) in the middle of the list of loops - in that case centroidHelp will immediately return a value without processing the remaining loops, correct? I think you might need to add a separate pattern-matching level to first match number of remaining loops and then check to see if the first loop is degenerate.

[ianmackenzie]

That would also mean you wouldn't have to reconstruct the first loop using first :: second :: remainingPoints, since you'd already have it as a single variable - probably a very small efficiency improvement but an improvement nonetheless 🙂

[w0rm]

@ianmackenzie good catch, will work to fix this

[w0rm]

@ianmackenzie I addressed this issue! It made me think about loops with only two points, but that is actually fine, because that doesn't change the sum, i.e. for xSum we first add p1 -> p2: (p1.x + p2.x) * (p1.x * p2.y - p2.x * p1.y) and then add p2 -> p1: (p2.x + p1.x) * (p2.x * p1.y - p1.x * p2.y), so the total increase will be 0.

[ianmackenzie]

Yeah, definitely makes sense to ignore degenerate loops with 0 or 1 points - I don't think you need to go into the math to see that a 'loop' with 0, 1 or frankly even 2 points has zero area and therefore zero 'mass' =)

I think the rest of the code looks good, but would you be willing to add one more test that uses the existing triangulation code to check the centroid calculation? It should be possible (although much more expensive than your implementation!) to compute the centroid of a polygon by triangulating the polygon and then calculating the weighted centroid of all the resulting triangles. It should be possible to test that with something similar to the existing triangulationHasCorrectArea test, although you'll have to write a bit of code to do the weighted centroid calculation; instead of the existing

Quantity.sum (List.map Triangle2d.area triangles)

you'll probably have to have something like (untested)

triangles
    |> List.map
        (\triangle ->
            Vector2d.from Point2d.origin (Triangle2d.centroid triangle)
                |> Vector2d.scaleBy (Area.inSquareMeters (Triangle2d.area triangle))
        )
    |> List.foldl Vector2d.plus Vector2d.zero
    |> Vector2d.scaleBy
        (1 / Area.inSquareMeters (Quantity.sum (List.map Triangle2d.area triangles)))

[ianmackenzie]

BTW just added a few more issues for functions that I realized would be useful as I was writing that sample weighted-centroid code - #120, #118 and the tangentially-related #119 =)

[w0rm]

@ianmackenzie added the test!

[ianmackenzie]

Thanks, looks great! Definitely an improvement on my hacky sample code =)

Do you want to look at tweaking the calculation to use coordinate values relative to the first vertex? I think it should just end up meaning adding a couple extra Float arguments (x0 and y0) to centroidHelp and then carrying those through everywhere, subtracting them when doing the area calculations and adding them back for the final result.

[w0rm]

@ianmackenzie sure, done in https://github.com/ianmackenzie/elm-geometry/pull/117/commits/00770899c0e2b68eec10aefda374349214902900

[ianmackenzie]

Cool, looks good! I think this is ready to go then - OK to merge?

[w0rm]

@ianmackenzie sure, let’s merge!

[ianmackenzie]

Coordination is definitely faster when we're in the same time zone, too bad I'm flying back to Toronto on Thursday morning 😛