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

new functions for compas geometry #569

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
96 changes: 96 additions & 0 deletions src/compas/geometry/_intersections/mesh_intersections.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
from __future__ import print_function
from __future__ import absolute_import
from __future__ import division

from compas.geometry import Point
from compas.geometry import length_vector
from compas.geometry import subtract_vectors
from compas.geometry import intersection_line_triangle
from compas.geometry import intersection_segment_plane

__all__ = [
'intersection_mesh_line',
'intersection_mesh_plane',
'mesh_vertices_to_points',
]


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Parameters
----------
mesh : compas.datastructures.Mesh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the documentation reference-able, this should read

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

and similarly for all references in this PR.

line : compas.geometry.Line

Returns
-------
Point : compas.geometry.Point
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:class:`compas.geometry.Point`

"""
for fkey in list(mesh.faces()):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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():

vertex_keys = mesh.face_vertices(fkey)
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
Comment on lines +32 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this work?

return Point(*intersection)


if len(vertex_keys) == 4:
triangle_2 = [vertices[2], vertices[3], vertices[0]]
intersection_2 = intersection_line_triangle(line, triangle_2)
if intersection_2:
return Point(intersection_2[0], intersection_2[1], intersection_2[2])
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

return None


def intersection_mesh_plane(mesh, plane, tol=0.0001):
"""Calculate the keys of the points of the intersection of a mesh with a plane
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a full stop at the end of the sentence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function not only returns the intersection points, but modifies the mesh structure, splitting any edge that cross the plane. I think that should be noted in the docstring.


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

Returns
-------
intersections: list of points as keys from mesh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list of int
    A list of keys identifying points in the mesh.

"""
intersections = []
for u, v in list(mesh.edges()):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

a = mesh.vertex_attributes(u, 'xyz')
b = mesh.vertex_attributes(v, 'xyz')
intersection = intersection_segment_plane((a, b), plane)
if not intersection:
continue
len_a_inters = length_vector(subtract_vectors(intersection, a))
len_a_b = length_vector(subtract_vectors(b, a))
t = len_a_inters / len_a_b
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that len_a_b will never be zero? I mean, ideally, sure it would never be zero, but maybe do to rounding errors...?

if t >= 1.0:
t = 1 - tol
elif t <= 0.0:
t = tol
intersection_key = mesh.split_edge(u, v, t=t, allow_boundary=True)
intersections.append(intersection_key)
return intersections


def mesh_vertices_to_points(mesh, v_keys):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Compute compas points from vertex keys from specific mesh
Returns list of compas points from a list of indexes of the vertexes of a mesh

Parameters
----------
mesh : compas.datastructures.Mesh
v_keys : list of vertex indexes of a mesh

Returns
-------
list of compas.geometry.Point
"""
return [Point(*mesh.vertex_attributes(v_key, 'xyz')) for v_key in v_keys]
27 changes: 26 additions & 1 deletion src/compas/geometry/_primitives/line.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from compas.geometry._primitives import Primitive
from compas.geometry._primitives import Point


__all__ = ['Line']


Expand Down Expand Up @@ -337,11 +336,37 @@ def transformed(self, T):
line.transform(T)
return line

def divide_by_count(self, number=10, include_ends=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a little confusing that the name of the function is divide_by_count and the 'count' argument is named number. What do you think of something like get_n_subdivision_points(self, n=10, include_ends=False)?

"""Return list of points from dividing the line by specific number of divisions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Return a list of points subdividing the line into the specified number of segments.? Also, full stop, please.


Parameters
----------
number : integer
number of divisions
includeEnds : boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be

include_ends : bool

True if including start and end point in division points
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double ` around True and False would make the documentation look a little more consistent, that is:

``True`` and ``False``

False if not including start and end point in division points

Returns
-------
list of :class:`compas.geometry.Point`
Point as sequence of values xyz

Example
--------
>>> line = Line([0.0, 0.0, 0.0], [5.0 ,0.0, 0.0])
>>> line.divide_by_count(5, True)
[Point(0.000, 0.000, 0.000), Point(1.000, 0.000, 0.000), Point(2.000, 0.000, 0.000), Point(3.000, 0.000, 0.000), Point(4.000, 0.000, 0.000), Point(5.000, 0.000, 0.000)]
"""
if include_ends:
return [self.point(i * float(1 / number)) for i in range(int(number)+1)]
else:
return [self.point(i * float(1.0 / number)) for i in range(int(number) + 1) if i != 0 or i != number]
# ==============================================================================
# Main
# ==============================================================================


if __name__ == '__main__':

import doctest
Expand Down
101 changes: 101 additions & 0 deletions src/compas/geometry/_primitives/polyline.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,11 +319,112 @@ def transformed(self, T):
polyline.transform(T)
return polyline

def shorten(self, start_distance=0, end_distance=0):
"""Return a new polyline which is shorter than the original in one end side, other or both by a given distance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like "Return a new polyline truncated by the given distances on either side."?


Parameters
----------
start_distance : float.
distance to shorten from the starting point of the polyline
Comment on lines +327 to +328
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start_distance : float
    Length to remove from the polyline beginning from its first point.

and similarly below

end_distance : float.
distance to shorten from the ending point of the polyline

Returns
-------
:class:`compas.geometry.Polyline`
The transformed copy.
"""
if start_distance != 0 or end_distance != 0:
points = []
acum_length = 0
switch = True
for i, line in enumerate(self.lines):
acum_length += line.length
if acum_length < start_distance:
continue
elif acum_length > start_distance and switch:
if start_distance == 0:
points.append(line.start)
else:
points.append(self.point(start_distance/self.length))
switch = False
else:
points.append(line.start)
if end_distance == 0:
if i == len(self.lines)-1:
points.append(line.end)
else:
if acum_length >= (self.length - end_distance):
points.append(self.point(1-(end_distance/self.length)))
break
Comment on lines +338 to +359
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about edge cases. Let's say you take Polyline([[0.0, 0.0, 0.0], [1.0, 0.0, 0.0]]) and start_distance=0.5 and end_distance=0.0. Then I don't think the end point [1.0, 0.0, 0.0] is ever appended to points. And consider the case when end_distance = 0.75, that is when the resulting polyline should be a subsegment of a line from the original. Or what is the expected behavior when you take 'start_distance=1.0andend_distance=0.0? Should it return None` or a reversed polyline? Here's a suggestion which should be tested (I probably messed up some indices by 1):

t_start = start_distance / self.length # let's hope the polyline isn't just a point...
t_end = end_distance / self.length
start_point = self.point(t_start)
end_point = self.point(t_end)

if t_end <= t_start:
    # what happens in this case needs to be decided
    pass

# find the first point of the polyline right of the start_point
accum_len = 0
i = 0
while accum_len < start_distance:
    line = Line(self.points[i], self.points[i+1])
    accum_len += line.length
    i += 1
first_pt_idx_right_of_start = i+1 if accum_len != start_distance else i + 2

# find the first point of the polyline to the left of the end_point
accum_len = 0
i = len(self.points) - 1
while accum_len < end_distance:
    line = Line(self.points[i-1], self.point[i])
    accum_len += line.length
    i -= 1
first_pt_idx_left_of_end = i if accum_len != end_distance else i-1

points = [start_point]
if first_pt_idx_right_of_start <= first_pt_idx_left_of_end:
    points.extend(self.points[first_pt_idx_right_of_start: first_pt_idx_left_of_end+1])
points.append(end_point)

return points
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a list, not a polyline as indicated in the docstring. So either the docstring should be changed or something along the lines of (maybe there's a better way):

new_polyline = self.copy()
new_polyline.points = points
return new_polyline

return self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a copy as might be indicated by the docstring. So either, the docstring should be changed or return self.copy().
Also, it's easier to read I think if you invert the if-then, that is

if start_distance == 0 and end_distance == 0:
   return self.copy()
points = []
...


def rebuild(self, number=20):
"""Reconstruct a polyline with evenly spaced points based on a number of interpolations
Returns new rebuilt polyline

Parameters
----------
number : integer.
number of points for the amount of definition of the polyline

Returns
-------
:class: 'compas.geometry.Polyline'
the rebuilt copy
"""
rebuilt_polyline = self.copy()
points = [self.point(i * float(1.0 / number)) for i in range(number)]
points.append(self.point(1))
Comment on lines +378 to +379
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

points = [self.point(i * float(1.0 / number)) for i in range(number + 1)]

rebuilt_polyline.points = [Point(x, y, z) for x, y, z in points]
return rebuilt_polyline

def divide_by_count(self, number=10, include_ends=False):
"""Divide a polyline by count. Returns list of Points from the division
Comment on lines +383 to +384
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I find the count/number discrepancy confusing. Whatever you choose to do with the Line version of this, should also be applied here.


Parameters
----------
number : integer.
number of divisions
Comment on lines +388 to +389
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number : int
    The number of subdivisions of the polyline.

includeEnds : boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include_ends : bool

True if including start and ending points.
False if not including start and ending points.

Returns
-------
list of :class: 'compas.geometry.Point'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
points = [self.point(i * float(1.0 / number)) for i in range(number)]
if include_ends:
points.append(self.point(1))
else:
points.pop(0)
return points
Comment on lines +398 to +403
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

range_ = range(number + 1) if include_ends else range(1, number)
return [self.point(i * float(1.0 / number)) for i in range_]


def tween(self, polyline_two, number=50):
"""Create an average polyline between two polylines interpolating their points
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to create a polyline or a list of points?


Parameters
----------
polyline_two : compas.geometry.Polyline
polyline to create the tween polyline
number : number of points of the tween polyline
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number : int
    The number of points of the tween polyline.


Returns
-------
list of :class: 'compas.geometry.Point'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
rebuilt_polyline_one = self.rebuild(number)
rebuilt_polyline_two = polyline_two.rebuild(number)
lines = [Line(point_one, point_two) for point_one, point_two in zip(rebuilt_polyline_one, rebuilt_polyline_two)]
return [line.midpoint for line in lines]

# ==============================================================================
# Main
# ==============================================================================


if __name__ == '__main__':

import doctest
Expand Down
95 changes: 95 additions & 0 deletions src/compas/geometry/_transformations/extend.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
from __future__ import print_function
from __future__ import absolute_import
from __future__ import division

from compas.geometry import Translation
from compas.geometry import Polyline
from compas.geometry import Vector

__all__ = [
'extend_line',
'extend_polyline',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two comments about these two functions. First, extend_line modifies the line that is given to it as an argument, while extend_polyline doesn't change the polyline that is given to it, and creates a new one that is extended. I think this subtle difference in behavior could lead to some annoying bugs. The second comment is: Why are these functions not methods of the classes Line and Polyline, resp.?

]


def extend_line(line, start_extension=0, end_extension=0):
"""Extend the given line from one end or the other, or both, depending on the given values

Parameters
----------
line : tuple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuple or Line or both?

Two points defining the line.
start_extension : float
The extension distance at the start of the line as float.
end_extension : float
The extension distance at the end of the line as float.

Returns
-------
extended line : tuple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuple or Line?

Two points defining the extended line.

Examples
--------
>>> line = Line([0.0, 0.0, 0.0], [1.0, 0.0, 0.0])
>>> extended_line = extend_line(line, 1, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be simply extend_line(line, 1, 1)? I would think the doctests might fail as it is....

Line([-1.0, 0.0, 0.0], [2.0, 0.0, 0.0])
"""
def calculate_translation(line, distance):
vector = line.direction.copy()
vector.scale(distance)
return Translation(vector)

if start_extension != 0:
translation = calculate_translation(line, -start_extension)
line.start.transform(translation)
if end_extension != 0:
translation = calculate_translation(line, end_extension)
line.end.transform(translation)

return line


def extend_polyline(polyline, start_extension=0, end_extension=0):
"""Extend a polyline by line from the vectors on segments at extreme sides
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention that this function returns a new polyline with 2 more points than was in the original polyline


Parameters
----------
polyline : list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list or Polyline or both?

list of points defining the polyline.
start_extension : float
The extension distance at the start of the polyline as float.
end_extension : float
The extension distance at the end of the polyline as float.

Returns
-------
extended polyline : compas.geometry.Polyline(points)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:class:`compas.geometry.Polyline` 

the documentation doesn't need to know what you called the variable internally.


Examples
--------
>>> polyline = Polyline([0.0, 0.0, 0.0], [1.0, 0.0, 0.0], [2.0, 1.0, 0.0], [3.0, 1.0, 0.0], [4.0, 0.0, 0.0], [5.0, 0.0, 0.0])
>>> extended_polyline = extend_polyline(polyline, 1, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as in example above.

Polyline([-1.0, 0.0, 0.0], [0.0, 0.0, 0.0], [1.0, 0.0, 0.0], [2.0, 1.0, 0.0], [3.0, 1.0, 0.0], [4.0, 0.0, 0.0], [5.0, 0.0, 0.0], [6.0, 0.0, 0.0])
"""
def calculate_translation_vector(vector, distance):
vector.unitize()
vector.scale(distance)
return Translation(vector)

points = polyline.points
if start_extension != 0:
point_start = polyline.points[0]
vec = Vector.from_start_end(polyline.points[1], point_start)
translation = calculate_translation_vector(vec, start_extension)
new_point_start = point_start.transformed(translation)
points.insert(0, new_point_start)

if end_extension != 0:
point_end = polyline.points[-1]
vec_end = Vector.from_start_end(polyline.points[-2], point_end)
translation = calculate_translation_vector(vec_end, end_extension)
new_point_end = point_end.transformed(translation)
points.append(new_point_end)

return Polyline(points)
5 changes: 5 additions & 0 deletions src/compas/geometry/splits/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from __future__ import print_function
from __future__ import absolute_import
from __future__ import division

__all__ = [name for name in dir() if not name.startswith('_')]
Loading