new functions for compas geometry #569

nikeftekhar · 2020-06-24T11:37:56Z

new functions regarding the following topics:
line divide by count
polyline shorten
polyline rebuild
polyline divide by count
polyline tween
transformations extend line
transformations extend polyline
intersection_mesh_line
intersection_mesh_plane
split polyline_plane
split mesh_plane

What type of change is this?

Bug fix in a backwards-compatible manner.
New feature in a backwards-compatible manner.
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)

line divide by count polyline shorten polyline rebuild polyline divide by count polyline tween transformations extend line transformations extend polyline intersection_mesh_line intersection_mesh_plane split polyline_plane split mesh_plane

gonzalocasas

I checked mostly for code style. I guess @brgcode would check for correctness in terms of geometry processing.
I added a bunch of comments, plus I think it would really be of use to add a few unit tests to this (and it should not be overly complex to do so).

src/compas/geometry/_intersections/mesh_intersections.py

src/compas/geometry/_primitives/line.py

src/compas/geometry/splits/splits.py

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

corrected errors invoke lint passed invoke test passed

nikeftekhar · 2020-06-26T13:03:59Z

src/compas/geometry/_intersections/mesh_intersections.py

+            intersection_2 = intersection_line_triangle(line, triangle_2)
+            if intersection_2:
+                return Point(intersection_2[0], intersection_2[1], intersection_2[2])
+    else:


beverlylytle

Hey Nik, nice PR! There's a lot going in this, so I've made a lot of comments, sorry if it is overwhelming. Really, there are three general themes to the comments.

The first category is about making the docstrings consistent and exact:

There should be consistency in using the same tense, capitalization, and punctuation (eg "Return blahblahblah. " vs "returns blahblahblah").

Types of function parameters and return objects should be double checked. Reference-able types should be included in the format required by Sphinx, eg

 :class:`compas.geometry.Point`

Docstrings should by default include examples that can be run as tests. I didn't explicitly ask for examples in my comments, but those should be added when possible.

The next category of comments is about tightening up the code. Using one line instead of two, or naming conventions. These are mostly about readability, and maybe are just my personal preference, but a couple of them are about optimization.
The last category of comments is about code correctness. I think in one or two places some edge cases were missed, and these need to be addressed.

Do let me know if anything is unclear.

beverlylytle · 2020-07-17T12:55:26Z

src/compas/geometry/_intersections/mesh_intersections.py

+
+    Parameters
+    ----------
+    mesh : compas.datastructures.Mesh


To make the documentation reference-able, this should read

mesh : :class:`compas.datastructures.Mesh`

and similarly for all references in this PR.

beverlylytle · 2020-07-17T12:57:15Z

src/compas/geometry/_intersections/mesh_intersections.py

+    -------
+    Point : compas.geometry.Point
+    """
+    for fkey in list(mesh.faces()):


As I understand it mesh.faces() returns a generator, so there's no need to instantiate a list (could be very taxing on memory), so it would be more efficient to have:

for fkey in mesh.faces():

beverlylytle · 2020-07-17T12:59:57Z

src/compas/geometry/_intersections/mesh_intersections.py

+        if not vertex_keys:
+            continue
+        vertices = [mesh.vertex_attributes(vkey, 'xyz') for vkey in vertex_keys]
+        if len(vertex_keys) not in (3, 4):
+            continue


Could this be simplified to

if not (vertex_keys and len(vertex_keys) in (3,4)): continue vertices = [mesh.vertex_atrributes(vkey, 'xyz') for vkey in vertex_keys]

well or a condition that fails early (the and will require both conditions to be checked at all times)

if not vertex_keys or len(vertex_keys) not in (3, 4): continue

beverlylytle · 2020-07-17T13:07:17Z

src/compas/geometry/_intersections/mesh_intersections.py

+    intersections: list of points as keys from mesh
+    """
+    intersections = []
+    for u, v in list(mesh.edges()):


Again, for efficiency for u, v in mesh.edges():

beverlylytle · 2020-07-17T13:09:25Z

src/compas/geometry/_intersections/mesh_intersections.py

+
+    Returns
+    -------
+    Point : compas.geometry.Point


:class:`compas.geometry.Point`

beverlylytle · 2020-07-20T08:59:26Z

src/compas/geometry/splits/splits.py

+    Parameters
+    ----------
+    mesh : compas.datastructures.Mesh
+    plane : compas.geometry.Plane


Please add a description of 'open'.

beverlylytle · 2020-07-20T09:07:53Z

src/compas/geometry/_intersections/mesh_intersections.py

+
+
+def intersection_mesh_line(mesh, line):
+    """Compute intersection between mesh faces and line. After one single intersection, stops searching for more.


You are very clear in the docstrings about the behavior of intersection_mesh_line and intersection_mesh_plane, but I don't like that the one fails early (returning the first point of intersection with a line) and the other gives all points of intersection. I think the difference should be reflected in the names.

rename both to intersections_mesh_xxx? with returned intersections sorted based on distance in the case of line?

beverlylytle · 2020-07-20T09:09:14Z

src/compas/geometry/_intersections/mesh_intersections.py

+        triangle = [vertices[0], vertices[1], vertices[2]]
+        intersection = intersection_line_triangle(line, triangle)
+        if intersection:
+            return Point(intersection[0], intersection[1], intersection[2])


would this work?

return Point(*intersection)

beverlylytle · 2020-07-20T09:15:51Z

src/compas/geometry/_intersections/mesh_intersections.py

+    return intersections
+
+
+def mesh_vertices_to_points(mesh, v_keys):


This function doesn't have much functionality, and I don't see much point of adding it to the library.

beverlylytle · 2020-07-20T09:22:22Z

src/compas/geometry/_primitives/polyline.py

+
+        Returns
+        -------
+        list of :class: 'compas.geometry.Point'


list of :class:`compas.geometry.Point`

nperraud · 2020-10-01T06:20:48Z

Hi everyone,

First, thank you for the great package. Would it be possible to know when this will be merged. I need the function intersection_mesh_line for one of my experiment.

All the best

tomvanmele · 2020-10-01T06:57:34Z

hi, it seems there are still quite a bit of unaddressed comments. if you resolve those, i am more than happy to merge immediately. if in the meantime you need mesh/line intersections urgently you could use intersection_mesh_ray of compas_libigl...

tomvanmele · 2020-10-01T07:05:44Z

also, in the case of intersection_mesh_plane you could use the slicing functionality of compas_cgal, if you need it urgently...

jf--- · 2024-04-16T12:47:37Z

@tomvanmele this is meaningful work, but this has been sitting here for a long time...
Since the author hasn't responded to the excellent code review, I think its fair to close it.

nikeftekhar · 2024-04-16T13:50:49Z

Hi I am the author, This commits were result of the work I was doing at mesh 4 years ago. Due to covid, funds of mesh got shortened and I could not continue the project. Now im doing a phd at DBT, and I would be delighted to finalise those commits and comments if you really think are a good contribution, but I have completely lost the environment and the code and everything. If you were so kind, we could set a meeting, may be you could guide me through on how to set the working environment and everything properly and I will be delighted to finish the contribution applying all the comments that were done 4 years ago. Best, Nik Eftekhar Olivo El 16.04.2024, a las 14:47, Jelle Feringa ***@***.***> escribió: @tomvanmele<https://github.com/tomvanmele> this is meaningful work, but this has been sitting here for a long time... Since the author hasn't responded to the excellent code review, I think its fair to close it. — Reply to this email directly, view it on GitHub<#569 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKK6EFS5JR3RF66EZEAS2F3Y5UMX7AVCNFSM4OGSN3MKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBVHEYDCMJUG4ZA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

gonzalocasas · 2024-04-16T15:53:24Z

we could set a meeting, may be you could guide me through on how to set the working environment and everything properly

@nikeftekhar yep, let's do that, feel free to poke me directly in person, or send me a calendar invite to do this

nikeftekhar · 2024-04-16T16:51:18Z

Hi Gonzalo, Perfect then ill try to approach you during this week or the following Best, Nik El 16.04.2024, a las 17:53, Gonzalo Casas ***@***.***> escribió: we could set a meeting, may be you could guide me through on how to set the working environment and everything properly @nikeftekhar<https://github.com/nikeftekhar> yep, let's do that, feel free to poke me directly in person, or send me a calendar invite to do this — Reply to this email directly, view it on GitHub<#569 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKK6EFUXTA47DJOKGMJJMG3Y5VCQXAVCNFSM4OGSN3MKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBVHE2DEMJSGY3A>. You are receiving this because you were mentioned.Message ID: ***@***.***>

new functions for compas geometry

5f9b52b

line divide by count polyline shorten polyline rebuild polyline divide by count polyline tween transformations extend line transformations extend polyline intersection_mesh_line intersection_mesh_plane split polyline_plane split mesh_plane

gonzalocasas requested changes Jun 24, 2020

View reviewed changes

nikeftekhar and others added 10 commits June 25, 2020 09:50

Update src/compas/geometry/_intersections/mesh_intersections.py

6408ef7

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_intersections/mesh_intersections.py

0e34204

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_intersections/mesh_intersections.py

401a7e2

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_primitives/line.py

2df9dcf

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/splits/splits.py

77549aa

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_primitives/line.py

829696c

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_primitives/line.py

ad06707

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_primitives/polyline.py

f29a933

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

Update src/compas/geometry/_transformations/extend.py

4eafb73

Co-authored-by: Gonzalo Casas <gonzalocasas@gmail.com>

all changes and tests

faf7bc8

corrected errors invoke lint passed invoke test passed

nikeftekhar commented Jun 26, 2020

View reviewed changes

nikeftekhar added 2 commits June 29, 2020 14:08

some fixes

e9209c8

slight change

8b75011

beverlylytle requested changes Jul 20, 2020

View reviewed changes

Base automatically changed from master to main February 5, 2021 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new functions for compas geometry #569

new functions for compas geometry #569

nikeftekhar commented Jun 24, 2020 •

edited

Loading

gonzalocasas left a comment

nikeftekhar Jun 26, 2020

beverlylytle left a comment

beverlylytle Jul 17, 2020

beverlylytle Jul 17, 2020

beverlylytle Jul 17, 2020

tomvanmele Oct 1, 2020

beverlylytle Jul 17, 2020

beverlylytle Jul 17, 2020

beverlylytle Jul 20, 2020

beverlylytle Jul 20, 2020

tomvanmele Oct 1, 2020

beverlylytle Jul 20, 2020

beverlylytle Jul 20, 2020

beverlylytle Jul 20, 2020

nperraud commented Oct 1, 2020

tomvanmele commented Oct 1, 2020

tomvanmele commented Oct 1, 2020

jf--- commented Apr 16, 2024

nikeftekhar commented Apr 16, 2024 via email

gonzalocasas commented Apr 16, 2024

nikeftekhar commented Apr 16, 2024 via email



		def intersection_mesh_line(mesh, line):
		"""Compute intersection between mesh faces and line. After one single intersection, stops searching for more.

		return intersections


		def mesh_vertices_to_points(mesh, v_keys):

new functions for compas geometry #569

Are you sure you want to change the base?

new functions for compas geometry #569

Conversation

nikeftekhar commented Jun 24, 2020 • 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

beverlylytle 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

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

nperraud commented Oct 1, 2020

tomvanmele commented Oct 1, 2020

tomvanmele commented Oct 1, 2020

jf--- commented Apr 16, 2024

nikeftekhar commented Apr 16, 2024 via email

gonzalocasas commented Apr 16, 2024

nikeftekhar commented Apr 16, 2024 via email

nikeftekhar commented Jun 24, 2020 •

edited

Loading