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

Udpate to ImGui v1.91.8. #15

Merged
merged 5 commits into from
Feb 6, 2025
Merged

Conversation

PMarcL
Copy link
Contributor

@PMarcL PMarcL commented Feb 3, 2025

Updated all ImGui C++ files to the latest version: https://github.com/ocornut/imgui/tree/v1.91.8/. As described in #13, this is useful to add support for the SDL3+GPU backend. Here's a few things to note:

  • Only tested on Windows. So, I couldn't test the OSX/Metal backend.
  • I used the available samples in the zig-gamedev repo to test this update. Unfortunately, this repo doesn't fully build with a recent nightly version of zig (I'm using 0.14.0-dev.2987+183bb8b08). In particular, I couldn't build minimal_zgpu_zgui. I was able to test all the other zgui samples successfully (with some modifications to the d3d12 samples).
  • I had to update the imgui_test_engine files as well to the latest version to successfully build the zig-gamedev repo.
  • I had to introduce some modifications in the node editor C++ files. This project simply doesn't build anymore with the latest version of ImGui (the repo has not been updated in 2 years it seems).
  • @nfginola I couldn't rerun your demo app for the vulkan backend with the updated files (I'm running into this issue: 'vulkan/vulkan.h' file not found, maybe I'm missing something on my machine). It would be nice if you have the bandwidth to rerun your demo with those changes as a sanity check before merging this PR.

Finally, this is a pretty large change that risk introducing a bunch of bugs. I'm open to feedback or running some additional tests if you can point me in the right direction.

@nfginola
Copy link
Contributor

nfginola commented Feb 3, 2025

Thanks for the great work!

I ran my demo app on adbe991.
I can verify that the Vulkan backend is working on Linux platform with dynamic rendering supported and using it.
'vulkan/vulkan.h' file not found is likely because you don't have Vulkan development environment setup.

I had to introduce some modifications in the node editor C++ files. This project simply doesn't build anymore with the latest version of ImGui (the repo has not been updated in 2 years it seems).

Its unfortunate that the node editor hasn't updated in a very long time. Generally we are deviating from the original source for small fixes to keep this bundle of libraries working, and I think your change is aligned with that 👍

// (Optional) Dynamic Rendering
// Need to explicitly enable VK_KHR_dynamic_rendering extension to use this, even for Vulkan 1.3.
bool UseDynamicRendering;
#ifdef IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING
VkPipelineRenderingCreateInfoKHR PipelineRenderingCreateInfo;
#else
unusedVkPipelineRenderingCreateInfo PipelineRenderingCreateInfo;
Copy link
Contributor

@nfginola nfginola Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be re-introduced as not having this structure breaks compatibility. We would like to have stable structure to be consumed from Zig regardless of IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING, which is the reason the unusedVkPipelineRenderingCreateInfo was introduced with identical memory structure.

IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING depends on the availability of Vulkan 1.3 support and dynamic rendering visible in vulkan_core.h which is not necessarily present for all devices.

#if defined(VK_VERSION_1_3) || defined(VK_KHR_dynamic_rendering) #define IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING #endif

I recommend adding the following comment // FIX(zig-gamedev) - Match PipelineRenderingCreateInfo structure for compatibility above it as well to prevent future accidental removals when this backend gets updated again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-applied the patch and added the comment. Thanks a lot for the background.

cbv_srv_heap: *const anyopaque, // ID3D12DescriptorHeap
font_srv_cpu_desc_handle: backend_dx12.D3D12_CPU_DESCRIPTOR_HANDLE,
font_srv_gpu_desc_handle: backend_dx12.D3D12_GPU_DESCRIPTOR_HANDLE,
init_info: backend_dx12.ImGui_ImplDX12_InitInfo,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API break. Worth doing this sooner rather than later 👍

cbv_srv_heap: *const anyopaque, // ID3D12DescriptorHeap*
font_srv_cpu_desc_handle: backend_dx12.D3D12_CPU_DESCRIPTOR_HANDLE,
font_srv_gpu_desc_handle: backend_dx12.D3D12_GPU_DESCRIPTOR_HANDLE,
init_info: backend_dx12.ImGui_ImplDX12_InitInfo,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another to fixup in d3d12 samples.

@hazeycode hazeycode merged commit c4fa25d into zig-gamedev:main Feb 6, 2025
3 checks passed
@hazeycode
Copy link
Member

Nice job. I tested the samples on macOS and Linux. I will fixup the D3D12 samples. Thanks!

hazeycode added a commit to zig-gamedev/zig-gamedev that referenced this pull request Feb 6, 2025
zgui D3d12 backend breaking change introduced by upgrade to Dear Imgui 1.91.8. See zig-gamedev/zgui#15
@PMarcL PMarcL deleted the udpate-imgui branch February 7, 2025 04:51
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.

3 participants