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

Bezier connection line not generating correct when connecting diagonally #256

Closed
natsu-k opened this issue May 29, 2020 · 3 comments
Closed

Comments

@natsu-k
Copy link

natsu-k commented May 29, 2020

If you connect a node with handles left and right, to a node with handles facing up and down the bezier curve doesn't generate correct for each use case.

e.g. Connecting TOP to LEFT handles generates correctly, but connecting RIGHT to BOTTOM handles generates a curve the same as LEFT to TOP

image

@moklick
Copy link
Member

moklick commented May 29, 2020

Hey @natsu-k

that's some crazy nodes :) Thanks for the feedback. We will check that.

@natsu-k
Copy link
Author

natsu-k commented May 30, 2020

Thanks for getting back to me @moklick

I just threw that together super quickly to demonstrate what I noticed occurring. I've been really inspired by this library. I've been using it and your example on custom nodes along with a background API to create a version of this that lets you build your node graph within the react app itself and then save it back to json. Things like being able to change text and colour of the nodes, and toggle the 4 handles between "off, source, and target".

I was looking a bit closer at the code behind the bezier curve tonight to see why the output was generating like so.

https://github.com/wbkd/react-flow/blob/39b6106f6f0b08fbe9eaa17a0f62de5c27509f8a/src/components/Edges/BezierEdge.tsx#L28-L33

I noticed the condition:

else if (leftAndRight.includes(sourcePosition) || leftAndRight.includes(targetPosition))

So if either the source or target position (but not both, as that is caught in the previous if statement) are left or right, then generate a bezier curve with both of the centre control points placed at the same location ${sourceX},${targetY}. This would only generate a correct looking curve for half of the possible configurations.

SOURCE position TARGET position Curve
LEFT TOP inverted
LEFT BOTTOM inverted
RIGHT TOP inverted
RIGHT BOTTOM inverted
TOP LEFT correct
TOP RIGHT correct
BOTTOM LEFT correct
BOTTOM RIGHT correct

So it looks like you might be able to resolve it by adding in an extra condition and adjusting the original on Line 31:

    } else if (!(leftAndRight.includes(sourcePosition)) && leftAndRight.includes(targetPosition)) {
      dAttr = `M${sourceX},${sourceY} C${sourceX},${targetY} ${sourceX},${targetY} ${targetX},${targetY}`;
    } else if (leftAndRight.includes(sourcePosition) && !(leftAndRight.includes(targetPosition))) {
      dAttr = `M${sourceX},${sourceY} C${targetX},${sourceY} ${targetX},${sourceY} ${targetX},${targetY}`;
    }

There might be a better way of writing this code, but I just wanted to contribute what I have figured out so far.

@moklick
Copy link
Member

moklick commented May 30, 2020

Thanks for your input! We just released a v3.0.0 (because we had a breaking change regarding custom nodes). The nodes now should be connected correctly: https://react-flow.netlify.app/horizontal

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

No branches or pull requests

2 participants