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

Fix ListClipper for single-item-list #3663

Closed
wants to merge 1 commit into from
Closed

Fix ListClipper for single-item-list #3663

wants to merge 1 commit into from

Conversation

nyorain
Copy link
Contributor

@nyorain nyorain commented Dec 19, 2020

ImGuiListClipper did not correctly calculate item height for single-item lists and therefore got the total y-offset wrong in the end. Code like this

for(auto& resList : resLists) {
  ImGui::Text("Begin");
  
  ImGuiListClipper clipper;
  clipper.Begin(int(resMap.size()));
  
  while(clipper.Step()) {
    for(auto i = clipper.DisplayStart; i < clipper.DisplayEnd; ++i) {
      auto& entry = resList[i];
      auto label = name(*entry.second);
      if(ImGui::Button(label.c_str())) {
        select(*entry.second.get());
      }
    }
  }
  
  ImGui::Text("End");
  ImGui::Separator();
}

Generates the following problem:

19 12_21 38 52

(This problem is present without the "Begin"/"End" texts and separators as well, they are for issue clarification).
Fixed this issue by moving the item height calculation before the early-out and End() call.
This is just a quick fix, it makes the code uglier and even more spaghettish, you might want to find something cleaner. I tested that it still works for lists with any number of items (including 0).

19 12_21 55 06

ImGuiListClipper did not correctly calculate item height for single-item
lists and therefore got the total y-offset wrong in the end.
Fix this issue by moving the item height calculation before the
early-out and End() call.
@ocornut ocornut added the bug label Dec 19, 2020
@ocornut
Copy link
Owner

ocornut commented Dec 21, 2020

Thanks for the PR. I could confirm the bug using this repro. Now going to look at the fix and add to our regression tests.

        ImGui::Begin("Test Window", NULL, ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoSavedSettings);
        const int counts[] = { 5, 1, 5, 1, 2 };
        for (int count_n = 0; count_n < IM_ARRAYSIZE(counts); count_n++)
        {
            ImGui::Text("Begin");

            ImGuiListClipper clipper;
            clipper.Begin(counts[count_n]);

            while (clipper.Step())
            {
                for (auto item_n = clipper.DisplayStart; item_n < clipper.DisplayEnd; item_n++)
                    ImGui::Button(Str30f("Section %d Button %d", count_n, item_n).c_str(), ImVec2(-FLT_MIN, 0.0f));
            }

            ImGui::Text("End");
            ImGui::Separator();
        }
        ImGui::End();

ocornut pushed a commit that referenced this pull request Dec 21, 2020
@ocornut
Copy link
Owner

ocornut commented Dec 21, 2020

Hello @nyorain,
I have merged a slightly different fix, tested over a larger battery of this, and attributed you to the commit.
See: a640698

Thank you!

@ocornut ocornut closed this Dec 21, 2020
@nyorain
Copy link
Contributor Author

nyorain commented Dec 21, 2020

Thanks a lot, keep up the good work!

@ocornut ocornut added the clipper label Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants