Docking: Fix dock split of current window before End() assertion trigger

[SuperRonan]

Problem:

An assertion is raised when calling ImGui::DockBuilderSplitNode() on a DockId which owns a window that is currently being declared (between Begin() and End()).

More precisely:

We have an ImGuiID dock_id, and an ImGuiDockNode* n correponding to it; we also have an ImGuiWindow* w that is currently being declared and such as n->Windows[0] = w (being the first window in the list is important).
We call ImGui::DockBuilderSplitNode(dock_id, ...), which calls ImGui::DockNodeTreeSplit(ctx, n, ...), which creates two child nodes, and moves windows from the parent n to one of the children.
DockNodeMoveWindows(dst, src) adds src's windows to dst one at the time using DockNodeAddWindow(). The latter sets window->DockIsActive = (node->Windows.Size > 1);, which is going to be false for the first window of the list to be moved (w).
Latter, during End() of w, an assertion will fail:

if ((window->Flags & ImGuiWindowFlags_ChildWindow) && !(window->Flags & ImGuiWindowFlags_DockNodeHost) && !window->DockIsActive)
    IM_ASSERT_USER_ERROR(g.WithinEndChildID == window->ID, "Must call EndChild() and not End()!");

This assertion should not be checked because window->DockIsActive should have been true (it would have been the case if w was not the first window in the list n->Window, and it is not the case for other windows docked to n).

I have made a basic example to illustrate this issue. It is available in a separate commit e81ac55.

Note that this issue does not exists if docking manipulation is done outside of window declaration (such as docking manipulation done with the mouse).

Solution:

I am proposing to fix the above w->DockIsActive == false that appears to be erroneous.
I don't have a complete overview of the docking system, so the solution I am proposing in this PR might not be the best (Still I checked that all tests of the test engine passed).

I prefer not touching to window->DockIsActive = (node->Windows.Size > 1); in DockNodeAddWindow().
I think it makes most sense to fix it in DockNodeMoveWindows() (this current PR). Although this issue was found with a splitting, other docking operations that move windows from one node to another may trigger the same assertion error, and the source of the problems is in DockNodeMoveWindows().

Alternatively, the initial error maybe was to use the DockBuilder on the dock of a window during its declaration. We could explicitely forbid these kind of manipulations, but I am not very fond of such restrictions.

[ocornut]

Thank you for your PR.

I have made a basic example to illustrate this issue. It is available in a separate commit e81ac55.

Repros are better provided as copy-and-pastable blocks in the thread, which we paste in any example. It's easier to manage and use anywhere, and it doesn't degrade (e.g. link failing after some time).

Make sure you are running imgui_test_suite with the modified code + consider adding a new test for it.

[SuperRonan]

Here is the example:

#include <imgui_internal.h>

ImGuiID GetOrCreateWindowDockID(ImGuiWindow* window)
{
    ImGuiID res = window->DockId;
    if (res == 0) // Dockspace not yet created by imgui
    {
        // Create it programmatically
        res = ImGui::DockBuilderAddNode();
        ImGui::DockBuilderSetNodePos(res, window->Pos);
        ImGui::DockBuilderSetNodeSize(res, window->Size);
        ImGui::DockBuilderDockWindow(window->Name, res);
        ImGui::DockBuilderFinish(res);
    }
    return res;
}

class Test
{
    struct Child
    {
        bool open = {};
        bool open_side = {};
        ImGuiID dock_id = {};
    };
    const char* _window_label = "Test Docking Split Bug";
    ImVector<Child> _children;
    bool _nested_children = true;
    bool _late_dock = false;
    bool _reset = true;

    static Test _g;

    Test()
    {
        _children.push_back(Child{});
        _children.push_back(Child{});
    };

    void openChild(ImGuiWindow* window, Child& child, bool split_open)
    {
        child.open = true;
        ImGuiID parent = GetOrCreateWindowDockID(window);
        if (split_open)
        {
            child.dock_id = ImGui::DockBuilderSplitNode(parent, ImGuiDir_Right, 0.5f, nullptr, nullptr);
            ImGui::DockBuilderFinish(parent);
        }
        else
        {
            child.dock_id = parent;
        }
    }

    void declareChildren()
    {
        for (size_t i = 0; i < _children.size(); ++i)
        {
            Child& child = _children[i];
            if (child.open)
            {
                char child_label[] = "Test  ";
                child_label[5] = '0' + char(i);
                if (_late_dock && child.dock_id == 0)
                {
                    openChild(ImGui::FindWindowByName(_window_label), child, child.open_side);
                }
                ImGui::SetNextWindowDockID(child.dock_id, ImGuiCond_Appearing);
                if (ImGui::Begin(child_label, &child.open))
                {
                    ImGui::Text("Lorem Ipsum %d", i);
                }
                child.dock_id = ImGui::GetWindowDockID();
                ImGui::End();
            }
        }
    }

    void declare()
    {
        if (_reset)
        {
            ImGui::SetNextWindowFocus();
            ImGui::SetNextWindowSize(ImVec2(512, 0), ImGuiCond_Always);
            _reset = false;
        }
        if (ImGui::Begin(_window_label))
        {
            ImGui::Checkbox("Use Nested children", &_nested_children);
            ImGui::SetItemTooltip(_nested_children ? "Child windows are declared before End()" : "Child windows are declared after End()");
            ImGui::Checkbox("Late child docking", &_late_dock);
            ImGui::SetItemTooltip(_late_dock ? "DockBuilder is called just before child's Begin()" : "DockBuilder is called when button is cliked (necessarily before End())");

            bool bug_should_trigger = !(!_nested_children && _late_dock);
            ImGui::BeginDisabled();
            ImGui::Checkbox("Bug should trigger", &bug_should_trigger);
            ImGui::EndDisabled();

            auto control_child = [&](int i)
            {
                Child& child = _children[i];
                char child_label[] = "Test  ";
                child_label[5] = '0' + char(i);
                ImGui::PushID(i);
                ImGui::SeparatorText(child_label);
                bool should_open = false;
                bool split_open = false;
                ImGui::BeginDisabled(child.open);
                if (ImGui::Button("Open Split Side ->"))
                {
                    should_open = true;
                    split_open = true;
                }
                ImGui::SetItemTooltip("This will trigger the bug.");
                if (ImGui::Button("Open Tab ^"))
                {
                    should_open = true;
                    split_open = false;
                }
                ImGui::SetItemTooltip("This works perfectly fine");
                ImGui::EndDisabled();
                ImGui::PopID();

                if (should_open)
                {
                    if (_late_dock)
                    {
                        child.open = true;
                        child.open_side = split_open;
                        child.dock_id = 0;
                    }
                    else
                    {
                        openChild(ImGui::GetCurrentWindow(), child, split_open);
                    }
                }
            };
            for (size_t i = 0; i < _children.size(); ++i)
            {
                control_child(i);
            }
        }
        if (_nested_children)
        {
            declareChildren();
        }
        ImGui::End();
        if (!_nested_children)
        {
            declareChildren();
        }
    }

public:
    // Call during an ImGui frame
    static void Declare()
    {
        _g.declare();
    }
};
Test Test::_g = {};

[SuperRonan]

Very weird, I am writting a test for this issue, but it does not trigger.
Even very weirder, the test sometimes triggers the issue, and sometimes does not. I actually just realized that my example now also has the same issue (even if I clean the .ini).

The current version of my test:

t = IM_REGISTER_TEST(e, "docking", "docking_split_open_window");
t->Flags |= ImGuiTestFlags_NoAutoFinish;
t->GuiFunc = [](ImGuiTestContext* ctx)
    {
        ImGuiTestGenericVars& vars = ctx->GenericVars;
        if (ctx->IsFirstGuiFrame())
        {
            vars.Bool1 = true;
        }

        if (ImGui::Begin("AAAA", NULL, ImGuiWindowFlags_NoSavedSettings))
        {

        }
        if (ctx->FrameCount == 2)
        {
            ImGuiWindow* window = ImGui::GetCurrentWindow();
            if (window->DockId == 0)
            {
                ImGuiID dock_id = ImGui::DockBuilderAddNode();
                ImGui::DockBuilderSetNodePos(dock_id, window->Pos);
                ImGui::DockBuilderSetNodeSize(dock_id, window->Size);
                ImGui::DockBuilderDockWindow(window->Name, dock_id);
                ImGui::DockBuilderFinish(dock_id);
            }
            IM_CHECK(window->DockId != 0);
            ImGuiID split_id = 0;
            {
                ImGuiID parent = window->DockId;
                split_id = ImGui::DockBuilderSplitNode(parent, ImGuiDir_Right, 0.5f, nullptr, nullptr);
                ImGui::DockBuilderFinish(parent);
            }
            ImGui::SetNextWindowDockID(split_id, ImGuiCond_Always);
        }
        if (ctx->FrameCount >= 2)
        {
            if (ImGui::Begin("BBBB", &vars.Bool1, ImGuiWindowFlags_NoSavedSettings))
            {

            }
            ImGui::End();
        }
        ImGui::End();

        if (ctx->FrameCount == 1000)
            ctx->Finish();
    };

More precisely during the End() that should trigger the issue:

if ((window->Flags & ImGuiWindowFlags_ChildWindow) && !(window->Flags & ImGuiWindowFlags_DockNodeHost) && !window->DockIsActive)
    IM_ASSERT_USER_ERROR(g.WithinEndChildID == window->ID, "Must call EndChild() and not End()!");

window->DockIsActive == false as expected (to fail), but this time window->Flags does not contain ImGuiWindowFlags_ChildWindow.

[SuperRonan]

Okay I think I found out!
There are a few more conditions to trigger the assertion:
The parent window w must already have a non zero DockID and DockNode* before Begin() so that the latter calls BeginDocked() which adds the ImGuiWindowFlags_ChildWindow flag.

So the assert will not be checked the first time if we start without a .ini, or Begin() with ImGuiWindowFlags_NoSavedSettings.

So I changed my test to make it reliably trigger the assersion error (without the fix):
I fist open a new tab on the w's dockspace, then the next frame I split and open a new window on the side.

I can create a PR for the test. Or you can integrate it yourself if you prefer:

t = IM_REGISTER_TEST(e, "docking", "docking_split_open_window");
t->Flags |= ImGuiTestFlags_NoAutoFinish;
t->GuiFunc = [](ImGuiTestContext* ctx)
    {
        if (ImGui::Begin("AAAA", NULL, ImGuiWindowFlags_NoSavedSettings))
        {

        }
        ImGuiWindow* window = ImGui::GetCurrentWindow();
        ImGuiID split_id = 0;
        if (ctx->FrameCount == 2)
        {
            IM_CHECK(window->DockId == 0);
            if (window->DockId == 0)
            {
                ImGuiID dock_id = ImGui::DockBuilderAddNode();
                ImGui::DockBuilderSetNodePos(dock_id, window->Pos);
                ImGui::DockBuilderSetNodeSize(dock_id, window->Size);
                ImGui::DockBuilderDockWindow(window->Name, dock_id);
                ImGui::DockBuilderFinish(dock_id);
            }
            IM_CHECK(window->DockId != 0);
        }
        if(ctx->FrameCount == 3)
        {
            {
                ImGuiID parent = window->DockId;
                IM_ASSERT(parent != 0);
                split_id = ImGui::DockBuilderSplitNode(parent, ImGuiDir_Right, 0.5f, nullptr, nullptr);
                ImGui::DockBuilderFinish(parent);
            }
        }
        if (ctx->FrameCount >= 2)
        {
            ImGui::SetNextWindowDockID(window->DockId, ImGuiCond_FirstUseEver);
            if (ImGui::Begin("BBBB", nullptr, ImGuiWindowFlags_NoSavedSettings))
            {

            }
            ImGui::End();
        }
        if(ctx->FrameCount >= 3)
        {
            if (split_id)
            {
                ImGui::SetNextWindowDockID(split_id, ImGuiCond_FirstUseEver);
            }
            if (ImGui::Begin("CCCC", nullptr, ImGuiWindowFlags_NoSavedSettings))
            {

            }
            ImGui::End();
        }
        ImGui::End();

        if (ctx->FrameCount == 5)
            ctx->Finish();
    };