Mitigate abnormally generated areas #989
Conversation
|
Just want to make sure @ikeough doesn't have any objections to this change (and folding it as an additional breaking change into 2.0) |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)
Elements/src/Geometry/Polygon.cs line 2169 at r1 (raw file):
/// Calculate the polygon's area in 3D. /// </summary> /// <returns>A double representing the signed or absolute value of this Polygon's area.</returns>
missing a comment for the signed parameter.
There was a problem hiding this comment.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)
Elements/src/Geometry/Polygon.cs line 2169 at r1 (raw file):
Previously, andrewheumann (Andrew Heumann) wrote…
missing a comment for the
signedparameter.
Done.
There was a problem hiding this comment.
Make sure to update the "changed" section of CHANGELOG.md with something like "Polygon.Area() now returns an unsigned area by default, and accepts a signed parameter to preserve the previous behavior."
Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)
Elements/src/Geometry/Polygon.cs line 2169 at r1 (raw file):
gonna be fussy and propose:
"If true, a counter-clockwise polygon will yield a positive area, while a clockwise polygon will give a negative area." or something like that. but LGTM!
There was a problem hiding this comment.
Just a ping, I think we want to get this in for the 2.0 release
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)
There was a problem hiding this comment.
Whoops! Added comments and change log note
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)
Elements/src/Geometry/Polygon.cs line 2169 at r1 (raw file):
Previously, andrewheumann (Andrew Heumann) wrote…
gonna be fussy and propose:
"If true, a counter-clockwise polygon will yield a positive area, while a clockwise polygon will give a negative area." or something like that. but LGTM!
Done.
…com/hypar-io/Elements into mitigate-abnormally-generated-areas
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 1 of 1 approvals obtained
There was a problem hiding this comment.
Some tests are failing. There may have been code in the library that depended on the signed area behavior which is now breaking.
Reviewable status: 1 change requests, 0 of 1 approvals obtained
There was a problem hiding this comment.
Also, there are many places in the code that call Math.Abs({something}.Area()). The .Abs calls are no longer needed and may be removed.
Reviewable status: 1 change requests, 0 of 1 approvals obtained
Volume of boolean subtracts area of inner polygon from outer.
…com/hypar-io/Elements into mitigate-abnormally-generated-areas
There was a problem hiding this comment.
Went through the tests and found that one of the Mesh Volume tests anticipated an area to be signed negative as it was a boolean mesh. The test itself is validating whether or not the Mesh.Volume() value is accurate, what are your thoughts on including another test for Polygon.Area(true) for signed areas?
Went through the code and removed Math.Abs() that wrapped Polygon.Area()
Reviewed 2 of 2 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained
There was a problem hiding this comment.
var c = Polygon.Rectangle(3.0, 2.0);
Assert.Equal(-6.0, c.Reversed().Area(true));
Reviewable status: 1 change requests, 0 of 1 approvals obtained
There was a problem hiding this comment.
Feel free to add that test if you feel like it! but this
Reviewed 6 of 7 files at r4, all commit messages.
Reviewable status:
BACKGROUND:
DESCRIPTION:
TESTING:
FUTURE WORK:
REQUIRED:
CHANGELOG.md.COMMENTS:
This change is