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

Popup graph_menu on pin drop #10

Closed
kang-sw opened this issue Mar 2, 2024 · 4 comments · Fixed by #12
Closed

Popup graph_menu on pin drop #10

kang-sw opened this issue Mar 2, 2024 · 4 comments · Fixed by #12

Comments

@kang-sw
Copy link
Contributor

kang-sw commented Mar 2, 2024

Currently right clicking on the panel is the only way to add graph menu. It'd be nice to making graph menu open on dragged pin is dropped to empty space of panel. Since we can track from which node what pin is being dragged, it'd be possible to auto-connect newly created node from the menu with the node pin currently being dragged.

Pseudo code:

let pin = await viewer.drag
await pin.dropped

let new_node = await viewer.graph_menu(..)

// Iterate until we find matching pin

if pin.is_input {
    for output_pin in new_node.outputs {
        
        if viewer.try_connect(pin, output_pin) {
            return;
        }
    }
} else {
    for input_pin in new_node.inputs {
        if viewer.try_connect(input_pin, pin) {
            return;
        }
    }
}

// if no pin is matching, it's just okay ...
@zakarumych
Copy link
Owner

zakarumych commented Mar 2, 2024

Yes, it'd be very useful.
SnarlViewer should use separate method to populate that menu with pins that needs to be connected in arguments.
It'll be able to use this context to, for example, list only nodes that may be connected.
And it should perform connections inside this method directly.

Would you like to work on this?

@kang-sw
Copy link
Contributor Author

kang-sw commented Mar 2, 2024

Ah, yes. I'll try. What is your opinion for adding new trait method? I think it'd be useful to provide contextual informations such as, which node/pin is being connected.


Oh; seems your comment is changed😄. I agree with creating separate method.

@kang-sw
Copy link
Contributor Author

kang-sw commented Mar 2, 2024

And for this, it seems 'SnarlViewer::connect()' method does not return any result when invoked. This is mandatory for verifying automatic-connection validity.

Is it okay to change the signature to return Result or boolean? It will be breaking change; and if it's okay to return Result, what should the error type be?

link

    /// Asks the viewer to connect two pins.
    ///
    /// This is usually happens when user drags a wire from one node's output pin to another node's input pin or vice versa.
    /// By default this method connects the pins and returns `Ok(())`.
    #[inline]
    fn connect(&mut self, from: &OutPin, to: &InPin, snarl: &mut Snarl<T>) {
        snarl.connect(from.id, to.id);
    }

And it should perform connections inside this method directly.

If logic goes this way, it seems method signature won't need to be changed.

@zakarumych
Copy link
Owner

SnarlViewer::connect is command to add connection.
Currently there's nothing to do if it fails, or intermediate note is added, or all nodes just vanish in result :)

Result in return can be added if there will be something to do about it.

UI commands should be self-contained. So one UI action should not cause multiple commands.

kang-sw added a commit to kang-sw/egui-snarl that referenced this issue Mar 3, 2024
kang-sw added a commit to kang-sw/egui-snarl that referenced this issue Mar 3, 2024
@kang-sw kang-sw mentioned this issue Mar 3, 2024
7 tasks
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 a pull request may close this issue.

2 participants