Skip to content

Stop using obsolete ImGui functions and enums - #2412

Open
totalgee wants to merge 2 commits into
cinder:masterfrom
totalgee:imgui_ODR_2411
Open

Stop using obsolete ImGui functions and enums#2412
totalgee wants to merge 2 commits into
cinder:masterfrom
totalgee:imgui_ODR_2411

Conversation

@totalgee

Copy link
Copy Markdown
Contributor

Summary:

  • enable IMGUI_DISABLE_OBSOLETE_FUNCTIONS in imconfig.h and port the deprecated uses: tab/nav colour enums, the NavNoCaptureKeyboard flag, clipboard callbacks, Image() to ImageWithBg(), and DX12 legacy SRV descriptors.
  • make an inline version of Initialize() so IMGUI_CHECKVERSION() runs in the caller's translation unit, where it can catch an app/library config mismatch instead of only comparing the Cinder lib version to itself.
  • to resolve issue Defining ImGui config macros in app code silently causes an ODR violation #2411.

Breaking change -- needs a maintainer decision

Enabling IMGUI_DISABLE_OBSOLETE_FUNCTIONS affects downstream apps, not just Cinder. The most likely thing people hit is renamed colour enums, since many people might customize style.Colors[]:

Deprecated Current Since
ImGuiCol_TabActive ImGuiCol_TabSelected 1.90.9
ImGuiCol_TabUnfocused ImGuiCol_TabDimmed 1.90.9
ImGuiCol_TabUnfocusedActive ImGuiCol_TabDimmedSelected 1.90.9
ImGuiCol_NavHighlight ImGuiCol_NavCursor 1.91.4
ImGuiChildFlags_Border ImGuiChildFlags_Borders 1.91.1
ImGuiTreeNodeFlags_AllowItemOverlap ...AllowOverlap 1.89.7

plus a handful of functions (SetWindowFontScale(), GetContentRegionMax(), SetItemAllowOverlap(), the old Combo()/ListBox() callback form) and the io.FontGlobalScale / io.*ClipboardTextFn members.

The blast radius is bounded: everything obsoleted before ~1.89 is already fully commented out in 1.92, so it is unavailable with or without this macro change! Only the dozen or so items still live in the obsolete section are affected here.

If that's too disruptive, the macro is a single line in imconfig.h and I'm happy to comment it out -- the internal migration stands on its own, since these APIs will be deleted upstream eventually.

Initialize() / InitializeImpl() split

IMGUI_CHECKVERSION() only detects a config mismatch when it is compiled in the caller's translation unit. The existing call in CinderImGui.cpp compares Cinder against itself and can never fail, which is why #2411 went undiagnosed. Making Initialize() an inline header wrapper is the smallest way to get the check instantiated in application code with no change to how apps call it.

Worth flagging: this moves the exported symbol from Initialize to InitializeImpl, so it is an ABI change for anyone linking a prebuilt cinder.lib. Rebuilding resolves it. If you'd prefer a different shape (a separate ImGui::CheckVersion() helper apps call themselves, or a differently named impl function), I could adjust it.

One migration that is not a simple rename

Image()ImageWithBg() is the only case where the compiler won't catch you: the old signature took (tint_col, border_col), the new one takes (bg_col, tint_col). Forwarding positionally compiles cleanly and renders incorrectly -- I hit that before spotting it.

Cinder's public ImGui::Image( Texture2dRef, size, uv0, uv1, tint, border ) signature is unchanged, and border_col is still honoured via ImGuiCol_Border plus ImGuiStyleVar_ImageBorderSize, mirroring ImGui's own obsolete shim in imgui_widgets.cpp (including forcing a minimum thickness when the colour has alpha, so a border still appears purely because a colour was supplied). However, the Cinder version of ImGui::Image() still takes the same arguments and order as before, so anyone using that (such as samples/ImGui will continue to work as before).

DX12 is untested

I wrote the SRV descriptor allocator without a DX12 setup to run it on. That said, the previous code could not have worked here regardless: the LegacySingleSrv* fields don't exist in 1.92, and the backend now allocates one descriptor per texture rather than one in total. I replaced them with the documented SrvDescriptorAllocFn / SrvDescriptorFreeFn callbacks over a simple free list on the existing heap, following the pattern in ImGui's example_win32_directx12.

It compiles, but someone with a D3D12 project (samples/D3d12ImageBasic would be the obvious test) should confirm it before this is relied on.

How I verified

  • Cinder builds clean in Debug and Release (MSVC v143, x64).
  • samples/ImGui builds and runs; verified interactively that the Tint Color and Border Color controls both affect the image as before.
  • grepped samples/, src/cinder/, include/cinder/ and blocks/ for remaining deprecated identifiers -- none found.
  • DX12 and non-Windows platforms not built.

- enable IMGUI_DISABLE_OBSOLETE_FUNCTIONS in imconfig.h and port the
  deprecated uses: tab/nav colour enums, the NavNoCaptureKeyboard
  flag, clipboard callbacks, Image() to ImageWithBg(), and DX12 legacy
  SRV descriptors.
- make an inline version of Initialize() so IMGUI_CHECKVERSION() runs
  in the caller's translation unit, where it can catch an app/library
  config mismatch instead of only comparing the Cinder lib version to
  itself.
- to resolve issue cinder#2411.
@totalgee

Copy link
Copy Markdown
Contributor Author

Verified against samples/D3d12ImageBasic: the allocator and free callbacks are exercised for ImGui's font atlas and the UI renders correctly. The multi-descriptor path (more than one texture live at once in DX12) is not exercised by any existing sample.

- removed IgnoreSpecificDefaultLibraries (LIBCMT;LIBCPMT) from
  samples/ImGui, which broke its Release x64 link (/MT requests
  exactly the libs being ignored, so the CRT was excluded with
  thousands of unresolved symbols).
- the same line appears in 67 sample projects; fixing the others seems
  out of scope here but is probably worth a separate pass.
@totalgee

Copy link
Copy Markdown
Contributor Author

Fixed broken Release build for samples/ImGui VS2022 project

  • removed IgnoreSpecificDefaultLibraries (LIBCMT;LIBCPMT) from samples/ImGui, which broke its Release x64 link (/MT requests exactly the libs being ignored, so the CRT was excluded with thousands of unresolved symbols).
  • the same line appears in 67 sample projects; fixing the others seems out of scope here but is probably worth a separate pass.

@totalgee

Copy link
Copy Markdown
Contributor Author

One last comment -- the most important part of this fix is the protection provided by doing IMGUI_CHECKVERSION() in the now-inlined ImGui::Initialize(), so the same class of bug can't go silent again on any platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant