From 67be041dc0971ea9776ea22305d934fb31551eca Mon Sep 17 00:00:00 2001 From: Pascal Thomet Date: Fri, 6 Sep 2024 11:30:08 +0200 Subject: [PATCH] Help users if they are reusing the same ID (Fix #7669) (fix for https://github.com/ocornut/imgui/issues/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 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(); } } ``` --- imgui.cpp | 110 ++++++++++++++++++++++++++++++++++++++++++++++ imgui_demo.cpp | 15 +++++-- imgui_internal.h | 26 +++++++++++ imgui_widgets.cpp | 63 ++++++++++++++++---------- 4 files changed, 188 insertions(+), 26 deletions(-) diff --git a/imgui.cpp b/imgui.cpp index 1d9d8b899efe..1f9cd6538558 100644 --- a/imgui.cpp +++ b/imgui.cpp @@ -4904,6 +4904,10 @@ void ImGui::NewFrame() g.HoverItemDelayTimer = g.HoverItemDelayClearTimer = 0.0f; // May want a decaying timer, in which case need to clamp at max first, based on max of caller last requested timer. } + // TRKID: inside ImGui::NewFrame, empty the list of items, but keep its allocated data. Also reset WasDuplicateTipDisplayedAlready + g.UidsThisFrame.resize(0); // Empty the list of items, but keep its allocated data + g.UidWasTipDisplayed = false; + // Drag and drop g.DragDropAcceptIdPrev = g.DragDropAcceptIdCurr; g.DragDropAcceptIdCurr = 0; @@ -5235,6 +5239,14 @@ void ImGui::EndFrame() return; IM_ASSERT(g.WithinFrameScope && "Forgot to call ImGui::NewFrame()?"); + // TRKID: inside ImGui::EndFrame(), ensure matched calls to Push/PopAllowDuplicateID, reset UidHighlightedDuplicate + IM_ASSERT(g.UidAllowDuplicatesStack == 0 && "Mismatched Push/PopAllowDuplicateID"); + if (g.UidHighlightedTimestamp != ImGui::GetFrameCount()) + { + g.UidHighlightedTimestamp = 0; + g.UidHighlightedDuplicate = 0; + } + CallContextHooks(&g, ImGuiContextHookType_EndFramePre); ErrorCheckEndFrameSanityChecks(); @@ -8485,8 +8497,106 @@ ImGuiID ImGui::GetID(int int_id) ImGuiWindow* window = GImGui->CurrentWindow; return window->GetID(int_id); } + IM_MSVC_RUNTIME_CHECKS_RESTORE +// TRKID: ReserveUniqueID impl: returns GetID(), but may display a warning. Should be called only once per frame with a given ID +// Developer notes (to be removed before merging) +// - The prefix "Reserve" is an attempt to make it clear that this method should be called only once per frame for each str id. +// It is already the case in the existing codebase of Dear ImGui. However, I opted to make it clear in the name of the function. +// - Some calls to GetID() were changed to ReserveUniqueID() inside imgui_widgets.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) + { + ImGui::GetWindowDrawList()->AddCircleFilled(ImGui::GetCursorScreenPos(), 3.f, 0xFF0000FF); + } + + // TRKID: Debug duplicates during development + // Only enable this temporarily when looking for duplicate ID throughout the code. Never enable in production, as it result in a O(n^2) search! +#ifdef IMGUI_DEBUG_PARANOID // can be defined inside imconfig.h + if (g.UidsThisFrame.contains(id)) + { + IMGUI_DEBUG_LOG("Detected Duplicated ID : 0x%08X (\"%s\")\n", id, str_id); + IM_ASSERT_PARANOID(false && "Duplicated ID detected"); + } +#endif + + // 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) + { + ImGui::Text("Duplicate ID detected: \"%s\"!", str_id); + ImGui::Text("Widgets with a red dot ("); + ImGui::SameLine(); + ImVec2 dot_position = ImGui::GetCursorScreenPos(); + dot_position.y += ImGui::GetFontSize() / 2.f; + dot_position.x -= ImGui::GetFontSize() / 15.f; + ImGui::GetWindowDrawList()->AddCircleFilled(dot_position, 3.f, 0xFF0000FF); + ImGui::Text(" )in their top-left corner use this same ID"); + ImGui::Separator(); + ImGui::BulletText("Either use \"##\" to pass a complement to the ID that \nwon't be visible to the end-user, for example:"); + ImGui::Text(" if (ImGui::Button(\"Save##foo1\")"); + ImGui::Text(" // Handle click action"); + ImGui::BulletText("Or use ImGui::PushID() / PopID() to change the ID stack in a given \ncontext, for example:"); + ImGui::Text(" ImGui::PushID(\"MyContext\")"); + ImGui::Text(" if (ImGui::Button(\"Save\")"); + ImGui::Text(" // Handle click action"); + ImGui::Text(" ImGui::PopID();"); + ImGui::Separator(); + + // Launch ItemPicker if Ctrl-P is pressed + ImGui::Text(" Press Ctrl-P to break in any of those items "); + ImGui::GetWindowDrawList()->AddRectFilled(ImGui::GetItemRectMin(), ImGui::GetItemRectMax(), IM_COL32(255, 255, 0, 32)); + if (ImGui::IsKeyPressed(ImGuiKey_P) && (ImGui::IsKeyDown(ImGuiKey_LeftCtrl) || ImGui::IsKeyDown(ImGuiKey_RightCtrl))) + g.DebugItemPickerActive = true; + } + else + { + // Add additional info at the bottom of the ItemPicker tooltip + ImGui::Separator(); + 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; +} + +// TRKID: Implement PushAllowDuplicateID / PopAllowDuplicateID +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--; +} + + //----------------------------------------------------------------------------- // [SECTION] INPUTS //----------------------------------------------------------------------------- diff --git a/imgui_demo.cpp b/imgui_demo.cpp index aabb220d6ab6..41630ec2b05d 100644 --- a/imgui_demo.cpp +++ b/imgui_demo.cpp @@ -2000,8 +2000,9 @@ static void ShowDemoWindowWidgets(ImGuiDemoWindowData* demo_data) ImGui::SameLine(); ImGui::SliderInt("Sample count", &display_count, 1, 400); float (*func)(void*, int) = (func_type == 0) ? Funcs::Sin : Funcs::Saw; - ImGui::PlotLines("Lines", func, NULL, display_count, 0, NULL, -1.0f, 1.0f, ImVec2(0, 80)); - ImGui::PlotHistogram("Histogram", func, NULL, display_count, 0, NULL, -1.0f, 1.0f, ImVec2(0, 80)); + // TRKID: imgui_demo.cpp / Widgets/Plotting: simple id fix + ImGui::PlotLines("Lines##2", func, NULL, display_count, 0, NULL, -1.0f, 1.0f, ImVec2(0, 80)); + ImGui::PlotHistogram("Histogram##2", func, NULL, display_count, 0, NULL, -1.0f, 1.0f, ImVec2(0, 80)); ImGui::Separator(); ImGui::Text("Need better plotting and graphing? Consider using ImPlot:"); @@ -2596,6 +2597,10 @@ static void ShowDemoWindowWidgets(ImGuiDemoWindowData* demo_data) IMGUI_DEMO_MARKER("Widgets/Drag and Drop/Drag to reorder items (simple)"); if (ImGui::TreeNode("Drag to reorder items (simple)")) { + // TRKID: Widgets/Drag and Drop/Drag to reorder items (simple) this demo will trigger a duplicated id during *one* frame (and *one* frame only) + // This is because it reorders items mid-flight, and may redisplay the same item in the frame where reordering happens. + // This is not a big issue : a red dot may flash for *one* frame (an assert may trigger if IMGUI_DEBUG_PARANOID is defined) + // Simple reordering HelpMarker( "We don't use the drag and drop api at all here! " @@ -4225,7 +4230,8 @@ static void ShowDemoWindowLayout() // down by FramePadding.y ahead of time) ImGui::AlignTextToFramePadding(); ImGui::Text("OK Blahblah"); ImGui::SameLine(); - ImGui::Button("Some framed item"); ImGui::SameLine(); + // TRKID: imgui_demo.cpp, "Layout/Text Baseline Alignment": simple ID fix + ImGui::Button("Some framed item##2"); ImGui::SameLine(); HelpMarker("We call AlignTextToFramePadding() to vertically align the text baseline by +FramePadding.y"); // SmallButton() uses the same vertical padding as Text @@ -7146,6 +7152,8 @@ static void ShowDemoWindowColumns() ImGui::Columns(columns_count, NULL, v_borders); for (int i = 0; i < columns_count * lines_count; i++) { + // TRKID: imgui_demo.cpp / Columns (legacy API)/Borders: added ImGui::PushID for each cell + ImGui::PushID(i); if (h_borders && ImGui::GetColumnIndex() == 0) ImGui::Separator(); ImGui::Text("%c%c%c", 'a' + i, 'a' + i, 'a' + i); @@ -7155,6 +7163,7 @@ static void ShowDemoWindowColumns() ImGui::Text("Long text that is likely to clip"); ImGui::Button("Button", ImVec2(-FLT_MIN, 0.0f)); ImGui::NextColumn(); + ImGui::PopID(); } ImGui::Columns(1); if (h_borders) diff --git a/imgui_internal.h b/imgui_internal.h index 5d23ebfef4fe..2251326e707e 100644 --- a/imgui_internal.h +++ b/imgui_internal.h @@ -2079,6 +2079,18 @@ struct ImGuiContext ImGuiID LastActiveId; // Store the last non-zero ActiveId, useful for animation. float LastActiveIdTimer; // Store the last non-zero ActiveId timer since the beginning of activation, useful for animation. + // TRKID: struct ImGuiContext: added fields UidIXXX (UidsThisFrame & co) + // Technical note (to remove upon merging): + // I considered adding these fields to ImGuiWindowTempData, but had to revert: + // since it is legal to call several times ImGui::Begin("MyWindow") within the same frame, + // clearing the list of ItemID inside ImGui::End() would not be sufficient, as identical + // IDs may appear within different calls to ImGui::Begin("MyWindow")/ImGui::End() and not be detected. + ImVector 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 + // Key/Input Ownership + Shortcut Routing system // - The idea is that instead of "eating" a given key, we can link to an owner. // - Input query can then read input by specifying ImGuiKeyOwner_Any (== 0), ImGuiKeyOwner_NoOwner (== -1) or a custom ID. @@ -2400,6 +2412,13 @@ struct ImGuiContext LastActiveId = 0; LastActiveIdTimer = 0.0f; + // TRKID: initialize ImGuiContext UidXXX fields + UidsThisFrame.clear(); + UidAllowDuplicatesStack = 0; + UidHighlightedDuplicate = 0; + UidHighlightedTimestamp = 0; + UidWasTipDisplayed = false; + LastKeyboardKeyPressTime = LastKeyModsChangeTime = LastKeyModsChangeFromNoneTime = -1.0; ActiveIdUsingNavDirMask = 0x00; @@ -2718,6 +2737,8 @@ struct IMGUI_API ImGuiWindow ImGuiID GetID(int n); ImGuiID GetIDFromPos(const ImVec2& p_abs); ImGuiID GetIDFromRectangle(const ImRect& r_abs); + // TRKID: ReserveUniqueID declaration: returns GetID(), but may display a warning. Should be called only once per frame with a given ID + ImGuiID ReserveUniqueID(const char* str_id); // 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. // We don't use g.FontSize because the window may be != g.CurrentWindow. ImRect Rect() const { return ImRect(Pos.x, Pos.y, Pos.x + Size.x, Pos.y + Size.y); } @@ -3219,6 +3240,10 @@ namespace ImGui IMGUI_API ImGuiID GetIDWithSeed(const char* str_id_begin, const char* str_id_end, ImGuiID seed); IMGUI_API ImGuiID GetIDWithSeed(int n, ImGuiID seed); + // TRKID: PushAllowDuplicateID() / PopAllowDuplicateID() declared inside imgui_internal + 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. + // Basic Helpers for widget code IMGUI_API void ItemSize(const ImVec2& size, float text_baseline_y = -1.0f); inline void ItemSize(const ImRect& bb, float text_baseline_y = -1.0f) { ItemSize(bb.GetSize(), text_baseline_y); } // FIXME: This is a misleading API since we expect CursorPos to be bb.Min. @@ -3548,6 +3573,7 @@ namespace ImGui IMGUI_API void Scrollbar(ImGuiAxis axis); IMGUI_API bool ScrollbarEx(const ImRect& bb, ImGuiID id, ImGuiAxis axis, ImS64* p_scroll_v, ImS64 avail_v, ImS64 contents_v, ImDrawFlags flags); IMGUI_API ImRect GetWindowScrollbarRect(ImGuiWindow* window, ImGuiAxis axis); + IMGUI_API ImGuiID ReserveWindowScrollbarID(ImGuiWindow* window, ImGuiAxis axis); IMGUI_API ImGuiID GetWindowScrollbarID(ImGuiWindow* window, ImGuiAxis axis); IMGUI_API ImGuiID GetWindowResizeCornerID(ImGuiWindow* window, int n); // 0..3: corners IMGUI_API ImGuiID GetWindowResizeBorderID(ImGuiWindow* window, ImGuiDir dir); diff --git a/imgui_widgets.cpp b/imgui_widgets.cpp index 7b86fb8bef9a..4339029a241a 100644 --- a/imgui_widgets.cpp +++ b/imgui_widgets.cpp @@ -717,7 +717,7 @@ bool ImGui::ButtonEx(const char* label, const ImVec2& size_arg, ImGuiButtonFlags ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const ImVec2 label_size = CalcTextSize(label, NULL, true); ImVec2 pos = window->DC.CursorPos; @@ -778,7 +778,7 @@ bool ImGui::InvisibleButton(const char* str_id, const ImVec2& size_arg, ImGuiBut // Cannot use zero-size for InvisibleButton(). Unlike Button() there is not way to fallback using the label size. IM_ASSERT(size_arg.x != 0.0f && size_arg.y != 0.0f); - const ImGuiID id = window->GetID(str_id); + const ImGuiID id = window->ReserveUniqueID(str_id); ImVec2 size = CalcItemSize(size_arg, 0.0f, 0.0f); const ImRect bb(window->DC.CursorPos, window->DC.CursorPos + size); ItemSize(size); @@ -799,7 +799,7 @@ bool ImGui::ArrowButtonEx(const char* str_id, ImGuiDir dir, ImVec2 size, ImGuiBu if (window->SkipItems) return false; - const ImGuiID id = window->GetID(str_id); + const ImGuiID id = window->ReserveUniqueID(str_id); const ImRect bb(window->DC.CursorPos, window->DC.CursorPos + size); const float default_size = GetFrameHeight(); ItemSize(size, (size.y >= default_size) ? g.Style.FramePadding.y : -1.0f); @@ -890,6 +890,12 @@ bool ImGui::CollapseButton(ImGuiID id, const ImVec2& pos) return pressed; } +// TRKID: ReserveWindowScrollbarID will perform the ID reservation, while GetWindowScrollbarID is only a query +ImGuiID ImGui::ReserveWindowScrollbarID(ImGuiWindow* window, ImGuiAxis axis) +{ + return window->ReserveUniqueID(axis == ImGuiAxis_X ? "#SCROLLX" : "#SCROLLY"); +} + ImGuiID ImGui::GetWindowScrollbarID(ImGuiWindow* window, ImGuiAxis axis) { return window->GetID(axis == ImGuiAxis_X ? "#SCROLLX" : "#SCROLLY"); @@ -913,7 +919,7 @@ void ImGui::Scrollbar(ImGuiAxis axis) { ImGuiContext& g = *GImGui; ImGuiWindow* window = g.CurrentWindow; - const ImGuiID id = GetWindowScrollbarID(window, axis); + const ImGuiID id = ReserveWindowScrollbarID(window, axis); // Calculate scrollbar bounding box ImRect bb = GetWindowScrollbarRect(window, axis); @@ -1103,7 +1109,7 @@ bool ImGui::ImageButton(const char* str_id, ImTextureID user_texture_id, const I if (window->SkipItems) return false; - return ImageButtonEx(window->GetID(str_id), user_texture_id, image_size, uv0, uv1, bg_col, tint_col); + return ImageButtonEx(window->ReserveUniqueID(str_id), user_texture_id, image_size, uv0, uv1, bg_col, tint_col); } #ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS @@ -1117,6 +1123,7 @@ bool ImGui::ImageButton(ImTextureID user_texture_id, const ImVec2& size, const I { // Default to using texture ID as ID. User can still push string/integer prefixes. PushID((void*)(intptr_t)user_texture_id); + const ImGuiID id = window->GetID("#image"); if (frame_padding >= 0) PushStyleVar(ImGuiStyleVar_FramePadding, ImVec2((float)frame_padding, (float)frame_padding)); bool ret = ImageButton("", user_texture_id, size, uv0, uv1, bg_col, tint_col); @@ -1136,7 +1143,7 @@ bool ImGui::Checkbox(const char* label, bool* v) ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const ImVec2 label_size = CalcTextSize(label, NULL, true); const float square_sz = GetFrameHeight(); @@ -1258,7 +1265,7 @@ bool ImGui::RadioButton(const char* label, bool active) ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const ImVec2 label_size = CalcTextSize(label, NULL, true); const float square_sz = GetFrameHeight(); @@ -1407,7 +1414,7 @@ bool ImGui::TextLink(const char* label) return false; ImGuiContext& g = *GImGui; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const char* label_end = FindRenderedTextEnd(label); ImVec2 pos = window->DC.CursorPos; @@ -1821,7 +1828,7 @@ bool ImGui::BeginCombo(const char* label, const char* preview_value, ImGuiComboF return false; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); IM_ASSERT((flags & (ImGuiComboFlags_NoArrowButton | ImGuiComboFlags_NoPreview)) != (ImGuiComboFlags_NoArrowButton | ImGuiComboFlags_NoPreview)); // Can't use both flags together if (flags & ImGuiComboFlags_WidthFitPreview) IM_ASSERT((flags & (ImGuiComboFlags_NoPreview | (ImGuiComboFlags)ImGuiComboFlags_CustomPreview)) == 0); @@ -2581,7 +2588,7 @@ bool ImGui::DragScalar(const char* label, ImGuiDataType data_type, void* p_data, ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const float w = CalcItemWidth(); const ImVec2 label_size = CalcTextSize(label, NULL, true); @@ -3173,7 +3180,7 @@ bool ImGui::SliderScalar(const char* label, ImGuiDataType data_type, void* p_dat ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const float w = CalcItemWidth(); const ImVec2 label_size = CalcTextSize(label, NULL, true); @@ -3341,7 +3348,7 @@ bool ImGui::VSliderScalar(const char* label, const ImVec2& size, ImGuiDataType d ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const ImVec2 label_size = CalcTextSize(label, NULL, true); const ImRect frame_bb(window->DC.CursorPos, window->DC.CursorPos + size); @@ -3556,6 +3563,8 @@ bool ImGui::TempInputText(const ImRect& bb, ImGuiID id, const char* label, char* // On the first frame, g.TempInputTextId == 0, then on subsequent frames it becomes == id. // We clear ActiveID on the first frame to allow the InputText() taking it back. ImGuiContext& g = *GImGui; + // TRKID: TempInputText reuses the widget ID, so we allow it temporarily (Push) + ImGui::PushAllowDuplicateID(); const bool init = (g.TempInputId != id); if (init) ClearActiveID(); @@ -3568,6 +3577,8 @@ bool ImGui::TempInputText(const ImRect& bb, ImGuiID id, const char* label, char* IM_ASSERT(g.ActiveId == id); g.TempInputId = g.ActiveId; } + // TRKID: TempInputText reuses the widget ID, so we allow it temporarily (Pop) + ImGui::PopAllowDuplicateID(); return value_changed; } @@ -4284,7 +4295,7 @@ bool ImGui::InputTextEx(const char* label, const char* hint, char* buf, int buf_ if (is_multiline) // Open group before calling GetID() because groups tracks id created within their scope (including the scrollbar) BeginGroup(); - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const ImVec2 label_size = CalcTextSize(label, NULL, true); const ImVec2 frame_size = CalcItemSize(size_arg, CalcItemWidth(), (is_multiline ? g.FontSize * 8.0f : label_size.y) + style.FramePadding.y * 2.0f); // Arbitrary default of 8 lines high for multi-line const ImVec2 total_size = ImVec2(frame_size.x + (label_size.x > 0.0f ? style.ItemInnerSpacing.x + label_size.x : 0.0f), frame_size.y); @@ -5978,7 +5989,7 @@ bool ImGui::ColorButton(const char* desc_id, const ImVec4& col, ImGuiColorEditFl return false; ImGuiContext& g = *GImGui; - const ImGuiID id = window->GetID(desc_id); + const ImGuiID id = window->ReserveUniqueID(desc_id); const float default_size = GetFrameHeight(); const ImVec2 size(size_arg.x == 0.0f ? default_size : size_arg.x, size_arg.y == 0.0f ? default_size : size_arg.y); const ImRect bb(window->DC.CursorPos, window->DC.CursorPos + size); @@ -6237,8 +6248,8 @@ bool ImGui::TreeNode(const char* label) ImGuiWindow* window = GetCurrentWindow(); if (window->SkipItems) return false; - ImGuiID id = window->GetID(label); - return TreeNodeBehavior(id, ImGuiTreeNodeFlags_None, label, NULL); + ImGuiID id = window->ReserveUniqueID(label); + return TreeNodeBehavior(id, 0, label, NULL); } bool ImGui::TreeNodeV(const char* str_id, const char* fmt, va_list args) @@ -6256,7 +6267,7 @@ bool ImGui::TreeNodeEx(const char* label, ImGuiTreeNodeFlags flags) ImGuiWindow* window = GetCurrentWindow(); if (window->SkipItems) return false; - ImGuiID id = window->GetID(label); + ImGuiID id = window->ReserveUniqueID(label); return TreeNodeBehavior(id, flags, label, NULL); } @@ -6284,7 +6295,7 @@ bool ImGui::TreeNodeExV(const char* str_id, ImGuiTreeNodeFlags flags, const char if (window->SkipItems) return false; - ImGuiID id = window->GetID(str_id); + ImGuiID id = window->ReserveUniqueID(str_id); const char* label, *label_end; ImFormatStringToTempBufferV(&label, &label_end, fmt, args); return TreeNodeBehavior(id, flags, label, label_end); @@ -6299,6 +6310,9 @@ bool ImGui::TreeNodeExV(const void* ptr_id, ImGuiTreeNodeFlags flags, const char ImGuiID id = window->GetID(ptr_id); const char* label, *label_end; ImFormatStringToTempBufferV(&label, &label_end, fmt, args); + + // TRKID: ImGui::TreeNodeExV uses GetID(void *) => this is the only place where I could not ensure uniqueness + // (all other calls use GetID(const char *) return TreeNodeBehavior(id, flags, label, label_end); } @@ -6726,7 +6740,7 @@ bool ImGui::CollapsingHeader(const char* label, ImGuiTreeNodeFlags flags) ImGuiWindow* window = GetCurrentWindow(); if (window->SkipItems) return false; - ImGuiID id = window->GetID(label); + ImGuiID id = window->ReserveUniqueID(label); return TreeNodeBehavior(id, flags | ImGuiTreeNodeFlags_CollapsingHeader, label); } @@ -6743,7 +6757,7 @@ bool ImGui::CollapsingHeader(const char* label, bool* p_visible, ImGuiTreeNodeFl if (p_visible && !*p_visible) return false; - ImGuiID id = window->GetID(label); + ImGuiID id = window->ReserveUniqueID(label); flags |= ImGuiTreeNodeFlags_CollapsingHeader; if (p_visible) flags |= ImGuiTreeNodeFlags_AllowOverlap | (ImGuiTreeNodeFlags)ImGuiTreeNodeFlags_ClipLabelForTrailingButton; @@ -6787,7 +6801,7 @@ bool ImGui::Selectable(const char* label, bool selected, ImGuiSelectableFlags fl const ImGuiStyle& style = g.Style; // Submit label or explicit size to ItemSize(), whereas ItemAdd() will submit a larger/spanning rectangle. - ImGuiID id = window->GetID(label); + ImGuiID id = window->ReserveUniqueID(label); ImVec2 label_size = CalcTextSize(label, NULL, true); ImVec2 size(size_arg.x != 0.0f ? size_arg.x : label_size.x, size_arg.y != 0.0f ? size_arg.y : label_size.y); ImVec2 pos = window->DC.CursorPos; @@ -8224,7 +8238,7 @@ int ImGui::PlotEx(ImGuiPlotType plot_type, const char* label, float (*values_get return -1; const ImGuiStyle& style = g.Style; - const ImGuiID id = window->GetID(label); + const ImGuiID id = window->ReserveUniqueID(label); const ImVec2 label_size = CalcTextSize(label, NULL, true); const ImVec2 frame_size = CalcItemSize(size_arg, CalcItemWidth(), label_size.y + style.FramePadding.y * 2.0f); @@ -8647,6 +8661,9 @@ bool ImGui::BeginMenuEx(const char* label, const char* icon, bool enabled) ImGuiContext& g = *GImGui; const ImGuiStyle& style = g.Style; + + // TRKID: In BeginMenuEx, We 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) const ImGuiID id = window->GetID(label); bool menu_is_open = IsPopupOpen(id, ImGuiPopupFlags_None); @@ -9060,7 +9077,7 @@ bool ImGui::BeginTabBar(const char* str_id, ImGuiTabBarFlags flags) if (window->SkipItems) return false; - ImGuiID id = window->GetID(str_id); + ImGuiID id = window->ReserveUniqueID(str_id); ImGuiTabBar* tab_bar = g.TabBars.GetOrAddByKey(id); ImRect tab_bar_bb = ImRect(window->DC.CursorPos.x, window->DC.CursorPos.y, window->WorkRect.Max.x, window->DC.CursorPos.y + g.FontSize + g.Style.FramePadding.y * 2); tab_bar->ID = id;