hypar-io / Elements

The smallest useful BIM.
https://www.hypar.io
MIT License
349 stars 74 forks source link

Polygon Area calculation returns negative #166

Open alvpickmans opened 5 years ago

alvpickmans commented 5 years ago

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());
        }
    }
}
anthonyhauck commented 5 years ago

Polygon area fix #167 pending review.

ikeough commented 5 years ago

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

alvpickmans commented 5 years ago

@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

ikeough commented 5 years ago

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?

alvpickmans commented 5 years ago

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.

ikeough commented 1 year ago

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