-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added straight skeleton 2 #29
Conversation
oh nice! will have a look... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks very good. just a few minor things...
@@ -0,0 +1,33 @@ | |||
import numpy as np | |||
from compas.geometry import normal_polygon | |||
from compas.tolerance import Tolerance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently you should do from compas.tolerance import TOL
, which is the global instance of the Tolerance
class.
i will replace this with a singleton in the future, but currently this would be better...
If the normal of the polygon is not [0, 0, 1]. | ||
""" | ||
points = list(points) | ||
tol = Tolerance() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
""" | ||
points = list(points) | ||
tol = Tolerance() | ||
if not tol.is_allclose(normal_polygon(points, True), [0, 0, 1]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TOL.is_allclose
src/straight_skeleton_2.cpp
Outdated
{ | ||
poly.push_back(Point(V(i, 0), V(i, 1))); | ||
} | ||
assert(poly.is_counterclockwise_oriented()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps it is worth checking what happens with these assertions on the Python side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed the assert, as the orientation check is anyway on python side
@@ -0,0 +1,16 @@ | |||
from compas_cgal.straight_skeleton_2 import create_interior_straight_skeleton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps it would be worth adding a few tests of "known"/"predictable" skeletons?
the build on windows fails here |
src/straight_skeleton_2.cpp
Outdated
} | ||
assert(poly.is_counterclockwise_oriented()); | ||
SsPtr iss = CGAL::create_interior_straight_skeleton_2(poly.vertices_begin(), poly.vertices_end()); | ||
Edges edgelist; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this typedef conflicts with the one from skeletonization.h
src/straight_skeleton_2.cpp
Outdated
if(&*v1 < &*v2){ | ||
std::vector<double> s_vec = {v1->point().x(), v1->point().y(), 0}; | ||
std::vector<double> t_vec = {v2->point().x(), v2->point().y(), 0}; | ||
Edge edge = std::make_tuple(s_vec, t_vec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here...
src/straight_skeleton_2.h
Outdated
typedef std::tuple<std::vector<double>, std::vector<double>> Edge; | ||
typedef std::list<Edge> Edges; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we can move these guys to the compas namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for the definitions in skeletonization.h
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i put the typedefs of Edge
and Edges
into the compas namespace and removed them from skeletonization
and straight_skeleton_2
.. still failing on windows 😒
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It finally builds! just forgot to add the "straight_skeleton_2.cpp" to the setup.py 😅
cool seems to work now 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
New feature:
Straight skeletons 2:
https://doc.cgal.org/latest/Straight_skeleton_2/index.html
Upcoming add ons: