[BUG] Invalid selected nodes

[brukted]

Recently I tried a node editor that is able to delete selected nodes. The problem is that imnodes uses the node index to keep track of selected nodes which causes from assertion fails to invalid nodes being reported as selected. The solution I took was to clear the selection every time I deleted a selected node. I can make a pr if you like the solution.

[Nelarius]

Oh drat, that is a wrinkle I haven't thought about 🤔 If you have some working code which fixes this, feel free to submit a pr!

[brukted]

A better solution would be to track selected nodes using their id instead of index and removing the id from the list at the EndNodeEditor() if the node was not drawn.

[potrepka]

Related to this, but not exactly the same issue (although likely related to the second comment by @brukted), is that when I delete a node, then create a new node and select it, the selected node index refers to the index of the node I just deleted.

Here is an example: I create two nodes with index 1, 3. Then I delete them. Then I create two more nodes and select them. GetSelectedNodes still refers to 1, 3 even though I am using 6, 9 when I render the nodes in my graphics code.

[sonoro1234]

A better solution would be to track selected nodes using their id instead of index and removing the id from the list at the EndNodeEditor() if the node was not drawn.

Yes, GetSelectedNodes is returning ids from nodes already deleted. I dont understand why a node needs to have an unique id but internally has also an index. Being id unique it seems that index is totally avoidable (Althought I dont know the codebase enough). Looking the code it seems that node_idx is needed because the pool of nodes is an ImVector so that correlative node_idx are needed.

IsNodeHovered also reports wrong ids: when a node is deleted and a new one created the old one id is reported.

The solution I took was to clear the selection every time I deleted a selected node. I can make a pr if you like the solution.

@brukted If it solves the issue anyhow, please make this PR. How should be your void ClearNodeSelection(); used to avoid the bug? If you use it every time you delete a selected node, when there are several nodes selected only the first one would be found?

[brukted]

A better solution would be to track selected nodes using their id instead of index and removing the id from the list at the EndNodeEditor() if the node was not drawn.

Yes, GetSelectedNodes is returning ids from nodes already deleted. I dont understand why a node needs to have an unique id but internally has also an index. Being id unique it seems that index is totally avoidable (Althought I dont know the codebase enough). Looking the code it seems that node_idx is needed because the pool of nodes is an ImVector so that correlative node_idx are needed.

The solution I took was to clear the selection every time I deleted a selected node. I can make a pr if you like the solution.

@brukted If it solves the issue anyhow, please make this PR. How should be your void ClearNodeSelection(); used to avoid the bug? If you use it every time you delete a selected node, when there are several nodes selected only the first one would be found?

@sonoro1234 You should call ClearNodeSelection() every time you delete a node that is selected, If you delete a node from the selected nodes calling ClearNodeSelection() would unselect the other nodes too.

[sonoro1234]

You should call ClearNodeSelection() every time you delete a node that is selected, If you delete a node from the selected nodes calling ClearNodeSelection() would unselect the other nodes too.

@brukted I have tried calling ClearNodeSelection() after GetSelectedNodes and deletion without success: calling GetSelectedNodes when there is a new selection gives me ids that were already deleted.

@Nelarius Just made a PR for the solution working for me.

[WookeyBiscotti]

Hi, guys i fix this. The bug appeared if the new node was created using memory of the node from free_nodes, in this case the node id was not updated

[Nelarius]

Ok, finally spent some time trying to get to the bottom of this. Thanks for the patience! 🙏

Like some of you already noticed, and submitted fixes for, there was an oversight in the way nodes were getting recycled from the free list. I took @sonoro1234 's fix since it was the simplest (and that was actually the way the id was written to memory before I got all fancy in the object pool...) Massive thanks!

The following frame after a node gets deleted, the function box_selector_update_selection clears and rebuilds the selection, and it will take in to account any nodes that the user deleted/left out. @brukted Does that work for you, or is there some other reason for the pull request that I've missed? https://github.com/Nelarius/imnodes/pull/63

[Nelarius]

@sonoro1234 Some extra context, since you were curious about the usage of indices instead of ids. You're right that both the ids and indices are unique and seem redundant. But I wanted the user to be able to identify nodes (and other elements) with non-contiguous integers, since that seemed the least restrictive. Internally though, I wanted to store everything in a contiguous container for the sake of simplicity. Node/link/pin indices are used all over the place instead of direct pointers/references for memory safety reasons, since a container's backing memory might change location. Node indices are often cached internally to minimize the number of id->index lookups.

Maybe it could work differently. The code has grown quite a bit more complicated in the two years since I started working on it, and particularly in the last couple of months, I've managed to break a number of things while making other changes, causing all kinds of regressions 😅 So, going forward, I think I really need to figure out a better way to test the full functionality of the UI when making changes.

[potrepka]

I prefer @WookeyBiscotti 's fix because it handles everything related to node_id within object_pool_find_or_create_index as early as possible. Logically, I would think that's where node_id should be set, or else why would you pass it in as an argument?

[brukted]

@Nelarius #63 worked for me. Other reason to merge it would be to give option to clear node and link selection.

[Nelarius]

@nspotrepka object_pool_find_or_create_index needs the node id whether we set it or not, since we need it to figure out if we should reuse an element, or append a new element to the storage vector. But you're right in that it would be more elegant to set the ids in the same place. I would probably go one step further than @WookeyBiscotti 's pr and use the NodeData constructor also in the else branch as well, so that the nodes are guaranteed to be in the same state, whether from the free list or not. That fact that they're not is probably a bug waiting to happen now that I think about it 😬

@brukted Good point, I suppose it would be nice to able to programmatically clear the selection as well! 👍

[potrepka]

@Nelarius It's worth mentioning that there is an almost identical function object_pool_find_or_create_index for type T right above that most likely needs fixing, too.

[Nelarius]

Yep, it does 😄