Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

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

Open
Tsudico opened this issue Aug 17, 2022 · 4 comments · Fixed by #59
Open

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

Tsudico opened this issue Aug 17, 2022 · 4 comments · Fixed by #59

Comments

@Tsudico
Copy link

Tsudico commented Aug 17, 2022

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

Tsudico commented Aug 17, 2022

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

kkngsm commented Aug 26, 2022

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

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 added a commit that referenced this issue Sep 15, 2022
This takes care of the second half of #54, where ports snapped
to incompatible colors even when the connection was not possible.
@setzer22
Copy link
Owner

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants