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

refactor Bezier function #1984

Merged
merged 8 commits into from
Mar 23, 2022
Merged

Conversation

Sec-ant
Copy link
Contributor

@Sec-ant Sec-ant commented Mar 18, 2022

  • 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: refactor Bezier function #1984 (comment).
  • 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.

default.mp4

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 18, 2022

well, after further reflection, #1981 (comment) , I still think the connection line should behave like this, and sourceCurvature and targetCurvature are not necessary:

default.mp4

@moklick
Copy link
Member

moklick commented Mar 18, 2022

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

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 18, 2022

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

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

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 18, 2022

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:

Edit sandpack-project (forked)

image

@joeyballentine
Copy link
Contributor

Amazing work, thanks for fixing this!

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 21, 2022

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
Copy link
Contributor

Looks great 👍. Hope it gets merged soon

@moklick
Copy link
Member

moklick commented Mar 22, 2022

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
Copy link
Contributor

joeyballentine commented Mar 22, 2022

@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
Copy link
Contributor

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

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 23, 2022

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 moklick merged commit b0bec16 into xyflow:main Mar 23, 2022
@moklick
Copy link
Member

moklick commented Mar 23, 2022

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
Copy link
Contributor Author

Sec-ant commented Mar 23, 2022

Looks good to me 😃

@moklick
Copy link
Member

moklick commented Mar 23, 2022

Released in v10.0.5 🎉

@Sec-ant Sec-ant deleted the refactor-bezier-function branch March 23, 2022 13:29
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.

None yet

3 participants