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

Wrong row returned in TableGetHoveredRow() #7350

Closed
GamingMinds-DanielC opened this issue Feb 26, 2024 · 3 comments
Closed

Wrong row returned in TableGetHoveredRow() #7350

GamingMinds-DanielC opened this issue Feb 26, 2024 · 3 comments

Comments

@GamingMinds-DanielC
Copy link
Contributor

Version/Branch of Dear ImGui:

Version 1.90.4 WIP (19032), Branch: master/docking

Back-ends:

imgui_impl_win32.cpp + imgui_impl_dx11/dx12.cpp

Compiler, OS:

Windows 10/11 + MSVC 2019/2022

Full config/build information:

Dear ImGui 1.90.4 WIP (19032)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: _WIN32
define: _WIN64
define: _MSC_VER=1929
define: _MSVC_LANG=201703
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_dx11
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: 1 fonts, Flags: 0x00000000, TexSize: 512,64
io.DisplaySize: 1264.00,761.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

I know it is an internal API, but currently TableGetHoveredRow() has situations in which it doesn't return the row that was actually hovered in the last frame. In the screenshots you can see that the header row (row 0) is hovered (not seen: in the bottom half of it). When scrolled to the top, the hovered row is reported correctly. When scrolled so that the first row (submitted for the list clipper) is completely hidden by the frozen header (but still within the tables clip rect), it is still hovered through the header row instead of the header row. Hovering the bottom of the header row where a partially scrolled out row would otherwise be visible, that row will be reported as hovered instead. Table setups with multiple frozen rows and no list clipper will have it worse.

The source of problem is here in imgui_tables.cpp:

void ImGui::TableEndRow(ImGuiTable* table)
{
    // ...

    const bool is_visible = (bg_y2 >= table->InnerClipRect.Min.y && bg_y1 <= table->InnerClipRect.Max.y);
    if (is_visible)
    {
        // Update data for TableGetHoveredRow()
        if (table->HoveredColumnBody != -1 && g.IO.MousePos.y >= bg_y1 && g.IO.MousePos.y < bg_y2)
            table_instance->HoveredRowNext = table->CurrentRow;

        // ...
    }

    // ...
}

If multiple rows operlap the mouse cursor, the last submitted one of those will get marked as hovered by this code. Adding table_instance->HoveredRowNext < 0 to the list of conditions will change this to the first submitted row, that should already be a proper fix. At least as long as frozen rows at the bottom of a table are not a thing, rows that are submitted first will always hide overlapping rows that are submitted later.

Screenshots/Video:

hovered_row

Minimal, Complete and Verifiable Example code:

	if ( ImGui::Begin( "Table Test" ) )
	{
		const int numRows = 30;
		const int numCols = 3;

		int hoveredRow = -1;
		int hoveredCol = -1;

		if ( ImGui::BeginTable( "test_table", numCols,
		                        ImGuiTableFlags_ScrollX | ImGuiTableFlags_ScrollY | ImGuiTableFlags_RowBg | ImGuiTableFlags_BordersV,
		                        ImVec2( 0.0f, -ImGui::GetTextLineHeightWithSpacing() ) ) )
		{
			ImGui::TableSetupScrollFreeze( 0, 1 );

			for ( int c = 0; c < numCols; ++c )
			{
				char colName[ 16 ];
				sprintf_s( colName, "Col %i", c );
				ImGui::TableSetupColumn( colName );
			}

			ImGui::TableHeadersRow();

			ImGuiListClipper clipper;
			clipper.Begin( numRows );

			while ( clipper.Step() )
			{
				for ( int r = clipper.DisplayStart; r < clipper.DisplayEnd; ++r )
				{
					ImGui::TableNextRow();

					for ( int c = 0; c < numCols; ++c )
					{
						ImGui::TableNextColumn();
						ImGui::Text( "row %2i, col %i", r + 1, c );
					}
				}
			}

			hoveredRow = ImGui::TableGetHoveredRow();
			hoveredCol = ImGui::TableGetHoveredColumn();

			ImGui::EndTable();
		}

		ImGui::Text( "Hovered row %i, col %i", hoveredRow, hoveredCol );
	}
	ImGui::End();
@ocornut
Copy link
Owner

ocornut commented Feb 26, 2024

Thank you for your report. I agree with your reasoning and correction and applied it as 725f919 (I gave you authorship of that commit).

At least as long as frozen rows at the bottom of a table are not a thing, rows that are submitted first will always hide overlapping rows that are submitted later.

Correct. Presently due to how we implement the draw channels I have little hope to introduce frozen rows at the bottom of a table without larger changes. But I think there is a rather acceptable workaround: to submit another table instance with just that row, and no spacing between both table instance.

Anecdotally, the "Assets Browser" demo in range-select also use a table for its header row only (without other rows).

Keeping this open as I'd like to write a test for it too.

@GamingMinds-DanielC
Copy link
Contributor Author

Thank you. I (currently) don't need frozen rows at the bottom, I just thought to mention them since this fix would then break hovering in those if they ever became a thing. If I ever do need them, synced table instances without a gap between them will do fine.

ocornut added a commit to ocornut/imgui_test_engine that referenced this issue Feb 26, 2024
@ocornut
Copy link
Owner

ocornut commented Feb 26, 2024

Added a simple test for it :
ocornut/imgui_test_engine@057b597

@ocornut ocornut closed this as completed Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants