CHANGE triangulation_earclip #1253

petrasvestartas · 2024-01-09T15:52:41Z

The change of compas\src\compas\geometry\triangulation_earclip.py code to a new one.
The issue with the current algorithm is described in the following forum thread:
COMPAS FORUM

The function name stays the same:

if __name__ == "__main__":
    points = [
        [377.952174, -3452, 1500.484283],
        [369.348527, -3265, 1488.197003],
        [459.973604, -3252, 1617.623026],
        [595.337643, -3295, 1810.942908],
        [718.083001, -3144, 1986.241446],
        [458.252875, -3120, 1615.16557],
        [336.081094, -3052, 1440.686184],
        [277.576297, -3227, 1357.132676],
        [219.071501, -3154, 1273.579167],
        [74.530239, -3315, 1067.152852],
        [69.368051, -3436, 1059.780484],
        [86.575344, -3695, 1084.355045],
        [-2.329003, -3686, 957.386478],
        [-51.083001, -3865, 887.758554],
        [63.05871, -4035, 1050.769811],
        [136.476494, -3688, 1155.621273],
        [192.686985, -3362, 1235.898173],
        [332.639635, -3414, 1435.771272],
        [278.149874, -3619, 1357.951828],
        [186.377644, -3672, 1226.887501],
        [189.245526, -3981, 1230.983261],
        [339.522553, -4195, 1445.601096],
        [441.619158, -3998, 1591.41016],
        [448.502075, -3634, 1601.239985],
        [477.180897, -3578, 1642.197587],
        [505.286143, -3908, 1682.336037],
        [642.370911, -3534, 1878.113376],
        [422.117559, -3301, 1563.558991],
        [332.639635, -3824, 1435.771272],
    ]

    polygon = Polygon(points)
    triangles = earclip_polygon(polygon)
    print(triangles)

What type of change is this?

Bug fix in a backwards-compatible manner.
New feature in a backwards-compatible manner. - replacement of existing code.
Breaking change: bug fix or new feature that involve incompatible API changes.
Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
I ran all tests on my computer and it's all green (i.e. invoke test).
I ran lint on my computer and there are no errors (i.e. invoke lint).
I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation (if appropriate)

src/compas/geometry/triangulation_earclip.py

CHANGELOG.md

gonzalocasas

The code looks good, I didn't check the math though. But it is missing tests, and since you mention that the previous algo failed in some cases, I would like to see those failing cases added as unit tests + some non-edge cases.

Co-authored-by: Gonzalo Casas <[email protected]>

tomvanmele · 2024-01-10T16:31:49Z

src/compas/geometry/triangulation_earclip.py

+        """Initialize an Ear instance.
+
+        Parameters
+        ----------
+        points : list
+            List of points representing the polygon.
+        indexes : list
+            List of indices of the points representing the polygon.
+        ind : int
+            Index of the vertex of the Ear triangle.
+
+        Attributes
+        ----------
+        index : int
+            Index of the vertex of the Ear triangle.
+        coords : list
+            Coordinates of the vertex of the Ear triangle.
+        next : int
+            Index of the next vertex of the Ear triangle.
+        prew : int
+            Index of the previous vertex of the Ear triangle.
+        neighbour_coords : list
+            Coordinates of the next and previous vertices of the Ear triangle.
+
+        """


add to class docstring

tomvanmele · 2024-01-10T16:32:21Z

src/compas/geometry/triangulation_earclip.py

+
+        Returns
+        -------
+        bool : True, if the point is a vertex of the Ear triangle, False otherwise.


bool True, if ...

tomvanmele · 2024-01-10T16:32:31Z

src/compas/geometry/triangulation_earclip.py

+
+        Returns
+        -------
+        bool: True if the Ear triangle is valid, False otherwise.


tomvanmele · 2024-01-10T16:32:38Z

src/compas/geometry/triangulation_earclip.py

+
+        Returns
+        -------
+        bool: True if the Ear triangle is convex, False otherwise.


tomvanmele · 2024-01-10T16:32:48Z

src/compas/geometry/triangulation_earclip.py

+
+        Returns
+        -------
+        list: List of vertex indices forming the Ear triangle.


tomvanmele · 2024-01-10T16:33:01Z

src/compas/geometry/triangulation_earclip.py

+        """Initialize an Earcut instance with the input points.
+
+        Parameters
+        ----------
+        points : list
+            List of points representing the polygon.
+
+        Attributes
+        ----------
+        vertices : list
+            List of points representing the polygon.
+        ears : list
+            List of Ear objects representing the Ears of the polygon.
+        neighbours : list
+            List of indices of the neighbouring vertices.
+        triangles : list
+            List of triangles forming the triangulation of the polygon.
+        length : int
+            Number of vertices of the polygon.
+
+        """


add to class docstring

petrasvestartas · 2024-01-11T12:05:03Z

Thank you for a review, the changes have been made, including pytest, and pushed once more.

tomvanmele · 2024-01-11T12:11:00Z

can you merge main back in please...

…us commit

petrasvestartas · 2024-01-11T12:48:42Z

earclip_polygon method is merged back.

gonzalocasas · 2024-01-11T13:17:12Z

CHANGELOG.md

+* Changed `compas.geometry.earclip_polygon` algorithm because the current one does not work in several cases.
+* Changed the docstrings in `compas.geometry.earclip_polygon` are corrected. The orientation to the XY Frame has been removed since it is handled by the `polygon.to_vertices_and_faces` method. Additionally, the test_polygon.py file has been modified with a new test for the ear_clip called `test_polygon_to_vertices_and_faces`. The `object` has been added to all `Ear` classes.
+* Changed the docstrings in `compas.geometry.earclip_polygon` is merged back, meaning the polygon is transformed to XY frame.



Don't need to add tests and docstring stuff to the changelog, the changelog is only for users of the library that need to understand what has changed

gonzalocasas · 2024-01-11T13:23:33Z

src/compas/geometry/triangulation_earclip.py

+                self.add_ear(next_ear_new)
+                continue
+
+        return self.triangles


 def earclip_polygon(polygon):


The unit test added does not explicitly test the earclip algorithm, and actually I find it hard to see the link from the code. I would add an additiona tests/compas/geometry/test_triangulation_earclip.py that tests directly the relevant methods of this module.

I have just made a new push to a new file: tests/compas/geometry/test_triangulation_earclip.py for testing.

I added one small detail to handle simple triangles.

…esting only the algorithm

gonzalocasas · 2024-01-11T14:30:44Z

CHANGELOG.md

@@ -82,6 +82,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Moved `compas.numerical.matrices` to `compas.topology.matrices`.
 * Moved `compas.numerical.linalg` to `compas.geometry.linalg`.
 * Changed `watchdog` dependency to be only required for platforms other than `emscripten`.
+* Changed `compas.geometry.earclip_polygon` algorithm because the current one does handle several cases.


Tiny typo ;)

Suggested change

* Changed `compas.geometry.earclip_polygon` algorithm because the current one does handle several cases.

* Changed `compas.geometry.earclip_polygon` algorithm because the current one does not handle several cases.

gonzalocasas

looks good to me! Besides the little typo, there's some linting required (the build is failing because of that). Once the build is green, I think this is good to go!

tomvanmele · 2024-01-11T15:31:34Z

@petrasvestartas run invoke check to find all such errors locally...

tomvanmele · 2024-01-11T15:32:08Z

to apply black to the src folder and fix things automatically black src

CHANGE triangulation_earclip

9630147

gonzalocasas reviewed Jan 9, 2024

View reviewed changes

src/compas/geometry/triangulation_earclip.py Outdated Show resolved Hide resolved

gonzalocasas reviewed Jan 9, 2024

View reviewed changes

src/compas/geometry/triangulation_earclip.py Outdated Show resolved Hide resolved

gonzalocasas reviewed Jan 9, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

gonzalocasas reviewed Jan 9, 2024

View reviewed changes

tomvanmele and others added 3 commits January 10, 2024 17:30

Update CHANGELOG.md

38abec8

Co-authored-by: Gonzalo Casas <[email protected]>

Update src/compas/geometry/triangulation_earclip.py

78b8361

Co-authored-by: Gonzalo Casas <[email protected]>

Update src/compas/geometry/triangulation_earclip.py

a425760

Co-authored-by: Gonzalo Casas <[email protected]>

tomvanmele reviewed Jan 10, 2024

View reviewed changes

UPDATE triangulation_earclip corrections and pytest are added

efb18f9

MERGE BACK the earclip_polygon method is brought back from the previo…

9e862be

…us commit

Merge branch 'main' into triangulation_earclip

d5f1173

gonzalocasas reviewed Jan 11, 2024

View reviewed changes

petrasvestartas added 2 commits January 11, 2024 15:04

CHANGE the triangulation_earclip test are added as a separate file, t…

a33ff26

…esting only the algorithm

CHANGELOG

2735b6e

gonzalocasas reviewed Jan 11, 2024

View reviewed changes

gonzalocasas approved these changes Jan 11, 2024

View reviewed changes

FIX Changelog typo and black formating

85c5434

tomvanmele merged commit 114eb12 into compas-dev:main Jan 11, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CHANGE triangulation_earclip #1253

CHANGE triangulation_earclip #1253

petrasvestartas commented Jan 9, 2024 •

edited

Loading

gonzalocasas left a comment

tomvanmele Jan 10, 2024

tomvanmele Jan 10, 2024

tomvanmele Jan 10, 2024

tomvanmele Jan 10, 2024

tomvanmele Jan 10, 2024

tomvanmele Jan 10, 2024

petrasvestartas commented Jan 11, 2024

tomvanmele commented Jan 11, 2024

petrasvestartas commented Jan 11, 2024 •

edited

Loading

gonzalocasas Jan 11, 2024

gonzalocasas Jan 11, 2024

petrasvestartas Jan 11, 2024

gonzalocasas Jan 11, 2024

gonzalocasas left a comment

tomvanmele commented Jan 11, 2024

tomvanmele commented Jan 11, 2024

	* Changed `compas.geometry.earclip_polygon` algorithm because the current one does handle several cases.
	* Changed `compas.geometry.earclip_polygon` algorithm because the current one does not handle several cases.

CHANGE triangulation_earclip #1253

CHANGE triangulation_earclip #1253

Conversation

petrasvestartas commented Jan 9, 2024 • edited Loading

What type of change is this?

Checklist

gonzalocasas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrasvestartas commented Jan 11, 2024

tomvanmele commented Jan 11, 2024

petrasvestartas commented Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzalocasas left a comment

Choose a reason for hiding this comment

tomvanmele commented Jan 11, 2024

tomvanmele commented Jan 11, 2024

petrasvestartas commented Jan 9, 2024 •

edited

Loading

petrasvestartas commented Jan 11, 2024 •

edited

Loading