Fattorino / ImNodeFlow

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

[MSVC compiler] Cleanup crash when having one OutPin connected with multiple InPin's #25

Open RoBaertschi opened 1 month ago

RoBaertschi commented 1 month ago

Version: master branch ImGui: 1.90.5-docking

Issue

Connecting a OutPin with 2 or more InPin's and then close the program. The deconstructor from ImFlow::Link produces a "Access violation on reading location ...".

image

One fix that I tried, was to replace m_left with m_right but that seems to break other things, like the node deletion logic.

Link::~Link() {
-    m_left->deleteLink();
+    m_right->deleteLink();
}

Code

I minimized my code to be still functional and replicate the issue here. It is not the most good looking code, but should be readable enough.

Also, I added all c++ code in the spoilers following.

main.cpp

```c++ #include "Window.h" int main() { CWindow window{}; window.run(); return 0; } ```

Window.h

```c++ #ifndef BOOLEANTABLE_WINDOW_H #define BOOLEANTABLE_WINDOW_H #include #include #include #include #include #include #include const int cWinHeight = 400; const int cWinWidth = 600; class CWindow { SDL_Window* window; SDL_GLContext context; bool running = true; std::shared_ptr imNodeFlow; std::vector variables; public: explicit CWindow(); ~CWindow(); void run(); }; #endif //BOOLEANTABLE_WINDOW_H ```

Window.cpp

```c++ #include "Window.h" #include "nodes/AndNode.h" CWindow::CWindow() : imNodeFlow(std::make_shared("Main Grid")) { if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_TIMER | SDL_INIT_GAMEPAD) != 0) { throw std::runtime_error("Could not initialize SDL3"); } SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1); SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, 4); SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 5); SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK, SDL_GL_CONTEXT_PROFILE_CORE); SDL_SetHint(SDL_HINT_IME_SHOW_UI, "1"); SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1); SDL_GL_SetAttribute(SDL_GL_DEPTH_SIZE, 24); SDL_GL_SetAttribute(SDL_GL_STENCIL_SIZE, 8); window = SDL_CreateWindow("Boolean Tables", cWinWidth, cWinHeight, SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE); if (window == nullptr) { throw std::runtime_error("Could not create a SDL3 Window"); } context = SDL_GL_CreateContext(window); if (context == nullptr) { throw std::runtime_error("Could not create a OpenGL Context"); } SDL_GL_MakeCurrent(window, context); SDL_GL_SetSwapInterval(1); int gl_version = gladLoadGL(SDL_GL_GetProcAddress); if (gl_version == 0) { throw std::runtime_error("Could not initialize OpenGL context"); } IMGUI_CHECKVERSION(); ImGui::CreateContext(); ImGuiIO& io = ImGui::GetIO(); (void)io; io.ConfigFlags |= ImGuiConfigFlags_NavEnableKeyboard; io.ConfigFlags |= ImGuiConfigFlags_NavEnableGamepad; io.ConfigFlags |= ImGuiConfigFlags_DockingEnable; io.ConfigFlags |= ImGuiConfigFlags_ViewportsEnable; ImGui::StyleColorsDark(); // When viewports are enabled we tweak WindowRounding/WindowBg so platform windows can look identical to regular ones. ImGuiStyle& style = ImGui::GetStyle(); if (io.ConfigFlags & ImGuiConfigFlags_ViewportsEnable) { style.WindowRounding = 0.0f; style.Colors[ImGuiCol_WindowBg].w = 1.0f; } // Setup Platform/Renderer backends ImGui_ImplSDL3_InitForOpenGL(window,context); ImGui_ImplOpenGL3_Init(); imNodeFlow->addNode(ImVec2{2, 2}); imNodeFlow->droppedLinkPopUpContent([this](ImFlow::Pin *pin) { if (ImGui::Button("And")) { auto node = this->imNodeFlow->placeNode(); pin->createLink(node->first.get()); } }); } CWindow::~CWindow() { imNodeFlow.reset(); ImGui_ImplOpenGL3_Shutdown(); ImGui_ImplSDL3_Shutdown(); ImGui::DestroyContext(); SDL_GL_DeleteContext(context); SDL_DestroyWindow(window); SDL_Quit(); } void CWindow::run() { ImGuiIO& io = ImGui::GetIO(); (void)io; while(running) { for(SDL_Event event; SDL_PollEvent(&event);) { ImGui_ImplSDL3_ProcessEvent(&event); switch (event.type) { case SDL_EVENT_QUIT: running = false; return; case SDL_EVENT_WINDOW_CLOSE_REQUESTED: if (event.window.windowID == SDL_GetWindowID(window)) { running = false; return; } break; default: break; } } ImGui_ImplOpenGL3_NewFrame(); ImGui_ImplSDL3_NewFrame(); ImGui::NewFrame(); ImGui::Begin("Boolean Editor"); imNodeFlow->update(); ImGui::End(); ImGui::Render(); glViewport(0, 0, (int)io.DisplaySize.x, (int)io.DisplaySize.y); glClearColor(0.f, 0.f, 0.f, 1.f); glClear(GL_COLOR_BUFFER_BIT); ImGui_ImplOpenGL3_RenderDrawData(ImGui::GetDrawData()); if (io.ConfigFlags & ImGuiConfigFlags_ViewportsEnable) { SDL_Window* backup_current_window = SDL_GL_GetCurrentWindow(); SDL_GLContext backup_current_context = SDL_GL_GetCurrentContext(); ImGui::UpdatePlatformWindows(); ImGui::RenderPlatformWindowsDefault(); SDL_GL_MakeCurrent(backup_current_window, backup_current_context); } SDL_GL_SwapWindow(window); } } ```

AndNode.h

```c++ #ifndef BOOLEANTABLE_ANDNODE_H #define BOOLEANTABLE_ANDNODE_H #include class CAndNode : public ImFlow::BaseNode { public: std::shared_ptr> first; std::shared_ptr> second; CAndNode(); }; #endif //BOOLEANTABLE_ANDNODE_H ```

AndNode.cpp

```c++ #ifndef BOOLEANTABLE_ANDNODE_H #define BOOLEANTABLE_ANDNODE_H #include class CAndNode : public ImFlow::BaseNode { public: std::shared_ptr> first; std::shared_ptr> second; CAndNode(); }; #endif //BOOLEANTABLE_ANDNODE_H ```

Fattorino commented 1 month ago

Thanks for the detailed issue. I haven't been able to reproduce the issue in the non-docking branch of ImGui, I'll have to try again with docking another day.

Also please note that in this minimal example, the end node will always return true, and it's also not taking advantage of the full potential of getInVal<>()

RoBaertschi commented 1 month ago

I haven't been able to reproduce the issue in the non-docking branch of ImGui, I'll have to try again with docking another day.

That is weird, I just moved to the non docking branch of imgui and that does not change anything. If you want to test that, I created a new branch in the repo. Also I switched to uids, but didn't change anything.

Maybe something is wrong with my setup, but I would not know what.

RoBaertschi commented 1 month ago

I recorded a demo of what is happening here, maybe it could help with understanding what is going wrong.

https://github.com/Fattorino/ImNodeFlow/assets/72601088/d5ba8ade-889b-4e84-b9d1-19b7a230ca48

RoBaertschi commented 1 month ago

So, I did some testing and found something. When I compile the program with MSVC, then it will crash. But with mingw, it does not crash and I don't know why. Also, using LLVM gives another error (These green bars can be ignored, they seem to come from some sort of other issue when taking the screenshot): image Maybe this could help, it is probably a MSVC issue.

eminor1988 commented 1 week ago

I have encountered the situation recently, too.

problem

I guess the root cause is that ImNodeFlow::m_nodes are destructed in any order when ImNodeFlow is destructing. Therefore, the instance of OutPin will be destructed before the Link.


The problem has gone after I added:

ImNodeFlow.h

~ImNodeFlow() { removeLinks(); }

void removeLinks();

ImNodeFlow.cpp

  void ImNodeFlow::removeLinks()
  {
      for (auto& itr : m_nodes) {
          for (auto& inPin : itr.second->getIns()) {
              inPin->deleteLink();
          }
      }
  }

Can you please see if this can solve your problem?

Fattorino commented 6 days ago

What compiler are you using?

eminor1988 commented 6 days ago

Visual C++ 2022 17.10.3

Compiler: MSVC 19.40.33811.0 on Windows-10.0.19045

Fattorino commented 5 days ago

I've implemented something similar with the latest commit in the dev branch. Could you check if it works?

eminor1988 commented 4 days ago

I tested it. The problem is still there.


The callstack:

problem2


I consider that because Link::m_left (OutPin) is already destructed with ~BaseNode() before the Link destructing.

problem3