ImDrawListSplitter::Merge - corrupted data in buffer (64k+ vertices) with 16-bits indices

[thedmd]

(Click "Preview" above ^ to turn URL into clickable links)

ImGui 1.76, vanila

I hit an issue while mixing ImDrawListSplitter, 16-bit ImDrawIdx and large meshes.

< 65536 vertices	> 65536 vertices

I'm using modified backend test from #2591 to reproduce an issue.

I changed it so every rect is drawn on separate channel. When _VtxCurrentOffset is start being used corrupted mesh is produced by Merge().

With 32-bit indices core runs fine.

Not sure if bug is in Merge() or somewhere else. I suspect places where new ImDrawCmd are issued and _VtxCurrentOffset isn't properly populated. I'm looking for solution, but so far not found one yet.

# include <random>
void ShowBackendCheckerWindow(bool* p_open)
{
    if (!ImGui::Begin("Dear ImGui Backend Checker", p_open))
    {
        ImGui::End();
        return;
    }

    ImGuiIO& io = ImGui::GetIO();
    ImGui::Text("Dear ImGui %s Backend Checker", ImGui::GetVersion());
    ImGui::Text("io.BackendPlatformName: %s", io.BackendPlatformName ? io.BackendPlatformName : "NULL");
    ImGui::Text("io.BackendRendererName: %s", io.BackendRendererName ? io.BackendRendererName : "NULL");
    ImGui::Separator();

    if (ImGui::TreeNode("0001: Renderer: Large Mesh Support"))
    {
        ImDrawList* draw_list = ImGui::GetWindowDrawList();
        {
            static int vtx_count = 60000;
            ImGui::SliderInt("VtxCount##1", &vtx_count, 0, 100000);

            static int map[100000 / 4];
            for (int n = 0; n < vtx_count / 4; n++)
                map[n] = n;
            std::srand(13);
            std::random_shuffle(&map[0], &map[vtx_count / 4], [](auto n) { return std::rand() % n; });

            auto* draw_list = ImGui::GetWindowDrawList();

            ImDrawListSplitter splitter;
            splitter.Split(draw_list, 100000 / 4);

            ImVec2 p = ImGui::GetCursorScreenPos();
            for (int n = 0; n < vtx_count / 4; n++)
            {
                splitter.SetCurrentChannel(draw_list, map[n]);

                float off_x = (float)(n % 100) * 3.0f;
                float off_y = (float)(n % 100) * 1.0f;
                ImU32 col = IM_COL32(((n * 17) & 255), ((n * 59) & 255), ((n * 83) & 255), 255);
                draw_list->AddRectFilled(ImVec2(p.x + off_x, p.y + off_y), ImVec2(p.x + off_x + 50, p.y + off_y + 50), col);
            }

            splitter.Merge(draw_list);

            ImGui::Dummy(ImVec2(300 + 50, 100 + 50));
            ImGui::Text("VtxBuffer.Size = %d", draw_list->VtxBuffer.Size);
        }
        {
            static int vtx_count = 60000;
            ImGui::SliderInt("VtxCount##2", &vtx_count, 0, 100000);
            ImVec2 p = ImGui::GetCursorScreenPos();
            for (int n = 0; n < vtx_count / (10*4); n++)
            {
                float off_x = (float)(n % 100) * 3.0f;
                float off_y = (float)(n % 100) * 1.0f;
                ImU32 col = IM_COL32(((n * 17) & 255), ((n * 59) & 255), ((n * 83) & 255), 255);
                draw_list->AddText(ImVec2(p.x + off_x, p.y + off_y), col, "ABCDEFGHIJ");
            }
            ImGui::Dummy(ImVec2(300 + 50, 100 + 20));
            ImGui::Text("VtxBuffer.Size = %d", draw_list->VtxBuffer.Size);
        }
        ImGui::TreePop();
    }

    ImGui::End();
}

Edit: Corrected range of the map to avoid using uninitialized memory for indexing

[ShironekoBen]

This looks like it's an issue with the code in ImDrawListSplitter not correctly preserving the vertex offset when switching channels. An official fix will hopefully be forthcoming soon but in the meantime here's an interim patch that seems to solve the problem:

---
 imgui_draw.cpp | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/imgui_draw.cpp b/imgui_draw.cpp
index d0d6a1e72..795277e2e 100644
--- a/imgui_draw.cpp
+++ b/imgui_draw.cpp
@@ -461,7 +461,7 @@ void ImDrawList::UpdateClipRect()
 
     // Try to merge with previous command if it matches, else use current command
     ImDrawCmd* prev_cmd = CmdBuffer.Size > 1 ? curr_cmd - 1 : NULL;
-    if (curr_cmd->ElemCount == 0 && prev_cmd && memcmp(&prev_cmd->ClipRect, &curr_clip_rect, sizeof(ImVec4)) == 0 && prev_cmd->TextureId == GetCurrentTextureId() && prev_cmd->UserCallback == NULL)
+    if (curr_cmd->ElemCount == 0 && prev_cmd && memcmp(&prev_cmd->ClipRect, &curr_clip_rect, sizeof(ImVec4)) == 0 && prev_cmd->VtxOffset == _VtxCurrentOffset && prev_cmd->TextureId == GetCurrentTextureId() && prev_cmd->UserCallback == NULL)
         CmdBuffer.pop_back();
     else
         curr_cmd->ClipRect = curr_clip_rect;
@@ -480,7 +480,7 @@ void ImDrawList::UpdateTextureID()
 
     // Try to merge with previous command if it matches, else use current command
     ImDrawCmd* prev_cmd = CmdBuffer.Size > 1 ? curr_cmd - 1 : NULL;
-    if (curr_cmd->ElemCount == 0 && prev_cmd && prev_cmd->TextureId == curr_texture_id && memcmp(&prev_cmd->ClipRect, &GetCurrentClipRect(), sizeof(ImVec4)) == 0 && prev_cmd->UserCallback == NULL)
+    if (curr_cmd->ElemCount == 0 && prev_cmd && prev_cmd->TextureId == curr_texture_id && memcmp(&prev_cmd->ClipRect, &GetCurrentClipRect(), sizeof(ImVec4)) == 0 && prev_cmd->VtxOffset == _VtxCurrentOffset && prev_cmd->UserCallback == NULL)
         CmdBuffer.pop_back();
     else
         curr_cmd->TextureId = curr_texture_id;
@@ -1403,6 +1403,8 @@ void ImDrawListSplitter::Merge(ImDrawList* draw_list)
         if (int sz = ch._IdxBuffer.Size) { memcpy(idx_write, ch._IdxBuffer.Data, sz * sizeof(ImDrawIdx)); idx_write += sz; }
     }
     draw_list->_IdxWritePtr = idx_write;
+    if ((draw_list->CmdBuffer.Size > 0) && (draw_list->CmdBuffer.Data[draw_list->CmdBuffer.Size - 1].VtxOffset != draw_list->_VtxCurrentOffset))
+        draw_list->AddDrawCmd(); // If the vertex offset has changed we need a new draw command
     draw_list->UpdateClipRect(); // We call this instead of AddDrawCmd(), so that empty channels won't produce an extra draw call.
     draw_list->UpdateTextureID();
     _Count = 1;
@@ -1413,6 +1415,7 @@ void ImDrawListSplitter::SetCurrentChannel(ImDrawList* draw_list, int idx)
     IM_ASSERT(idx >= 0 && idx < _Count);
     if (_Current == idx)
         return;
+    ImDrawCmd* old_curr_cmd = (draw_list->CmdBuffer.Size > 0) ? &draw_list->CmdBuffer.Data[draw_list->CmdBuffer.Size - 1] : NULL; // The "old" current command
     // Overwrite ImVector (12/16 bytes), four times. This is merely a silly optimization instead of doing .swap()
     memcpy(&_Channels.Data[_Current]._CmdBuffer, &draw_list->CmdBuffer, sizeof(draw_list->CmdBuffer));
     memcpy(&_Channels.Data[_Current]._IdxBuffer, &draw_list->IdxBuffer, sizeof(draw_list->IdxBuffer));
@@ -1420,6 +1423,13 @@ void ImDrawListSplitter::SetCurrentChannel(ImDrawList* draw_list, int idx)
     memcpy(&draw_list->CmdBuffer, &_Channels.Data[idx]._CmdBuffer, sizeof(draw_list->CmdBuffer));
     memcpy(&draw_list->IdxBuffer, &_Channels.Data[idx]._IdxBuffer, sizeof(draw_list->IdxBuffer));
     draw_list->_IdxWritePtr = draw_list->IdxBuffer.Data + draw_list->IdxBuffer.Size;
+    // If the vertex offset or clip rect changed then we need a new draw call (FIXME: What about user callbacks?)
+    if (draw_list->CmdBuffer.Size > 0)
+    {
+        ImDrawCmd* new_curr_cmd = &draw_list->CmdBuffer.Data[draw_list->CmdBuffer.Size - 1]; // The "new" current command
+        if ((new_curr_cmd->VtxOffset != draw_list->_VtxCurrentOffset) || (new_curr_cmd->TextureId != (draw_list->_TextureIdStack.Size ? draw_list->_TextureIdStack.Data[draw_list->_TextureIdStack.Size - 1] : (ImTextureID)NULL)) || (old_curr_cmd && (memcmp(&new_curr_cmd->ClipRect, &old_curr_cmd->ClipRect, sizeof(ImVec4)) != 0)))
+            draw_list->AddDrawCmd();
+    }
 }
 
 //-----------------------------------------------------------------------------

Hope that helps - shout if not and I can take another look.

[thedmd]

So far I managed to fix one issue with pre-allocated draw commands.
If after split threshold of 64k vertices is passed all pre-allocated draw commands have wrong VtxOffset, because _VtxCurrentOffset was bumped in draw list.

This solve problem for happy case, where draws are ordered and channels collapse back producing same output like direct drawing.

void ImDrawListSplitter::SetCurrentChannel(ImDrawList* draw_list, int idx)
{
    IM_ASSERT(idx >= 0 && idx < _Count);
    if (_Current == idx)
        return;
    // Overwrite ImVector (12/16 bytes), four times. This is merely a silly optimization instead of doing .swap()
    memcpy(&_Channels.Data[_Current]._CmdBuffer, &draw_list->CmdBuffer, sizeof(draw_list->CmdBuffer));
    memcpy(&_Channels.Data[_Current]._IdxBuffer, &draw_list->IdxBuffer, sizeof(draw_list->IdxBuffer));
    _Current = idx;
    memcpy(&draw_list->CmdBuffer, &_Channels.Data[idx]._CmdBuffer, sizeof(draw_list->CmdBuffer));
    memcpy(&draw_list->IdxBuffer, &_Channels.Data[idx]._IdxBuffer, sizeof(draw_list->IdxBuffer));
    draw_list->_IdxWritePtr = draw_list->IdxBuffer.Data + draw_list->IdxBuffer.Size;

    // thedmd:
    if (sizeof(ImDrawIdx) == 2 && (draw_list->Flags & ImDrawListFlags_AllowVtxOffset))
    {
        // Each time more than 64k vertices were pushed to buffer
        // ImDrawList reset _VtxCurrentIdx and bump _VtxCurrentOffset.
        // Later is used to initialize new ImDrawCmd instances
        // (see ImDrawList::AddDrawCmd).
        //
        // In case of channels one instance of ImDrawCmd is created
        // for every channel in Split() operation. That mean they
        // may have outdated VtxOffset. So, if last command was not used
        // yet we update VtxOffset, otherwise new draw command.

        ImDrawCmd& last_cmd = draw_list->CmdBuffer.back();
        if (last_cmd.VtxOffset != draw_list->_VtxCurrentOffset)
        {
            if (last_cmd.ElemCount == 0 && !last_cmd.UserCallback)
                last_cmd.VtxOffset = draw_list->_VtxCurrentOffset;
            else
                draw_list->AddDrawCmd();
        }
    }
}

Case when channels are mixed (aka drawing to 7th, 2nd, 5th, 1st and so on) is still corrupted. I suspect Merge with this one. I didn't managed to pin out the issue yet. Any inside would be appreciated.

[thedmd]

@ShironekoBen Thanks, I saw you post after posing mine. I will try your patch.

Edit:
I picked your patch with an exception of changes in SetCurrentChannel where I leaved my changes and output now is looking good:

[thedmd]

@ShironekoBen I can confirm your patch work. I also pushed check for UserCallback in SetCurrentChannel(). Feel free to revert it if you think that was wrong. I think this behavior is correct since I'm don't think two commands with user callbacks can be merged.

[ocornut]

(copy of post in #3163)

Should be fixed now

We spent a long while working on this (thanks @thedmd for the precious help), it's been quite complicated to get it right without altering performance (the initial PR for #3163 had too much overhead in column switching). Because the matter was so confusing we split the work leading to this only many commits:

78d5ccf
e22e3f3
003153b
0320e72
41f47c8
f6120f8
a6bb047
117d57d
e35a813
b1f2eac
3bef743
84862ec

We simplified much of the ImDrawList command handling code, and in turn some code could get further optimizations (columns switch are now faster).