search

ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
60.63k stars 10.23k forks source link

New Tables API (alpha available for testing) #2957

Closed ocornut closed 3 years ago

ocornut commented 4 years ago

TABLES ARE NOW MERGED, MOVING THIS TO #3740

I have pushed an experimental tables branch: https://github.com/ocornut/imgui/tree/tables Providing a long awaited full-featured replacement to the old "Columns" API (#125).

I have been working on this for an embarassingly looong time.. What seemingly started as "let's refactor columns" became a multi-month thing with many rewrites/iterations. A large portion of this work been sponsored by Blizzard Entertainment. Several internal changes pushed to 1.71-1.74 were in preparation for this as I often try to have changes trickle down to master whenever possible to reduce complication of branching.

Please read <3 (this post will be occasionally updated)

Basic Usage: https://github.com/ocornut/imgui/issues/2957#issuecomment-569725733

TODO List: https://github.com/ocornut/imgui/issues/2957#issuecomment-569726095

WIP API breaking changes Sept 2020 https://github.com/ocornut/imgui/issues/2957#issuecomment-698317698

Question? Feedback? Bug report? Feature request? Please create a NEW ISSUE!

Status

Looking for early testers

Git Flow

Features

Some screenshots

image

image

image

image

image

sdhawk commented 4 years ago

It seems that too many nested popups that use tables will crash with the error:

Assertion failed: _Current == 0 && _Count <= 1 && "Nested channel splitting is not supported. Please use separate instances of ImDrawListSplitter.", file imgui_draw.cpp, line 1444

I should note that in my actual project this same error occurs with just one popup after being open/closed roughly 20 or so times, but I've been unable to isolate that version of it (I suspect they have the same root cause).

In this sample, the error happens after opening 8 popups:

void Table(int count)
{
    ImGui::SetNextWindowSize(ImVec2(300, 300));

    char tableId[32]; sprintf(tableId, "table %d", count);
    char popupId[32]; sprintf(popupId, "popup %d", count);

    if (ImGui::BeginTable(tableId, 2))
    {
        ImGui::TableNextRow();
        ImGui::TableSetColumnIndex(0);
        ImGui::Text("Column 0");

        ImGui::TableSetColumnIndex(1);
        if (ImGui::Button("open"))
        {
            ImGui::OpenPopup(popupId);
        }

        if (ImGui::BeginPopupModal(popupId))
        {
            Table(count + 1);
            ImGui::EndPopup();
        }

        ImGui::EndTable();
    }
}

void Sample()
{
    ImGui::SetNextWindowSize(ImVec2(300, 300));

    ImGui::Begin("Sample");
    Table(0);
    ImGui::End();
}
ocornut commented 4 years ago

Everyone:

BREAKING CHANGES (SEPT 2020)

I haved broken theTableGetSortSpecs() API to make user explicitely clear the dirty flag:

const ImGuiTableSortSpecs* sorts_specs = ImGui::TableGetSortSpecs();
if (sorts_specs && sorts_specs->SpecsChanged)
{
    // [...] Perform sort
}

To:

ImGuiTableSortSpecs* sorts_specs = ImGui::TableGetSortSpecs();
if (sorts_specs && sorts_specs->SpecsDirty)
{
    // [...] Perform sort
    sorts_specs->SpecsDirty = false; // Clear dirty flag
}

The reason being that TableGetSortSpecs() had user-visible side-effect of effectively clearing that flag so multiple calls would lead to confusing bugs. Current version is less bug prone. See change 68ef531b06

Everyone: Prefer opening a new issue to submit a bug.

@sdhawk

It seems that too many nested popups that use tables will crash with the error:

Thank you for reporting with a nice repro! it was a bug holding on a table pointer accross resizing the pool used to store them. Our ImVector implementation by default would typically allocate 8 items ahead of them so bug wasn't noticeable. Fixed with 4cb0431e6.

ocornut commented 4 years ago

BREAKING CHANGES (SEPT 2020)

I'll be making some more breaking changes toward trying to stabilizing the api for 1.80.

Recently

ocornut commented 4 years ago

BREAKING CHANGES (OCT 2020)

Doing some more breaking as I'm hoping to promote this branch a lots more after releasing 1.79, toward merging the branch into master in 1.80.

Those changes reflected recurrent feedback from first-time users, so I'm biting the bullet and changing things now. Note that most the demo has been suggesting using TableSetColumnIndex() so code written using that won't be affected.

- In most situations you can use TableNextRow() + TableSetColumnIndex(xx) to start appending into a column.
- If you are using tables as a sort of grid, where every columns is holding the same type of contents,
  you may prefer using TableNextColumn() instead of TableNextRow() + TableSetColumnIndex().
  TableNextColumn() will automatically wrap-around into the next row if needed.
- IMPORTANT: Comparatively to the old Columns() API, we need to call TableNextColumn() for the first column!
- Here's a summary of possible call flow:
  ----------------------------------------------------------------------------------------------------------------
    TableNextRow() -> TableSetColumnIndex(0) -> Button("Hello 0") -> TableSetColumnIndex(1) -> Button("Hello 1")   // OK
    TableNextRow() -> TableNextColumn()         Button("Hello 0") -> TableNextColumn()      -> Button("Hello 1")   // OK
                      TableNextColumn()         Button("Hello 0") -> TableNextColumn()      -> Button("Hello 1")   // OK: TableNextColumn() automatically gets to next row!
    TableNextRow()                              Button("Hello 0") ................................................ // Not OK! Missing TableSetColumnIndex() or TableNextColumn()!
  ----------------------------------------------------------------------------------------------------------------
ocornut commented 3 years ago

@marinjurjevic Please post this in a new issue as instructed above and delete message here, thank you!

ocornut commented 3 years ago

Recent changes to Tables:

Before shipping 1.80 the things I would like to solve are:

ocornut commented 3 years ago

BREAKING CHANGES (NOV 2020)

Everyone, I have pushed a few "major" breaking changes yesterday. I think they may be the final batch of breaking changes going in before going to master.

Sorry for the disturbance, hopefully this is for the good!

frink commented 3 years ago

Is the plan to obsolete the old Column API completely or are both approaches sharing the same set of flags...

ocornut commented 3 years ago

@frink the public columns API have no flags, only internal api has, see: 72de6f336070bb28281b758609f4097a58adf901 https://github.com/ocornut/imgui/issues/125#issuecomment-729996960

frink commented 3 years ago

I stand corrected. I misread your post on #125 this morning.

ocornut commented 3 years ago

Hello,

After some internal monologue, I have undid this specific change mentioned two days ago:

Renamed ImGuiTableColumnFlags_XXX to ImGuiColumnFlags_XXX, ImGuiTableRowFlags_XXX to ImGuiRowFlags_XXX.

Effectively nuked top-most commit 694f6cb from tables/ history. It's not worth creating the small inconsistency to simplify/shorten those symbols and they are not so often used, so backtracked on that decision.

ocornut commented 3 years ago

I realized that one essential feature of tables was not demonstrated anywhere in the demo, and poorly documented. Added a bit of demo code now.

Tables with the same identifier share all settings (width, visibility, order, etc) tables_synced

This is also de-facto a sensible workaround for creating contents that span an entire row (aka #3565) + ref #2136.

ocornut commented 3 years ago

BREAKING CHANGES (DEC 2020)

INCOMING RELEASE

Tables have been merged into master today. I'll make a few extra changes, gather extra feedback, open a new thread soon (and close this one), and tag as 1.80 hopefully next week.

ice1000 commented 3 years ago

Hi, is there going to be a summary of the tables branch (and I guess it might be the new thread you mentioned in the above comment)?

ocornut commented 3 years ago

Hi, is there going to be a summary of the tables branch (and I guess it might be the new thread you mentioned in the above comment)?

Yes I'm going to open a new threads when ready, but the top posts here + demo are giving a good summary already.

I found one niggle with sizing policy which I want to settle before tagging 1.80 hence the delay.

Related to table, since last week:

BREAKING CHANGES (DEC 2020)

ocornut commented 3 years ago

BREAKING CHANGES (DEC 2020)

To support #3605 I made a little change to the default value and handling of the ImVec2 outer_size parameter.

The new meaning of outer_size.x == 0.0f is "automatic width". If Scrolling is enabled or if any columns is set to Stretch, outer_size.x == 0.0f is the same as outer_size == -FLT_MIN (will right-align). If Scrolling is disabled and all columns are set to Non-Stretch, then outer_size.x == 0.0f allows to create tables which don't use the full window width while not having to specify a width ahead:

image

Effectively this is unlikely to break much code as outer_size is only required for scrolling tables. (As a data point: The only things affected in the demo were demo tables which had a toggle to switch between scrolling and not-scrolling, and therefore had specific height provided and yet could have their scrolling disabled, which is not likely to appear in real code.)

iocafe commented 3 years ago

Just a positive note. Tables work well and are very functional, I especially like fixed row and column headers and fast display of even large tables. The architecture is clean and understandable. During the last months, judging by GitHub updates, the development in this area has been fast, now I am looking forward to the next stable version to test. Thank you:)

ocornut commented 3 years ago

BREAKING CHANGES (JAN 2021)

I know the joke is getting old now :) but last month while looking at a totally unrelated thing (#1149) I came up with a few realization and situations where the sizing policies were inadequate, which eventually got me into a larger/deeper exploration of various sizing-related ideas and a mild refactor of the exposed policilies. As a result I've again broken the API (but only in a minor way and only if you specified a table sizing policy explicitly).

TL;DR: ImGuiTableFlags_ColumnsWidthFixed > ImGuiTableFlags_SizingFixedFit ImGuiTableFlags_ColumnsWidthStretch > ImGuiTableFlags_SizingStretchSame.

If you are using Tables I would appreciate if you could update to latest to make sure everything's alright. And then hopefully we can tag 1.80....

Here's a run down of table sizing policies:

// Sizing Policy (read above for defaults)
ImGuiTableFlags_SizingFixedFit    = 1 << 13,  // Columns default to _WidthFixed or _WidthAuto (if resizable or not resizable), matching contents width.
ImGuiTableFlags_SizingFixedSame   = 2 << 13,  // Columns default to _WidthFixed or _WidthAuto (if resizable or not resizable), matching the maximum contents width of all columns. Implicitly enable ImGuiTableFlags_NoKeepColumnsVisible.
ImGuiTableFlags_SizingStretchProp = 3 << 13,  // Columns default to _WidthStretch with default weights proportional to each columns contents widths.
ImGuiTableFlags_SizingStretchSame = 4 << 13,  // Columns default to _WidthStretch with default weights all equal, unless overriden by TableSetupColumn().

New section in the demo:

tables sizing policies

e.g. a use of ImGuiTableFlags_SizingStretchProp:

SizingStretchProp

There are many details under the hood which I don't have the energy to explain right now (most you don't really need to know until you hit very specific use cases) but added variety of comments and added myself more notes.

Also, previously default table policy in an auto-resizing window (e.g. a tooltip) would lead to stretching columns with same weight which was wholly inadequate, we're now using ImGuiTableFlags_SizingFixedFit by default in an auto-resizing window.

One last thing I'd like to do is clarify per-column Auto vs Fixed policies and the effect of some of the internals unexposed api to override current width (not default width).

Bonus fact: Our regression tests are now covering 97% lines of imgui_tables.cpp, aka 61 uncovered and many are asserts/error handlers.

iocafe commented 3 years ago

Quick test Dear ImGui, 9.1.2021 docking branch, commit tag 70703da7829ba74a92a08fe3c6fe0664c39c97c9

I switched to use this version, all started working quickly. Notes:

Based on the quick test, all good with the newest table code. Best regards, Pekka

ocornut commented 3 years ago

@iocafe

ImGuiTableFlags_SizingPolicyStretchX -> ImGuiTableFlags_SizingStretchProp

Note that the old ImGuiTableFlags_SizingPolicyStretchX is == ImGuiTableFlags_SizingStretchSame. ImGuiTableFlags_SizingStretchProp is a slightly different behavior.

TableGetHoveredColumn() now internal, included "imgui_internal.h". (I need to look for better way to do this some point).

Post above says: "Removed TableGetHoveredColumn() in favor of using TableGetColumnFlags() & ImGuiTableColumnFlags_IsHovered."

I hit to 64 columns limit but decided to live with this for now. Support to more columns maybe in future, the original code has preparation for more columns (typedef ImGuiTableColumnIdx and draw channels). I hope for a real tested version sometime in the future (much preferred over my own quick and dirty patches).

Yes I think we'll support it eventually.

iocafe commented 3 years ago

Thank you for help and advice. When I work with ImGui, it feels like it is on the verge of break though to much wider use. Recent docking and table features extended the application area to office, database and data manipulation. It is good, fast and clean, and provides "game like" immediate response to the end user. It is open source, can be integrated easily to any application project and be adopted to run on practically any modern hardware. And most of all, it is ready and reliable. Many thanks, tables are great!

On Mon, Jan 11, 2021 at 1:24 PM omar notifications@github.com wrote:

@iocafe https://github.com/iocafe

ImGuiTableFlags_SizingPolicyStretchX -> ImGuiTableFlags_SizingStretchProp

Note that the old ImGuiTableFlags_SizingPolicyStretchX is == ImGuiTableFlags_SizingStretchSame. ImGuiTableFlags_SizingStretchProp is a slightly different behavior.

TableGetHoveredColumn() now internal, included "imgui_internal.h". (I need to look for better way to do this some point).

Post above says: "Removed TableGetHoveredColumn() in favor of using TableGetColumnFlags() & ImGuiTableColumnFlags_IsHovered."

I hit to 64 columns limit but decided to live with this for now. Support to more columns maybe in future, the original code has preparation for more columns (typedef ImGuiTableColumnIdx and draw channels). I hope for a real tested version sometime in the future (much preferred over my own quick and dirty patches).

Yes I think we'll support it eventually.

  • The ImU64 mask will be changed to something like ImBitArray or ImBitVector. Neither are currently satisfactory as one would require a worst-case limit and the other would heap-allocate. Instead we will allocate everything we need for the 4 masks inside the RawData single-alloc and call the lower-level ImBitArrayXXX functions directly. The MAIN purpose of all those packed masks is to avoid touching unnecessary cache-lines for non-visible columns (otherwise we could naively move all those flags as bool stored into each columns, but that would defeat their purpose).
  • I would like to move ImDrawSplitter and some other transient data out of ImGuiTable and into a structure shared by all tables for a given recursion level, that will alleviate the recurrent cost of high-columns tables.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ocornut/imgui/issues/2957#issuecomment-758136035, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANCVOSGWHRVRE7EO6NPYHZ3SZM64NANCNFSM4KBOCHAQ .