Skip to content

Changed get_label_position func to account for edge curvature#457

Merged
ntamas merged 3 commits intoigraph:masterfrom
Sriram-Pattabiraman:patch-1
Nov 17, 2021
Merged

Changed get_label_position func to account for edge curvature#457
ntamas merged 3 commits intoigraph:masterfrom
Sriram-Pattabiraman:patch-1

Conversation

@Sriram-Pattabiraman
Copy link
Copy Markdown
Contributor

get_label_position now computes the bezier curve for the edge that would be drawn, then finds the parameter based midpoint by putting .5 as the parameter and taking the output as the label position.

get_label_position now computes the bezier curve for the edge that would be drawn, then finds the parameter based midpoint by putting `.5` as the parameter and taking the output as the label position.
@ntamas
Copy link
Copy Markdown
Member

ntamas commented Nov 15, 2021

This looks good, thanks! One problem that I have with this PR is that now there are three places in igraph/drawing/edge.py where we need to derive the control points of a Bézier curve that goes from one vertex of a graph to another with a given curvature. Would it be possible to refactor this logic into a separate private function (say def _get_bezier_control_points_for_curved_edge(x0, y0, x1, y1, curvature)) and call this private function from all the three places so we don't duplicate code unnecessarily any more? (Look for occurrences of edge.curved to find the other two places).

@ntamas ntamas self-assigned this Nov 15, 2021
Previously, the code for calculating the bezier control points was repeated multiple times. Now, there is a function to calculate bezier curve control points accordingly using the start point, the end point, and the edge curvature in a function called get_bezier_control_points_for_curved_edge
@ntamas
Copy link
Copy Markdown
Member

ntamas commented Nov 17, 2021

Looks good, thanks! I'll merge this if the CI tests pass.

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Nov 17, 2021

CI failure is unrelated so merging, thanks a lot!

@ntamas ntamas merged commit f12cd47 into igraph:master Nov 17, 2021
@Sriram-Pattabiraman Sriram-Pattabiraman deleted the patch-1 branch November 18, 2021 02:51
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