Fattorino / ImNodeFlow

Node based editor/blueprints for ImGui
MIT License
119 stars 15 forks source link

The deletion of a node cause a crash #15

Closed aiekick closed 3 months ago

aiekick commented 4 months ago

Hello,

thanks for this good lib.

btw when you delete a node by using the key "delete", cause a crash.

its because you destroy a node from him internally while iterated from a loop.

m_inf->getNodes().erase(m_uid)

Destroying a node internally via a global pointer is a dangerous behavior, and here cause the issue. after that line, m_inf point on a release memory and cause the crash for next calls after line 186 of ImFlowNode.cpp

for me, you need to mark a node as "to delete" and delete it from the editor after nodes iteration

i have fixed it here (but maybe not the way you like :) )

ps : a demo app branch can be cool for see how to use it and test it

Fattorino commented 4 months ago

Thank you for the suggestion, I'll have a look at your code later today. But for now, I want to direct your attention to BaseNode::destroy() because I think that's what you need. Let me know if this helps you.

EDIT: I'm an idiot, and misread your comment, in my internal testing I had no problems with BaseNode::destroy(), could you provide an example of where the crash occurs in your code?

aiekick commented 4 months ago

its not in my code. when i select a node and press the delete key. the crash is coming.

in your BaseNode::update() code :

if (ImGui::IsKeyPressed(ImGuiKey_Delete) && !ImGui::IsAnyItemActive() && isSelected())
            destroy();

        bool onHeader = ImGui::IsMouseHoveringRect(offset + m_pos - paddingTL, offset + m_pos + headerSize);
        if (onHeader && mouseClickState)
        {
            m_inf->consumeSingleUseClick();
            m_dragged = true;
            m_inf->draggingNode(true);
        }

the crash happen at "m_inf->consumeSingleUseClick();" because the node was destroyed so m_inf point to a bad adress

For me this code is bad, and the crash is logic. So if you dont have the issue, its because you may have another objects who is saving the shared_ptr of m_nodes outside of your lib. this is why you have this curious variable m_destroyed...

in my case, the shared_ptr in only in m_nodes, so the destroy cause the invalidate of the rest of the code and the crash when we call m_inf->what_you_want

Fattorino commented 4 months ago

Ok, now I know what went wrong, when I changed a few things for the latest release I forgot to fix one thing. I'll do it by the end of today

aiekick commented 4 months ago

np, thanks

Fattorino commented 3 months ago

Fixed with commit 8fee461, now included in latest release v1.2.1

aiekick commented 3 months ago

Thanks for your quick fix. and sorry for my agressivity :)

Fattorino commented 3 months ago

don't even worry about it