Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polygon.centroid() #777

Open
mattiskoh opened this issue Feb 25, 2021 · 9 comments
Open

Polygon.centroid() #777

mattiskoh opened this issue Feb 25, 2021 · 9 comments

Comments

@mattiskoh
Copy link
Contributor

mattiskoh commented Feb 25, 2021

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
@tomvanmele
Copy link
Member

nice investigation :)
will have a look...

@tomvanmele
Copy link
Member

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

@mattiskoh
Copy link
Contributor Author

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.

@tomvanmele
Copy link
Member

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
Copy link
Member

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
Copy link
Member

tomvanmele commented Feb 25, 2021

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...

@mattiskoh
Copy link
Contributor Author

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

@tomvanmele
Copy link
Member

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

@tomvanmele
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants