Elements icon indicating copy to clipboard operation
Elements copied to clipboard

Polygon Area calculation returns negative

Open alvpickmans opened this issue 5 years ago • 6 comments

Describe the bug When polygon's vertices are defined clockwise, the area calculation returns negative value.

To Reproduce

using Elements.Geometry;
using NUnit.Framework;
using System.Linq;

namespace Tests
{
    public class Tests
    {

        [Test]
        public void PolygonAreaTest()
        {
            double side = 20;
            Vector3[] vertices = new Vector3[4]
            {
                new Vector3(0,0,0),
                new Vector3(side, 0, 0),
                new Vector3(side, side, 0),
                new Vector3(0, side, 0)
            };

            Polygon ccw = new Polygon(vertices);
            Polygon cw = new Polygon(vertices.Reverse().ToArray());

            double expected = side * side;

            // This passes
            Assert.AreEqual(expected, ccw.Area());

            // This fails
            Assert.AreEqual(expected, cw.Area());
            Assert.IsTrue(ccw.Area() == cw.Area());
        }
    }
}

alvpickmans avatar Jul 04 '19 11:07 alvpickmans

Polygon area fix #167 pending review.

anthonyhauck avatar Jul 05 '19 17:07 anthonyhauck

@alvpickmans The fix should be in 0.3.2. Please let us know if it resolves your issue.

ikeough avatar Jul 09 '19 16:07 ikeough

@ikeough sadly the unit test above still fails. I think the issue is on the formula that calculates the area, as it assumes a counter-clockwise vertex order(?). A simple fix would be to return the absolute value, but unsure if it will have a side effect somewhere else. https://github.com/hypar-io/Elements/blob/master/src/Elements/Geometry/Polygon.cs#L596 Sketch

alvpickmans avatar Jul 09 '19 18:07 alvpickmans

That is true. Polygon winding is assumed to be CCW. The simple fix would be nice :), but it would most likely break other area calculations. As "holes" are represented with CW winding. Having the ability to calculate an aggregate area for a perimeter and some holes simply by summing the result of each Area() is nice.

A more elaborate fix would be to provide a method which identifies the winding of a polygon. Then the user can make a determination what to do with the result of the area calculation. I'm guessing that your system uses CW winding for polygons?

ikeough avatar Jul 11 '19 16:07 ikeough

I get your point, but I wouldn't agree on having the design/behaviour of a high level class like Polygon depending on a particular implementation preference for a lower level class like Perimeter or Floor. I'm sure it's easier to understand that a perimeter's area is the outer polygon's area minus the sum of the holes than adding an extra method call to define if a polygon is CCW or CW.

Besides of the point that a polygon with negative area doesn't make much sense in geometry 😅

In our case, the polygon winding is defined by the user when drawing the polygon geometry, so at the moment we need to always return the absolute value of the area.

alvpickmans avatar Jul 11 '19 21:07 alvpickmans

The comment of the Polygon.Area method should indicate that negative values will be returned for CW wound polygons.

ikeough avatar Apr 17 '23 17:04 ikeough