Removed column skip items assert

[eclbtownsend]

because IsVisibleY was changed to always be true for columns and is_visible changed to only care about the X-axis, the assert for is_visible on if the column items should be skipped is now an erroneous assert because of the case where the column is part of a table that has clipped off the top or bottom of the cliprect. In those cases, is_visible will always be true (horizontally) but hostSkipItems will be false.

[ocornut]

Thank you! Could you clarify where this could be reproed in the Demo? Or maybe a gif from Tables>Advanced.

[eclbtownsend]

I'm so sorry I spent a lot of time trying to get it to repro in the demo but it's proving really challenging. The bad assert happens in the somehow case of the early-out outer_size clip check doesnt return true:

imgui/imgui_tables.cpp

Lines 334 to 339 in a5cf4fe

	if (use_child_window && IsClippedEx(outer_rect, 0) && !outer_window_is_measuring_size)
	{
	ItemSize(outer_rect);
	ItemAdd(outer_rect, id);
	return false;
	}

but then in the same frame when the child window gets created, that window turns out to be clipped and returns false:

imgui/imgui_tables.cpp

Lines 414 to 421 in a5cf4fe

	// Create scrolling region (without border and zero window padding)
	ImGuiWindowFlags child_window_flags = (flags & ImGuiTableFlags_ScrollX) ? ImGuiWindowFlags_HorizontalScrollbar : ImGuiWindowFlags_None;
	BeginChildEx(name, instance_id, outer_rect.GetSize(), ImGuiChildFlags_None, child_window_flags);
	table->InnerWindow = g.CurrentWindow;
	table->WorkRect = table->InnerWindow->WorkRect;
	table->OuterRect = table->InnerWindow->Rect();
	table->InnerRect = table->InnerWindow->InnerRect;
	IM_ASSERT(table->InnerWindow->WindowPadding.x == 0.0f && table->InnerWindow->WindowPadding.y == 0.0f && table->InnerWindow->WindowBorderSize == 0.0f);

This HostSkipItems state causes the issue described in the PR. Everywhere I've tried in the demo doesnt have this issue because the early-out bounds check stops us from ever getting here. My actual repro in my own side is using an ImGui Canvas library that scales all the styles for zooming, and I can only get it to happen when zooming out a little (making the sizes smaller) and then scrolling the canvas to make the table scroll off the top. In the one frame where the BeginChild that contains the table returns true but the internal child window inside the table returns false. Could even be down to floating point error.

So sorry to bother you with this, I thought I had it fully nailed down but this assert isnt as obviously an error as it seems, Im just going to remove it on my own fork and the fate of this PR isn't important

[ocornut]

I will investigate this. Everything is important :) thanks for reporting.

[ocornut]

Basically I don't want to remove an assert without strongly understanding the issue at play.
is_visible being true would be an information available to the user via the ImGuiTableColumnFlags_IsVisible flag so I would like to get to the bottom of this eventually.

If you feel you can share a video it would help me understand the context at a glance, and if you think you can manufacture a custom I would greatly appreciate it (I can turn this repro into a regression test case as well). I appreciate your report.

[eclbtownsend]

I made a video to try and show as much debugging data for it as possible:

https://www.youtube.com/watch?v=Gy-7lXvfxak

This is using a modified version of ImNodes

It would be best if I could repro this in regular imgui demo to rule out the node canvas or any changes we made on our side, but the code for that video is simple within the canvas BeginNode/EndNode:

auto dpiAndZoom = ImGui::ecl::GetContentScale();
if (ImGui::BeginTable("Table", 3, ImGuiTableFlags_ScrollY, ImVec2(200.0f * dpiAndZoom, 200.0f * dpiAndZoom))) {

   ImGui::TableSetupColumn("A");
   ImGui::TableSetupColumn("B");
   ImGui::TableSetupColumn("C");
   ImGui::TableHeadersRow();

   for (int i = 0; i < 100; ++i) {
      for (int j = 0; j < 3; ++j) {
         ImGui::TableNextColumn();
         ImGui::TextUnformatted("hi");
      }
   }

   ImGui::EndTable();
}

ImNodes scales all the style vars by a zoom amount, and we multiple that zoom and dpi scale to make the outer_size of the table correct. I did find that if I didnt do the dpiandzoom scale in the above example the table obviously doesnt zoom but also the assert doesnt happen... so maybe some mismatch with outer_size changing or maybe floating point issue

edit: so sorry heres the build info:

Dear ImGui 1.89.3 (18930)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: IMGUI_DISABLE_OBSOLETE_FUNCTIONS
define: _WIN32
define: _WIN64
define: _MSC_VER=1935
define: _MSVC_LANG=201703
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_sdl2
io.BackendRendererName: imgui_impl_opengl3
io.ConfigFlags: 0x0000C441
 NavEnableKeyboard
 DockingEnable
 ViewportsEnable
 DpiEnableScaleViewports
 DpiEnableScaleFonts
io.ConfigViewportsNoDecoration
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x00001C0E
 HasMouseCursors
 HasSetMousePos
 PlatformHasViewports
 HasMouseHoveredViewport
 RendererHasVtxOffset
 RendererHasViewports
--------------------------------
io.Fonts: 10 fonts, Flags: 0x00000000, TexSize: 2048,4096
io.DisplaySize: 1280.00,720.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 12.00,12.00
style.WindowBorderSize: 1.00
style.FramePadding: 8.00,4.00
style.FrameRounding: 12.00
style.FrameBorderSize: 1.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

[eclbtownsend]

Oh hey I found it!

Modifying one of the y-clipping tables in the demo for some random floating point shenanigans:

Produces the assert by scrolling it off the screen:

[ocornut]

Hello Brianna,

I have investigated this and my conclusion for now is that I'd rather fix the issue at the source which is the mismatch between the IsClippedEx() call in BeginTable() and the value of SkipItems computed by BeginChild()->Begin().
I have pushed a tentative fix 71714ea could you evaluate it on your end?

I do believe it is likely that there are other cases that may requires further adjustments closer to the the assert you mentioned, but for now I would prefer to avoid removing it to avoid situation where a table is needlessly iterated on.

[eclbtownsend]

@ocornut Thank you so much! Your fix resolves the issue for my use cases. I definitely was expecting removing the assert to be the wrong fix after everything we learned here, I'm so glad you were able to find the right fix! I'll close this PR, have a nice day!