From cc08285a0431385f7eca0da1b9e7ab5b28672bb6 Mon Sep 17 00:00:00 2001 From: dbr Date: Fri, 18 Mar 2022 22:07:27 +1100 Subject: [PATCH 1/6] Update the list clipper: 1. Avoid calling step() in destructor - incorrect and unnecessary, as it is not required to fully "use" the clipper (you can stop rendering half-way through) 2. Add a harder to misuse iterator interface to the clipper as an alterantive to the C++ style .step() interface Closes #610 --- imgui-examples/examples/long_list.rs | 17 ++- imgui/src/list_clipper.rs | 176 +++++++++++++++++++++++++-- 2 files changed, 181 insertions(+), 12 deletions(-) diff --git a/imgui-examples/examples/long_list.rs b/imgui-examples/examples/long_list.rs index 2204c53..e4b019a 100644 --- a/imgui-examples/examples/long_list.rs +++ b/imgui-examples/examples/long_list.rs @@ -15,8 +15,10 @@ fn main() { let system = support::init(file!()); system.main_loop(move |_, ui| { + // Show the C++ style API ui.window("Hello long world") - .size([300.0, 110.0], Condition::FirstUseEver) + .size([100.0, 500.0], Condition::FirstUseEver) + .position([10.0, 10.0], crate::Condition::Always) .build(|| { let mut clipper = imgui::ListClipper::new(lots_of_words.len() as i32) .items_height(ui.current_font_size()) @@ -27,5 +29,18 @@ fn main() { } } }); + + // Show the more Rust'y iterator + ui.window("Hello long world (iterator API)") + .size([100.0, 500.0], Condition::FirstUseEver) + .position([150.0, 10.0], crate::Condition::Always) + .build(|| { + let clipper = imgui::ListClipper::new(lots_of_words.len() as i32) + .items_height(ui.current_font_size()) + .begin(ui); + for row_num in clipper.iter() { + ui.text(&lots_of_words[row_num as usize]); + } + }); }); } diff --git a/imgui/src/list_clipper.rs b/imgui/src/list_clipper.rs index cfe4ff5..9363556 100644 --- a/imgui/src/list_clipper.rs +++ b/imgui/src/list_clipper.rs @@ -1,5 +1,4 @@ use std::marker::PhantomData; -use std::thread; use crate::sys; use crate::Ui; @@ -22,6 +21,7 @@ pub struct ListClipper { } impl ListClipper { + /// Begins configuring a list clipper. pub const fn new(items_count: i32) -> Self { ListClipper { items_count, @@ -45,6 +45,11 @@ impl ListClipper { } } +/// List clipper is a mechanism to efficiently implement scrolling of +/// large lists with random access. +/// +/// For example you have a list of 1 million buttons, and the list +/// clipper will help you only draw the ones which are visible. pub struct ListClipperToken<'ui> { list_clipper: *mut sys::ImGuiListClipper, _phantom: PhantomData<&'ui Ui>, @@ -58,37 +63,186 @@ impl<'ui> ListClipperToken<'ui> { } } + /// Progress the list clipper. + /// + /// If this returns returns `true` then the you can loop between + /// between `clipper.display_start() .. clipper.display_end()`. + /// If this returns false, you must stop calling this method. + /// + /// Calling step again after it returns `false` will cause imgui + /// to abort. This mirrors the C++ interface. + /// + /// It is recommended to use the iterator interface! pub fn step(&mut self) -> bool { unsafe { sys::ImGuiListClipper_Step(self.list_clipper) } } + /// This is automatically called back the final call to + /// `step`. You can call it sooner but typically not needed. pub fn end(&mut self) { unsafe { sys::ImGuiListClipper_End(self.list_clipper); } } + /// First item to call, updated each call to `step` pub fn display_start(&self) -> i32 { unsafe { (*self.list_clipper).DisplayStart } } + /// End of items to call (exclusive), updated each call to `step` pub fn display_end(&self) -> i32 { unsafe { (*self.list_clipper).DisplayEnd } } + + /// Get an iterator which outputs all visible indexes. This is the + /// recommended way of using the clipper. + pub fn iter(self) -> ListClipperIterator<'ui> { + ListClipperIterator::new(self) + } } impl<'ui> Drop for ListClipperToken<'ui> { fn drop(&mut self) { - if !self.step() { - unsafe { - sys::ImGuiListClipper_destroy(self.list_clipper); - }; - } else if !thread::panicking() { - panic!( - "Forgot to call End(), or to Step() until false? \ - This is the only token in the repository which users must call `.end()` or `.step()` \ - with. See https://github.com/imgui-rs/imgui-rs/issues/438" - ); + unsafe { + sys::ImGuiListClipper_destroy(self.list_clipper); + }; + } +} + +pub struct ListClipperIterator<'ui> { + list_clipper: ListClipperToken<'ui>, + exhausted: bool, + last_value: Option, +} + +impl<'ui> ListClipperIterator<'ui> { + fn new(list_clipper: ListClipperToken<'ui>) -> Self { + Self { + list_clipper, + exhausted: false, + last_value: None, } } } + +impl Iterator for ListClipperIterator<'_> { + type Item = i32; + + fn next(&mut self) -> Option { + if let Some(lv) = self.last_value { + // Currently iterating a chunk (returning all values + // between display_start and display_end) + let next_value = lv + 1; + + if lv >= self.list_clipper.display_end() - 1 { + // If we reach the end of the current chunk, clear + // last_value so we call step below + self.last_value = None; + } else { + // Otherwise just increment it + self.last_value = Some(next_value); + } + } + + if let Some(lv) = self.last_value { + // Next item within current step's chunk + Some(lv) + } else { + // Start iterating a new chunk + + if self.exhausted { + // If the clipper is exhausted, don't call step again! + None + } else { + // Advance the clipper + let ret = self.list_clipper.step(); + if !ret { + self.exhausted = true; + None + } else { + // Setup iteration for this step's chunk + let start = self.list_clipper.display_start(); + let end = self.list_clipper.display_end(); + + if start == end { + // Somewhat special case: if a single item, we + // don't store the last_value so we call + // step() again next iteration + self.last_value = None; + } else { + self.last_value = Some(start); + } + Some(start) + } + } + } + } +} + +#[test] +fn cpp_style_usage() { + // Setup + let (_guard, mut ctx) = crate::test::test_ctx_initialized(); + let ui = ctx.frame(); + + let _window = ui + .window("Example") + .position([0.0, 0.0], crate::Condition::Always) + .size([100.0, 800.0], crate::Condition::Always) + .begin(); + + // Create clipper + let clip = ListClipper::new(1000); + let mut tok = clip.begin(&ui); + + let mut ticks = 0; + + while dbg!(tok.step()) { + for row_num in dbg!(tok.display_start())..dbg!(tok.display_end()) { + dbg!(row_num); + ui.text("..."); + ticks += 1; + } + } + + // Check it's called an expected amount of time (only the ones + // visible in given sized window) + assert_eq!(ticks, 44); + + // Calling end multiple times is fine albeit redundant + tok.end(); + tok.end(); + tok.end(); + tok.end(); + tok.end(); + tok.end(); +} + +#[test] +fn iterator_usage() { + // Setup + let (_guard, mut ctx) = crate::test::test_ctx_initialized(); + let ui = ctx.frame(); + + let _window = ui + .window("Example") + .position([0.0, 0.0], crate::Condition::Always) + .size([100.0, 800.0], crate::Condition::Always) + .begin(); + + // Create clipper + let clip = ListClipper::new(1000); + + let mut ticks = 0; + + let tok = clip.begin(&ui); + for row_num in tok.iter() { + dbg!(row_num); + ui.text("..."); + ticks += 1; + } + + // Should be consistent with size in `cpp_style_usage` + assert_eq!(ticks, 44); +} From 39e45251b3a03c3237c6c1d2498a953647698318 Mon Sep 17 00:00:00 2001 From: dbr Date: Fri, 18 Mar 2022 22:07:58 +1100 Subject: [PATCH 2/6] Example showing how to use list clipper+table --- imgui-examples/examples/long_table.rs | 52 +++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 imgui-examples/examples/long_table.rs diff --git a/imgui-examples/examples/long_table.rs b/imgui-examples/examples/long_table.rs new file mode 100644 index 0000000..8b300e9 --- /dev/null +++ b/imgui-examples/examples/long_table.rs @@ -0,0 +1,52 @@ +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 clip = imgui::ListClipper::new(num_rows).begin(ui); + for row_num in clip.iter() { + 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)); + } + } + } + }); + }); +} From 68a5af174c5b6c31391ee20130ed4e1819e3f646 Mon Sep 17 00:00:00 2001 From: dbr Date: Sat, 19 Mar 2022 11:42:06 +1100 Subject: [PATCH 3/6] fmt --- imgui-examples/examples/long_table.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/imgui-examples/examples/long_table.rs b/imgui-examples/examples/long_table.rs index 8b300e9..c610ef1 100644 --- a/imgui-examples/examples/long_table.rs +++ b/imgui-examples/examples/long_table.rs @@ -40,11 +40,11 @@ fn main() { // Create clipper with st let clip = imgui::ListClipper::new(num_rows).begin(ui); for row_num in clip.iter() { - 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)); - } + 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)); + } } } }); From 5a76ed7ce6c6655206826da5fd119731f4b9d823 Mon Sep 17 00:00:00 2001 From: dbr Date: Sat, 19 Mar 2022 11:43:14 +1100 Subject: [PATCH 4/6] Clippy the wildly pedantic --- imgui/src/list_clipper.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/imgui/src/list_clipper.rs b/imgui/src/list_clipper.rs index 9363556..140e10a 100644 --- a/imgui/src/list_clipper.rs +++ b/imgui/src/list_clipper.rs @@ -194,7 +194,7 @@ fn cpp_style_usage() { // Create clipper let clip = ListClipper::new(1000); - let mut tok = clip.begin(&ui); + let mut tok = clip.begin(ui); let mut ticks = 0; @@ -236,7 +236,7 @@ fn iterator_usage() { let mut ticks = 0; - let tok = clip.begin(&ui); + let tok = clip.begin(ui); for row_num in tok.iter() { dbg!(row_num); ui.text("..."); From 091e5a6a15b2764f831a08f69c8b8714c052ae2b Mon Sep 17 00:00:00 2001 From: dbr Date: Sat, 19 Mar 2022 20:31:06 +1100 Subject: [PATCH 5/6] Workaround to turn imgui <~ 1.88 segfault into a panic --- imgui/src/list_clipper.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/imgui/src/list_clipper.rs b/imgui/src/list_clipper.rs index 140e10a..7b11684 100644 --- a/imgui/src/list_clipper.rs +++ b/imgui/src/list_clipper.rs @@ -53,6 +53,15 @@ impl ListClipper { pub struct ListClipperToken<'ui> { list_clipper: *mut sys::ImGuiListClipper, _phantom: PhantomData<&'ui Ui>, + + /// In upstream imgui < 1.87, calling step too many times will + /// cause a segfault due to null pointer. So we keep track of this + /// and panic instead. + /// + /// Fixed in https://github.com/ocornut/imgui/commit/dca527b which + /// will likely be part of imgui 1.88 - at which point this can be + /// removed. + consumed_workaround: bool, } impl<'ui> ListClipperToken<'ui> { @@ -60,6 +69,7 @@ impl<'ui> ListClipperToken<'ui> { Self { list_clipper, _phantom: PhantomData, + consumed_workaround: false, } } @@ -74,7 +84,19 @@ impl<'ui> ListClipperToken<'ui> { /// /// It is recommended to use the iterator interface! pub fn step(&mut self) -> bool { - unsafe { sys::ImGuiListClipper_Step(self.list_clipper) } + let is_imgui_1_88_or_higher = false; + if is_imgui_1_88_or_higher { + unsafe { sys::ImGuiListClipper_Step(self.list_clipper) } + } else { + if self.consumed_workaround { + panic!("ListClipperToken::step called after it has previously returned false"); + } + let ret = unsafe { sys::ImGuiListClipper_Step(self.list_clipper) }; + if ret { + self.consumed_workaround = true; + } + ret + } } /// This is automatically called back the final call to From a658073bd78e865de6375d2de28e8a1d6ceb6e76 Mon Sep 17 00:00:00 2001 From: dbr Date: Sat, 19 Mar 2022 21:07:57 +1100 Subject: [PATCH 6/6] Booleano --- imgui/src/list_clipper.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imgui/src/list_clipper.rs b/imgui/src/list_clipper.rs index 7b11684..a7c000c 100644 --- a/imgui/src/list_clipper.rs +++ b/imgui/src/list_clipper.rs @@ -92,7 +92,7 @@ impl<'ui> ListClipperToken<'ui> { panic!("ListClipperToken::step called after it has previously returned false"); } let ret = unsafe { sys::ImGuiListClipper_Step(self.list_clipper) }; - if ret { + if !ret { self.consumed_workaround = true; } ret