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

support shortest path layout between anchors #120

Closed

Conversation

marcocheungkami
Copy link

Referring to #119

@pierpo
Copy link
Owner

pierpo commented Nov 16, 2020

Thank you for the PR! It looks like it does the job 😊

I'll have a deeper look in the coming days. Thanks!

@pierpo
Copy link
Owner

pierpo commented Nov 17, 2020

Okay, I had a look! Unfortunately, it does not work properly yet:

  • The code does not run. You forgot to declare the shortestPath argument in other places.

  • The API is not satisfying, unfortunately. We have two booleans "noCurves", "shortestPath"... But having "noCurves" to "false" and "shortestPath" to "true" does not really make sense. My point is that we should probably have a "drawingMode" props that has multiple states: "shortest-path", "curves", "no-curves" (needs to be thought properly, this is just an idea)

  • Even when the above is fixed, there is an offset once the arrow is drawn. Indeed, the path of the arrow isn't as obvious as we think it should be: the arrowhead has some logic to be drawn. We have to draw a line from the start to the beginning minus the arrowhead length (because it is not included in the path). Here the offset on the left element corresponds as the arrowhead height that has been substracted to the top anchor. image

I'm afraid this issue is more complex than it looks like 😞
The drawing logic needs a deeper work.

@marcocheungkami
Copy link
Author

marcocheungkami commented Nov 19, 2020

I see. There is an offset problem needed to be fixed among vertical linkages. Alright, I may try to fix this issue in the coming days.
Guess I need to fine-tune the last two parameters of path d. Also, for the name, I agree with what you have mentioned on drawingModel prop and there will be three modes for people to pick. 👍

@marcocheungkami
Copy link
Author

@pierpo I have modified the logic and seems there won't be any extra spacing.

image

@pierpo
Copy link
Owner

pierpo commented Nov 25, 2020

Thank you for the updates 😊
And sorry for not coming back earlier!

So, it's better indeed, thanks for handling this. But I don't think it's in a mergeable state, because it's still not looking perfect. The fact that the arrow is angled in the end is not really what one would expect 😢

I made a little drawing of what's necessary. It's way more complex.
On the left is what you made, on the right is what we'd aim at.

image

With the theta angle being atan2(x_starting_elem - xa, y_starting_elem - ya)
I'm not really good with geometry, so there might be simpler calculations!

Anyway, this is quite a hard issue in the end. There's no similar mechanism in the lib.
I always wanted to redraw the drawing logic from scratch to have a more flexible thing. That could've be a part of it.

You can try to add this to the existing logic if you're still motivated 😊

@pierpo
Copy link
Owner

pierpo commented Apr 16, 2021

Closed in favor of #135 😊

@pierpo pierpo closed this Apr 16, 2021
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