Nelarius / imnodes

A small, dependency-free node editor for dear imgui
MIT License
2.01k stars 244 forks source link

Deleted nodes and links remain selected #76

Open beddueimpossibile opened 3 years ago

beddueimpossibile commented 3 years ago

When a node is selected by clicking it or by drawing a selection box, we can know about it by calling GetSelectedNodes and finding its id in an array of NumSelectedNodes elements. I noticed that, if the node is deleted while selected, those functions still return its id until a new click interaction is performed by the user. While this is not a big problem, I found it out because it generated a little issue for me.

I tried to implement locally a temporary fix, but before sending a PR I would like to have some feedback about the issue itself and the need to solve it. For now I thought it might be useful to simply show here the fix I just tried and wait for people's opinion.

Since NumSelectedNodes must be called after EndNodeEditor, in EndNodeEditor I inserted the following code, starting from line 2175, after pools update and before the call to draw_list_sort_channels_by_depth

//for each selected node
for (int selection_idx = 0; selection_idx < editor.selected_node_indices.Size;
     selection_idx++)
{
    //check if the node is currently in use
    int *node_id_ptr = &editor.selected_node_indices[selection_idx];
    if (!editor.nodes.in_use[*node_id_ptr])
    {
        //if it is not used, then from now on it is also not selected
        editor.selected_node_indices.erase_unsorted(node_id_ptr);
    }
}

Something very similar follows to handle the same issue for the selected links.

francesco-cattoglio commented 3 years ago

This is probably known, in fact there is also a function for clearing the selection of links or nodes: https://github.com/Nelarius/imnodes/blob/master/imnodes.h#L270 The comment itself says: ``Clears the list of selected nodes/links. Useful if you want to delete a selected node or link.'', which to me implies that the user is responsible for doing that in their own code. I think this is reasonable because there might be use cases in which you might want to hide a node for a few frames, and then have it reappear keeping the selection (e.g: making a group of nodes).

beddueimpossibile commented 3 years ago

That's a very useful piece of information that I obviously missed!

It's not the first time that I overlook something here, and I think it's because the way some functionalities are implemented appears a bit confusing to me. Now I start to see the whole picture and it looks like some things are "done" by the library (link creation and selections being two of them), but they must be eventually "undone" by the client, if needed. I am finally aware that this is an issue with my expectations and it will change in the future.