ListClipper + table doens't create scrollbars with imgui 1.86

[dbr]

For some reason with the latest main version of imgui-rs, a simple usage of the ListClipper cominbed with the table API will not produce scrollbars. Either with SCROLL_Y (which creates a new child window and scrolls within that) or without (creates items in current window and causes that to scroll)

The clipper appears to be mostly doing it's job - only the visible items are created, and it correctly adjusts as the window is resized etc.. just, there is never a scrollbar created

The same code was working in previous imgui-rs release (which used a previous version of upstream imgui)

Most confusingly, the examples in ImGui's Demo Window work fine, and the wrappers around the ListClipper are pretty thin

There was a bunch of bugfixes with imgui 1.86 to help with very large tables, so it's presumably something to do with these chnages.. but I'm very unclear if this is a problem in the bindings (likely since it works in the demo window, but strange as the bindings look correct on first glance), or if it's a problem with the upstream changes

@ocornut any thoughts on if this could be a problem on the C++ side of things?

[dbr]

The above example, which I created at imgui-rs/imgui-examples/examples/long_table.rs and ran cargo run --example long_table

use imgui::*;

mod support;

fn main() {
    let system = support::init(file!());

    system.main_loop(move |_, ui| {
        ui.show_demo_window(&mut true);

        ui.window("Table with list clipper")
            .size([800.0, 700.0], Condition::FirstUseEver)
            .build(|| {
                let num_cols = 3;
                let num_rows = 1000;

                let flags = imgui::TableFlags::ROW_BG
                    | imgui::TableFlags::RESIZABLE
                    | imgui::TableFlags::BORDERS_H
                    | imgui::TableFlags::BORDERS_V
                    ;//| imgui::TableFlags::SCROLL_Y;

                if let Some(_t) = ui.begin_table_with_sizing(
                    "longtable",
                    num_cols,
                    flags,
                    [300.0, 100.0],
                    /*inner width=*/ 0.0,
                ) {
                    ui.table_setup_column("A");
                    ui.table_setup_column("B");
                    ui.table_setup_column("C");

                    // Freeze first row so headers are visible even
                    // when scrolling
                    ui.table_setup_scroll_freeze(num_cols, 1);

                    // Done with headers row
                    ui.table_headers_row();

                    // Create clipper with st
                    let mut clip = imgui::ListClipper::new(num_rows).begin(ui);
                    while clip.step() {
                        for row_num in clip.display_start()..clip.display_end() {
                            ui.table_next_row();
                            for col_num in 0..num_cols {
                                ui.table_set_column_index(col_num);
                                ui.text(format!("Hello {},{}", col_num, row_num));
                            }
                        }
                    }
                }
            });
    });
}

[ocornut]

Quick lazy answer (not in front of the code now); Someone mentioned an issue on Lua, where some changes made code rely on the timing of the call to ImGuiListClipper destructor, which in Lua was lazily called “too late”. Can you try calling clipper.End() at the end of the loop to see if it makes a difference?

if that fixes it I should be able to fix things on my end.

[dbr]

Someone mentioned an issue on Lua, where some changes made code rely on the timing of the call to ImGuiListClipper destructor

Ahh I had seen that issue - I had opened a similar issue here, #610 which seems very similar (but, strangely, our lazily-called destrctor was causing a segfault rather than hitting that assert)

Can you try calling clipper.End() at the end of the loop to see if it makes a difference?

Yes calling clipper.End() before the end of the table makes it work correctly 😅

So this doesn't work (no scroll bars):

if let Some(_table) = ui.begin_table_with_sizing(...) {
    let mut clipper = imgui::ListClipper::new(num_rows).begin(ui);
    ....make table..
    // dropping calls clipper.end
}
// dropping calls table.end

..but reordering makes it work:

let mut clipper = imgui::ListClipper::new(num_rows).begin(ui);
if let Some(_t) = ui.begin_table_with_sizing(...) {
    ....make table..
}
// dropping calls table.end
// dropping calls clipper.end

Reordering the creation is a good enough workaround for me for now, but if the ordering requirement can be relaxed then that would be excellent 🥳

[ocornut]

@dbr Are you able to confirm a change on C++ side that would fix it without an intervention?
At the very end of ImGuiListClipper::Step() before the return false;, simply add an End() call.

[ocornut]

Pretty sure this should catch both cases now
ocornut/imgui@d9e60d2

[dbr]

Hmm, I quickly backported those changes to the version bundled into imgui-rs (1.86), but it doesn't seem to work - now the "no scrollbar table" causes the same segfault as the "double clipper.end()" call, crashing in ImGuiListClipper::Step because data is NULL:

2444	    if (data->StepNo == 0 && table != NULL && !table->IsUnfrozenRows)
(gdb) print(data)
$1 = (ImGuiListClipperData *) 0x0

It's entirely possible I made some silly mistake copying the changes over - changes I made, based on 32cf2b9 :

diff --git a/imgui-sys/third-party/imgui-master/imgui/imgui.cpp b/imgui-sys/third-party/imgui-master/imgui/imgui.cpp
index d7653d4..68be057 100644
--- a/imgui-sys/third-party/imgui-master/imgui/imgui.cpp
+++ b/imgui-sys/third-party/imgui-master/imgui/imgui.cpp
@@ -2396,15 +2396,14 @@ void ImGuiListClipper::Begin(int items_count, float items_height)
 
 void ImGuiListClipper::End()
 {
-    // In theory here we should assert that we are already at the right position, but it seems saner to just seek at the end and not assert/crash the user.
     ImGuiContext& g = *GImGui;
-    if (ItemsCount >= 0 && ItemsCount < INT_MAX && DisplayStart >= 0)
-        ImGuiListClipper_SeekCursorForItem(this, ItemsCount);
-    ItemsCount = -1;
-
-    // Restore temporary buffer and fix back pointers which may be invalidated when nesting
     if (ImGuiListClipperData* data = (ImGuiListClipperData*)TempData)
     {
+        // In theory here we should assert that we are already at the right position, but it seems saner to just seek at the end and not assert/crash the user.
+        if (ItemsCount >= 0 && ItemsCount < INT_MAX && DisplayStart >= 0)
+            ImGuiListClipper_SeekCursorForItem(this, ItemsCount);
+
+        // Restore temporary buffer and fix back pointers which may be invalidated when nesting
         IM_ASSERT(data->ListClipper == this);
         data->StepNo = data->Ranges.Size;
         if (--g.ClipperTempDataStacked > 0)
@@ -2414,6 +2413,7 @@ void ImGuiListClipper::End()
         }
         TempData = NULL;
     }
+    ItemsCount = -1;
 }
 
 void ImGuiListClipper::ForceDisplayRangeByIndices(int item_min, int item_max)
@@ -2546,8 +2546,8 @@ bool ImGuiListClipper::Step()
     // Advance the cursor to the end of the list and then returns 'false' to end the loop.
     if (ItemsCount < INT_MAX)
         ImGuiListClipper_SeekCursorForItem(this, ItemsCount);
-    ItemsCount = -1;
 
+    End();
     return false;
 }

[ocornut]

The patch looks good, but I'm confused as to how the crash can happen:

now the "no scrollbar table" causes the same segfault as the "double clipper.end()" call, crashing in ImGuiListClipper::Step because data is NULL:

That pointer is only set to NULL by End(), at a point where Step() returned false.
I assume you are not calling Step() after calling End()..

I may need you to investigate this further because I am not sure I understand how this could happen.

[dbr]

I assume you are not calling Step() after calling End()..

Not explicitly, but the drop impl (aka destructor) for the Rust imgui::ListClipper currently calls Step again, which is done to check if the clipper has been exhaused properly (and if not, it throws an error to the user) - and since Step() now calls End() on last iteration we end up effectively calling End twice

https://github.com/imgui-rs/imgui-rs/blob/v0.8.2/imgui/src/list_clipper.rs#L67-L81

..which with a bunch of prints added,

Step
Step called in C++
Step
Step called in C++
Step
Step called in C++
End called in C++
Drop impl running, which calls step again
Step
Step called in C++
Segmentation fault

I've no idea if that's a sensible thing to do (it's just like that from the original PR when someone added the clipper bindings), but seems like the problem occurs from just calling Step() after it has already returned false

[ocornut]

Yes that seeems pretty incorrect to do. Tho i suppose its not costly to make Step() support extra calls, i can add that.

[ocornut]

Tho it would be more sensible for me to make Step() assert there, and your code only call End().

[ocornut]

for the Rust imgui::ListClipper currently calls Step again, which is done to check if the clipper has been exhaused properly (and if not, it throws an error to the user) -

I have pushed an assert for it ocornut/imgui@dca527b

I think this is invalid behavior, you are adding an extra constraint that the original clipper doesn't have (it should be possible to stop rendering before the end of the list, End() will seek correctly, and I expect to add support for a INT_MAX mode to allow situations where list size is unknown). The only expectation is that an early out would need to call .End() or destructor earlier.
So IHMO that Drop code can simply call destructor.

[dbr]

Thanks!

Fixed on imgui-rs side in #626

I duplicated the C++ "make Step() assert there" behaviour in imgui-rs for current release, and also added an iterator based wrapper to the ListClipper which makes this impossible to trigger (since iter() takes ownership of the imgui::ListClipper, user is unable to call .step() manually)

In imgui/1.88 we could remove the panic from the rust side (noted by the let is_imgui_1_88_or_higher = false; variable), but it's also a pretty trivial amount of code so if we forget to remove it for a while, then, 🤷