https://github.com/libsdl-org/SDL/pull/9312 Skip to content Navigation Menu Toggle navigation Sign in * Product + Actions Automate any workflow + Packages Host and manage packages + Security Find and fix vulnerabilities + Codespaces Instant dev environments + GitHub Copilot Write better code with AI + Code review Manage code changes + Issues Plan and track work + Discussions Collaborate outside of code Explore + All features + Documentation + GitHub Skills + Blog * Solutions By size + Enterprise + Teams + Startups By industry + Healthcare + Financial services + Manufacturing By use case + CI/CD & Automation + DevOps + DevSecOps * Resources Topics + AI + DevOps + Security + Software Development + View all Explore + Learning Pathways + White papers, Ebooks, Webinars + Customer Stories + Partners * Open Source + GitHub Sponsors Fund open source developers + The ReadME Project GitHub community articles Repositories + Topics + Trending + Collections * Enterprise + Enterprise platform AI-powered developer platform Available add-ons + Advanced Security Enterprise-grade security features + GitHub Copilot Enterprise-grade AI features + Premium Support Enterprise-grade 24/7 support * Pricing Search or jump to... Search code, repositories, users, issues, pull requests... Search [ ] Clear Search syntax tips Provide feedback We read every piece of feedback, and take your input very seriously. [ ] [ ] Include my email address so I can be contacted Cancel Submit feedback Saved searches Use saved searches to filter your results more quickly Name [ ] Query [ ] To see all available qualifiers, see our documentation. Cancel Create saved search Sign in Sign up Reseting focus You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session. You switched accounts on another tab or window. Reload to refresh your session. Dismiss alert {{ message }} libsdl-org / SDL Public * Notifications You must be signed in to change notification settings * Fork 1.7k * Star 9.2k * Code * Issues 476 * Pull requests 49 * Actions * Security * Insights Additional navigation options * Code * Issues * Pull requests * Actions * Security * Insights 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. Sign up for GitHub 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 Jump to bottom The GPU API MkII #9312 Merged slouken merged 1 commit into libsdl-org:main from thatcosmonaut:gpu Aug 29, 2024 Merged The GPU API MkII #9312 slouken merged 1 commit into libsdl-org:main from thatcosmonaut:gpu Aug 29, 2024 +60,218 -66 Conversation 311 Commits 1 Checks 39 Files changed 96 Conversation This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters Show hidden characters flibitijibibo Copy link Collaborator @flibitijibibo flibitijibibo commented Mar 19, 2024 * edited Loading With 3.0 quickly on the horizon, we've decided to fast-track this so we can get more eyeballs sooner: A close relative to the FNA project is the MoonWorks project, which intends to be more of a successor to XNA rather than a reimplementation like FNA. The graphics component is called Refresh, which is like FNA3D but targets modern APIs like Vulkan. With SDL_gpu still in the early stages, and with Refresh in the process of revamping its API for a 2.0 release anyway, we've decided to just turn Refresh into a possible candidate for the final GPU API. Currently, Refresh supports Vulkan and the PS5 graphics API, with D3D11 deferred context support on the way. It is being used in production for Samurai Gunn 2 on PC and console. It also includes a shader system, but it takes a different approach by [DEL:doing compilation offline, at least for now:DEL] fixed! @thatcosmonaut will be the main Point of Contact since he's the author, but the FNA core team will try to be involved as well if this seems okay to move forward with. Our issue tracker can be found here: https://github.com/thatcosmonaut /SDL/issues Sorry, something went wrong. 35 kg, WimeSTone, Terria-K, theoparis, ghishadow, larsonjj, corentin35000, IgorAlexey, fxesminus, NoelFB, and 25 more reacted with thumbs up emoji 7 tapir2342, kirkegaard, gnat, rudedogg, martintc, guidoschmidt, and andymandias reacted with hooray emoji All reactions * 35 reactions * 7 reactions @darkerbit Copy link darkerbit commented Mar 19, 2024 Would like to state my support for this, with the following points: 1. Compute is a first-class feature of Refresh, and I feel SDL_GPU's previous draft not including it was a massive oversight. Compute is a feature in every modern graphics API (and even some older ones) and Refresh's API makes them delightfully simple to work with, which I feel is good to encourage. A more niche point is that compute shaders also solve every practical usecase of a GPU-timeline buffer->texture and texture->buffer copy, which is not a feature that is possible to implement in every graphics API (notably, D3D11) 2. The shader formats being existing formats is a good thing in my opinion, as there exists standard tooling for creating and debugging them. The benefits are addressed in detail in Proposal: Consume Shaders in DXBC Format SDL_shader_tools#17, though that isn't the solution Refresh is using right now. 3. Refresh already has an excellent Vulkan backend. This is going to save a tremendous amount of development time, as Refresh's Vulkan backend not only already exists but is also, since 2.0, incredibly performant. 4. Refresh 2.0 puts great care into ensuring performant resource safety via cycling when possible. SDL_GPU's previous draft included APIs for this, however I don't think this is something that should be exposed to the user, as this is fundamentally backend-specific and complicates API usage. Refresh performs resource safety internally, with fantastic results. 6 kg, theoparis, ghishadow, photex, p1utoze, and domschl reacted with thumbs up emoji All reactions * 6 reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Mar 19, 2024 Hello, I'm the author so I'll take some time to explain this proposal. First, the shader situation. This API's shader solution is a script called shaderbuild.py, it's essentially a frontend for offline shader building tools on the client's machine, and it globs various formats together so they can individually be sent to the appropriate render backends. I'm aware that the original proposal included online shader compilation. This solution doesn't forbid this, because future SDLSL source could just be included in the binary and the CreateShaderModule function can translate it into the desired backend's bytecode on the fly. We wouldn't have to break the public API to allow this, which is a nice plus, and it prevents the shader compiler from being a blocker on using this API. When authoring a shader the API expects certain set layouts depending on the resource and shader stage: vertex samplers are set 0, fragment samplers are set 1, vertex uniforms are set 2 and fragment uniforms are set 3. This is a modern-style rendering API, so almost all tasks occur in a deferred context and are broken up into render passes, compute passes, and copy passes. All operations that write to a resource have the ability to cycle to avoid inter-frame dependencies - handles to graphics resources like GpuBuffers are just containers so we can cycle references to internal resources. There are some data quirks due to AMD D3D11 drivers being not great - this is why there are a few different WriteOptions enums. I'd like to get rid of these but I haven't fully been able to work around the fact that D3D11 data APIs do not work as advertised on AMD. Presentation is handled via SDL_GpuAcquireSwapchainTexture, which associates the given command buffer with a swapchain image. When SDL_GpuSubmit is called with this command buffer, the presentation structures are automatically configured and submitted. SDL_GpuSubmit automatically handles submission fences, but the client can choose to explicitly synchronize by calling SDL_GpuSubmitAndAcquireFence and using the returned SDL_GpuFence handle. The rest of the API is fairly bog-standard binding, render, and compute dispatch calls. All reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Mar 19, 2024 I noticed that the CI treats all warnings as errors, which is fine, but there are some things like unused function variables or pointer size casts that I need to be able to work around. Are there macros for handling these warnings? I was also having a difficult time getting preprocessor defines showing up correctly in the project from CMake, so I could use some help with that. All reactions Sorry, something went wrong. @slouken slouken requested review from icculus and madebr March 19, 2024 17:25 sezero sezero reviewed Mar 19, 2024 View reviewed changes include/SDL3/SDL_gpu.h Outdated Show resolved Hide resolved @icculus Copy link Collaborator icculus commented Mar 19, 2024 This is not what I was expecting to see in my inbox this morning. I'm going to take some time to study this and think on it. I want to be clear, this might get combined with the other branch somewhere between "this particular idea was better than mine so a pivot makes sense" to "this piece got copy/pasted into the my branch," to "this is better in all ways, so I put my branch in the trash." I don't know how this will shake out yet. Also to be clear: there is no scenario where either this PR or my branch is landing in the 3.2.0 release in the name of expediency. 7 slouken, bradallred, ValorZard, photex, lmptg, DerTee, and DEF7 reacted with thumbs up emoji All reactions * 7 reactions Sorry, something went wrong. @slime73 Copy link Contributor slime73 commented Mar 19, 2024 * edited Loading This API's shader solution is a script called shaderbuild.py, it's essentially a frontend for offline shader building tools on the client's machine, Some 2c from an outsider: I understand some of the reasons for purely offline shader compilation (runtime compilation isn't supported on every possible platform anyway, including third party cross-compile libraries might bloat the binary size considerably - or maintaining custom cross-compilation is a massive maintenance burden, optimization can be more aggressive, etc.) - but purely offline compilation incompatible with the engine I develop, so if SDL_gpu does this it immediately puts it off the table for me. Also, the way it's set up right now seems to need a lot of setup from developers using it: an install of python in their path, an install of glslc in their path, an install of spirv-cross in their path, some per-project setup to run the script on the appropriate files if you don't want to manually run everything in a terminal, etc. If I did use this, that part would probably feel really frustrating to me. 1 DEF7 reacted with thumbs up emoji All reactions * 1 reaction Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Mar 19, 2024 * edited Loading As I mentioned, I know there are perfectly good reasons to support online shader compilation and the proposed format intentionally does not preclude that from being possible. This was the best solution I can think of that does not involve pulling in an enormous dependency and works today. At the end of the day something has to compile a shader language to actual bytecode or else we are completely blocked. I mostly went with python because there are a few other scripts in SDL that use it and there's not really a precedent for SDL shipping something like this. But as long as the input format is well-defined it wouldn't really matter what developers use to generate it. All reactions Sorry, something went wrong. @slime73 Copy link Contributor slime73 commented Mar 19, 2024 * edited Loading and works today. I don't want to speak for the SDL team too much but I don't get the sense this is really the top priority for SDL_gpu in the current moment - figuring out something that works cleanly and robustly tomorrow is likely more important than something simply working in any state immediately. If you are blocked by a lack of SDL_gpu right now then it probably makes sense to use something else in the meantime instead of hoping it ships in the very near future. Given the rest of SDL's ecosystem, the most idiomatic offline shader compilation tool for SDL I can think of would be a separate satellite library, which could avoid needing system-wide packages like python/ glslc for people using it, and might also let people ship it with their app if they want runtime or otherwise on-device compilation I suppose. But maybe that's getting a bit too far into the weeds for now given Ryan's previous message, and I should just let him think on it without all the noise from me. :) 2 bradallred and DEF7 reacted with thumbs up emoji All reactions * 2 reactions Sorry, something went wrong. @1bsyl Copy link Contributor 1bsyl commented Mar 20, 2024 (if possible, SDL could run (with compilation flags) in different flavors: legacy SDL_Render, Ryan's SDL_GPU , etc.. ) All reactions Sorry, something went wrong. @flibitijibibo flibitijibibo mentioned this pull request Mar 20, 2024 Implement SDLRenderer with SDL_gpu FNA-XNA/FNA3D#162 Closed @sezero Copy link Contributor sezero commented Mar 20, 2024 Got type redefinition errors: In file included from /tmp/SDL3-gpu-9312/src/gpu/../video/khronos/vulkan/vulkan.h:11, from /tmp/SDL3-gpu-9312/src/gpu/SDL_gpu_vulkan.c:31: /tmp/SDL3-gpu-9312/src/gpu/../video/khronos/vulkan/vulkan_core.h:101: error: redefinition of typedef 'VkInstance' /tmp/SDL3-gpu-9312/include/SDL3/SDL_vulkan.h:52: note: previous declaration of 'VkInstance' was here In file included from /tmp/SDL3-gpu-9312/src/gpu/../video/khronos/vulkan/vulkan.h:11, from /tmp/SDL3-gpu-9312/src/gpu/SDL_gpu_vulkan.c:31: /tmp/SDL3-gpu-9312/src/gpu/../video/khronos/vulkan/vulkan_core.h:7513: error: redefinition of typedef 'VkSurfaceKHR' /tmp/SDL3-gpu-9312/include/SDL3/SDL_vulkan.h:53: note: previous declaration of 'VkSurfaceKHR' was here make[2]: *** [CMakeFiles/SDL3-shared.dir/src/gpu/SDL_gpu_vulkan.c.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [CMakeFiles/SDL3-shared.dir/all] Error 2 make: *** [all] Error 2 Easy fix: diff --git a/src/gpu/SDL_gpu_vulkan.c b/src/gpu/SDL_gpu_vulkan.c index 7feafda..acf57d2 100644 --- a/src/gpu/SDL_gpu_vulkan.c +++ b/src/gpu/SDL_gpu_vulkan.c @@ -22,14 +22,14 @@ #include "SDL_internal.h" #if SDL_GPU_VULKAN -#include - /* Needed for VK_KHR_portability_subset */ #define VK_ENABLE_BETA_EXTENSIONS #define VK_NO_PROTOTYPES #include "../video/khronos/vulkan/vulkan.h" +#include + #include "SDL_gpu_driver.h" #define VULKAN_INTERNAL_clamp(val, min, max) SDL_max(min, SDL_min(val, max)) Minor annoying 'maybe used uninitialized' warning (seems bogus): /tmp/SDL3-gpu-9312/src/gpu/SDL_gpu_vulkan.c: In function 'VULKAN_INTERNAL_DeterminePhysicalDevice': /tmp/SDL3-gpu-9312/src/gpu/SDL_gpu_vulkan.c:11338: warning: 'suitableQueueFamilyIndex' may be used uninitialized in this function Silencing: diff --git a/src/gpu/SDL_gpu_vulkan.c b/src/gpu/SDL_gpu_vulkan.c --- a/src/gpu/SDL_gpu_vulkan.c +++ b/src/gpu/SDL_gpu_vulkan.c @@ -11385,6 +11385,7 @@ static Uint8 VULKAN_INTERNAL_DeterminePhysicalDevice( /* Any suitable device will do, but we'd like the best */ suitableIndex = -1; + suitableQueueFamilyIndex = 0; highestRank = 0; for (i = 0; i < physicalDeviceCount; i += 1) { Noticed that dynapi isn't touched: Running gendynapi.py under src/ dynapi/ will update it. (It will emit lots of missing documentation warnings along the way..) 2 thatcosmonaut and DEF7 reacted with thumbs up emoji All reactions * 2 reactions Sorry, something went wrong. @meyraud705 Copy link Contributor meyraud705 commented Mar 22, 2024 As I am interested in SDL_gpu and am working on an OpenGL implementation of it (icculus#7), here what I think: I like the idea of SDL_gpu handling cycles. I am not sure why some combination are not allowed: no safe for GPU buffer and no unsafe for texture. Most of the structs are similar: Device, Pipeline... TransferBuffer which is like the CpuBuffer but looks useful. There is no *Pass struct, user can't do anything with them but they avoid mixing of render and copy command so I think it is better to have them. For the functions I prefer @icculus version: "put this buffer/texture at this location". Also, while passing ten parameters to a function is ugly, pointer to struct for 2 parameters and nested struct (TextureRegion -> TextureSlice -> Texture) is not better. I would like to see the spinning cube test (https://github.com/ icculus/SDL/blob/5db1fd7491ccf1d9b209c91ba163da0b2fb54f6f/test/ testgpu_spinning_cube.c) with this API (including its shader), especially how you handle the absence of binding location. 1 DEF7 reacted with thumbs up emoji All reactions * 1 reaction Sorry, something went wrong. @ericoporto ericoporto mentioned this pull request Mar 26, 2024 GPU API Feedback icculus/SDL#1 Closed @flibitijibibo flibitijibibo force-pushed the gpu branch from e1487e9 to b767fd9 Compare March 26, 2024 16:55 @flibitijibibo Copy link Collaborator Author flibitijibibo commented Mar 26, 2024 * edited Loading Latest push starts the work on Metal support, and reworks the shader system slightly to make a clear distinction between "raw" shaders and "portable" shaders - in particular, we're using the FNA3D port as a stress test for supporting shader creation at runtime. We're currently using the MojoShader SPIR-V emitter, which can be passed directly when Vulkan is active, and for other backends we're using a separate (and more importantly, optional) library to translate: https://github.com/thatcosmonaut/SDL_shader We're using SPIR-V since MojoShader already supports it, but the same idea would apply to SDL's shader format: A separate library would take in the blob and emit what the active SDL_gpu backend needs, and SDL itself doesn't know or care where the shaders came from. One thing we haven't really touched at all is the formatting - I seem to remember formatting tools getting involved when the decision was made to change some of SDL's style guidelines, but I can't find them anywhere. If that exists somewhere the next push should have the formatting cleaned up. All reactions Sorry, something went wrong. @flibitijibibo flibitijibibo force-pushed the gpu branch from 5a45122 to 5f84ee4 Compare March 26, 2024 17:39 @flibitijibibo Copy link Collaborator Author flibitijibibo commented Mar 28, 2024 As of the latest push, we're in-game: image Still some stuff to iron out but we've now got this working with no offline shader compilation needed! Once we've taken care of all the FNA3D TODOs/FIXMEs we'll start cleaning up the SDL side next, and that should set the stage for porting to D3D12/Metal. I believe we have a test program ported from Ryan's draft as well, it just hasn't been added to this repo yet. All reactions Sorry, something went wrong. @flibitijibibo Copy link Collaborator Author flibitijibibo commented Mar 29, 2024 Activity on this will probably calm down over the weekend, so the code shouldn't move too much if anyone's waiting to review this... in the meantime, here's the current revision booting up Streets of Rage 4 (with some bugs on our end, but still!) image All reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Mar 29, 2024 The main thing I want to look at is streamlining the various WriteOptions. I'll look into that and writing up SDL-style doc comments on the header next week. All reactions Sorry, something went wrong. @aganm aganm mentioned this pull request Mar 31, 2024 Improving the formatting of header files #9405 Open @flibitijibibo flibitijibibo force-pushed the gpu branch from 1641244 to debe7a3 Compare April 1, 2024 20:39 @flibitijibibo Copy link Collaborator Author flibitijibibo commented Apr 1, 2024 Latest push gets Wizorb running on Metal! We're still pushing forward via FNA3D but if anyone is interested in looking at Metal/D3D12 support in particular, please get in touch. All reactions Sorry, something went wrong. @flibitijibibo flibitijibibo force-pushed the gpu branch from 0e2f06f to 6fcde03 Compare April 5, 2024 01:35 @flibitijibibo Copy link Collaborator Author flibitijibibo commented Apr 5, 2024 Latest push gets in-game for a good chunk of our trace database - for fun, here's Celeste: image Interestingly enough, the one component that's actually pretty close to done is the shader system; at this point I'm really happy with how the MojoShader implementation turned out and it shows a good example of supporting shaders without necessarily needing an SDLSL first: https://github.com/FNA-XNA/FNA3D/blob/sdl_gpu/src/mojoshader_sdlgpu.c There are some remaining features left before we start focusing on the backends, but we're now pretty much at a point where the overall design is there but may not have the minimum feature set needed just yet (for example, the next push will add hardware instancing support). 5 Green-Sky, ghishadow, corentin35000, theoparis, and DEF7 reacted with hooray emoji All reactions * 5 reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Apr 6, 2024 Instancing and occlusion queries are in, just need to fill those in on Metal. We'll just keep going through our test cases to ferret out bugs. All reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Apr 6, 2024 Boiled the cycle concept down to a bool and got rid of all the various WriteOption enums. I've documented how this works thoroughly in the code now. Right now we're working on making the shader module creation struct take the respective IR format of each backend so that SDL itself doesn't have to wrap any shader compilers. All reactions Sorry, something went wrong. @flibitijibibo flibitijibibo mentioned this pull request Apr 10, 2024 Implement SDLGPUDriver FNA-XNA/FNA3D#201 Merged 6 tasks @flibitijibibo flibitijibibo force-pushed the gpu branch from cc75739 to 99b32ce Compare April 11, 2024 21:12 295 hidden items Load more... @ewichuu Copy link ewichuu commented Aug 29, 2024 woooooo! All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 29, 2024 Yay! Do you consider this ready to merge? Also, did we settle on Gpu vs GPU? (@icculus?) All reactions Sorry, something went wrong. @thatcosmonaut @icculus @flibitijibibo Add the SDL_GPU API ... d42fab9 Project Lead: Evan Hemsley Co-designer, Metal Port, Console Ports: Co-authored-by: Caleb Cornett Production, QA, Debug: Co-authored-by: Ethan Lee SDL_Render Driver, Bugfixes: Co-authored-by: Andrei Alexeyev Additional D3D12 Programming, Bugfixes: Co-authored-by: Bart van der Werf Bugfixes and Feedback: Co-authored-by: Zakary Strange Co-authored-by: meyraud705 Co-authored-by: Joshua T. Fisher Co-authored-by: Topi Ritala Co-authored-by: David Gow Original API Proposal: Co-authored-by: Ryan C. Gordon @flibitijibibo flibitijibibo force-pushed the gpu branch from f932294 to d42fab9 Compare August 29, 2024 22:19 @flibitijibibo Copy link Collaborator Author flibitijibibo commented Aug 29, 2024 Yay! Do you consider this ready to merge? Also, did we settle on Gpu vs GPU? (@icculus?) At this point we'd be okay with this getting merged; we can send in bugfixes/documentation updates directly to upstream without any trouble. I think Ryan leaned towards Gpu, and we're not too picky either way. If the namespacing is correct this should be good to go. All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 29, 2024 Okay, I'll merge this and then do a review of the naming and public API 7 caspark, theoparis, lumpalette, Green-Sky, LucidSigma, superzazu, and smoogipoo reacted with hooray emoji All reactions * 7 reactions Sorry, something went wrong. Hide details View details @slouken slouken merged commit 2e7d5bb into libsdl-org:main Aug 29, 2024 22 of 39 checks passed @slouken Copy link Collaborator slouken commented Aug 29, 2024 It's merged. Please hold off for a bit with any changes while I review and tweak? 6 thatcosmonaut, flibitijibibo, theoparis, Green-Sky, superzazu, and smoogipoo reacted with thumbs up emoji 5 LucidSigma, smoogipoo, lukaasm, v1993, and MatheusKS95 reacted with hooray emoji All reactions * 6 reactions * 5 reactions Sorry, something went wrong. @flibitijibibo Copy link Collaborator Author flibitijibibo commented Aug 29, 2024 Will do - we're all going to get some rest now [?] 3 superfunc, MatheusKS95, and BryceBarbara reacted with heart emoji All reactions * [?] 3 reactions Sorry, something went wrong. @flibitijibibo flibitijibibo deleted the gpu branch August 29, 2024 23:06 @slouken Copy link Collaborator slouken commented Aug 29, 2024 How do you feel about size_t instead of Uint32 for byte sizes in API function signatures? All reactions Sorry, something went wrong. @flibitijibibo Copy link Collaborator Author flibitijibibo commented Aug 29, 2024 Will defer to @thatcosmonaut for the backend details but I think we decided against it for most cases, which eventually became all - one that comes to mind is buffers, which could be size_t but then that would probably make simultaneous 32/64-bit compatibility hard if the buffers start to get huge. For places where size_t could apply I could maybe see Uint64 instead...? All reactions Sorry, something went wrong. @icculus Copy link Collaborator icculus commented Aug 29, 2024 Leave them as Uint32 in the GPU API. 1 slouken reacted with thumbs up emoji All reactions * 1 reaction Sorry, something went wrong. @sezero Copy link Contributor sezero commented Aug 29, 2024 In the config files, i.e. include/build_config/ SDL_build_config_???.h, things like #define SDL_GPU_VULKAN SDL_VIDEO_VULKAN or #define SDL_GPU_METAL SDL_VIDEO_METAL look like they rely on the right side being defined either as 0 or 1. It will be problem if the right-side macro, e.g. SDL_VIDEO_METAL or SDL_VIDEO_VULKAN is not defined at all. All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 Here are some tweaks for review, they're all up for discussion by the GPU folks: #10622 All reactions Sorry, something went wrong. @sezero Copy link Contributor sezero commented Aug 30, 2024 P.S.: Is UTF-8 BOM in gpu src files really necessary? $ find src/gpu | xargs file | grep BOM src/gpu/d3d11/SDL_gpu_d3d11.c: UTF-8 Unicode (with BOM) C program text src/gpu/SDL_sysgpu.h: UTF-8 Unicode (with BOM) C program text src/gpu/d3d12/SDL_gpu_d3d12.c: UTF-8 Unicode (with BOM) C program text src/gpu/SDL_gpu.c: UTF-8 Unicode (with BOM) C program text All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 In the config files, i.e. include/build_config/ SDL_build_config_???.h, things like #define SDL_GPU_VULKAN SDL_VIDEO_VULKAN or #define SDL_GPU_METAL SDL_VIDEO_METAL look like they rely on the right side being defined either as 0 or 1. It will be problem if the right-side macro, e.g. SDL_VIDEO_METAL or SDL_VIDEO_VULKAN is not defined at all. Fixed in #10622 All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 P.S.: Is UTF-8 BOM in gpu src files really necessary? $ find src/gpu | xargs file | grep BOM src/gpu/d3d11/SDL_gpu_d3d11.c: UTF-8 Unicode (with BOM) C program text src/gpu/SDL_sysgpu.h: UTF-8 Unicode (with BOM) C program text src/gpu/d3d12/SDL_gpu_d3d12.c: UTF-8 Unicode (with BOM) C program text src/gpu/SDL_gpu.c: UTF-8 Unicode (with BOM) C program text Fixed in #10622 All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 I'm going to go ahead and merge my tweaks so people trying out the beta get the new function names. Please let me know if you disagree with any of the changes. All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 * edited Loading FYI, D3D12_ClaimWindow() always sets up the swap chain with vsync, so things like testsprite are unexpectedly running at 60 FPS. All reactions Sorry, something went wrong. @sezero Copy link Contributor sezero commented Aug 30, 2024 P.S.: Is UTF-8 BOM in gpu src files really necessary? Fixed in #10622 include/SDL3/SDL_gpu.h has BOM too (sorry, I seem to have missed it) All reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Aug 30, 2024 FYI, D3D12_ClaimWindow() always sets up the swap chain with vsync, so things like testsprite are unexpectedly running at 60 FPS. This is intentional - swapchain queries on most backends require a pre-existing surface to be initialized, so the intended workflow is to claim a window, which sets up a swapchain with the universally supported parameters (SDR and VSYNC) and then query for support and call SetSwapchainParameters if you want something different. All reactions Sorry, something went wrong. @Akaricchi Copy link Contributor Akaricchi commented Aug 30, 2024 I'm pretty sure the render driver is calling SetSwapchainParameters after claiming the window... at least it should be. testsprite is definitely not running with vsync for me on Vulkan, for what it's worth. All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 Yep, I confirmed that it is, but something is still limiting the framerate to 60 FPS in the D3D12 driver. All reactions Sorry, something went wrong. @thatcosmonaut Copy link Collaborator thatcosmonaut commented Aug 30, 2024 Got it, will investigate. All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 Okay, everything is merged and I'm done partying on your code. Feel free to start merging changes. Please let me know who you'd like to have commit permissions, and I'll set them up. 1 thatcosmonaut reacted with thumbs up emoji All reactions * 1 reaction Sorry, something went wrong. @flibitijibibo Copy link Collaborator Author flibitijibibo commented Aug 30, 2024 Let's add @thatcosmonaut, since chances are good he'll need to review incoming changes anyhow. All reactions Sorry, something went wrong. @slouken Copy link Collaborator slouken commented Aug 30, 2024 Let's add @thatcosmonaut, since chances are good he'll need to review incoming changes anyhow. Added! All reactions Sorry, something went wrong. @Foxhunt Foxhunt mentioned this pull request Aug 30, 2024 Daily Hacker News 30-08-2024 Foxhunt/daily-hackernews#52 Open @xueyuanl xueyuanl mentioned this pull request Aug 30, 2024 Daily Hacker News 30-08-2024 xueyuanl/daily-hackernews#1432 Open Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment Reviewers @slime73 slime73 slime73 left review comments @slouken slouken slouken left review comments @thatcosmonaut thatcosmonaut thatcosmonaut left review comments @TellowKrinkle TellowKrinkle TellowKrinkle left review comments @sezero sezero sezero left review comments @TheSpydog TheSpydog TheSpydog left review comments @Joshua-Ashton Joshua-Ashton Joshua-Ashton left review comments @icculus icculus Awaiting requested review from icculus @madebr madebr Awaiting requested review from madebr Assignees No one assigned Labels None yet Projects None yet Milestone No milestone Development Successfully merging this pull request may close these issues. None yet 29 participants @flibitijibibo @darkerbit @thatcosmonaut @icculus @slime73 @1bsyl @sezero @meyraud705 @corentin35000 @kg @slouken @Green-Sky @ewichuu @Akaricchi @duerdak @TheSpydog @pragma37 @namandixit @Helco @TellowKrinkle @kariem2k @bartwe @lukaasm @captain0xff @ritalat @RandyGaul @superfunc @jaedan @Joshua-Ashton Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later. Footer (c) 2024 GitHub, Inc. Footer navigation * Terms * Privacy * Security * Status * Docs * Contact * Manage cookies * Do not share my personal information You can't perform that action at this time.