compas icon indicating copy to clipboard operation
compas copied to clipboard

Polygon.centroid()

Open mattiskoh opened this issue 4 years ago • 9 comments

Describe the bug Polygon.centroid returns a strange value for a polygon with 4 points that is self interesecting (see image), image

Using the same points (of the polygon), but cycled differently we get a polygon that is more conventional... image

To Reproduce Steps to reproduce the behavior:

  1. Python script:
from compas.geometry.primitives import Polygon, Point
polygon1 = Polygon([Point(0.000, -5.536, 1977.967), Point(0.000, 144.352, -0.000), Point(0.000, -148.968, -0.000), Point(0.000, 264.624, 1977.967)])
polygon2 = Polygon([Point(0.000, 144.352, -0.000), Point(0.000, 264.624, 1977.967), Point(0.000, -5.536, 1977.967), Point(0.000, -148.968, -0.000)])

print(polygon1.centroid)
print(polygon2.centroid)
>>>>Point(0.000, -471.050, -7031.803)
>>>>Point(0.000, 62.715, 975.434)

Expected behavior I would expect them to have the same centroid.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: Windows
  • Python version 3.8.6
  • Python package manager conda
  • Compas version 1.0.0

mattiskoh avatar Feb 25 '21 10:02 mattiskoh

nice investigation :) will have a look...

tomvanmele avatar Feb 25 '21 12:02 tomvanmele

maybe briefly, just FYI, the recommended way to import compas objects is to always to this from "the second level", which is where the public API is defined. these imports should never change. below that level we sometimes reshuffle things if that is necessary. so in this case the recommended option is

from compas.geometry import Polygon, Point

tomvanmele avatar Feb 25 '21 12:02 tomvanmele

maybe briefly, just FYI, the recommended way to import compas objects is to always to this from "the second level", which is where the public API is defined. these imports should never change. below that level we sometimes reshuffle things if that is necessary. so in this case the recommended option is

from compas.geometry import Polygon, Point

Thanks for the heads-up. I used to do it "correctly", but the problem for me was that I could never get VSCode to properly auto-complete... I remember discussing this extensively with @yck011522 and @gonzalocasas a while back.

mattiskoh avatar Feb 25 '21 12:02 mattiskoh

maybe briefly, just FYI, the recommended way to import compas objects is to always to this from "the second level", which is where the public API is defined. these imports should never change. below that level we sometimes reshuffle things if that is necessary. so in this case the recommended option is

from compas.geometry import Polygon, Point

Thanks for the heads-up. I used to do it "correctly", but the problem for me was that I could never get VSCode to properly auto-complete... I remember discussing this extensively with @yck011522 and @gonzalocasas a while back.

yes, i know, especially with PyLance this is indeed a problem. we are working on that...

tomvanmele avatar Feb 25 '21 12:02 tomvanmele

about the centroid... the issue is related to how the centroid of the polygon object is calculated.

from compas.geometry import Polygon, Point
from compas.geometry import centroid_points, centroid_polygon

polygon1 = Polygon([Point(0.000, -5.536, 1977.967), Point(0.000, 144.352, -0.000), Point(0.000, -148.968, -0.000), Point(0.000, 264.624, 1977.967)])
polygon2 = Polygon([Point(0.000, 144.352, -0.000), Point(0.000, 264.624, 1977.967), Point(0.000, -5.536, 1977.967), Point(0.000, -148.968, -0.000)])
  1. using the method of the object
print(polygon1.centroid)
print(polygon2.centroid)
  1. using centroid_points
print(centroid_points(polygon1))
print(centroid_points(polygon2))
  1. using centroid_polygon
print(centroid_polygon(polygon1))
print(centroid_polygon(polygon2))

the output of 1 and 3 is the same, but i guess you were expecting the outcome of 2 :)

tomvanmele avatar Feb 25 '21 12:02 tomvanmele

the polygon object uses centroid_polygon in the background to calculate the centroid, which considers the actual polygon surface rather than the corner points to avoid skewed results when there are many points on the boundary that are potentially clustered together and change the location of the centroid. but this method only makes sense if the polygon is not self-intersecting.

unfortunately (apparently), we have not yet established full equivalence between the older functional approach and the newer oo system. this is something that should be resolved in one of the upcoming releases...

tomvanmele avatar Feb 25 '21 12:02 tomvanmele

Ok thanks for the clarification! I indeed expected the outcome of 2. I also just looked into the api ref. of the centroid_polygon function and there is a big disclaimer for self-intersecting polygons :) so... feel free to close to the issue !

image

mattiskoh avatar Feb 25 '21 12:02 mattiskoh

will leave it open as a reminder that the current implementation can lead to unexpected results :)

tomvanmele avatar Feb 25 '21 12:02 tomvanmele

@gonzalocasas needs to be discussed in the next dev meeting...

tomvanmele avatar Feb 25 '21 12:02 tomvanmele