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

Changing straight skeleton return data into Graph #33

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

romanarust
Copy link
Member

Updating the return value of straight skeleton 2 from lines to graph. This allows to include more information from the skeleton, e.g. which edges are inner bisectors, bisectors and boundary edges.

@romanarust romanarust requested a review from tomvanmele July 31, 2024 16:11
@romanarust
Copy link
Member Author

@tomvanmele Friendly ping!

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

i think it makes sense to provide a conversion function from skeleton data to graph. but perhaps some users want the raw data instead of the graph. so perhaps the conversion can be optional, or left up to the user?

viewer.scene.add(graph, edgecolor=(1.0, 0.0, 0.0))
viewer.scene.add(polygon)
for edge in graph.edges():
line = Line(*graph.edge_coordinates(edge))
Copy link
Member

Choose a reason for hiding this comment

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

just fyi, there is line = graph.edge_line(edge)


def graph_from_skeleton_data(skeleton_data) -> Graph:
Mv, Mvi, Me, Mei = skeleton_data
Copy link
Member

Choose a reason for hiding this comment

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

why not split these parameters?
now it is quite difficult for a user to know what the input is supposed to be...

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, made the changes and added doc for graph_from_skeleton_data

@@ -31,10 +48,10 @@ def create_interior_straight_skeleton(points) -> PolylinesNumpy:
if not TOL.is_allclose(normal, [0, 0, 1]):
raise ValueError("The normal of the polygon should be [0, 0, 1]. The normal of the provided polygon is {}".format(normal))
V = np.asarray(points, dtype=np.float64)
return straight_skeleton_2.create_interior_straight_skeleton(V)
return graph_from_skeleton_data(straight_skeleton_2.create_interior_straight_skeleton(V))
Copy link
Member

Choose a reason for hiding this comment

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

if you "star" the output, you could split the params in graph_from_skeleton_data


def graph_from_skeleton_data(points, indices, edges, edge_types) -> Graph:
Copy link
Member

Choose a reason for hiding this comment

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

since you annotate the output, perhaps you could also annotate the input?

Copy link
Member Author

Choose a reason for hiding this comment

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

i am new to using types for in- and outputs. but I hope this is what you meant now. ;)

@@ -31,10 +64,10 @@ def create_interior_straight_skeleton(points) -> PolylinesNumpy:
if not TOL.is_allclose(normal, [0, 0, 1]):
raise ValueError("The normal of the polygon should be [0, 0, 1]. The normal of the provided polygon is {}".format(normal))
V = np.asarray(points, dtype=np.float64)
return straight_skeleton_2.create_interior_straight_skeleton(V)
return graph_from_skeleton_data(*straight_skeleton_2.create_interior_straight_skeleton(V))
Copy link
Member

Choose a reason for hiding this comment

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

just pointing out that the user can still "only" get a graph as a result and no longer has the option to use the low level output.

perhaps you could add a parameter as_graph and only convert to a graph if that parameter is set to True, which could even be the default...

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

LGTM

@romanarust romanarust merged commit 4d1f48e into main Sep 18, 2024
10 checks passed
@romanarust romanarust deleted the feature/ss2_result branch September 18, 2024 12:40
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

Successfully merging this pull request may close these issues.

2 participants