search

Nelarius / imnodes

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

Fix issues with reusing ids from old nodes/attributes #124

Closed Auburn closed 2 years ago

Auburn commented 2 years ago

I had experienced issues with node attributes becoming unresponsive. I tracked the issue down to multiple entries in Pins.Pool sharing the same id, only one of these entries was InUse the rest were stale data.

This was from v0.4 of imnodes but I can't see anything that would have fixed this on HEAD. Here is the debug view of editor.Pins when the issue occurred. Notice how several pins have id=1536 but only 1 is marked InUse. The current state of the node editor at this point was a single node with 2 attributes.

image

When ObjectPoolUpdate is called at the end of every frame it loops over all the pins (including stale) and any not InUse get their id invalidated in the IdMap. This is bad since it invalidates the IdMap for the valid pin with a matching id that is still InUse https://github.com/Nelarius/imnodes/blob/c986af0bbaf45f2adadabca42f48e972e8339dd5/imnodes_internal.h#L357

This change fixes that by checking that the IdMap actually points to the current entry before invalidating it. By doing this you can also avoid having to clear the FreeList every frame and also calling the destructor on not InUse objects every frame too, which C++ classes as undefined behaviour.

I only saw the issue happen with pins, but I don't see why it couldn't happen for the other object pools, so I changed the node specialisation too.

Nelarius commented 2 years ago

Thanks again for fixing flaws of my original code @Auburn 😁 I'll take it!

As a heads-up, I'm currently working on removing the object pools from the code base entirely, and replacing them with simple ImVectors. The goal is to solve id reuse problems, such as the one you just fixed, as well as simplify the code. It's taking a while though, and in the meanwhile this fix is a good quality of life improvement. I'll create a pr eventually, and let you & other contributors know.