refactor Bezier function

Sec-ant commented 2 years ago

I rewrote the Bezier function to make it more clear and different positions are handled uniformly.
~~I added the sourceCurvature and targetCurvature to address the connection line problem, where the destination is not known in advance when connecting.~~ Upon reflection I removed these two parameter: https://github.com/wbkd/react-flow/pull/1984#issuecomment-1072294366.
I splitted getBezierPath and getSimpleBezierPath into two functions. As I understand, simple Bezier and a Bezier with curvature equals to 0 are two different shapes. Simple Bezier actually has control points at the center of the edge, while curvature = 0 means the control points coincide with the start or end point. In the existing code, setting curvature to 0 and 0.000001 will create two totally different shapes.

This demo below was when I added sourceCurvature and targetCurvature, and wanted to only specify the curvature at the start point and use0 as the curvature at the end point for the connection line. Later I decided to drop this design.

https://user-images.githubusercontent.com/10386119/158987740-9e1e56a2-1038-4221-8c1b-41fca88b2547.mp4

Sec-ant commented 2 years ago

well, after further reflection, https://github.com/wbkd/react-flow/pull/1981#issuecomment-1072023283 , I still think the connection line should behave like this, and sourceCurvature and targetCurvature are not necessary:

https://user-images.githubusercontent.com/10386119/158988999-4857fa5a-af2c-4832-8864-12de648536ea.mp4

moklick commented 2 years ago

wuut!! The videos look so good ❤️ Thanks @Sec-ant!

Sec-ant commented 2 years ago

wuut!! The videos look so good ❤️ Thanks @Sec-ant!

You're welcome, I'm glad I can help 😉

Sec-ant commented 2 years ago

Just a few more words. Because I published my fork on npm (main branch, but commits in this PR included), you can also test the edges and connection line in this online example, where different positions of source and target handles can be connected to see all kinds of edges:

joeyballentine commented 2 years ago

Amazing work, thanks for fixing this!

Sec-ant commented 2 years ago

The original center calculation of (simple) Bezier edges could not handle mixed-position edges, so I added new functions to calculate the center point of Bezier edges. The arc mid point of a cubic bezier curve is difficult to calculate, so the i use the t=0.5 mid point:

Edge Type	Original	Fixed
Simple Bezier
Bezier (with curvature)

joeyballentine commented 2 years ago

Looks great 👍. Hope it gets merged soon

moklick commented 2 years ago

Hey @Sec-ant

this looks really good! Thanks again for your help.

I have one question. Why do you want to introduce a new prop name for sourceNode and sourceHandle. Do you think that fromNode and fromHandle are more intuitive?

joeyballentine commented 2 years ago

@moklick I believe the reason (he stated it in my closed PR I think) is that since the connection line can technically be from target to source if dragging the other way, it doesn't make sense to name it like that. Instead, from and to make sure the naming is consistent with that it can be in either direction. This is different from the actual edge, which will always be from source to target

joeyballentine commented 2 years ago

Oh, it was actually in #1981 he explained it, in the first comment

Sec-ant commented 2 years ago

In that the connection line can be dragged from either the source or target handle, the source and target naming can be confusing. I suggest using from and to in a connection line, where the from or to can either be a source or a target.

Meanwhile, the source and target passed to the CustomConnectionLineComponent should be consistent with the created edge when the connection is finished.

^ A quote from my last PR, just like what Joey said, @moklick. When you drag a connection line from the target, you don't know the source node, but you always know the from node.

moklick commented 2 years ago

What do you think about these exports:

export {
  default as BezierEdge,
  getBezierPath,
  getBezierCenter as getBezierEdgeCenter,
} from './components/Edges/BezierEdge';

export {
  default as SimpleBezierEdge,
  getSimpleBezierPath,
  getSimpleBezierCenter as getSimpleBezierEdgeCenter,
} from './components/Edges/SimpleBezierEdge';

We already have getEdgeCenter. So I think getBezierEdgeCenter and getSimpleBezierEdgeCenter are consistent names.

Sec-ant commented 2 years ago

Looks good to me 😃

moklick commented 2 years ago

Released in v10.0.5 🎉

xyflow / xyflow

refactor Bezier function #1984