search

excalidraw / mermaid-to-excalidraw

Generate Excalidraw diagrams from Mermaid
https://mermaid-to-excalidraw.vercel.app
MIT License
265 stars 46 forks source link

fix: flowchart rendering issues #61

Closed igorwessel closed 5 months ago

igorwessel commented 5 months ago

Closes https://github.com/excalidraw/mermaid-to-excalidraw/issues/45

Revision

8d58faa38229d2b30b2301f1173c85f525d1995b

There are cases where one entity can have multiple relationships to another entity, we were only considering that there was just one relationship.

31fea663c573940876425ed04e95732955d3b4fb

The filter condition was updated to include points that change in either the x or y direction. This ensures that points on a straight line in the same direction are included in the reflectionPoints array, and ensures line commands when only changes direction.

By ignoring the directions that a line command can make we were only having straight lines.

Before After
Captura de Tela 2024-05-06 às 13 48 26 Captura de Tela 2024-05-06 às 13 48 48

Testcases

Captura de Tela 2024-05-06 às 07 18 48
vercel[bot] commented 5 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
mermaid-to-excalidraw ✅ Ready (Inspect) Visit Preview May 16, 2024 5:20am
igorwessel commented 5 months ago

@ad1992 It was also mentioned about escaping HTML. Does it make sense for mermaid-to-excalidraw to bring this as well? I see that this is a bit out of the scope of this PR, it has to be something a bit more global for all diagrams.

ad1992 commented 5 months ago

@ad1992 It was also mentioned about escaping HTML. Does it make sense for mermaid-to-excalidraw to bring this as well? I see that this is a bit out of the scope of this PR, it has to be something a bit more global for all diagrams.

I guess you meant font awesome support ? we don't support it yet, it's out of scope of this PR. We can look into that later.

igorwessel commented 5 months ago

I guess you meant font awesome support ? we don't support it yet, it's out of scope of this PR. We can look into that later.

No but basic support for some HTML tags, for example: <br>, although you could use \n. I don't know to what extent we have possibilities here.

In this issue #45, we have some entities using HTML tags to format labels, although I believe we should not support this.

Captura de Tela 2024-05-07 às 07 03 14
igorwessel commented 5 months ago

The arrows seems to be breaking in this PR 👇🏻

@ad1992 Apparently, the problem is that the penultimate point is very close to the last point. It may be solved by calculating the distance between the penultimate point and the last point, and only including the penultimate point when it is greater than some limit we define. But I feel there might be something more that I'm not seeing at the moment.

Captura de Tela 2024-05-07 às 07 46 06
ad1992 commented 5 months ago

The arrows seems to be breaking in this PR 👇🏻

@ad1992 Apparently, the problem is that the penultimate point is very close to the last point. It may be solved by calculating the distance between the penultimate point and the last point, and only including the penultimate point when it is greater than some limit we define. But I feel there might be something more that I'm not seeing at the moment.

Captura de Tela 2024-05-07 às 07 46 06

@igorwessel this was working earlier for all arrows but not anymore. Since it is breaking post the changes in this PR, we should be able to fix it in this PR itself.

igorwessel commented 5 months ago

@ad1992 I implemented two solutions but I'm still undecided about which is the best approach:

  1. In the second from the last, we check if the previous point in some axis remains the same, if it remains the same we can affirm that it is a straight line either on the X or Y axis. In this case we can just return as false.
  2. In the second from the last the same check for first solution, but we set a minimum distance for it. In this example only second from the last with a distance greather than 50 is included in reflection points.

I sent the commit with both solutions, both have their pros/cons but should work in the majority of cases. Only comment the check for distance for 1 solution (testing some values I think I reached the ideal of 50.)

https://github.com/igorwessel/mermaid-to-excalidraw/blob/13e2a3f50ee891b22cbdb146d15209a0a3692d51/src/utils.ts#L131-L138

And we could check the closure because the problem of repeated edges has returned 😢

igorwessel commented 5 months ago

I'm will pull the updates from main, and create some tests to ensure.

igorwessel commented 5 months ago

@ad1992 friendly reminder, you think this distance calculation solution is enough?

ad1992 commented 5 months ago

@ad1992 friendly reminder, you think this distance calculation solution is enough?

Hi @igorwessel sorry for the delayed response! I am gonna check this soon either tonight or tomorrow morning

igorwessel commented 5 months ago

@ad1992 it's great! I didn't remember there was a function for hypotenuse in Math haha

ad1992 commented 5 months ago

Pushed a fix for excluding point when its same as prev point Merging, thanks @igorwessel ✨