Crash in Metal backend/docking branch (and fix)

[meshula]

Version/Branch of Dear ImGui:

Version: git latest
Branch: docking

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_metal.cpp + imgui_impl_osx.cpp
Compiler: clang/Xcode 11
Operating System: Big Sur

My Issue/Question:

Attempting to dock a window at the bottom of the screen results in a render encoder crash, at the moment the dragged window begins clipping against the bottom of the screen, because the calculated scissor window extends beyond the surface bounds.

Screenshots/Video

validateMTLScissorRect:2650: failed assertion `(rect.y(1434) + rect.height(8))(1442) must be <= render pass height(1440)'

Standalone, minimal, complete and verifiable example: (see #2261)

Here's the fix, which I believe belongs in all branches.

                if (clip_rect.x < fb_width && clip_rect.y < fb_height && clip_rect.z >= 0.0f && clip_rect.w >= 0.0f)
                {
                    // Apply scissor/clipping rectangle
                    MTLScissorRect scissorRect =
                    {
                        .x = NSUInteger(clip_rect.x),
                        .y = NSUInteger(clip_rect.y),
                        .width = NSUInteger(clip_rect.z - clip_rect.x),
                        .height = NSUInteger(clip_rect.w - clip_rect.y)
                    };
                    
                    // START FIX
                    // check if the computed scissorRect would violate setScissorRect assert
                    if (scissorRect.y + scissorRect.height > fb_height) {
                        scissorRect.height = fb_height - scissorRect.y;
                    }
                    
                    if (scissorRect.height > 0) {
                        [commandEncoder setScissorRect:scissorRect];
                    }
                    // END FIX

With this fix, my application does not crash and window rendering is as expected as it intersects the bottom of the screen.

[ocornut]

Hello,
Thanks for the report.
Could it be the same as #4414 ? (despite the same error/same line, i believe #4414 miiight be caused by something else)

[ocornut]

(1)
I'm not sure I understand this well (I don't have a machine to run Metal) but even before digging, this line which was ALREADY there seems fishy to me:

if (clip_rect.x < fb_width && clip_rect.y < fb_height && clip_rect.z >= 0.0f && clip_rect.w >= 0.0f)

GL and Vulkan backends also have this. It was merged in as part of a refactor 85f9694 and initially added by early viewports work here: 3bd3693. We shouldn't need those IHMO, so that's something we'll investigate.

(2)
As for your specific issue, will need further investigation as well.
Could you report what the value for draw_data->DisplayPos and draw_data->DisplaySize are when the assertion triggers, as well as the specific values of that assertion in that session.
It could be that the timing of update for those with those backends makes dear imgui render for previous position and previous size?

[rokups]

Attempting to dock a window at the bottom of the screen

Hey @meshula, could you clarify what does this mean exactly? Do you mean a bottom of main viewport or a bottom of your desktop? Also do you experience this issue with one of examples or do you only experience it in your application, but not in any of examples? Could you record a video showcasing how to reproduce this issue? So far i was unable to reproduce it and im not sure if i misunderstood something or if this stems from a difference of OS version (i tested on High Sierra).

[meshula]

I should have made the video first, sorry about that.

DisplayPos	ImVec2	
x	float	0
y	float	0
DisplaySize	ImVec2	
x	float	1200
y	float	720

validateMTLScissorRect:2650: failed assertion `(rect.y(1434) + rect.height(8))(1442) must be <= render pass height(1440)'

I am using the bundled example program, to which I have added the ImGuizmo ImSequencer, and slight boilerplate to enable docking.

    // this docking boiler plate is from https://ruurdsdevlog.wordpress.com/2020/03/07/c-desktop-application-with-dear-imgui/

    static ImGuiDockNodeFlags dockspace_flags = ImGuiDockNodeFlags_PassthruCentralNode;

    // We are using the ImGuiWindowFlags_NoDocking flag to make the parent window not dockable into,
    // because it would be confusing to have two docking targets within each others.
    ImGuiWindowFlags window_flags = ImGuiWindowFlags_MenuBar | ImGuiWindowFlags_NoDocking;

    ImGuiViewport* viewport = ImGui::GetMainViewport();
    ImGui::SetNextWindowPos(viewport->Pos);
    ImGui::SetNextWindowSize(viewport->Size);
    ImGui::SetNextWindowViewport(viewport->ID);
    ImGui::PushStyleVar(ImGuiStyleVar_WindowRounding, 0.0f);
    ImGui::PushStyleVar(ImGuiStyleVar_WindowBorderSize, 0.0f);
    window_flags |= ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoCollapse | ImGuiWindowFlags_NoResize | ImGuiWindowFlags_NoMove;
    window_flags |= ImGuiWindowFlags_NoBringToFrontOnFocus | ImGuiWindowFlags_NoNavFocus;


    // When using ImGuiDockNodeFlags_PassthruCentralNode, DockSpace() will render our background and handle the pass-thru hole, so we ask Begin() to not render a background.
    if (dockspace_flags & ImGuiDockNodeFlags_PassthruCentralNode)
        window_flags |= ImGuiWindowFlags_NoBackground;

    // Important: note that we proceed even if Begin() returns false (aka window is collapsed).
    // This is because we want to keep our DockSpace() active. If a DockSpace() is inactive,
    // all active windows docked into it will lose their parent and become undocked.
    // We cannot preserve the docking relationship between an active window and an inactive docking, otherwise
    // any change of dockspace/settings would lead to windows being stuck in limbo and never being visible.
    ImGui::PushStyleVar(ImGuiStyleVar_WindowPadding, ImVec2(0.0f, 0.0f));
    ImGui::Begin("DockSpace", nullptr, window_flags);
    ImGui::PopStyleVar();
    ImGui::PopStyleVar(2);

    // DockSpace
    if (io.ConfigFlags & ImGuiConfigFlags_DockingEnable)
    {
        ImGuiID dockspace_id = ImGui::GetID("MyDockSpace");
        ImGui::DockSpace(dockspace_id, ImVec2(0.0f, 0.0f), dockspace_flags);

        static auto first_time = true;
        if (first_time)
        {
            first_time = false;

            ImGui::DockBuilderRemoveNode(dockspace_id); // clear any previous layout
            ImGui::DockBuilderAddNode(dockspace_id, dockspace_flags | ImGuiDockNodeFlags_DockSpace);
            ImGui::DockBuilderSetNodeSize(dockspace_id, viewport->Size);

            // split the dockspace into 2 nodes -- DockBuilderSplitNode takes in the following args in the following order
            //   window ID to split, direction, fraction (between 0 and 1), the final two setting let's us choose which id we want (which ever one we DON'T set as NULL, will be returned by the function)
            //                                                              out_id_at_dir is the id of the node in the direction we specified earlier, out_id_at_opposite_dir is in the opposite direction
            auto dock_id_left = ImGui::DockBuilderSplitNode(dockspace_id, ImGuiDir_Left, 0.2f, nullptr, &dockspace_id);
            auto dock_id_down = ImGui::DockBuilderSplitNode(dockspace_id, ImGuiDir_Down, 0.25f, nullptr, &dockspace_id);

            // we now dock our windows into the docking node we made above
            ImGui::DockBuilderDockWindow("Down", dock_id_down);
            ImGui::DockBuilderDockWindow("Left", dock_id_left);
            ImGui::DockBuilderFinish(dockspace_id);
        }
    }

    ImGui::End();

Ah, it's a bug in the ImSequencer. All other panels dock properly.

I notice now that there are a couple of open issues on ImSequencer arguing that the clip region is not calculated properly.

So my patch works around a bug in ImSequencer, apparently?

CedricGuillemet/ImGuizmo#57
CedricGuillemet/ImGuizmo#195

So I think that there is no bug in Dear ImGui, hopefully Cedric can catch the issue on his side, but,,,, this patch lets me not crash with Cedric's code. Maybe I should maintain it as a local patch for now?

dockCrash.mov

[meshula]

PS, I can't reproduce the full screen issue in #4414, so I don't think the issue I am reporting is related to latency in the fbsize update.

[rokups]

I can reproduce the issue by doing this in our standard example:

        ImGui::Begin("Another Window", &show_another_window);   // Pass a pointer to our bool variable (the window will have a closing button that will clear the bool when clicked)
        ImRect r = ImGui::GetCurrentWindow()->Rect();
        r.Expand(10);
        ImGui::PushClipRect(r.Min, r.Max, false);
        ImGui::Text("Hello from another window!");
        if (ImGui::Button("Close Me"))
            show_another_window = false;
        ImGui::PopClipRect();
        ImGui::End();

Crash can be worked around by adding:

                if (clip_rect.z > fb_width) clip_rect.z = fb_width;       // These two lines
                if (clip_rect.w > fb_height) clip_rect.w = fb_height;     //

                if (clip_rect.x < fb_width && clip_rect.y < fb_height && clip_rect.z >= 0.0f && clip_rect.w >= 0.0f)

Maybe a proper fix would be getting rid of that fishy if and just clamping clip rect to available dimensions?

[ocornut]

Maybe a proper fix would be getting rid of that fishy if and just clamping clip rect to available dimensions

Yes, could you investigate the result of that test case over all our backend?
Also worth adding a test like that (it's not going to run over all backends, but better than nothing)

[ocornut]

So I think that there is no bug in Dear ImGui,

Still is a bug in Dear ImGui ihmo, we shouldn't be able to crash the backend.
It's just that the core library behave "too well" in term of pushing clipping rectangle (*) and thus this wasn't exercised properly.

* Because the ImGui:: side of PushClipRect will alter coarse clipping of items as well, it is very much preferable to behave well.

[ocornut]

Pushed fixes: 2b0bd40

And standardized handling across backends a bit better (but only clamping but API that needs it)

[meshula]

Thanks for the investigation and fix, much appreciated!