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

Help users if they are reusing the same ID (Fix #7669) #7961

Closed
wants to merge 1 commit into from

Conversation

pthom
Copy link
Contributor

@pthom pthom commented Sep 6, 2024

(fix for #7669)

There is a common user issue which is that developers may reuse the same ID, and will not understand why their widget is not responsive.

This PR will:

  • detect when the same ID is used on different widgets (only if the widget is hovered, to avoid a O(n^2) search at each frame)
  • display a warning tooltip to the user, with hints on how to fix the issue (e.g. use PushID, or add a ## to the label)
  • allow the user to break in the debugger on any of the widgets that use the same ID
  • enable to disable the warning when needed (e.g. when reusing the ID is intentional, via PushAllowDuplicateID / PopAllowDuplicateID)
  • enable to debug all duplicates at once (by defining IMGUI_DEBUG_PARANOID)

View from 10,000 feet

  1. This PR adds a distinction between
// Same as before
ImGuiID     GetID(const char* str, const char* str_end = NULL);

and

// returns GetID(), but may display a warning tooltip if the ID is not unique. Should be called only once per frame with a given ID stack.
ImGuiID     ReserveUniqueID(const char* str_id);

This distinction was applied to several widgets inside imgui_widgets.cpp which now call ReserveUniqueID instead of GetID,
when their intention is to reserve a unique ID for the widget.

  1. For grepability, most of the changes are highlighted with a comment // TRKID.
    The only changes which are not marked with this are where GetID() was simply replace by ReserveUniqueID() inside imgui_widgets.cpp
    These // TRKID comments should be removed before merging, and they are only here to help reviewing the changes.

  2. The full ImGui::ShowDemoWindow() was reviewed to make sure that no duplicated ID was used.
    Very few changes were required, and they are detailed in the PR.

  3. The tooltip will look like this
    image

5.See this video for a quick overview of the feature and how it is implemented:

https://traineq.org/ImGui/ImGui_PR_Warn_reuse_ID_Explanation.mp4


Note: if desired, a compile flag (e.g. IMGUI_NO_WARN_DUPLICATE_ID or its inverse) could be added to disable the warning and make ReserveUniqueID behave like GetID()

Summary of changes

struct ImGuiContext: added fields UidIXXX (UidsThisFrame & co)

Inside imgui_internal.h / struct ImGuiContext

struct ImGuiContext
{
    ...
    // Declaration
    ImVector<ImGuiID>       UidsThisFrame;                      // A list of item IDs submitted this frame (used to detect and warn about duplicate ID usage).
    int                     UidAllowDuplicatesStack;            // Stack depth for allowing duplicate IDs (if > 0, duplicate IDs are allowed). See PushAllowDuplicateID / PopAllowDuplicateID
    ImGuiID                 UidHighlightedDuplicate;            // Will be set if a duplicated item is hovered (all duplicated items will appear with a red dot in the top left corner on the next frame)
    int                     UidHighlightedTimestamp;            // Timestamp (FrameCount) of the highlight (which will be shown on the next frame)
    bool                    UidWasTipDisplayed;                 // Will be set to true to avoid displaying multiple times the same tooltip
    ...

    // Constructor
    ImGuiContext(ImFontAtlas* shared_font_atlas) {
        ...
        UidsThisFrame.clear();
        UidAllowDuplicatesStack = 0;
        UidHighlightedDuplicate = 0;
        UidHighlightedTimestamp = 0;
        UidWasTipDisplayed = false;
        ...
    }
};

struct ImGuiWindow: added ReserveUniqueID(), beside GetID()

imgui_internal.h / near GetID() existing methods

struct ImGuiWindow {
    ...
    // ReserveUniqueID: returns GetID(), but may display a warning. Should be called only once per frame with a given ID
    ImGuiID     ReserveUniqueID(const char* str_id);
    ...
};

Added PushAllowDuplicateID / PopAllowDuplicateID

imgui_internal.h / near related ID functions (SetActiveID, GetIDWithSeed, ...)

    ...
    IMGUI_API void          PushAllowDuplicateID();         // Disables the tooltip warning when duplicate IDs are detected.
    IMGUI_API void          PopAllowDuplicateID();          // Re-enables the tooltip warning when duplicate IDs are detected.
    ...

imgui.cpp:3873 / near ReserveUniqueID() implementation

IMGUI_API void ImGui::PushAllowDuplicateID()
{
    ImGuiContext& g = *GImGui;
    g.UidAllowDuplicatesStack++;
}

IMGUI_API void ImGui::PopAllowDuplicateID()
{
    ImGuiContext& g = *GImGui;
    IM_ASSERT(g.UidAllowDuplicatesStack > 0 && "Mismatched PopAllowDuplicateID()");
    g.UidAllowDuplicatesStack--;
}

Note: doing this via ItemFlags was not feasible as many items (e.g. Button, Checkbox) do not expose flags

Main logic: ImGui::EndFrame(), NewFrame() and ImGuiWindow::ReserveUniqueID()

ImGui::NewFrame():

    ...
    g.UidsThisFrame.resize(0);  // Empty the list of items, but keep its allocated data
    g.UidWasTipDisplayed = false;

ImGui::EndFrame():

    IM_ASSERT(g.UidAllowDuplicatesStack == 0 && "Mismatched Push/PopAllowDuplicateID");
    if (g.UidHighlightedTimestamp != ImGui::GetFrameCount())
    {
        g.UidHighlightedTimestamp = 0;
        g.UidHighlightedDuplicate = 0;
    }

ImGuiWindow::ReserveUniqueID()

in imgui.cpp, at the end of "[SECTION] ID STACK".
The pseudocode below summarizes its behavior:

IMGUI_API ImGuiID ImGuiWindow::ReserveUniqueID(const char* str_id) {
    ImGuiContext& g = *GImGui;
    ImGuiID id = GetID(str_id);

    // If PushAllowDuplicateID was called, do not check for uniqueness
    if (g.UidAllowDuplicatesStack != 0)
    return id;

    // Visual aid: a red circle in the top-left corner of all widgets that use a duplicate of the id
    if (id == g.UidHighlightedDuplicate)
        Draw Red dot on top left corner

    // Only check for uniqueness if hovered (this avoid a O(n^2) search at each frame)
    if (id == ImGui::GetHoveredID())
    {
        if (g.UidsThisFrame.contains(id) && !g.UidWasTipDisplayed)
        {
            // Prepare highlight on the next frame for widgets that use this ID
            g.UidHighlightedDuplicate = id;
            g.UidHighlightedTimestamp = ImGui::GetFrameCount();
            if (ImGui::BeginTooltip())
            {
                if (! g.DebugItemPickerActive)
                {
                    // Display tooltip showing the duplicate Id as a string
                    ...
                    // Show hints for correction (PushID, ##)
                    ...

                    ImGui::Text("    Press Ctrl-P to break in any of those items    ");
                    if Ctrl-P
                        g.DebugItemPickerActive = true;
                }
                else
                {
                    // Add additional info at the bottom of the ItemPicker tooltip
                    ImGui::Text("Click on one of the widgets which uses a duplicated item");
                }
                ImGui::EndTooltip();
            }
            g.UidWasTipDisplayed = true;
        }
    }
    g.UidsThisFrame.push_back(id);
    return id;
}

imgui_widgets.cpp

Use ReserveUniqueID, allow duplicates when needed (TempInputText, BeginMenuEx)
I went through all the usages of window->GetID() and changed them to ReserveUniqueID when it was possible / desirable.

Places with one-line changes

In all the places below, the change was extremely simple: I simply replaced window->GetID by window->ReserveUniqueID.

For example, in ImGui::ButtonEx

bool ImGui::ButtonEx(const char* label, const ImVec2& size_arg, ImGuiButtonFlags flags)
    ...
    const ImGuiID id = window->ReserveUniqueID(label);  // Instead of window->GetID

Below is the list of all function where this simple change was applied:

    InvisibleButton
    ArrowButtonEx
    ImageButton  (both versions)
    Checkbox
    RadioButton
    BeginCombo
    DragScalar
    SliderScalar
    VSliderScalar
    InputTextEx
    ColorButton
    CollapsingHeader
    Selectable
    PlotEx
    BeginTabBar
    TextLink
    TreeNode(const char *)
    TreeNodeEx(const char *)
    TreeNodeExV(const char *)

Places where duplicates are needed

TempInputText

TempInputText create text input in place of another active widget (e.g. used when doing a CTRL+Click on drag/slider widgets).
It reuses the widget ID, so we allow it temporarily via PushAllowDuplicateID / PopAllowDuplicateID

bool ImGui::TempInputText(const ImRect& bb, ImGuiID id, const char* label, char* buf, int buf_size, ImGuiInputTextFlags flags)
{
    ImGui::PushAllowDuplicateID();
    ...
    ImGui::PopAllowDuplicateID();
}

GetWindowScrollbarID

GetWindowScrollbarID had to be split in two: ReserveWindowScrollbarID will perform the ID reservation, while GetWindowScrollbarID is only a query.
Luckily, this is the only instance where this was required.

BeginMenuEx

In BeginMenuEx, we initially call GetID() instead of ReserveUniqueID() because were are not "consuming" this ID:
instead, we are only querying IsPopupOpen().
The ID will be consumed later by calling ImGui::PushID(label) + Selectable("") (which will lead to the same ID)

bool ImGui::BeginMenuEx(const char* label, const char* icon, bool enabled) {
    const ImGuiID id = window->GetID(label);
    bool menu_is_open = IsPopupOpen(id, ImGuiPopupFlags_None);

    ...

    PushID(label);
    pressed = Selectable("", menu_is_open, selectable_flags, ImVec2(w, label_size.y));

    ...
}

Places with no changes, but worth mentioning

  • TreeNode(const void *): unchanged
    TreeNode(const void* ptr_id) and TreeNodeExV(const void *) are the only instances which call window->GetID(const void *).
    As a consequence, they cannot use the unique id mechanism, but I think their usage is rare enough that is not a big concern.

  • ImGui::TabBarCalcTabID: unchanged
    It still uses window->GetID, to avoid issues when merging with the docking branch.

  • BeginChild, BeginPopup and co are unchanged:
    since they use ImGui::Begin()/End() internally), I suspect they might be called multiple times with the same string,
    since it is allowed with ImGui::Begin/End.

imgui_demo.cpp: some minor changes

I went through the full demo Window to look for instances where a duplicate ID might be used, and corrected them.
Very few changes were required:

  • "Layout/Text Baseline Alignment": simple ID fix (Added ##)
  • "Widgets/Plotting": simple id fix (Added ##)
  • "Columns (legacy API)/Borders": added ImGui::PushID for each cell
  • "Widgets/Drag and Drop/Drag to reorder items (simple)": no change, but a transient duplicate ID may appear during reordering
    (for one frame only: see comments in the code)

Demo code in order to test this PR

Add and call this function in any imgui example. This code will trigger the warnings for duplicated IDs,
and will also enable to make sure that the fixes that had to be done are OK.

char text1[500] = "Hello,";
char text2[500] = "world!";
char text3[500] = "Hello,";
char text4[500] = "world!";
int value = 0;
bool flag1 = false, flag2 = false;
float f1 = 0.f, f2 = 0.f, f3 = 0.f, f4 = 0.f;

void Gui()
{
    // Some widgets to show the duplicated ID warnings
    ImGui::SetNextWindowSize(ImVec2(400, 500));
    if (ImGui::Begin("Duplicated ID examples", nullptr, ImGuiWindowFlags_MenuBar))
    {
        ImGui::TextWrapped("Example of widgets using duplicated IDs");
        ImGui::Separator();

        ImGui::TextWrapped("Only the first button is clickable (same ID)");
        if (ImGui::Button("button")) printf("Button1 pressed\n");
        if (ImGui::Button("button")) printf("Button2 pressed\n");

        ImGui::Separator();

        ImGui::TextWrapped("Those text inputs will share the same value (editing one will overwrite the other)");
        ImGui::InputText("text", text1, IM_ARRAYSIZE(text1));
        ImGui::InputText("text", text2, IM_ARRAYSIZE(text2));

        ImGui::Separator();

        ImGui::TextWrapped("Those radio buttons will share the same value (selecting one will overwrite the other)");
        ImGui::RadioButton("radio", &value, 0);
        ImGui::RadioButton("radio", &value, 1);

        ImGui::Separator();

        ImGui::TextWrapped("The second checkbox cannot be toggled");
        ImGui::Checkbox("checkbox", &flag1);
        ImGui::Checkbox("checkbox", &flag2);

        ImGui::Separator();

        ImGui::TextWrapped("Those two sliders will share the same value (editing one will overwrite the other");
        ImGui::SliderFloat("slider", &f1, 0.f, 1.f);
        ImGui::SliderFloat("slider", &f2, 0.f, 1.f);

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

    // Some widgets to test the fixes that had to be done
    ImGui::SetNextWindowSize(ImVec2(400, 400));
    if (ImGui::Begin("Regression tests", nullptr, ImGuiWindowFlags_MenuBar))
    {
        ImGui::TextWrapped("Small issues had to be fixed inside DragFloat (cf TempInputText), InputTextMultiline and BeginMenuBar.\n"
            "This window is here to test those fixes.");

        ImGui::Separator();

        if (ImGui::BeginMenuBar())
        {
            if (ImGui::BeginMenu("My Menu"))
            {
                ImGui::MenuItem("Item 1");
                ImGui::EndMenu();
            }
            ImGui::EndMenuBar();
        }

        ImGui::TextWrapped("DragFloat will reuse the Item ID when double clicking. This is allowed via Push/PopAllowDuplicateID");
        ImGui::DragFloat("dragf", &f3);

        ImGui::Separator();

        ImGui::TextWrapped("An issue was fixed with the InputTextMultiline vertical scrollbar, which trigerred a duplicate ID warning, which was fixed");
        ImGui::InputTextMultiline("multiline", text1, 500);

    }
    ImGui::End();

    // A window that is populated in several steps should still detect duplicates
    {
        ImGui::SetNextWindowSize(ImVec2(400, 200));

        ImGui::Begin("Window populated in several steps");
        ImGui::TextWrapped("This window is populated between several ImGui::Begin/ImGui::End calls (with the same window name).\n"
                           "Duplicates should be detected anyhow.");
        ImGui::Text("A first InputText");
        ImGui::InputText("TTT", text3, 500);
        ImGui::End();

        ImGui::Begin("Window populated in several steps");
        ImGui::Separator();
        ImGui::Text("The second InputText below reuses the same ID, \nin a different ImGui::Begin/ImGui::End context \n(but with the same window name)");
        ImGui::InputText("TTT", text4, 500);
        ImGui::End();
    }
}

(fix for ocornut#7669)

There is a common user issue which is that developers may reuse the same ID, and will not understand why their widget is not responsive.

This PR will:
* detect when the same ID is used on different widgets (only if the widget is hovered, to avoid a O(n^2) search at each frame)
* display a warning tooltip to the user, with hints on how to fix the issue (e.g. use PushID, or add a ## to the label)
* allow the user to break in the debugger on any of the widgets that use the same ID
* enable to disable the warning when needed (e.g. when reusing the ID is intentional, via PushAllowDuplicateID / PopAllowDuplicateID)
* enable to debug all duplicates at once (by defining IMGUI_DEBUG_PARANOID)

**View from 10,000 feet**

1. This PR adds a distinction between

```cpp
// Same as before
ImGuiID     GetID(const char* str, const char* str_end = NULL);
 ```
and
```cpp
// returns GetID(), but may display a warning tooltip if the ID is not unique. Should be called only once per frame with a given ID stack.
ImGuiID     ReserveUniqueID(const char* str_id);
```

This distinction was applied to several widgets inside imgui_widgets.cpp which now call ReserveUniqueID instead of GetID,
when their intention is to reserve a unique ID for the widget.

2. For grepability, most of the changes are highlighted with a comment `// TRKID`.
   The only changes which are not marked with this are where GetID() was simply replace by ReserveUniqueID() inside imgui_widgets.cpp
   These `// TRKID` comments should be removed before merging, and they are only here to help reviewing the changes.

3. The full `ImGui::ShowDemoWindow()` was reviewed to make sure that no duplicated ID was used.
   Very few changes were required, and they are detailed in the PR.

4.See this video for a quick overview of the feature and how it is implemented:

https://traineq.org/ImGui/ImGui_PR_Warn_reuse_ID_Explanation.mp4

----

> _Note: if desired, a compile flag (e.g. IMGUI_NO_WARN_DUPLICATE_ID or its inverse) could be added to disable the warning and make ReserveUniqueID behave like GetID()_

Summary of changes
==================

struct ImGuiContext: added fields UidIXXX (UidsThisFrame & co)
--------------------------------------------------------------

Inside imgui_internal.h / struct ImGuiContext
```cpp
struct ImGuiContext
{
    ...
    // Declaration
    ImVector<ImGuiID>       UidsThisFrame;                      // A list of item IDs submitted this frame (used to detect and warn about duplicate ID usage).
    int                     UidAllowDuplicatesStack;            // Stack depth for allowing duplicate IDs (if > 0, duplicate IDs are allowed). See PushAllowDuplicateID / PopAllowDuplicateID
    ImGuiID                 UidHighlightedDuplicate;            // Will be set if a duplicated item is hovered (all duplicated items will appear with a red dot in the top left corner on the next frame)
    int                     UidHighlightedTimestamp;            // Timestamp (FrameCount) of the highlight (which will be shown on the next frame)
    bool                    UidWasTipDisplayed;                 // Will be set to true to avoid displaying multiple times the same tooltip
    ...

    // Constructor
    ImGuiContext(ImFontAtlas* shared_font_atlas) {
        ...
        UidsThisFrame.clear();
        UidAllowDuplicatesStack = 0;
        UidHighlightedDuplicate = 0;
        UidHighlightedTimestamp = 0;
        UidWasTipDisplayed = false;
        ...
    }
};
```

struct ImGuiWindow: added ReserveUniqueID(), beside GetID()
-----------------------------------------------------------

imgui_internal.h / near GetID() existing methods
```cpp
struct ImGuiWindow {
    ...
    // ReserveUniqueID: returns GetID(), but may display a warning. Should be called only once per frame with a given ID
    ImGuiID     ReserveUniqueID(const char* str_id);
    ...
};
```

Added PushAllowDuplicateID / PopAllowDuplicateID
------------------------------------------------

imgui_internal.h  / near related ID functions (SetActiveID, GetIDWithSeed, ...)
```cpp
    ...
    IMGUI_API void          PushAllowDuplicateID();         // Disables the tooltip warning when duplicate IDs are detected.
    IMGUI_API void          PopAllowDuplicateID();          // Re-enables the tooltip warning when duplicate IDs are detected.
    ...
```

imgui.cpp:3873 / near ReserveUniqueID() implementation
```cpp
IMGUI_API void ImGui::PushAllowDuplicateID()
{
    ImGuiContext& g = *GImGui;
    g.UidAllowDuplicatesStack++;
}

IMGUI_API void ImGui::PopAllowDuplicateID()
{
    ImGuiContext& g = *GImGui;
    IM_ASSERT(g.UidAllowDuplicatesStack > 0 && "Mismatched PopAllowDuplicateID()");
    g.UidAllowDuplicatesStack--;
}
```

> _Note: doing this via ItemFlags was not feasible as many items (e.g. Button, Checkbox) do not expose flags_

Main logic: ImGui::EndFrame(), NewFrame() and ImGuiWindow::ReserveUniqueID()
----------------------------------------------------------------------------

### ImGui::NewFrame():

```cpp
    ...
    g.UidsThisFrame.resize(0);  // Empty the list of items, but keep its allocated data
    g.UidWasTipDisplayed = false;
```

### ImGui::EndFrame():
```cpp
    IM_ASSERT(g.UidAllowDuplicatesStack == 0 && "Mismatched Push/PopAllowDuplicateID");
    if (g.UidHighlightedTimestamp != ImGui::GetFrameCount())
    {
        g.UidHighlightedTimestamp = 0;
        g.UidHighlightedDuplicate = 0;
    }
```

### ImGuiWindow::ReserveUniqueID()
in imgui.cpp, at the end of "[SECTION] ID STACK".
The pseudocode below summarizes its behavior:

```cpp
IMGUI_API ImGuiID ImGuiWindow::ReserveUniqueID(const char* str_id)
{
    ImGuiContext& g = *GImGui;
    ImGuiID id = GetID(str_id);

    // If PushAllowDuplicateID was called, do not check for uniqueness
    if (g.UidAllowDuplicatesStack != 0)
    return id;

    // Visual aid: a red circle in the top-left corner of all widgets that use a duplicate of the id
    if (id == g.UidHighlightedDuplicate)
        Draw Red dot on top left corner

    // Only check for uniqueness if hovered (this avoid a O(n^2) search at each frame)
    if (id == ImGui::GetHoveredID())
    {
        if (g.UidsThisFrame.contains(id) && !g.UidWasTipDisplayed)
        {
            // Prepare highlight on the next frame for widgets that use this ID
            g.UidHighlightedDuplicate = id;
            g.UidHighlightedTimestamp = ImGui::GetFrameCount();
            if (ImGui::BeginTooltip())
            {
                if (! g.DebugItemPickerActive)
                {
                    // Display tooltip showing the duplicate Id as a string
                    ...
                    // Show hints for correction (PushID, ##)
                    ...

                    ImGui::Text("    Press Ctrl-P to break in any of those items    ");
                    if Ctrl-P
                        g.DebugItemPickerActive = true;
                }
                else
                {
                    // Add additional info at the bottom of the ItemPicker tooltip
                    ImGui::Text("Click on one of the widgets which uses a duplicated item");
                }
                ImGui::EndTooltip();
            }
            g.UidWasTipDisplayed = true;
        }
    }
    g.UidsThisFrame.push_back(id);
    return id;
}
```

imgui_widgets.cpp
-----------------

**Use ReserveUniqueID, allow duplicates when needed (TempInputText, BeginMenuEx)**
I went through all the usages of window->GetID() and changed them to ReserveUniqueID when it was possible / desirable.

### Places with one-line changes

In all the places below, the change was extremely simple: I simply replaced `window->GetID` by `window->ReserveUniqueID`.

For example, in ImGui::ButtonEx
```cpp
bool ImGui::ButtonEx(const char* label, const ImVec2& size_arg, ImGuiButtonFlags flags)
    ...
    const ImGuiID id = window->ReserveUniqueID(label);  // Instead of window->GetID
```

Below is the list of all function where this simple change was applied:
```cpp
    InvisibleButton
    ArrowButtonEx
    GetWindowScrollbarID
    ImageButton  (both versions)
    Checkbox
    RadioButton
    BeginCombo
    DragScalar
    SliderScalar
    VSliderScalar
    InputTextEx
    ColorButton
    CollapsingHeader
    Selectable
    PlotEx
    BeginTabBar
    TextLink
    TreeNode(const char *)
    TreeNodeEx(const char *)
    TreeNodeExV(const char *)
```

### Places where duplicates are needed

#### TempInputText

TempInputText create text input in place of another active widget (e.g. used when doing a CTRL+Click on drag/slider widgets).
It reuses the widget ID, so we allow it temporarily via PushAllowDuplicateID / PopAllowDuplicateID
```cpp
bool ImGui::TempInputText(const ImRect& bb, ImGuiID id, const char* label, char* buf, int buf_size, ImGuiInputTextFlags flags)
{
    ImGui::PushAllowDuplicateID();
    ...
    ImGui::PopAllowDuplicateID();
}

```

#### GetWindowScrollbarID
GetWindowScrollbarID had to be split in two: ReserveWindowScrollbarID will perform the ID reservation, while GetWindowScrollbarID is only a query.
Luckily, this is the only instance where this was required.

#### BeginMenuEx

In BeginMenuEx, we initially call GetID() instead of ReserveUniqueID() because were are not "consuming" this ID:
instead, we are only querying IsPopupOpen().
The ID will be consumed later by calling ImGui::PushID(label) + Selectable("") (which will lead to the same ID)

```cpp
bool ImGui::BeginMenuEx(const char* label, const char* icon, bool enabled)
{
    const ImGuiID id = window->GetID(label);
    bool menu_is_open = IsPopupOpen(id, ImGuiPopupFlags_None);

    ...

    PushID(label);
    pressed = Selectable("", menu_is_open, selectable_flags, ImVec2(w, label_size.y));

    ...
}
```

### Places with no changes, but worth mentioning

* TreeNode(const void *): unchanged
`TreeNode(const void* ptr_id)` and `TreeNodeExV(const void *)` are the only instances which call `window->GetID(const void *)`.
As a consequence, they cannot use the unique id mechanism, but I think their usage is rare enough that is not a big concern.

* ImGui::TabBarCalcTabID: unchanged
It still uses window->GetID, to avoid issues when merging with the docking branch.

* BeginChild, BeginPopup and co are unchanged:
  since they use ImGui::Begin()/End() internally), I suspect they might be called multiple times with the same string,
  since it is allowed with ImGui::Begin/End.

imgui_demo.cpp: some minor changes
----------------------------------

I went through the full demo Window to look for instances where a duplicate ID might be used, and corrected them.
Very few changes were required:

* **"Layout/Text Baseline Alignment"**: simple ID fix  (Added ##)
* **"Widgets/Plotting"**: simple id fix (Added ##)
* **"Columns (legacy API)/Borders"**: added ImGui::PushID for each cell
* **"Widgets/Drag and Drop/Drag to reorder items (simple)"**: no change, but a transient duplicate ID may appear during reordering
    (for *one* frame only: see comments in the code)

Demo code in order to test this PR
==================================

Add and call this function in any imgui example. This code will trigger the warnings for duplicated IDs,
and will also enable to make sure that the fixes that had to be done are OK.

```cpp
char text1[500] = "Hello,";
char text2[500] = "world!";
char text3[500] = "Hello,";
char text4[500] = "world!";
int value = 0;
bool flag1 = false, flag2 = false;
float f1 = 0.f, f2 = 0.f, f3 = 0.f, f4 = 0.f;

void Gui()
{
    // Some widgets to show the duplicated ID warnings
    ImGui::SetNextWindowSize(ImVec2(400, 500));
    if (ImGui::Begin("Duplicated ID examples", nullptr, ImGuiWindowFlags_MenuBar))
    {
        ImGui::TextWrapped("Example of widgets using duplicated IDs");
        ImGui::Separator();

        ImGui::TextWrapped("Only the first button is clickable (same ID)");
        if (ImGui::Button("button")) printf("Button1 pressed\n");
        if (ImGui::Button("button")) printf("Button2 pressed\n");

        ImGui::Separator();

        ImGui::TextWrapped("Those text inputs will share the same value (editing one will overwrite the other)");
        ImGui::InputText("text", text1, IM_ARRAYSIZE(text1));
        ImGui::InputText("text", text2, IM_ARRAYSIZE(text2));

        ImGui::Separator();

        ImGui::TextWrapped("Those radio buttons will share the same value (selecting one will overwrite the other)");
        ImGui::RadioButton("radio", &value, 0);
        ImGui::RadioButton("radio", &value, 1);

        ImGui::Separator();

        ImGui::TextWrapped("The second checkbox cannot be toggled");
        ImGui::Checkbox("checkbox", &flag1);
        ImGui::Checkbox("checkbox", &flag2);

        ImGui::Separator();

        ImGui::TextWrapped("Those two sliders will share the same value (editing one will overwrite the other");
        ImGui::SliderFloat("slider", &f1, 0.f, 1.f);
        ImGui::SliderFloat("slider", &f2, 0.f, 1.f);

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

    // Some widgets to test the fixes that had to be done
    ImGui::SetNextWindowSize(ImVec2(400, 400));
    if (ImGui::Begin("Regression tests", nullptr, ImGuiWindowFlags_MenuBar))
    {
        ImGui::TextWrapped("Small issues had to be fixed inside DragFloat (cf TempInputText), InputTextMultiline and BeginMenuBar.\n"
            "This window is here to test those fixes.");

        ImGui::Separator();

        if (ImGui::BeginMenuBar())
        {
            if (ImGui::BeginMenu("My Menu"))
            {
                ImGui::MenuItem("Item 1");
                ImGui::EndMenu();
            }
            ImGui::EndMenuBar();
        }

        ImGui::TextWrapped("DragFloat will reuse the Item ID when double clicking. This is allowed via Push/PopAllowDuplicateID");
        ImGui::DragFloat("dragf", &f3);

        ImGui::Separator();

        ImGui::TextWrapped("An issue was fixed with the InputTextMultiline vertical scrollbar, which trigerred a duplicate ID warning, which was fixed");
        ImGui::InputTextMultiline("multiline", text1, 500);

    }
    ImGui::End();

    // A window that is populated in several steps should still detect duplicates
    {
        ImGui::SetNextWindowSize(ImVec2(400, 200));

        ImGui::Begin("Window populated in several steps");
        ImGui::TextWrapped("This window is populated between several ImGui::Begin/ImGui::End calls (with the same window name).\n"
                           "Duplicates should be detected anyhow.");
        ImGui::Text("A first InputText");
        ImGui::InputText("TTT", text3, 500);
        ImGui::End();

        ImGui::Begin("Window populated in several steps");
        ImGui::Separator();
        ImGui::Text("The second InputText below reuses the same ID, \nin a different ImGui::Begin/ImGui::End context \n(but with the same window name)");
        ImGui::InputText("TTT", text4, 500);
        ImGui::End();
    }
}
```
@ocornut ocornut added the label/id and id stack implicit identifiers, pushid(), id stack label Sep 9, 2024
@ocornut
Copy link
Owner

ocornut commented Sep 9, 2024

I am sorry but this seems too complicated (and not free in term of CPU cost). It seems like either you misunderstood my comment (#7669 (comment)) either my approach has a flaw I didn't envision yet.

I will instead implement a proof of concept for my approach, it seems easier than discussing this.

@ocornut
Copy link
Owner

ocornut commented Sep 9, 2024

This is not a full polished feature but to convey the idea:

It's a lazier check and doesn't cover every situation but it's much simpler.

I think we can come up with a wrap-around version that work regardless of order and doesn't false detect a single moving item.

@ocornut
Copy link
Owner

ocornut commented Sep 9, 2024

Ignore previous patch I have found a better solution that works for more cases:

It adds two extra int32 read from struct + compare in the hot path, and those new fields are stored nearby to maximize cache line reuse.

@ocornut
Copy link
Owner

ocornut commented Sep 9, 2024

Since you have investigated this topic in detail, I'd appreciate your feedback on this alternative idea, before we can look into polishing it (better tooltip, opt-out config flag).

I wonder if we should instead maybe show a tooltip-like but interactive window in the corner, with a link to the FAQ (now that we have links) and a button to disable all.

The uttermost reason what I have not implemented this before is the following:
I don't want the Dear ImGui product to add an extra layer of "here's a notification that 100+ end-users will close fifty times a day without looking because there's an error in the code/data". Therefore I think we should probably first introduce this as an opt-in feature, to get larger waves of feedback from early adopter and better understand how we can avoid this scenario.

@ocornut
Copy link
Owner

ocornut commented Sep 9, 2024

Moving this to a branch master...features/debug_conflicting_id with further improvements.

ocornut pushed a commit that referenced this pull request Sep 9, 2024
@pthom
Copy link
Contributor Author

pthom commented Sep 10, 2024

Many thanks for taking the time to review this and for providing a better alternative!

I really do think this will be a net positive for ImGui users, especially for new users.

You are right that your version is less costly. I had not thought of using ItemHoverable() instead of changing GetID(),
and I see that it avoids the need to maintain a list of IDs per frame.

I checked out your branch, and thoroughly checked the entire ImGui demo, as well as all tools windows (Metrics, ID Stack tool, etc.) in search for potential warnings, and it seems that all warnings were successfully removed.The regressions I had identified withing TempInputText() / Menu / TextInputMultiLine are also OK.

Items with duplicate ID created inside distinct calls to `Begin("SameName") / End()" are also correctly detected

Some potential typos in your branch:

imgui.h, line 2272:

    // - Code should use ##/### syntax or PushID()/PopID() to make  //  ==> "to make IDs unique"?

imgui.cpp / EndFrame()
Is the "+" after "%d" intentional?

Text("%d+ items with conflicting ID!", g.HoveredIdPreviousFrameItemCount);

Now, to address your concerns:

I wonder if we should instead maybe show a tooltip-like but interactive window in the corner, with a link to the FAQ (now that we have links) and a button to disable all.

I had tried it. Based on my tests, opening a new window when a widget is hovered, and opening it far away from the mouse cursor it easy to visually miss: a user may see this window long after hovering the widget, and wonder why it is here.

The uttermost reason what I have not implemented this before is the following:
I don't want the Dear ImGui product to add an extra layer of "here's a notification that 100+ end-users will close fifty times a day without looking because there's an error in the code/data". Therefore, I think we should probably first introduce this as an opt-in feature, to get larger waves of feedback from early adopter and better understand how we can avoid this scenario.

IMHO, this a why a tooltip is perhaps less annoying than an interactive window. Perhaps, adding more red on "This is a programmer error" could convince more developers to correct it before shipping. Something like:

image

(I just lerped more red into the text color)

@ocornut
Copy link
Owner

ocornut commented Sep 10, 2024

Is the "+" after "%d" intentional?

It was intentional because only unclipped items are tested, which makes the cost near invisible and for what it does pretty much solves enough of the problem anyway. I changed it to "%d visible items ..." it seem adequate and less misleading.

I had tried it. Based on my tests, opening a new window when a widget is hovered, and opening it far away from the mouse cursor it easy to visually miss: a user may see this window long after hovering the widget, and wonder why it is here.

Right.

I ended up using a shortcut to link to the FAQ. It seems like a good workaround for this.

I also added this feature directly in the Tools menu (I've been thinking adding a link to your manual there too).

Pushed some tweaks.
For me the remaining issue to the Drag and Drop demo that you mentioned, then I think I can push it.

@ocornut
Copy link
Owner

ocornut commented Sep 10, 2024

I will eagerly push it now AND enable it by default as a way to enforce feedback :)

I only had to run the capture_faq_idstack_gif script in test engine to regenerate that gif (now in the FAQ alongside code)
capture_faq_idstack_gif_0000

@ocornut
Copy link
Owner

ocornut commented Sep 10, 2024

Merged as 67cd4ea. Tagging 60+ past issues :)

@pthom
Copy link
Contributor Author

pthom commented Sep 10, 2024

Merged as 67cd4ea. Tagging 60+ past issues :)

Good news!

I will eagerly push it now AND enable it by default as a way to enforce feedback :)

Actually I had done the same with my previous hackish implementation in Dear ImGui Bundle. An old time user (quite familiar with ImGui indeed), was greeted with the warning and actually thanked me. I'll replace it with yours.

I've been thinking adding a link to your manual there too

I would be pleased. This made me think I should update it. It is now based on ImGui v1.91.1

@pthom
Copy link
Contributor Author

pthom commented Sep 10, 2024

PS: as far as the last demo code is concerned a possible fix could be:

            bool reordering = false;   // added this
            for (int n = 0; n < IM_ARRAYSIZE(item_names); n++)
            {
                ImGui::PushItemFlag(ImGuiItemFlags_AllowDuplicateId, reordering); // added this
                const char* item = item_names[n];
                ImGui::Selectable(item);

                if (ImGui::IsItemActive() && !ImGui::IsItemHovered())
                {
                    int n_next = n + (ImGui::GetMouseDragDelta(0).y < 0.f ? -1 : 1);
                    if (n_next >= 0 && n_next < IM_ARRAYSIZE(item_names))
                    {
                        item_names[n] = item_names[n_next];
                        item_names[n_next] = item;
                        ImGui::ResetMouseDragDelta();
                        reordering = true;  // added this
                    }
                }
                ImGui::PopItemFlag(); // added this
            }

However, since this is intended to be a "simple" version for Drag a Drop, this addition does defeat a little the purpose of being simple. Since the warning is barely visible for only one frame, I'd say it is OK to keep it unchanged, waiting for a future better alternative.

@ocornut
Copy link
Owner

ocornut commented Sep 10, 2024

Thank you for updating manual!
Started some work on refactoring InputText so hopefully by 2025 we can add syntax coloring and embed this in default lib.

PS: as far as the last demo code is concerned a possible fix could be:

I have added a similar code in my commit:

if (ImGui::TreeNode("Drag to reorder items (simple)"))
{
    // FIXME: there is temporary (usually single-frame) ID Conflict during reordering as a same item may be submitting twice.
    // This code was always slightly faulty but in a way which was not easily noticeable.
    // Until we fix this, enable ImGuiItemFlags_AllowDuplicateId to disable detecting the issue.
    ImGui::PushItemFlag(ImGuiItemFlags_AllowDuplicateId, true);

Added section in Wiki:
https://github.com/ocornut/imgui/wiki/Debug-Tools#highlight-id-conflicts

ocornut added a commit that referenced this pull request Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
label/id and id stack implicit identifiers, pushid(), id stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants