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

Need a better way to detect when hovering over table cells #6250

Closed
lailoken opened this issue Mar 16, 2023 · 15 comments
Closed

Need a better way to detect when hovering over table cells #6250

lailoken opened this issue Mar 16, 2023 · 15 comments

Comments

@lailoken
Copy link

Version: v1.89.4 WIP
Branch: docking
Backend: Windows (also emscripten)

I need a way to detect a mouse hover over cells where the cells could be empty or have variable length text.

I've been using IsItemHovered to detect if I'm hovering over the text, but this is not a great solution.

My current solution is the following helper function:

bool IsMouseHoveringCurrentTableCell()
{
    ImVec2 cellMin = ImGui::GetCursorScreenPos();
    cellMin.y = ImGui::GetItemRectMin().y;
    ImVec2 cellMax = ImVec2(cellMin.x + ImGui::GetColumnWidth(), cellMin.y + ImGui::GetFrameHeight());
    return ImGui::IsMouseHoveringRect(cellMin, cellMax);
}

Also, this function may be invoked before or after I add content to the cell, and the content could in some cases not even be text.

I find that GetCursorScreenPos().x will reliably be at the start of the cell, but the y will not, whereas the GetItemRectMin().y reliably gives the start of the cell and the x not. So I have to mix the two to get a stable function.

This is very combersome and seems to slow down a little when working with dozens of tables with hundreds of lines and many columns. I'm also not 100% sure these functions are correctly returning these positions. Is the GetCursorScreenPos() returning the correct x because of ImGui::SameLine() behavior?

If these functions are returning correct values in these cases, then I can safely continue to use this utility method. However it would be better to have a more optimal ImGui::IsMouseHoveringCurrentTableCell() native call. Is this possible? Or am I missing a better way to do this?

@lailoken
Copy link
Author

In the future I think that this function may fail if there was a dangling ImGui::SameLine() call or if there was some kind of manual cursor set operation, which I may be doing for some fields, so this may be an issue. (Will have to do this call before I draw anything in those cases...)

@ocornut
Copy link
Owner

ocornut commented Mar 21, 2023

I cannot comprehend the code above, e.g. how it is using GetItemRectMin().

I'm also not 100% sure these functions are correctly returning these positions

If you are not sure of value you should display them on screen to validate, e.g.

if (g.IO.KeyShift)
    ImGui::GetForegroundDrawList()->AddRect(....);

But mostly, you can look at how TableGetCellBgRect() is implemented...

However it is important to understand we cannot tell a row or cell height until the end of the row.

Most of the times the recommended workaround is to submit a minimum height in TableNextRow() and honor the contract of not stepping outside this height, then if you honor that contract table->RpwPosY2 is correct while appending into the row.

In the general sense outside of this idea it's not possible to get the rectangle of a cell of any row because we'd be missing the Y2 component.

However we CAN rely on temporal coherency to tell specifically if e.g. the mouse is hovering a row because for a finite number of check (here the single input being Mouse Y position) we can freely record the row number as we go. We should probably provide this function, as TableGetHoveredRow() and then you can combine it with TableGetHoveredColumn() and do your thing more correctly and efficiently.

@lailoken
Copy link
Author

lailoken commented Mar 21, 2023

In response to your answer I only now see that it is a min_row_height (which I did set in my new row call), apologies.

In my case I do adhere to a contract not to go larger, so I should modify my function to include the rowHeight:

bool IsMouseHoveringCurrentTableCell(float rowHeight)
{
    ImVec2 cellMin = ImGui::GetCursorScreenPos();
    ImVec2 cellMax = ImVec2(cellMin.x + ImGui::GetColumnWidth(), cellMin.y + rowHeight);
    return ImGui::IsMouseHoveringRect(cellMin, cellMax);
}

All I need to make sure of is to call this before I do any Text() writes, or is there another way to get the top-left irrespective without needing internal calls?

@ocornut
Copy link
Owner

ocornut commented Mar 22, 2023

Your call ImGui::GetColumnWidth() only sort-of accidentally work because it does:

    ImGuiOldColumns* columns = window->DC.CurrentColumns;
    if (columns == NULL)
        return GetContentRegionAvail().x;

Again, I said this which you may have skimmed over:
"But mostly, you can look at how TableGetCellBgRect() is implemented..."
This has all the info your need. You can poll TableGetHoveredColumn() before your loop and then combine this info with using table->RowPosY1 and table->RowPosY2.

I have a working version of TableGetHoveredRow() but I don't think it's a good idea push it as the one-frame delay in updating makes it asymmetrical with TableGetHoveredColumn() and a mismatch with other means of checking inputs e.g. per row Selectable(), so I worry it would be a confusing or dangerous function.

@lailoken
Copy link
Author

I have looked at TableGetCellBgRect() but to be able to do anything like it I would need a ImGuiTable instance, which is internal. And I cannot seem to find any api call that would allow me to get at those internal memebers either.

All I need is a public interface to get the current cell min.x and max.x (and min.y). I understand that max.y cannot be determined until the line is done, and that's ok. I have my rowHeight.

I'm really trying hard not to use internal functions and conform to the API contract. Is this misguided?

@ocornut
Copy link
Owner

ocornut commented Mar 23, 2023

And I cannot seem to find any api call that would allow me to get at those internal memebers either.

ImGuiTable* GetCurrentTable()

All I need is a public interface to get the current cell min.x and max.x (and min.y)

There's no public interface for it yet. You can use the private stuff, or as I mentioned you are much better of using a single TableGetHoveredColumn() before the loop which will essentially test min.x/max.x for us.

I'm really trying hard not to use internal functions and conform to the API contract. Is this misguided?

It's not misguided but you'll face more limits and I can only move things to public API when I have strong confidence about the design and use cases.

@ocornut
Copy link
Owner

ocornut commented Mar 23, 2023

Btw I forgot to ask but typically if we knew how you wanted to use the result "detect when hovering over table cells" we would be able to give you a better answer.

@lailoken
Copy link
Author

So the reason I need it is that I have about 7 columns in one table, and I will have about 10 - 10000 rows (of which only about 50 would be visible). Then I have about 100 of these open at the same time and my framerate is dropping to about 10fps.

I would like to be able to click on these cells, so what I generally do is get a bool once for left-click and/or right-click at the start of my table drawing and have the ability to click on about 5 of these columns.

Now some of these columns have left-aligned text, some right aligned numbers, and some have no text, but have small graphs in the cells.

I found it annoying that mis-clicking the text in the cells did not work, so needed to be able to get the entire cell as a click.

Correct me if I'm wrong, but since I'm already facing FPS issues, should I still draw a hidden selectable or something else?

PS: I also have mouseover hover hints with extra info when shift is held.

@ocornut
Copy link
Owner

ocornut commented Mar 23, 2023

So the reason I need it is that I have about 7 columns in one table, and I will have about 10 - 10000 rows (of which only about 50 would be visible). Then I have about 100 of these open at the same time and my framerate is dropping to about 10fps.

Aren't you using the ImGuiListClipper for every table?

Correct me if I'm wrong, but since I'm already facing FPS issues, should I still draw a hidden selectable or something else?

Thanks for the details. Either a hidden Selectable() either or your approach is viable, combining MinX/MaxX and RowY1, RowY2. This is exactly what TableGetCellBgRect() does I would simply be using that, and you can still call TableGetHoveredColumn() ahead of the loop to only call TableGetCellBgRect() on the one table that is being the hovered one.

@lailoken
Copy link
Author

lailoken commented Mar 23, 2023

I tried using ImGuiListClipper but for this particular table I sometimes have -millions- billions of rows (cryptocurrency ganularity sucks) so the NextRow call was killing me. I just maintain my own local calulation and create a single row with all the hidden height. It works well and is much faster. For other tables I use the clipper. I also do take care to add an extra row at the end to deal with scrolling etc. It's working well and I don't render most of them.

However, just the visible rows with the amount of cells on-screen at the same time is also a (albeirt less severe) issue.

@lailoken
Copy link
Author

lailoken commented Mar 23, 2023

But as to my main query, I think that is solved.

And I'll try and limit the use of internal calls, but I'll start considering them when they work.

At this point I am relying on them for:

  1. the scrollbar heatmap to get the scrollbar rect (is there a better way?).
  2. to get the current window size/pos to be able to implement my own tiled/stacked window layout helper.

PS: There is no way for me to know whether SetNextWindowSize() with the ImGuiCond_FirstUseEver flag was actually a firstUseEver window or an existing window. So what I do is to check window->Pos and window->Size if they matched what I asked for and then increment my layout. (I realize that at the time I call SetNextWindowSize I have no idea which window will be drawn afterwards, so I can only get that info after the Begin()). A better way for me is to keep a separate state of all the window Ids I have encountered and increment them if it is a new one. It's just a pity since these states area already maintained in ImGui, and that flag would help me.

@ocornut
Copy link
Owner

ocornut commented Mar 23, 2023

I tried using ImGuiListClipper but for this particular table I sometimes have -millions- billions of rows (cryptocurrency ganularity sucks) so the NextRow call was killing me.

Those statements don't make sense. The whole purpose of ImGuiListClipper is to avoid submitting millions of rows. You seem to be misunderstanding its use and purpose and to be jumping into XY problems.

Please don't ask unrelated questions in same thread, it's too taxing for me to track and less useful for others. You seem to be not providing enough context for the exchange to be fruitful.

@lailoken
Copy link
Author

My implementation is equivalent to to ImGuiListClipper I do not render rows or cells for out of bounds.
However, with the exreme zooms and fractions of cryptocurriencies that we can display, the ImGuiListClipper still relied on the Step() method to be called. Now for a table with millions or tens of millions of rows (or more), with 100 of these, the UI basically locked up.

Since I don't do that now, it only slows down with loads of tables.

@ocornut
Copy link
Owner

ocornut commented Mar 23, 2023

Step() is called 2 or 3 times per table, so your statement seems incorrect.

@lailoken
Copy link
Author

As for unrelated, my primary question was about the correct/better way to do something (cell hover). And related to that was whether I could use internal API calls. I got a bit side-tracked with related bad calls, sorry.

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