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

Clipping is not related to the window with the latest imgui #205

Closed
Namek opened this issue Mar 2, 2023 · 24 comments
Closed

Clipping is not related to the window with the latest imgui #205

Namek opened this issue Mar 2, 2023 · 24 comments

Comments

@Namek
Copy link

Namek commented Mar 2, 2023

I have updated imgui from 1.88dock to 1.89.3dock. The screen is cut, depending on the position. It seems the editor has the size of the window but its clipping is not related to the window but rather to the world space (?).

The recording should explain what's going on, notice the huge black parts around the editor:

app_Gsf0CuRIrC.mp4

What's interesting though, I've found this code in the imgui_canvas.cpp:

    // #debug: Canvas content.
    //m_DrawList->AddRectFilled(m_StartPos, m_StartPos + m_CurrentSize, IM_COL32(0, 0, 0, 64));
    //m_DrawList->AddRect(m_WidgetRect.Min, m_WidgetRect.Max, IM_COL32(255, 0, 255, 64));

I have uncommented the last line and it seems to fix everything (it is, remove the black parts and not crop the content):
obraz

However, it doesn't seem to be a proper fix since it is commented as debug code.

Namek added a commit to Namek/imgui-node-editor that referenced this issue Mar 2, 2023
@Namek
Copy link
Author

Namek commented Mar 2, 2023

After further testing it turned it the workaround with uncommenting the line doesn't work when Suspend/Resume is used.

nodes.Suspend();
c.igBeginTooltip();
c.igTextUnformatted(text, null);
c.igEndTooltip();
nodes.Resume();

See how the appearing tooltip brings back the issue:

app_ASilm6ugow.mp4

@rtryan98
Copy link

rtryan98 commented Mar 6, 2023

Got a similar issue. Uncommenting the second line will bring back the canvas into the docked window space. However, my popup does appear in some other coordinate system and doesn't bring back the window background.

@azonenberg
Copy link

I updated from 1.89.1, I think? to latest in order to debug some suspend/resume issues and got hit by this instead.

Interestingly, the workaround seems to work great when the node editor window is docked. It only fails calling Suspend/Resume when undocked.

@Namek
Copy link
Author

Namek commented Mar 8, 2023

I reviewed the diff between tags v1.88 and v1.89 and I believe the change might be found in one of those commits:

  • 4eb9066997a95c556336b7ab5f246f0cdbf151c2
  • 72096bf6986d0bcc4543537dcfc581451d36ec29

@azonenberg
Copy link

Given the dependency between docking and the workaround, is it possible this is an upstream bug? @ocornut have you seen this before?

@ocornut
Copy link
Contributor

ocornut commented Mar 14, 2023

I reviewed the diff between tags v1.88 and v1.89 and I believe the change might be found in one of those commits:

Those commits are related to the ImGuiListClipper which seems to have nothing to do with this.

This is not my codebase so hard to say, first you would need to do a proper git bisect to find the commit that changed the behavior and then we can figure out if this is a dear imgui behavior change/bug or a bug in the node editor code.

Also try to open Metrics->DrawList and inspect the data here to see if there's a correlation.

@ocornut
Copy link
Contributor

ocornut commented Mar 14, 2023

I tried to rebase the stack-layout imgui/, checked out imgui-node-editor/develop, updated the copy with latest rebased imgui + stack-layout (with few fixes pushed to #208) and couldn't see an issue.

Please actually provide details as to how to reproduce the issue in imgui-node-editor .
I don't even know what example of node-editor looks like the one you posted screenshot of above.

@Namek
Copy link
Author

Namek commented Mar 14, 2023

@azonenberg @rtryan98 I welcome you to make a SSCCEE to reproduce that issue, as stated above. As this is not critical for me to update imgui right now, I am not pursuing it in the following days.

@thedmd
Copy link
Owner

thedmd commented May 2, 2023

@Namek Does updating ImGui solved the issue for you?

@azonenberg
Copy link

Just checked with latest imgui from git and the issue is still present.

pthom added a commit to pthom/imgui-node-editor that referenced this issue Aug 2, 2023
This is a workaround for
         thedmd#205
         pthom/imgui_bundle#117
     Adding a simple pixel at m_WidgetRect.Max is enough to bypass the issue
pthom added a commit to pthom/node_window_clipping_issue that referenced this issue Aug 2, 2023
pthom added a commit to pthom/node_window_clipping_issue that referenced this issue Aug 2, 2023
pthom added a commit to pthom/node_window_clipping_issue that referenced this issue Aug 2, 2023
pthom added a commit to pthom/node_window_clipping_issue that referenced this issue Aug 2, 2023
@pthom
Copy link

pthom commented Aug 2, 2023

@Namek I created a minimal reproducible example in this repository

This code is self-contained in a short file: node_clipping_issue.cpp

However it was not possible to use the ImGui version inside node editor, since it is outdated, and adds stack layout patches to ImGui which are not trivial to rebase. So it uses HelloImGui, which is based on a quite recent version of the ImGui docking branch.

@Namek
Copy link
Author

Namek commented Aug 3, 2023

@ocornut you asked for a reproducible example, please see the comment above.

@pthom thank you. I tried to downgrade imgui to the version that I was using when it worked (1.88) which is commit 81160fee56027226bc80b48e196d0332f5541a8c from the docking branch (listed here https://github.com/ocornut/imgui/commits/docking?after=f8704cd085c4347f835c21dc12a3951924143872+664&branch=docking&qualified_name=refs%2Fheads%2Fdocking). However, helloimgui gets in the way as it's incompatible with it. I think it would be best to find which commit of imgui actually broke the editor. The workaround doesn't work for me. It changes things but not to better.

@ocornut
Copy link
Contributor

ocornut commented Aug 3, 2023

Took me 30 mins to figure out how to build and run this but since pthom made the effort to make it I went ahead.

I don't see any issue and or maybe don't know what to look for.
(I tried with and without pthom@2265a4b)

@pthom
Copy link

pthom commented Aug 5, 2023

Bonjour @ocornut :-) , hi @thedmd and @Namek

Took me 30 mins to figure out how to build and run this but since pthom made the effort to make it I went ahead.

I'm sorry it took you so long to build the example. I think it was an error on my side to use a third party such as HelloImGui.


I rewrote the minimal repro repository so that it uses only ImGui, imgui-node-editor and SDL (and nothing else).

This demo is able to trigger different subtle issues that arise when using docked windows, and/or windows whose size is unknown at the first frame (so that the canvas does not know its size)

They are detailed in the Readme.

Also, I made a short video in order to demonstrate them, as well as the use of the repo (in the hope to make it easier to setup). I think the video is a good way to get a more complete picture of the issue.

thanks


Note: I can confirm that the issue seems to be absent with ImGui docking v1.88 (commit 58eb40db76783f5da09e592ca3eb421f4f2197e3). I'm not sure if that helps, though. Anyhow, I created a branch imgui_1_88 in the repository. It uses the SDL2 branch of SDL (you will need to remove external/ and re-run fetch_external.sh).

pthom added a commit to pthom/node_window_clipping_issue that referenced this issue Aug 5, 2023
The issue thedmd/imgui-node-editor#205 was absent with ImGui 1.88. Not sure if that helps, though
@ocornut
Copy link
Contributor

ocornut commented Aug 6, 2023

I'm sorry it took you so long to build the example. I think it was an error on my side to use a third party such as HelloImGui.

Sorry I would have been more explicit with it (and I appreciate/admire your thorough help and patience here): the bulk of my problems came from the fact that I'm running a PC i'm using less often for development, and even though I have five millions copies of git.exe and cmake.exe scattered in various installation directories, it took me a while to find a shell where I could access sh, git and cmake together. Once I managed to do it all, and compile, the application failed to load assets even after I tried different combinations of working directory, tried to debug into the code trying to find the assets directory, and eventually caved in and manually called SetAssetsFolder() as suggested by the well-positioned assert.
If you are interested in investigating the finding-assets-folder procedure with you, in the event it may be useful for hello imgui to "just work" on that setup I'd be happy to look at it with you.

I'm cloning the new stuff now and will look into it.

@ocornut
Copy link
Contributor

ocornut commented Aug 6, 2023

The first thing was to add a call to ShowMetricsWindow() in the app.

Here I could notice that when the bug appear, the ClipRect of the main draw command is invalid:

Bug 205

Here you can see the ClipRect is (0,0)...(1264,672) when it should be the similar as the draw command below it (not exactly same as there's some padding involved).

Although I don't know imgui-node-editor codebase, I know that thedmd is well acquainted with advanced details of ImDrawList and may be inclined to pull some neat low-level use, which either could reveal a bug in imgui, either could easily break if some underlying logic changes.

I changed your workaround to use m_WidgetRect.Min instead of m_WidgetRect.Max and noticed it STILL fixed the problem.
That means no one is scanning vertices to use their positional data, but this seems to be an accidental corruption of ImDrawCmd data, possibly through a bad merge or very specific use of advance features such as channel splitter.

If I grep for ClipRect in imgui_node_editor codebase I find does looks of funky stuff with them, so I would say this is a bug in imgui-node-editor.. Tho bissecting imgui version may help finding the cause. The sample using SDL3 makes it harder to bisect with old versions but i'll try to do that. EDIT saw the SDL2 branch.

@ocornut
Copy link
Contributor

ocornut commented Aug 6, 2023

The issue started happening in an imgui update between 1.89.1 and 1.89.2, specifically between Nov 24 and Dec 1.
The two commits in this range which relates to our issue are:

@ocornut
Copy link
Contributor

ocornut commented Aug 6, 2023

If I comment the contents of ImDrawList::_PopUnusedDrawCmd() I noticed that the issue disappear:

// Pop trailing draw command (used before merging or presenting to user)
// Note that this leaves the ImDrawList in a state unfit for further commands, as most code assume that CmdBuffer.Size > 0 && CmdBuffer.back().UserCallback == NULL
void ImDrawList::_PopUnusedDrawCmd()
{
#if 0
    while (CmdBuffer.Size > 0)
    {
        ImDrawCmd* curr_cmd = &CmdBuffer.Data[CmdBuffer.Size - 1];
        if (curr_cmd->ElemCount != 0 || curr_cmd->UserCallback != NULL)
            return;// break;
        CmdBuffer.pop_back();
    }
#endif
}

So I add a dynamic breakpoint:

void ImDrawList::_PopUnusedDrawCmd()
{
#if 1
    while (CmdBuffer.Size > 0)
    {
        ImDrawCmd* curr_cmd = &CmdBuffer.Data[CmdBuffer.Size - 1];
        if (curr_cmd->ElemCount != 0 || curr_cmd->UserCallback != NULL)
            return;// break;
        if (ImGui::GetIO().KeyShift)
            printf("breakpoint\n"); // set breakpoint here
        CmdBuffer.pop_back();
    }
#endif
}

Get myself into the situation where the bug appears and press SHIFT to trigger the breakpoint.

I noticed that 2 empty commands are popped during the ChannelsMerge() call in ed::EditorContext::End()..
If I change the while to and if, which essentially reverts ocornut/imgui@9825f7f, I can't see the bug anymore.

_PopUnusedDrawCmd() seems correct to me, so it could be that @thedmd code relied on knowing there was an extra leading empty draw cmd to do something, and now that the popping got improved to pop two empty leading draw cmd, it breaks.

@ocornut
Copy link
Contributor

ocornut commented Aug 6, 2023

I'll need to let @thedmd finish this investigation, but I looked at iteration into CmdBuffer[] and noticed they started from m_DrawListCommadBufferSize and wanted to see how it was setup:

# if IMGUI_EX_CANVAS_DEFERED()
    m_Ranges.resize(m_Ranges.Size + 1);
    m_CurrentRange = &m_Ranges.back();
    m_CurrentRange->BeginComandIndex = ImMax(m_DrawList->CmdBuffer.Size - 1, 0);
    m_CurrentRange->BeginVertexIndex = m_DrawList->_VtxCurrentIdx + ImVtxOffsetRef(m_DrawList);
# endif
    m_DrawListCommadBufferSize       = ImMax(m_DrawList->CmdBuffer.Size - 1, 0);
    m_DrawListStartVertexIndex       = m_DrawList->_VtxCurrentIdx + ImVtxOffsetRef(m_DrawList);

Since the value is set multiple times every frame, to visualize I use this neat trick:

ImGui::Begin("DEBUG");
ImGui::Text("m_DrawListCommadBufferSize = %d", m_DrawListCommadBufferSize);
ImGui::End();

Creating and appending into a window declared locally inside that function.

When the bug appears a third line appears:
image

I suspect changing
m_DrawListCommadBufferSize = ImMax(m_DrawList->CmdBuffer.Size - 1, 0);
to
m_DrawListCommadBufferSize = ImMax(m_DrawList->CmdBuffer.Size, 0);
Might be the correct fix for this, but I'll need @thedmd to investigate and confirm.
It certainly appears to fix the issue but I don't know if that's the correct fix.

FIx should be applied from IMGUI_VERSION_NUM > 18913.

@pthom
Copy link

pthom commented Aug 6, 2023

@omar
Just a quick note: I suspect that you might be correct on the corruption of the ImDrawCmd, since I observed that the issue did disappear for a while, and reappeared later with no code change.

@ocornut
Copy link
Contributor

ocornut commented Aug 6, 2023

It wasn't a corruption in the "memory stomping" sense, neither a ImDrawCmd merging issue, simply it looks like the first drawcmd cliprect is not properly transformed because that computation for m_DrawListCommadBufferSize made some assumption about how imgui merged the draw commands.

@thedmd
Copy link
Owner

thedmd commented Aug 7, 2023

Thank you @ocornut for taking a time to investigate. I appreciate that.

You're correct. Canvas did made some assumption about how ImGui deal with ImDrawCmd. If I can recall correctly there was always an empty one ready in the buffer.

I will investigate the issue.

@thedmd
Copy link
Owner

thedmd commented Sep 1, 2023

Thank you @Namek, @pthom, @ocornut all for takin time to investigate the issue.

@pthom This is most awesome repro I saw so far. Thank you very much it was very helpful.

Fixes for all issues listed here were pushed to develop.

@thedmd thedmd closed this as completed Sep 1, 2023
@pthom
Copy link

pthom commented Sep 7, 2023

Once I managed to do it all, and compile, the application failed to load assets even after I tried different combinations of working directory, tried to debug into the code trying to find the assets directory, and eventually caved in and manually called SetAssetsFolder() as suggested by the well-positioned assert.
If you are interested in investigating the finding-assets-folder procedure with you, in the event it may be useful for hello imgui to "just work" on that setup I'd be happy to look at it with you.

@ocornut :
many thanks for your offer to help :-)
I investigated, and indeed there was an issue in the default setup under windows, which I corrected. This was due to a bad handling of the utf8 to utf16 conversion for the assets filenames. It now works out of the box under windows.


(the part below is only for info if you want to give HelloImGui a try)

In order to develop quick and dirty tests on the ImGui docking branch you could do this (by default they will use glfw as a backend).

  1. Create an external/ folder with ImGui (docking) and HelloImGui
mkdir external
cd external 
git clone -b docking https://github.com/ocornut/imgui.git         # any version on the docking branch should work
git clone -b master  https://github.com/pthom/hello_imgui.git
cd ..
  1. Create a simple CMakeLists.txt
cmake_minimum_required(VERSION 3.12)
project(hello)
set(CMAKE_CXX_STANDARD 17)


# Add ImGui & HelloImGui
# i. tell HelloImGui to use our own version of Dear ImGui in external/imgui/
set(HELLOIMGUI_IMGUI_SOURCE_DIR "${CMAKE_CURRENT_LIST_DIR}/external/imgui")
add_subdirectory(external/hello_imgui)
# ii. Make `hello_imgui_add_app` available
list(APPEND CMAKE_MODULE_PATH ${HELLOIMGUI_CMAKE_PATH})
include(hello_imgui_add_app)

# Build your app
hello_imgui_add_app(hello_world hello_world.main.cpp)
target_link_libraries(hello_world PRIVATE hello_imgui)
  1. create a c++ demo file, for example hello_world.main.cpp
#include "hello_imgui/hello_imgui.h"
#include "imgui.h"

void Gui()
{
    ImGui::Text("Hello, world!");
}

int main(int, char **)
{
    HelloImGui::Run(
        Gui,
        "Hello!",
        true // window_size_auto
        // Uncomment the next line to restore window position and size from previous run
        // , true // windowRestorePreviousGeometry
    );

    return 0;
}

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
                );
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

6 participants