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

Rework/simplify ListClipper drop impl #610

Closed
dbr opened this issue Feb 10, 2022 · 4 comments · Fixed by #626
Closed

Rework/simplify ListClipper drop impl #610

dbr opened this issue Feb 10, 2022 · 4 comments · Fixed by #626
Assignees
Milestone

Comments

@dbr
Copy link
Contributor

dbr commented Feb 10, 2022

If you call .end on the list clipper, it'll crash when the destructor also calls the same

I don't think this happened in the last imgui-rs release, but happens in latest main branch (e.g 69fb34f)

The long_list example works fine as-is, but crashes if you do this:

$ git diff
diff --git a/imgui-examples/examples/long_list.rs b/imgui-examples/examples/long_list.rs
index 2204c53..fb7bab4 100644
--- a/imgui-examples/examples/long_list.rs
+++ b/imgui-examples/examples/long_list.rs
@@ -26,6 +26,7 @@ fn main() {
                         ui.text(&lots_of_words[row_num as usize]);
                     }
                 }
+                clipper.end();
             });
     });
 }
@dbr
Copy link
Contributor Author

dbr commented Feb 11, 2022

The crash happens in bool ImGuiListClipper::Step() on this line, because data is NULL:

if (data->StepNo == 0 && table != NULL && !table->IsUnfrozenRows)

The crashing call to step is happening via the Drop impl on ListClipper, which does this:

    fn drop(&mut self) {
        if !self.step() {
            unsafe {
                sys::ImGuiListClipper_destroy(self.list_clipper);
            };
        } else if !thread::panicking() {
            panic!(

This might be a regression in Dear ImGui, but..

Currently the ListClipper drop behaviour seems inconvinient - ignoring this bug, it'll panic if you don't exhaust the token, so I think something like

while clip.step() {
    ui.text(format!("Thing: {}", some_call()?));
}

..would panic in the case some_call()? returns early, which would be surprising at best

Given this..

  1. isn't impacting any released imgui-rs version
  2. the token is the last one with a panicing drop
  3. the upstream API has changed
  4. workaround in current main version is simple (just don't call .end(), as per long_list example)

Then it seems like we should just update the ListClipper to be inline with the permissive-dropping and new upstream API

@dbr dbr added this to the v0.9 milestone Feb 11, 2022
@dbr
Copy link
Contributor Author

dbr commented Feb 11, 2022

This most likely relates to ocornut/imgui#4822 in some way (although not exactly as that problem was hitting an intentional asser)

@ocornut
Copy link
Contributor

ocornut commented Feb 17, 2022

Pushed a fix upstream for this and #611
ocornut/imgui@d9e60d2

@dbr dbr changed the title Segfault with 1.86 if ListClipper.end is called twice Rework/simplify ListClipper drop impl Feb 26, 2022
@dbr
Copy link
Contributor Author

dbr commented Feb 26, 2022

Then it seems like we should just update the ListClipper to be inline with the permissive-dropping and new upstream API

This indeed seems like the right thing to do as per #611 (comment)

The drop impl should just call end if it's not already - the extra call to check Step() == false is a bad idea (there's nothing wrong with stopping it half-way through a table, for example)

While changing this, should also investigate if the list clipper should act as an iterator, so instead of

let mut clip = imgui::ListClipper::new(table.total_rows() as i32).begin(ui);

while clip.step() {
    for row_num in clip.display_start()..clip.display_end() {
        ui.text("...")
    }
}

we can instead write:

let mut clip = imgui::ListClipper::new(table.total_rows() as i32).begin(ui);

for row in clip.iter_rows() {
    ui.text("...")
}

Should be easy to retain both interfaces as both are useful

@dbr dbr self-assigned this Mar 18, 2022
dbr added a commit to dbr/imgui-rs that referenced this issue Mar 18, 2022
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 imgui-rs#610
@dbr dbr closed this as completed in #626 Mar 22, 2022
ctrlcctrlv pushed a commit to MFEK/imgui.rlib that referenced this issue Sep 24, 2022
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 imgui-rs#610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants