Enhanced ImGuiListClipper for drag&drop operations #3841
Conversation
…lude the forced element.
|
Thank you for the PR. I think we should aim to reformulate this
Note how underlying I haven't extensively tested it but gut reaction since the PR is that it adds too much duplicate code, and although the Clipper is currently a little of a tricky mess we should strive to untangle it rather than not. If anything, supporting (A) which is more generic than (B) may force refactoring the code in a neater way. I feel like instead of using sequential numbered steps the clipper could for each step "figure what's coming next", return that, repeat there's no more. If you have ideas of how to improve this feel free to toy around, otherwise I'll probably investigate it at some point, with your PR as a starting point. Thank you! |
|
I didn't think of ranges, but that might come in handy. Which of these variants would you prefer? // count variant (single item is default, ranges must be converted to start + count)
IMGUI_API void ForceDisplay(int item_start, int item_count = 1);
// range variant (one overload for a single item, start + explicit count must be converted to range)
IMGUI_API void ForceDisplay(int item_start, int item_end);
inline void ForceDisplay(int item_index) { ForceDisplay(item_index, item_index + 1); }
// both combined (single item, start + explicit count and range are all handled, but needs two separate function names)
IMGUI_API void ForceDisplayRange(int item_start, int item_end);
inline void ForceDisplay(int item_start, int item_count = 1) { ForceDisplayRange(item_start, item_start + item_count); }On first glance, I would favor the combined variant since all use cases are there without the user having to convert anything. Regarding code duplication, a better way would be to have a fixed list (2 entries should already be enough) of ranges. These can then be sorted (just a conditional swap) and fused (if continuous or overlapping). That should reduce the code needed quite a bit, I will look into it as soon as I have some time. I don't know the internal navigation code so far, but I will take a look if it can be incorporated without complications. |
I meant provided as Y coordinates, not necessarily item count. |
|
Oh, OK. Internally for the navigation only or for the user too? I'll take a look at that as well. |
Those navigation things come to mind now but I sense it wouldn't hurt to provide the api to the user. I'm not sure about the |
|
On that specific example you gave: I think it would be perfectly adequate to store the bounding box of the ActiveId and automatically have clipper takes that into account. So that would be a case where the clipping could automatically add a y-min/y-max range and then none of the extra API would be required to solve your issue here. (But the extra API can be useful in many other situations). |
|
I didn't get around to the y position range yet, but I made huge improvements with the ranges so far. Now the user can force an item, a number of items from a starting index or a range of items. The generated ranges get fused together to reduce the number of calls to Step(). Step() itself is a lot simpler, down from 145 lines in my previous commit to now 109. The new version no longer has to rely (as much) on fixed StepNo values. Here is some new test code for it: void TestForcedElement()
{
const int NumItems = 100;
static int draggedItem = -1;
int stepsProcessed = 0;
int itemsSubmitted = 0;
static bool explicitHeight = false;
static int forceDisplayMode = 1;
static int forceRangeStart = 90;
static int forceRangeEnd = 95;
if (ImGui::GetDragDropPayload() == nullptr)
draggedItem = -1;
// instructions and test settings
ImGui::SetNextWindowSize(ImVec2(320, 240), ImGuiCond_FirstUseEver);
if (ImGui::Begin("Forced Element Settings"))
{
ImGui::PushTextWrapPos();
ImGui::TextUnformatted("Drag any item while scrolling the list so that the dragged item is no longer visible.");
ImGui::PopTextWrapPos();
ImGui::Checkbox("Set explicit Item Height", &explicitHeight);
ImGui::RadioButton("No forced Display", &forceDisplayMode, 0);
ImGui::RadioButton("Force Display of dragged Item", &forceDisplayMode, 1);
ImGui::RadioButton("Force Display of Range", &forceDisplayMode, 2);
ImGui::SliderInt("Forced Range Start", &forceRangeStart, 0, NumItems);
ImGui::SliderInt("Forced Range End", &forceRangeEnd, 0, NumItems);
}
ImGui::End();
// test window, use drag&drop on items
ImGui::SetNextWindowSize(ImVec2(320, 480), ImGuiCond_FirstUseEver );
if (ImGui::Begin("Forced Element Test"))
{
ImGuiListClipper clipper;
clipper.Begin(100, explicitHeight ? 17.0f : -1.0f);
#if 1 // deactivate this for unenhanced list clipper
if ((forceDisplayMode == 1) && (draggedItem >= 0))
clipper.ForceDisplay(draggedItem); // this item will be displayed regardless of visibility
if ((forceDisplayMode == 2) && (forceRangeStart < forceRangeEnd))
clipper.ForceDisplayRange(forceRangeStart, forceRangeEnd);
#endif
while (clipper.Step())
{
stepsProcessed++;
for (int i = clipper.DisplayStart; i < clipper.DisplayEnd; ++i)
{
char buffer[32];
sprintf_s(buffer, "item #%d", i);
ImGui::Selectable(buffer);
if (ImGui::BeginDragDropSource())
{
int data = i;
ImGui::SetDragDropPayload("Test Payload", &data, sizeof(data));
ImGui::TextUnformatted(buffer);
ImGui::EndDragDropSource();
draggedItem = i;
}
itemsSubmitted++;
}
}
}
ImGui::End();
// append metrics to test settings window
if (ImGui::Begin("Forced Element Settings"))
{
if (ImGui::BeginTable("Metrics", 2))
{
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Number of Items:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", NumItems);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Steps processed:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", stepsProcessed);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Items submitted:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", itemsSubmitted);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Dragged item index:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", draggedItem);
ImGui::EndTable();
}
}
ImGui::End();
}A few things to look out for in the tracked stats:
|
|
I added the range based on Y coordinates. I didn't modify Here is also a new test function with improved metrics to show the steps in more detail: void TestForcedDisplay()
{
const int NumItems = 100;
float cursorStartY = 0.0f;
float itemHeight = ImGui::GetTextLineHeight() + ImGui::GetStyle().ItemSpacing.y;
static int draggedItem = -1;
int itemsSubmitted = 0;
int stepsProcessed = 0;
int displayStart[8];
int displayEnd[8];
static bool explicitHeight = false;
static int forceDisplayMode = 1;
static int forceRangeStart = 90;
static int forceRangeEnd = 95;
static float forceYRangeMin = 0.0f;
static float forceYRangeMax = 0.0f;
if (ImGui::GetDragDropPayload() == nullptr)
draggedItem = -1;
// instructions and test settings
ImGui::SetNextWindowPos(ImVec2(80, 80), ImGuiCond_FirstUseEver);
ImGui::SetNextWindowSize(ImVec2(400, 480), ImGuiCond_FirstUseEver);
if (ImGui::Begin("Forced Display Settings"))
{
ImGui::PushTextWrapPos();
ImGui::TextUnformatted("Drag any item while scrolling the list so that the dragged item is no longer visible.");
ImGui::PopTextWrapPos();
ImGui::Checkbox("Set explicit Item Height", &explicitHeight);
ImGui::RadioButton("No forced Display", &forceDisplayMode, 0);
ImGui::RadioButton("Force Display of dragged Item", &forceDisplayMode, 1);
ImGui::RadioButton("Force Display of Range", &forceDisplayMode, 2);
ImGui::RadioButton("Force Display of Y Range", &forceDisplayMode, 3);
ImGui::SliderInt("Forced Range Start", &forceRangeStart, 0, NumItems);
ImGui::SliderInt("Forced Range End", &forceRangeEnd, 0, NumItems);
ImGui::DragFloat("Forced Y Range Min", &forceYRangeMin);
ImGui::DragFloat("Forced Y Range Max", &forceYRangeMax);
}
ImGui::End();
// test window, use drag&drop on items
ImGui::SetNextWindowPos(ImVec2(500, 80), ImGuiCond_FirstUseEver);
ImGui::SetNextWindowSize(ImVec2(320, 480), ImGuiCond_FirstUseEver);
if (ImGui::Begin("Forced Display Test"))
{
cursorStartY = ImGui::GetCursorScreenPos().y;
ImGuiListClipper clipper;
clipper.Begin(100, explicitHeight ? itemHeight : -1.0f);
#if 1 // deactivate this for unenhanced list clipper
if ((forceDisplayMode == 1) && (draggedItem >= 0))
clipper.ForceDisplay(draggedItem); // this item will be displayed regardless of visibility
if ((forceDisplayMode == 2) && (forceRangeStart < forceRangeEnd))
clipper.ForceDisplayRange(forceRangeStart, forceRangeEnd);
if ((forceDisplayMode == 3) && (forceYRangeMin <= forceYRangeMax))
clipper.ForceDisplayYRange(forceYRangeMin, forceYRangeMax);
#endif
while (clipper.Step())
{
displayStart[stepsProcessed] = clipper.DisplayStart;
displayEnd[stepsProcessed] = clipper.DisplayEnd;
stepsProcessed++;
for (int i = clipper.DisplayStart; i < clipper.DisplayEnd; ++i)
{
char buffer[32];
sprintf_s(buffer, "item #%d", i);
ImGui::Selectable(buffer);
if (ImGui::BeginDragDropSource())
{
int data = i;
ImGui::SetDragDropPayload("Test Payload", &data, sizeof(data));
ImGui::TextUnformatted(buffer);
ImGui::EndDragDropSource();
draggedItem = i;
}
itemsSubmitted++;
}
}
itemHeight = clipper.ItemsHeight;
}
ImGui::End();
// append metrics to test settings window
if (ImGui::Begin("Forced Display Settings"))
{
if (ImGui::BeginTable("Metrics", 2))
{
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Number of Items:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", NumItems);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Screen Cursor Start Y:");
if (ImGui::TableNextColumn()) ImGui::Text("%.3f", cursorStartY);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Item Height:");
if (ImGui::TableNextColumn()) ImGui::Text("%.3f", itemHeight);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Dragged item index:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", draggedItem);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Items submitted:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", itemsSubmitted);
if (ImGui::TableNextColumn()) ImGui::TextUnformatted("Steps processed:");
if (ImGui::TableNextColumn()) ImGui::Text("%d", stepsProcessed);
for (int i = 0; i < stepsProcessed; ++i)
{
if (ImGui::TableNextColumn()) ImGui::BulletText("Step %d:", i + 1);
if (ImGui::TableNextColumn()) ImGui::Text("%d - %d", displayStart[i], displayEnd[i]);
}
ImGui::EndTable();
}
}
ImGui::End();
}Btw, I'm also not heavily invested in the names or internal workings of the new functions, you can of course change them however you see fit. As long as a way to force the display of a single item will be included, I'm happy. |
ocornut
force-pushed
the
master
branch
2 times, most recently
from
0c1e5bd
to
bb6a60b
Compare
|
I will shortly be looking at this since I need a similar thing to finish solving a problem I'm on. Note that once we can manage multiple ranges we can automatically add the bounding box of the currently focused item (g.NavId) which would likely solve your specific case without calling any API, since clicking the drag source would make it the focused item.
Similarly, the two I don't think it is possible to keep the |
…the reference point from window->Pos to window->DC.CursorStartPos. This commit should have no effect. Current point makes rectangle invalid right after a scroll, for interactive actions it's rarely a problem but e.g. clipper will want to use g.NavID rect rel while scrolling. (#3841)
…artPos to be independent of scrolling. Will facilitate rework of clipper (#3841) + Extracted code into NavUpdateCreateWrappingRequest() + Fix for PVS in NavUpdate() assigning g.NavMousePosDirty twice.
- Focused/NavId now always included in display range. - Any number of steps (while preserving zero-alloc policy). - Non contiguous ranges for nav processing - Moved new fields internally (+ moved StepNo away from sight so it doesn't get missused). - Generally tweaks/refactors.
…ns functions until we find a need for them, since #3841 is now solved automatically.
|
Finally got around to finish the work on this... it eventually became quite an hairy amount of yak-shaving in order to have navigation system fully work, spent several days on this.
Thanks for this and also thanks a lot for the nice test-bed! |
Based on #3578 I have now added |
This partially reverts commit 6a7e2c7.
…the reference point from window->Pos to window->DC.CursorStartPos. This commit should have no effect. Current point makes rectangle invalid right after a scroll, for interactive actions it's rarely a problem but e.g. clipper will want to use g.NavID rect rel while scrolling. (ocornut#3841)
…artPos to be independent of scrolling. Will facilitate rework of clipper (ocornut#3841) + Extracted code into NavUpdateCreateWrappingRequest() + Fix for PVS in NavUpdate() assigning g.NavMousePosDirty twice.
- Focused/NavId now always included in display range. - Any number of steps (while preserving zero-alloc policy). - Non contiguous ranges for nav processing - Moved new fields internally (+ moved StepNo away from sight so it doesn't get missused). - Generally tweaks/refactors.
…ns functions until we find a need for them, since ocornut#3841 is now solved automatically.
This partially reverts commit 6a7e2c7.
|
FYI I have made two changes to those API now:
|
…s an already included range. (#3841)
Current limitation
When using the list clipper, only the visible subset of elements is actually calculated, which saves a lot of processing time in larger lists. However, if a drag&drop source is scrolled out and gets clipped, the tooltip no longer gets generated and jumps back to "...". If that is not good enough, the current workaround would be to detect if an item was displayed and fall back to an alternative with additional code which can complicate things.
Solution
I enhanced the list clipper with a new method "ForceDisplay", with that the user can specify a single element that will be displayed even if it isn't visible. This allows the user to generate drag&drop tooltips (or possibly for other reasons) as if the element were visible. The forced element is optional (no change in functionality if it isn't used). It introduces another instance of Step() returning true only if the element is not handled by another step. The order of elements is also maintained, the additional step happens before or after the main step depending on element indices.
Example test code
When running this example code, drag an element and scroll it out of the visible set with the mouse wheel. With the "Use ForceDisplay" checkbox checked, the tooltip gets continually updated, with it unchecked the tooltip reverts to "...". If you comment out the single call to ForceDisplay, the code works with the current ImGui version.