Multithreaded ImDrawList usage causing flicks

[Skreamex]

Version/Branch of Dear ImGui:

Version: v1.89.3
Branch: features/shadows and master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_dx9.cpp + imgui_impl_win32.cpp
Operating System: Windows 10

My Issue/Question:

Hello. I was using 1.84 WIP to use separated drawlists.
Fist thread fills some draw data to draw list, locks mutex and copies to rendering list
Main d3d9 thread
Begin() function

m_draw_list->_ResetForNewFrame();
m_draw_list->PushTextureID(ImGui::GetIO().Fonts->TexID);
m_draw_list->PushClipRectFullScreen();

End() function

render_mutex.lock();
*m_draw_list_act = *m_draw_list;
render_mutex.unlock();

Get data to draw

ImDrawList* CD3DRender::GetScene() {

    if (render_mutex.try_lock()) {
      *m_draw_list_rendering = *m_draw_list_act;
      render_mutex.unlock();
    }
    
    return m_draw_list_rendering;
}

D3D call looks like

ImGui::EndFrame();
ImGui::Render(pRenderer->GetScene());

void ImGui::Render(ImDrawList* list)
{
    ImGuiContext& g = *GImGui;
    IM_ASSERT(g.Initialized);

    if (g.FrameCountEnded != g.FrameCount)
        EndFrame();
    const bool first_render_of_frame = (g.FrameCountRendered != g.FrameCount);
    g.FrameCountRendered = g.FrameCount;
    g.IO.MetricsRenderWindows = 0;

    CallContextHooks(&g, ImGuiContextHookType_RenderPre);

    // Add background ImDrawList (for each active viewport)
    for (int n = 0; n != g.Viewports.Size; n++)
    {
        ImGuiViewportP* viewport = g.Viewports[n];
        viewport->DrawDataBuilder.Clear();
        if (viewport->DrawLists[0] != NULL)
            AddDrawListToDrawData(&viewport->DrawDataBuilder.Layers[0], GetBackgroundDrawList(viewport));

       // Add data from other thread
        if(!list->CmdBuffer.empty())
            AddDrawListToDrawData(&viewport->DrawDataBuilder.Layers[0], list);
    }
   . . .

After upgrade I ran into problem that all vertexes are flickering
I've started to search what causes it and after some compares realized that ImDrawListSharedData has new member

ImVector<ImVec2> TempBuffer;

So it means two thread accesses same TempBuffer variable in same time

I've done simpe quick fix to solve problem
TempBuffer member now function

ImVector<ImVec2>& ImDrawListSharedData::TempBuffer() {
	static std::unordered_map<unsigned long, ImVector<ImVec2>*> frame_buffers;
	auto thread_id = GetCurrentThreadId();
	auto it = frame_buffers.find(thread_id);

	if (it != frame_buffers.end()) {
		return *(it->second);
	}
	else {
		auto inst = new ImVector<ImVec2>();
		frame_buffers[thread_id] = inst;
		return *inst;
	}
}

And some replacements like

void ImDrawList::AddPolyline(const ImVec2* points, const int points_count, ImU32 col, ImDrawFlags flags, float thickness)
{
...
 _Data->TempBuffer().reserve_discard(points_count * ((use_texture || !thick_line) ? 3 : 5));
...
}

Now it works fine without any glitches

[ocornut]

So it means two thread accesses same TempBuffer variable in same time

Only if actual AddXXX functions are called at the same time, which never worked even before (as they would manipulate VtxBuffer IdxBuffer etc.) so I don't think this is your issue and you are looking at and fixing a wrong thing.

FYI this is likely incorrect:
*m_draw_list_act = *m_draw_list;
There's no deep copy constructor of draw list.

[ocornut]

EDIT You can use ImDrawList::CloneOutput() but I think I will now look at reworking this function signature to allow passing a preinstanciated version.

[Skreamex]

I've spent whole day to determine what causes it.
Even without copying

init(){
  m_draw_list = new ImDrawList(ImGui::GetDrawListSharedData());
}

render(){
  m_draw_list->_ResetForNewFrame();
  m_draw_list->PushTextureID(ImGui::GetIO().Fonts->TexID);
  m_draw_list->PushClipRectFullScreen();
  m_draw_list->AddRect( ... );
  End(); // EMPTY function
}

You can simply reproduce this bug with CreateThread winapi function and code above

AddRect calls:

1. PathRect -> is ok, adds points to array
2. PathStroke -> goes into AddPolyline which accesses that variable ( TempBuffer )

// Temporary buffer
// The first <points_count> items are normals at each line point, then after that there are either 2 or 4 temp points for each line point
_Data->TempBuffer.reserve_discard(points_count * ((use_texture || !thick_line) ? 3 : 5));

And other thread can also access this variable while menu is building vertexes to pass to d3d_impl

Other ImDrawList AddXXX functions that doesnt use TempBuffer works fine, without glitches

[Skreamex]

So it means two thread accesses same TempBuffer variable in same time

Only if actual AddXXX functions are called at the same time, which never worked even before (as they would manipulate VtxBuffer IdxBuffer etc.) so I don't think this is your issue and you are looking at and fixing a wrong thing.

FYI this is likely incorrect: *m_draw_list_act = *m_draw_list; There's no deep copy constructor of draw list.

Yes, but version with allocators worked fine with that.
How it was in 1.84 WIP

void ImDrawList::AddPolyline(const ImVec2* points, const int points_count, ImU32 col, ImDrawFlags flags, float thickness) {
. . .
  // Temporary buffer
  // The first <points_count> items are normals at each line point, then after that there are either 2 or 4 temp points for each line point
  ImVec2* temp_normals = (ImVec2*)alloca(points_count * ((use_texture || !thick_line) ? 3 : 5) * sizeof(ImVec2)); //-V630
  ImVec2* temp_points = temp_normals + points_count;
. . .
}

Edit:
Also ImDrawList * m_draw_list member only being accessed from single separated thread

[ocornut]

And other thread can also access this variable while menu is building vertexes to pass to d3d_impl

If you mean calling EndFrame() or Render(), you should not call them from a separate thread. Call it before copying the draw list.
The copied draw list should be read-only then.

Other ImDrawList AddXXX functions that doesnt use TempBuffer works fine, without glitches

It was only accidental luck that it worked before.

[ocornut]

You can simply reproduce this bug with CreateThread winapi function and code above

This is ambiguous, please clarify.

And other thread can also access this variable while menu is building vertexes to pass to d3d_impl

I repeat, you can never call ImDrawList functions from those threads. This was never allowed.
EndFrame()/Render() calls a few functions in ImDrawList().
This has nothing to do with the TempBuffer which has the same lifetime and locking requirements as other elements.

[ocornut]

You can simply reproduce this bug with CreateThread winapi function and code above

Sorry for multiple messages. I think I understand what you meant.

This last example suggests you want to append to different draw lists at the same time in different thread. (A)

m_draw_list = new ImDrawList(ImGui::GetDrawListSharedData());

Notice this is called GetDrawListSharedData() and the type is ImDrawListSharedData.
If you want to do this, you indeed it to create a unique ImDrawListSharedData instance per thread.

It would still be lucky that your ImGui code / main thread wasn't manipulating other fields of the ImDrawListSharedData structure while adding to ImDrawList from another thread.

This is different from copying a drawlist so it can be rendered later (B), which is more common when a Render thread runs in parallel with an Update thread. For this, you can swap or clone your drawlist in the Update thread after calling EndFrame()/Render(), then store that data and let the Render thread process is later.

If I understand you are trying to do (A) ?

[Skreamex]

Yes, you are right. Here is working example of what I mean https://github.com/Skreamex/mtrenderer_issue
Build in x86 Release
As you said about creation shared data, I've tried this, but no output on screen, I dont know what I have to copy from main ImDrawListSharedData, so I decided to split TempBuffer for each thread

Examples from app

[ocornut]

As you said about creation shared data, I've tried this, but no output on screen, I dont know what I have to copy from main ImDrawListSharedData, so I decided to split TempBuffer for each thread

You may initialize your ImDrawListSharedData instance with a copy of *ImGui::GetDrawListSharedData().

Other ImGui functions may perfectly interfere with this data, e.g. if you call ImGui::PushFont() from one thread and use the drawlist with same shared data from another thread, the ImDrawListSharedData::Font/FontSize fields will change. So your solution is not correct.

To slightly increase discoverability I have amend the old Changelog:
a1b8457#diff-cd43a1f9805983d7231947465806eac59936642392a6a0827bd1c246d27b4094R383

VERSION 1.89 (Released 2022-11-15)
....
- ImDrawList: Not using alloca() anymore, lift single polygon size limits. (#5704, #1811)
  - Note: now using a temporary buffer stored in ImDrawListSharedData.
    This change made it more visible than you cannot append to multiple ImDrawList from multiple
    threads if they share the same ImDrawListSharedData. Previously it was a little more likely
    for this to "accidentally" work, but was already incorrect. (#6167)

[Skreamex]

As you said about creation shared data, I've tried this, but no output on screen, I dont know what I have to copy from main ImDrawListSharedData, so I decided to split TempBuffer for each thread

You may initialize your ImDrawListSharedData instance with a copy of *ImGui::GetDrawListSharedData().

Other ImGui functions may perfectly interfere with this data, e.g. if you call ImGui::PushFont() from one thread and use the drawlist with same shared data from another thread, the ImDrawListSharedData::Font/FontSize fields will change. So your solution is not correct.

Yeah, but in not my case. I use ImDrawList to render my data, I dont use GUI.
Also thanks for replies!
Ok if I do

auto shared = new ImDrawListSharedData(*ImGui::GetDrawListSharedData());

What field I should copy from main shared data before call my Begin/End functions? I create and use fonts only in my renderer class

[ocornut]

You'll need to investigate how the fields are set to find out where.
But I think if you copy after ImGui ran for 1 frame you may be good.

[Skreamex]

Ok, thanks for information.
Now its solved.
Hope this thread will help someone.