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

SDL_GPU Backend #8163

Closed
wants to merge 14 commits into from
Closed

SDL_GPU Backend #8163

wants to merge 14 commits into from

Conversation

DeltaW0x
Copy link
Contributor

@DeltaW0x DeltaW0x commented Nov 19, 2024

New PR because I irreparably messed up the commit history on the old one.
Now that SDL3 has reached ABI stability, I've added a SDL_Gpu backend to ImGui. The only notable difference from other backends lies in the fact that SDL_Gpu doesn't allow copy passes to occur during a renderpass.
There are two solutions to this problem (that I thought of):

  • Take control of the entire RenderPass for ImGui, which could (and will) limit what the user can do with the rendered texture (like rendering to a different RenderTarget).
  • Require the user to call a new function Imgui_ImplSDLGPU_PrepareDrawData before starting a user-created RenderPass, which uploads all the vertex/index buffer data.

I chose the second option because I thought it was the more useful one, but the code can be rewritten easily to support the former if deemed necessary. I've included shaders for SPIRV, DXCB and METALLIB for MacOS, I'll be able to compile the remaining ones for iOS and tvOS as soon as I'm able to install the SDKs for those.

I also wrote an example for the Visual Studio solution and added a Make file for other platforms

Added source sharder files for the SDL_GPU Backend and instructions on how to compile them
Update example_sdl3_sdlgpu.vcxproj
Changed sdl3-config --libs to pkg-config --libs sdl3 when linking libraries
@DeltaW0x
Copy link
Contributor Author

DeltaW0x commented Nov 19, 2024

I think I've added everything, now the Makefile works on both Linux and MacOS. Let me know if there's anything else to fix or add or test

Forward-declare enums ad structs instead of including SDL_gpu.h in header file
@povik
Copy link

povik commented Nov 20, 2024

I might be doing something wrong but when integrating this I had to change the target_info on the pipeline to match my other pipelines in order to have depth and stencil test configuration from the imgui pipeline properly applied. This is on Metal.

Without the patch below the imgui content rendered behind, not in front of the scene:

diff --git a/backends/imgui_impl_sdlgpu3.cpp b/backends/imgui_impl_sdlgpu3.cpp
index cfcf12d4..e2a88b4d 100644
--- a/backends/imgui_impl_sdlgpu3.cpp
+++ b/backends/imgui_impl_sdlgpu3.cpp
@@ -505,7 +505,8 @@ static void ImGui_ImplSDLGPU_CreateGraphicsPipeline()
     SDL_GPUGraphicsPipelineTargetInfo target_info = {};
     target_info.num_color_targets = 1;
     target_info.color_target_descriptions = color_target_desc;
-    target_info.has_depth_stencil_target = false;
+    target_info.depth_stencil_format = SDL_GPU_TEXTUREFORMAT_D16_UNORM;
+    target_info.has_depth_stencil_target = true;
     
     SDL_GPUGraphicsPipelineCreateInfo pipeline_info = {};
     pipeline_info.vertex_shader = bd->VertexShader;

@DeltaW0x
Copy link
Contributor Author

DeltaW0x commented Nov 20, 2024

If you want to apply it on top of other rendered geometry in an existing renderpass then yes, you would need to use a depth buffer, but I believe that usually you would render ImGui as the last renderpass of your application, so it gets drawn on top of everything else without problems or a zbuffer. I can add a custom pipeline input though, so you can pass what you need without problems

@povik
Copy link

povik commented Nov 20, 2024

Sounds like I need to separate out a render pass for imgui, in which case the imgui backend will work as-is. Thanks! (My knowledge of modern rendering APIs is limited.)

@DeltaW0x
Copy link
Contributor Author

DeltaW0x commented Nov 20, 2024

I've added a custom pipeline input in ImGui_ImplSDLGPU_RenderDrawData, you can try that or just do the separate renderpass. Having a separate pass is useful when you want to disable rending ImGui at all too, like when shipping for a game or something similar

@povik
Copy link

povik commented Nov 20, 2024

I've made it work with a separate render pass.

@ocornut
Copy link
Owner

ocornut commented Nov 20, 2024

Good job, thanks!
I'm presently trying to focus on other features but I hope to get back to this soon-ish.

@DeltaW0x
Copy link
Contributor Author

Please take your time, I'll keep working on it to improve some things anyways

@WoozChucky
Copy link

Any updates on this ?
I saw at least other PR #7998 with the same goal.

@DeltaW0x
Copy link
Contributor Author

This pr is mostly complete, I've been using it for my game for months now, it just needs to be checked by somebody else and merged eventually

@WoozChucky
Copy link

This pr is mostly complete, I've been using it for my game for months now, it just needs to be checked by somebody else and merged eventually

I'm going to give it a try on my game and report back then 👍

@ManifoldFR
Copy link

I'm currently using this and it seems to work quite well.

@ocornut
Copy link
Owner

ocornut commented Jan 9, 2025

Hello,

I have almost finished reviewing it and making small amends, should be merged down.
I dislike that there is 130 kb worth of source code for encoded shaders (for effectively 17902 bytes of data) so I'm reworking on squeezing that now.

It would be good to eventually amend the build instructions with actual scripts that ends up emitting all arrays exactly as there will be in the committed file (i'm using misc/fonts/binary_to_compressed_c.cpp now) so it becomes a non-issue if we ever have to iterate on that further.

ocornut pushed a commit that referenced this pull request Jan 9, 2025
+Squashed: Optimized shader source code encoding by ocornut (#8163, #7998, #7988)
(squashed to avoid storing both in git history, 130 KB->62 KB)
@ocornut
Copy link
Owner

ocornut commented Jan 9, 2025

Now merged as two commits:

(1) 8bbccf7 is a squash of your PR commits, over which I've applied a few changes

  • renamed files from imgui_impl_sdlgpu.* to imgui_impl_sdlgpu3.* (also in various scripts)
  • optimized source code encoding of shader blobs from 130 KB to 62 KB
  • shader raw size also included in array declaration for reference.

I included that optimization in your commit to avoid storing them twice in git history.

(2) e799849 renaming symbols, fixing coding styles, adding various docs/comments.

Note that I haven't tested the shaders on platform other than win11.

This is what I used to export and reencode the shaders:

#include <stdio.h>
#include <stdlib.h>
void emit(const uint8_t* data, size_t data_size, const char* filename)
{
    FILE* f = fopen(filename, "wb");
    fwrite(data, 1, data_size, f);
    fclose(f);

    char buf[300];
    sprintf(buf, "binary_to_compressed_c.exe -u8 -nocompress \"%s\" \"%s\" > %s.h", filename, filename, filename);
    system(buf);
}
    emit(spirv_vertex, sizeof(spirv_vertex), "spirv_vertex");
    emit(spirv_fragment, sizeof(spirv_fragment), "spirv_fragment");

    emit(dxbc_vertex, sizeof(dxbc_vertex), "dxbc_vertex");
    emit(dxbc_fragment, sizeof(dxbc_fragment), "dxbc_fragment");

    emit(metallib_vertex, sizeof(metallib_vertex), "metallib_vertex");
    emit(metallib_fragment, sizeof(metallib_fragment), "metallib_fragment");

    emit(metallib_vertex_iphone, sizeof(metallib_vertex_iphone), "metallib_vertex_iphone");
    emit(metallib_fragment_iphone, sizeof(metallib_fragment_iphone), "metallib_fragment_iphone");

I temporarily removed the ifdef APPLE to get access.

Thanks a lot for your help!

Closing this. I would like eventually to add full fledged scripts to recompile the shaders but we'll see if things ever change much.

@ocornut
Copy link
Owner

ocornut commented Jan 9, 2025

Next step would be adding multi-viewports to it, if anyone fancies that!

ocornut added a commit that referenced this pull request Jan 13, 2025
@ocornut
Copy link
Owner

ocornut commented Jan 16, 2025

FYI small breaking change b4a5d1d i have renamed ImGui_ImplSDLGPU3_InitInfo::GpuDevice to Device for consistency.

ocornut added a commit that referenced this pull request Jan 16, 2025
…_CreateDeviceObjects(), ImGui_ImplSDLGPU3_DestroyDeviceObjects(). Misc renaming. (#8163, #7998, #7988)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants