ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
59.56k stars 10.15k forks source link

Scroll.y too large when shrinking child window. Possible solution(s) included. #573

Closed johanwendin closed 6 years ago

johanwendin commented 8 years ago

Greetings!

When shrinking a child window, the child window's "window->Scroll.y" becomes "ItemSpacing.y" too large.

This is because of line 3752 (imgui.cpp, master branch - I've looked at commit 26be151 which is the current one when writing this) where window->SizeContents.y gets calculated like so: (window->DC.CursorMaxPos.y - window->Pos.y) + window->Scroll.y;

However, "CursorMaxPos.y" will always be "style.ItemSpacing.y" bigger than it should, probably due to ImGui::ItemSize adding "ItemSpacing.y" to "CursorPos", resulting in an extra ItemSpacing.y at the very end.

One possible solution, although somewhat contrived would be to changing the line 3752 to:

window->SizeContents.y = (float)(int)((window->SizeContentsExplicit.y != 0.0f) ? window->SizeContentsExplicit.y : ((window_is_new ? 0.0f : window->DC.CursorMaxPos.y - style.ItemSpacing.y - window->Pos.y) + window->Scroll.y));

Another, possibly better solution would probably be to just subtract ItemSpacing.y from CursorMaxPos.y inside "ImGui::End". (I went for the latter locally)

Note: The problem doesn't show itself if I resize while hovering the scrollbar. I haven't looked closer as to why, but my guess would be that "CursorMaxPos.y" gets updated to not include the itemspacing.

ocornut commented 8 years ago

Hi Johan!

When shrinking a child window, the child window's "window->Scroll.y" becomes "ItemSpacing.y" too large.

I understand the remaining of the post, but before considering a fix, I am not sure what's the net effect of it, or what step would you use to repro and exhibit an issue? How do you shrink a child window? Is the child window sized programmatically, or auto-fitted and then you manually resize the parent window? How does Scroll.y gets affected by SizeContents.y ?

On the fix: A few bits in the code assume that CursorMax is off by one ItemSpacing, so it can be used while appending to a window, so I would say that doing the subtraction at the point of calculating SizeContents is safer than altered CursorMaxPos.y. Also consider that End() may be called multiple times, as Begin/End pairs can be used to append into a same window at different points in time.

johanwendin commented 8 years ago

I'm setting the width and height of the child window each frame (i.e. programmatically).

Scroll.y gets affected from these lines (further down under //Apply scrolling)

if (!window->Collapsed && !window->SkipItems) window->Scroll = ImMin(window->Scroll, ImMax(ImVec2(0.0f, 0.0f), window->SizeContents - window->SizeFull + window->ScrollbarSizes));

ocornut commented 8 years ago

I still am not sure to understand the sequence of events here. I tried that:

        ImGui::Begin("Bug #573");

        static float h = 300; 
        ImGui::DragFloat("height", &h);
        if (ImGui::Button("-40")) h -= 40.0f;
        ImGui::SameLine();
        if (ImGui::Button("+40")) h += 40.0f;

        ImGui::BeginChild("child", ImVec2(h, h), true);
        ImVec2 start_pos = ImGui::GetCursorStartPos();
        ImVec2 contents_size = GImGui->CurrentWindow->SizeContents;
        float scroll_y = ImGui::GetScrollY();
        float scroll_max_y = ImGui::GetScrollMaxY();

        ImGui::Image(ImGui::GetIO().Fonts->TexID, ImVec2(h,h), ImVec2(0,0), ImVec2(1,1));

        //for (int n = 0; n < 50; n++)
        //    ImGui::Text("Hello %d", n);
        ImGui::EndChild();

        ImGui::Text("cursor start pos %.2f, %.2f", start_pos.x, start_pos.y);
        ImGui::Text("contents size %.2f, %.2f", contents_size.x, contents_size.y);
        ImGui::Text("child scroll %.2f / %.2f", scroll_y, scroll_max_y);
        ImGui::End();
johanwendin commented 8 years ago

I've attached a zip file showing how the error presents itself: wrong.zip

After either of the two changes mentioned in the original post, the magenta pixel is precisely where it should be next to the cross. :)

johanwendin commented 8 years ago

i.e. when it arrives to ImGui::Image, window->DC.CursorPos is wrong, due to this:

window->DC.CursorStartPos = window->Pos + ImVec2(window->DC.ColumnsStartX + window->DC.ColumnsOffsetX, window->TitleBarHeight() + window->MenuBarHeight() + window->WindowPadding.y - window->Scroll.y);

in Begin.

johanwendin commented 8 years ago

Mind you, this could have been fixed in other ways since v1.46 WIP which I'm still using, in which case I apologize. :)

ocornut commented 8 years ago

Will all goodwill, it is hard or impossible to guess what your code may be doing from that video. I would really appreciate a clearer repro. CursorPos is in absolute screen space meaning when your cursor starts above the visible region it would be < window->Pos or might even be < 0.0f.

ocornut commented 8 years ago

And I can't begin to be fixing a bug if I don't understand it precisely. Those small changes may have lots of side effects.

Changing the window size every frame programmatically might lead to bugs, I could imagine the possibility that some part of the code were relying on either the old or new size that could possibly break something for one frame.

johanwendin commented 8 years ago

Well, it's in script (Squirrel), but hopefully it's easy enough to follow:

UI.Begin(panel.title);
UI.SetCursorPos(panel.pan);               // child window position inside panel

local width = texture.width * zoom;       // texture size is 64x64 pixels at the moment,
local height = texture.height * zoom;     // changing by a drag-float might be too kind for the bug to present itself
UI.BeginChild("##workarea", width, height, false);
UI.Image(texture.id, width, height);
RenderCrosshair();
UI.EndChild();
UI.End();

Nothing too complex, but kind of difficult to see. I first noticed it because the image didn't completely fill the child window (4 pixels gap between bottom of image and bottom of child window). But that's with ItemSpacing set to 4 pixels too.

Edit: It only happens when you shrink the child window - not when you enlarge it. And I shrink it by 64 pixels at a time, not a smooth shrink.

I've solved it for my own use, so there's no rush on my behalf. Just thought I'd present the issue and possible solutions after I found them. :)

ocornut commented 8 years ago
johanwendin commented 8 years ago

RenderCrossHair draws two lines:

function RenderCrossHair() {
        local x = (UI.GetWindowXPos() + (origo.x*zoom)).tointeger();
        local y = (UI.GetWindowYPos() + (origo.y*zoom)).tointeger();

        local w = (tex2.width);   // *zoom to fill the child window
        local h = (tex2.height);  // *zoom to fill the child window

        UI.DrawLine(x-w, y, x+w, y, 0xFFFFFFFF, 1);    // horizontal line at 32px, 1px width
        UI.DrawLine(x, y-h, x, y+h, 0xFFFFFFFF, 1);    // vertical line at 32px, 1 px width
    }

I've used a child window because I need the image to scroll/pan inside the work area - and it can be bigger than the actual work area (and thus scroll) if you zoom in really really close.

Edit: Or you have a really small monitor, making the Work Area smaller. :)

ocornut commented 8 years ago

But nothing in that sequence implies any scrolling happening ever:

UI.BeginChild("##workarea", width, height, false);
UI.Image(texture.id, width, height);
RenderCrosshair();
UI.EndChild();
johanwendin commented 8 years ago

The scrolling happens when the Image (and thus the child window) is bigger than the containing work area window? scrolling.zip

See attached zip containing a movie demonstrating the scrolling.

ocornut commented 8 years ago

Your first statement was

When shrinking a child window, the child window's "window->Scroll.y" becomes "ItemSpacing.y" too large.

Here it is the parent that is scrolling.

ocornut commented 8 years ago

By the look of it, you could completely remove BeginChild()/EndChild() from your code it has no use.

johanwendin commented 8 years ago

Yes, and that's still true.

The child window's Scroll.y becomes ItemSpacing.y when I shrink the child window by 64 pixels. (containing Work Area window isn't affected, just changing zoom in the script code above).

It doesn't show because there's no scroll list (because it doesn't need to scroll) - but it affects the CursorPosition (i.e. ImGui::Image position).

ocornut commented 8 years ago

Would you mind looking at my (updated) test code above and tell me how it is different from your problem? and/or displaying values from your code. Your zooming/positioning/crosshair code might have an issue as well.

ocornut commented 8 years ago

(Either way you could just remove BeginChild/EndChild and draw your image in the parent and you'll be settled, but I would like to investigate it and fix it if there is indeed a bug)

With panning:

        ImGui::Begin("Bug #573");

        static ImVec2 panel_pan = ImVec2(200,200);

        static float h = 300; 
        ImGui::DragFloat("height", &h);
        if (ImGui::Button("-64")) h -= 64;
        ImGui::SameLine();
        if (ImGui::Button("+64")) h += 64;

        ImGui::Button("PAN");
        if (ImGui::IsItemActive())
        {
            panel_pan.x += ImGui::GetIO().MouseDelta.x;
            panel_pan.y += ImGui::GetIO().MouseDelta.y;
        }

        ImGui::SetCursorPos(panel_pan);
        ImGui::BeginChild("child", ImVec2(h, h), false); // can be commented
        ImVec2 start_pos = ImGui::GetCursorStartPos();
        ImVec2 contents_size = GImGui->CurrentWindow->SizeContents;
        float scroll_y = ImGui::GetScrollY();
        float scroll_max_y = ImGui::GetScrollMaxY();

        ImGui::Image(ImGui::GetIO().Fonts->TexID, ImVec2(h,h), ImVec2(0,0), ImVec2(1,1));

        //for (int n = 0; n < 50; n++)
        //    ImGui::Text("Hello %d", n);
        ImGui::EndChild(); // can be commented

        ImGui::Text("cursor start pos %.2f, %.2f", start_pos.x, start_pos.y);
        ImGui::Text("contents size %.2f, %.2f", contents_size.x, contents_size.y);
        ImGui::Text("child scroll %.2f / %.2f", scroll_y, scroll_max_y);
        ImGui::End();
johanwendin commented 8 years ago

Removing the Child window means no background color to the image (it's fully transparent, the child window is providing the background color), and I have to pan the crosshair and it gets out of sync when I scroll (i.e. I have to keep track of the scroll myself instead of letting the child window do all that :) ).

Looking at the test code above - you're doing the same thing I am (as long as you're wrapping it inside another Begin/End pair)

When I log the values I'd get: SizeContents.y = ItemSpacing.y too large, compared to SizeFull.y Scroll.y = ItemSpacing.y

(i.e. Make sure you actually have an ItemSpacing.y != 0.0 otherwise it won't ever show a discrepancy)

ocornut commented 8 years ago

Removing the Child window means no background color to the image (it's fully transparent, the child window is providing the background color),

You could draw a quad! ImGui::GetWindowDrawList()->AddRectFilled(ImGui::GetCursorScreenPos()... etc.

I have to keep track of the scroll myself instead of letting the child window do all that :) ).

I am still confused. There is no scroll happening on the child at all.

SizeContents.y = ItemSpacing.y too large, compared to SizeFull.y

Yes, that is "correct" (aka known), not ideal but shouldn't have a noticeable effect on you in theory (notice the value isn't even exposed).

Scroll.y = ItemSpacing.y

I always get Scroll.y = 0.0f

johanwendin commented 8 years ago

Well, I guess I should try upgrading to the latest version then and see if it's solved in another way. :D

Thanks for your patience. :)

ocornut commented 8 years ago

PS: I can see a million other bugs with sizing and child windows, they are all very subtle and interdependent this is also why I want to make 100% sure we understand each other and I understand what you are reporting.

ocornut commented 8 years ago

@johanwendin Any news with latest version?

johanwendin commented 8 years ago

@ocornut sorry, I've been ill all week. I'll try it out next week and get back to you. :)

ocornut commented 8 years ago

@johanwendin no worry. take care! :)

ocornut commented 6 years ago

Hello @johanwendin - was looking at some scrolling issues recently, some changes were made in the past few months, and today I pushed another set of changes which probably invalidate part of this discussion: https://github.com/ocornut/imgui/commit/c36e586cce1279b419ab4b443ca6ce63b03ce7cc

Note that I always failed to repro your bug report clearly suggesting I don't fully understand it. The video in "scrolling.zip" doesn't show an issue afaik. But I'm aware there was various inconsistency with padding and hopefully they are fixed now. Note that by default if you have a border, a child window uses WindowPadding, you may want to use PushStyleVar to make that zero.

I'd like to close this old bug if that's allright, but waiting for your feedback.

Test code:

ImGui::Begin("Bug #573");

static float h = 300;
ImGui::DragFloat("height", &h);
if (ImGui::Button("-40")) h -= 40.0f;
ImGui::SameLine();
if (ImGui::Button("+40")) h += 40.0f;

ImGui::BeginChild("child", ImVec2(h, h), true, ImGuiWindowFlags_HorizontalScrollbar);
ImVec2 start_pos = ImGui::GetCursorStartPos();
float scroll_y = ImGui::GetScrollY();
float scroll_max_y = ImGui::GetScrollMaxY();

// Either use a better texture, or just a dummy button with a custom size
ImGui::Image(ImGui::GetIO().Fonts->TexID, ImVec2(ImGui::GetIO().Fonts->TexWidth, ImGui::GetIO().Fonts->TexHeight), ImVec2(0, 0), ImVec2(1, 1));

ImGui::EndChild();

ImGui::Text("cursor start pos %.2f, %.2f", start_pos.x, start_pos.y);
ImGui::Text("child scroll %.2f / %.2f", scroll_y, scroll_max_y);
ImGui::End();
johanwendin commented 6 years ago

@ocornut feel free to close this "bug". (which most likely was due to windowpadding anyway like you say). It's a non-issue for me after a rewrite anyway :)