setzer22 / egui_node_graph

Build your node graph applications in Rust, using egui
MIT License
715 stars 134 forks source link

Port Snapping doesn't take InputParamKind::ConstantOnly into account #54

Open Tsudico opened 2 years ago

Tsudico commented 2 years ago

I made a node that has one input that is InputParamKind::ConstantOnly. If I try to click near an output port of the same type as the input port I crash with the following:

at egui_node_graph-9cc9145316a46463\eeecd63\egui_node_graph\src\editor_ui.rs:189
       187 │                     .iter()
       188 │                     .find_map(|(port_id, _)| {
       189 >                         let port_pos = port_locations[&port_id.into()];
       190 │                         if port_pos.distance(cursor_pos) < DISTANCE_TO_CONNECT {
       191 │                             Some(port_pos)

I can connect from an input back to the output without issues.So it appears that .find_map from line 188 does not exclude port_ids that do not have port_locations associated with them such as the InputParamKind::ConstantOnly, which should be filtered out prior to the mapping of existing locations.

Tsudico commented 2 years ago

Addendum: This affects any output port, no matter the type as long as at least one input in the graph has the InputParamKind::ConstantOnly. Changing the InputParamKind to allow connections makes the crash disappear.

As an aside, I have noticed that without the crash the UI will snap to ports that are not the same color even though it won't actually make a connection. I don't know if this behavior should be standard or if it might confuse users who may think the snap indicates the port is valid.

kkngsm commented 2 years ago

The crashing issue has been corrected. I will PR it later.

As an aside, I have noticed that without the crash the UI will snap to ports that are not the same color even though it won't actually make a connection. I don't know if this behavior should be standard or if it might confuse users who may think the snap indicates the port is valid.

Indeed, I had not thought of this. However, thinking about it again, it is a difficult problem because it would be inconsistent to allow different colors to be connected.

setzer22 commented 2 years ago

This was wrongly closed by GitHub when I merged #59, there is still the pending issue of only snapping to compatible ports with the same color.

setzer22 commented 1 year ago

However, thinking about it again, it is a difficult problem because it would be inconsistent to allow different colors to be connected.

@kkngsm The current system will get reworked once #30 is merged (might take a while) to allow specifying compatibility between different kinds of ports using a custom function. Once we have that function, we'll be able to use it to indicate more complex logic for when two ports are compatible. For the time being, the semantics for this library is that a connection cannot be made between ports of different type.