Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Fix area for higher dimensions #182

Merged
merged 9 commits into from
Oct 18, 2019
Merged

Conversation

CarpeNecopinum
Copy link
Contributor

While the signature of area(contour) accepts points of any dimension as polygon to compute the area of, the implementation doesn't generalize to dimensions larger than 2.

In particular, computing the area of a 3d polygon returns a 3d point as "area" (due to the cross-product returning a vector rather than a scalar for 3d):

> area(rand(GeometryTypes.Point3f0, 3))
3-element Point{3,Float32} with indices SOneTo(3):
 -0.15032549
  0.1441128 
 -0.0745391

This PR thus constrains the existing implementation to 2d points and implements a variant of the algorithm that works for (approximately planar) 3d polygons as well (for non-planar polygons the area depends on the triangulation, so there is no general answer).

I also added 2 test cases.

@CarpeNecopinum
Copy link
Contributor Author

CarpeNecopinum commented Oct 16, 2019

Wondering how I managed to seemingly decrease test coverage by submitting a fully covered method and a test for a previously non-covered method. ¯\(ツ)/¯ Hope that isn't a deal-breaker.

@SimonDanisch
Copy link
Member

Looks good to me :) @sjkelly, any comments?!

@CarpeNecopinum CarpeNecopinum changed the title Fix area for higher dimensions WIP: Fix area for higher dimensions Oct 16, 2019
@CarpeNecopinum
Copy link
Contributor Author

CarpeNecopinum commented Oct 16, 2019

Just noticed my 3d variant can't do non-convex polygons, so I WIPed this PR until I get around to fixing that and adding a test for it. Okay, done.

@CarpeNecopinum CarpeNecopinum changed the title WIP: Fix area for higher dimensions Fix area for higher dimensions Oct 17, 2019
src/polygons.jl Outdated Show resolved Hide resolved
src/polygons.jl Outdated Show resolved Hide resolved
test/polygons.jl Show resolved Hide resolved
@sjkelly sjkelly merged commit c42a2b3 into JuliaGeometry:master Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants