Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suspend & Resume may lead to corrupted ImDrawCmd in some cases #282

Open
pthom opened this issue Mar 4, 2024 · 0 comments
Open

Suspend & Resume may lead to corrupted ImDrawCmd in some cases #282

pthom opened this issue Mar 4, 2024 · 0 comments

Comments

@pthom
Copy link

pthom commented Mar 4, 2024

Hello Michał,

I encountered another issue with Suspend and Resume: in some cases, the ImDrawData CmdBuffer might be corrupted.

I put together a small repro, which you can find on the suspend_resume_issue2 branch of the same repro repository I used before (please use the suspend_resume_issue2 branch)

Here is a quick summary of what happens:

I use the node editor in a simple way, and I use suspend and resume to create popups:

        static ed::NodeId node_id = 1;

        BeginMainWindow();        // See explanations below

        ed::Begin("Graph");

        ed::BeginNode(node_id);

        if (ImGui::Button("O"))
        {
            ed::Suspend();
            ImGui::OpenPopup("expandable_str_popup");
            ed::Resume();
        }

        ed::Suspend();
        if (ImGui::BeginPopup("expandable_str_popup"))
        {
            ImGui::Text("AAAA");
            ImGui::EndPopup();
        }
        ed::Resume();

        ed::EndNode();

        ed::End();

        EndMainWindow();

And I encounter a bug inside ImGui_ImplOpenGL3_RenderDrawData

An issue is triggered at startup inside void ImGui_ImplOpenGL3_RenderDrawData(ImDrawData* draw_data)

void    ImGui_ImplOpenGL3_RenderDrawData(ImDrawData* draw_data)
{

   ...

    for (int cmd_i = 0; cmd_i < cmd_list->CmdBuffer.Size; cmd_i++)
    {
        const ImDrawCmd* pcmd = &cmd_list->CmdBuffer[cmd_i];
        if (pcmd->UserCallback != nullptr)
        {
            // User callback, registered via ImDrawList::AddCallback()
            // (ImDrawCallback_ResetRenderState is a special callback value used by the user to request the renderer to reset render state.)
            if (pcmd->UserCallback == ImDrawCallback_ResetRenderState)
                ImGui_ImplOpenGL3_SetupRenderState(draw_data, fb_width, fb_height, vertex_array_object);
            else
                pcmd->UserCallback(cmd_list, pcmd);     // <== the bug is triggered here
        }

When the bug occurs, here is the content of pcmd

pcmd = {const ImDrawCmd *} 0x15a63c860 
 ClipRect = {ImVec4} 
 TextureId = {ImTextureID} 0x1 
 VtxOffset = {unsigned int} 0
 IdxOffset = {unsigned int} 930
 ElemCount = {unsigned int} 0
 UserCallback = {ImDrawCallback} 0xfffffffffffffffe        // This is a bad pointer 
 UserCallbackData = {void *} NULL

In order to trigger the bug, I had to create the main background window with several flags:

    void BeginMainWindow()
    {
        // the following code will create a background window that will cover the entire viewport
        // The bug is quite subtle and needs all the flags below to be set

        ImGuiViewport* viewport = ImGui::GetMainViewport();

        ImGui::SetNextWindowPos(viewport->Pos);
        ImVec2 viewportSize = viewport->Size;
        ImGui::SetNextWindowSize(viewportSize);
        ImGui::SetNextWindowViewport(viewport->ID);
        ImGui::SetNextWindowBgAlpha(0.0f);

        ImGui::PushStyleVar(ImGuiStyleVar_WindowRounding, 0.0f);
        ImGui::PushStyleVar(ImGuiStyleVar_WindowBorderSize, 0.0f);
        ImGui::PushStyleVar(ImGuiStyleVar_WindowPadding, ImVec2(0.0f, 0.0f));
        static bool p_open = true;

        ImGuiWindowFlags window_flags = 0;
        window_flags = window_flags
            | ImGuiWindowFlags_NoDocking
            | ImGuiWindowFlags_NoTitleBar
            | ImGuiWindowFlags_NoCollapse
            | ImGuiWindowFlags_NoResize
            | ImGuiWindowFlags_NoMove
            | ImGuiWindowFlags_NoScrollbar
            | ImGuiWindowFlags_NoSavedSettings
            | ImGuiWindowFlags_NoBringToFrontOnFocus
            | ImGuiWindowFlags_NoNavFocus
            ;

        ImGui::Begin("Main window", & p_open, window_flags);
        ImGui::PopStyleVar(3);

    }

In a sense, this might be related to of 205, since there were some discussions there about ImDrawCmd handling, and I think we are experiencing an issue around this here also.

Cheers!

pthom added a commit to pthom/fiatlight that referenced this issue Mar 8, 2024
pthom added a commit to pthom/imgui that referenced this issue Mar 19, 2024
… sentinel callback

                // Patch for imgui-node-editor, which sometimes leaves
                // its sentinel callback ImDrawCallback_ImCanvas on the command list
                // (ImDrawCallback_ImCanvas is actually -2 cast to ImDrawCallback
                // and thus, not a valid pointer, since it equals to 0xfffffffffffffffe
                // See issues:
                //     thedmd/imgui-node-editor#282
                // and thedmd/imgui-node-editor#205
                // + partial solution: thedmd/imgui-node-editor@af7fa51
                bool isUserCallbackAddressValid = (
                    reinterpret_cast<uintptr_t>(pcmd->UserCallback) < 0xffffffffffffff00
                );
pthom added a commit to pthom/imgui-node-editor that referenced this issue Mar 20, 2024
…tFirstCommandIndex (proposed fix for thedmd#282)

        Analysis of the bug thedmd#282
        when using this repro: https://github.com/pthom/node_window_clipping_issue/tree/suspend_resume_issue2

        At the 4th call of LeaveLocalSpace(), we have:

            this = {ImGuiEx::Canvas *}
                 m_DrawList = {ImDrawList *}                      mDrawList is of size 4
                     CmdBuffer = {ImVector<ImDrawCmd>}            its elements at index 2
                         Size = {int} 4                           has UserCallback == {ImDrawCallback_ImCanvas}
                         Capacity = {int} 8
                         Data = {ImDrawCmd *}
                 m_ExpectedChannel = {int} 0
                 m_DrawListFirstCommandIndex = {int} 2
                 m_DrawListCommadBufferSize = {int} 1
                 m_DrawListStartVertexIndex = {int} 8

         We have m_DrawListFirstCommandIndex = 2, however, the tests in the original code will test only at index 0 and 1!
                if (m_DrawList->CmdBuffer.size() > m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize);
                else if (m_DrawList->CmdBuffer.size() >= m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize - 1].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize - 1);

        // Proposed solution: test all commands from index >= m_DrawListFirstCommandIndex
        // and remove the one with UserCallback == ImDrawCallback_ImCanvas
        // (based on the original code, it seems there can be only one)
        int idxCommand_ImDrawCallback_ImCanvas = -1;
        for (int i = m_DrawListFirstCommandIndex; i < m_DrawList->CmdBuffer.size(); ++i)
        {
            auto & command = m_DrawList->CmdBuffer[i];
            if (command.UserCallback == ImDrawCallback_ImCanvas)
            {
                idxCommand_ImDrawCallback_ImCanvas = i;
                break;
            }
        }
        if (idxCommand_ImDrawCallback_ImCanvas >= 0)
            m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + idxCommand_ImDrawCallback_ImCanvas);
pthom added a commit to pthom/imgui-node-editor that referenced this issue Mar 20, 2024
…tFirstCommandIndex (proposed fix for thedmd#282)

        Analysis of the bug thedmd#282
        when using this repro: https://github.com/pthom/node_window_clipping_issue/tree/suspend_resume_issue2

        At the 4th call of LeaveLocalSpace(), we have:

            this = {ImGuiEx::Canvas *}
                 m_DrawList = {ImDrawList *}                      mDrawList is of size 4
                     CmdBuffer = {ImVector<ImDrawCmd>}            its elements at index 2
                         Size = {int} 4                           has UserCallback == {ImDrawCallback_ImCanvas}
                         Capacity = {int} 8
                         Data = {ImDrawCmd *}
                 m_ExpectedChannel = {int} 0
                 m_DrawListFirstCommandIndex = {int} 2
                 m_DrawListCommadBufferSize = {int} 1
                 m_DrawListStartVertexIndex = {int} 8

         We have m_DrawListFirstCommandIndex = 2, however, the tests in the original code will test only at index 0 and 1!
                if (m_DrawList->CmdBuffer.size() > m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize);
                else if (m_DrawList->CmdBuffer.size() >= m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize - 1].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize - 1);

        // Proposed solution: test all commands from index >= m_DrawListFirstCommandIndex
        // and remove the one with UserCallback == ImDrawCallback_ImCanvas
        // (based on the original code, it seems there can be only one)
        int idxCommand_ImDrawCallback_ImCanvas = -1;
        for (int i = m_DrawListFirstCommandIndex; i < m_DrawList->CmdBuffer.size(); ++i)
        {
            auto & command = m_DrawList->CmdBuffer[i];
            if (command.UserCallback == ImDrawCallback_ImCanvas)
            {
                idxCommand_ImDrawCallback_ImCanvas = i;
                break;
            }
        }
        if (idxCommand_ImDrawCallback_ImCanvas >= 0)
            m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + idxCommand_ImDrawCallback_ImCanvas);
pthom added a commit to pthom/imgui_bundle that referenced this issue Mar 20, 2024
pthom added a commit to pthom/imgui-node-editor that referenced this issue May 12, 2024
…tFirstCommandIndex (proposed fix for thedmd#282)

        Analysis of the bug thedmd#282
        when using this repro: https://github.com/pthom/node_window_clipping_issue/tree/suspend_resume_issue2

        At the 4th call of LeaveLocalSpace(), we have:

            this = {ImGuiEx::Canvas *}
                 m_DrawList = {ImDrawList *}                      mDrawList is of size 4
                     CmdBuffer = {ImVector<ImDrawCmd>}            its elements at index 2
                         Size = {int} 4                           has UserCallback == {ImDrawCallback_ImCanvas}
                         Capacity = {int} 8
                         Data = {ImDrawCmd *}
                 m_ExpectedChannel = {int} 0
                 m_DrawListFirstCommandIndex = {int} 2
                 m_DrawListCommadBufferSize = {int} 1
                 m_DrawListStartVertexIndex = {int} 8

         We have m_DrawListFirstCommandIndex = 2, however, the tests in the original code will test only at index 0 and 1!
                if (m_DrawList->CmdBuffer.size() > m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize);
                else if (m_DrawList->CmdBuffer.size() >= m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize - 1].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize - 1);

        // Proposed solution: test all commands from index >= m_DrawListFirstCommandIndex
        // and remove the one with UserCallback == ImDrawCallback_ImCanvas
        // (based on the original code, it seems there can be only one)
        int idxCommand_ImDrawCallback_ImCanvas = -1;
        for (int i = m_DrawListFirstCommandIndex; i < m_DrawList->CmdBuffer.size(); ++i)
        {
            auto & command = m_DrawList->CmdBuffer[i];
            if (command.UserCallback == ImDrawCallback_ImCanvas)
            {
                idxCommand_ImDrawCallback_ImCanvas = i;
                break;
            }
        }
        if (idxCommand_ImDrawCallback_ImCanvas >= 0)
            m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + idxCommand_ImDrawCallback_ImCanvas);
pthom added a commit to pthom/fiatlight that referenced this issue Jun 19, 2024
pthom added a commit to pthom/imgui-node-editor that referenced this issue Sep 10, 2024
…tFirstCommandIndex (proposed fix for thedmd#282)

        Analysis of the bug thedmd#282
        when using this repro: https://github.com/pthom/node_window_clipping_issue/tree/suspend_resume_issue2

        At the 4th call of LeaveLocalSpace(), we have:

            this = {ImGuiEx::Canvas *}
                 m_DrawList = {ImDrawList *}                      mDrawList is of size 4
                     CmdBuffer = {ImVector<ImDrawCmd>}            its elements at index 2
                         Size = {int} 4                           has UserCallback == {ImDrawCallback_ImCanvas}
                         Capacity = {int} 8
                         Data = {ImDrawCmd *}
                 m_ExpectedChannel = {int} 0
                 m_DrawListFirstCommandIndex = {int} 2
                 m_DrawListCommadBufferSize = {int} 1
                 m_DrawListStartVertexIndex = {int} 8

         We have m_DrawListFirstCommandIndex = 2, however, the tests in the original code will test only at index 0 and 1!
                if (m_DrawList->CmdBuffer.size() > m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize);
                else if (m_DrawList->CmdBuffer.size() >= m_DrawListCommadBufferSize && m_DrawList->CmdBuffer[m_DrawListCommadBufferSize - 1].UserCallback == ImDrawCallback_ImCanvas)
                    m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + m_DrawListCommadBufferSize - 1);

        // Proposed solution: test all commands from index >= m_DrawListFirstCommandIndex
        // and remove the one with UserCallback == ImDrawCallback_ImCanvas
        // (based on the original code, it seems there can be only one)
        int idxCommand_ImDrawCallback_ImCanvas = -1;
        for (int i = m_DrawListFirstCommandIndex; i < m_DrawList->CmdBuffer.size(); ++i)
        {
            auto & command = m_DrawList->CmdBuffer[i];
            if (command.UserCallback == ImDrawCallback_ImCanvas)
            {
                idxCommand_ImDrawCallback_ImCanvas = i;
                break;
            }
        }
        if (idxCommand_ImDrawCallback_ImCanvas >= 0)
            m_DrawList->CmdBuffer.erase(m_DrawList->CmdBuffer.Data + idxCommand_ImDrawCallback_ImCanvas);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant