[Bug] Knot not working well with new step connection style, also with backward connection directions (right -> left, bottom -> up) in general

[hendrikp]

Firstly the new step connection is a great addition, however i noticed that with the knot it leads to even more strange looking connections then the knot had before.

Describe the bug

Case A: enabled step connection for pretty normal knot placement (same orientation all connectors)

Case B: connections pointing backward in different linestyles (same orientation all connectors)

B with step: in this case the step even goes through the node, i think it should like the other connections styles first go a bit to the right likely.

To Reproduce See screenshots

Expected behavior The knot input/output should not be on same side and should be based on the actual direction the input is coming from not based on the connector.

Possibly the knot can be fixed for those cases to not expect input to be left or top, but dynamically adjusting itself to the actual connection direction.

Also in general it would be good that placing a knot never causes a connection to cross/loop with itself.

[miroiu]

I created a PR to fix this issue, and also added vertical orientation for knot nodes. I'm considering removing Direction and Orientation from connections and adding SourcePosition and TargetPosition instead. This way we can have this type of flow for all connection styles:

As you can see in the picture above there's still an issue with the connector offset when both connectors have the same position. This is because the offset is currently calculated based on Direction and Orientation.

Please let me know if we can close this issue.

[hendrikp]

hm yeah i have the orientation for my playground actually not on the Node, but on the Connectors themselves (because some Blocks can have a mix of vertical and horizontal connectors connections),

    public class FlowToConnectorPositionConverter : IValueConverter
    {
        public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
        {
            if (value is ConnectorViewModel connector)
            {
                if (connector.Orientation == Orientation.Horizontal)

(not connector.Node.Orientation) So yeah it'd recommend also to remove Orientation from Nodes, and move it to Connector, or having it in both places, but the Nodes one being more of a hint for the node generator/display of the playground sample itself.

For the knots themselves the approach you showed seems good.

Also i think in general it would be good for the connections control points that they maybe take the target/source block size into account so that they dont go through their associated nodes (happens often with back reference when e.g. using circuit)

I will test now.

[hendrikp]

For reference thats how im doing it on the knotviewmodel, i have an OnUpdateLayout function there that is called when something disconnects/connects to the knot.

    public class KnotViewModel ....
    {
....
        public override void OnUpdateLayout()
        {
            base.OnUpdateLayout();

            // Check if vertically oriented 
            var startVertical = Connector.Connections.FirstOrDefault(c =>
                c.Input.Orientation == Orientation.Vertical && c.Input.Node != this);

            if (startVertical != null)
            {
                Connector.Orientation = Orientation.Vertical;
            }
            else
            {
                Connector.Orientation = Orientation.Horizontal;
            }
        }

[miroiu]

I'd recommend also to remove Orientation from Nodes, and move it to Connector, or having it in both places, but the Nodes one being more of a hint for the node generator/display of the playground sample itself

I may consider doing this in the future, but for now, I find the playground app to be good enough for testing new features