Skip to content

sdl3::gpu refactor #158

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

quaternic
Copy link
Contributor

@quaternic quaternic commented Apr 7, 2025

While trying out the gpu-module, I hit various problems including #147 . That led me to do some experimenting with the API design, as implemented in this PR.

This definitely isn't finalized, but I'm posting it here since the API design needs discussion.

The most fundamental idea is to define the Rust equivalents for SDL's opaque types like

pub type Buffer = Extern<SDL_GPUBuffer>;

where Extern is just a transparent wrapper. This means that the Rust-side can pass around &Buffer, normal Rust references, and under the hood that is just the same *mut SDL_GPUBuffer that the SDL calls work with. This makes the abstraction a lot "thinner", when e.g. a function binding storage buffers can take a &[&Buffer] and just give the slice's pointer and length directly to SDL.

When such resources are created, you receive an Owned<'_, T>, which borrows the &Device it was obtained from, and will release the resource when dropped. It also derefs to &T.

The different CommandBuffer passes, RenderPass, CopyPass and ComputePass, are all mutually exclusive on a given command buffer at a time, so these are provided as methods on &mut CommandBuffer which take a closure for what the user wants to do with the pass. The same pattern is also used for cpu-side access to a TransferBuffer.

Of course not everything is so clear cut; for example, the command buffer itself should be either submitted or cancelled, and the latter only if you haven't obtained a swapchain on it yet. So should its Drop auto-submit?

Any thoughts on how resource management should work with this crate more generally?

@gustafla
Copy link
Contributor

I can't answer any of your questions but I like your idea very much and wish that the current API abstractions were better. I have a (still private) project using this crate and the GPU API, I've been thinking about refactoring my application code against this PR in order to provide feedback. If you're still interested in moving this forward, how about I'll try it next weekend?

@gustafla
Copy link
Contributor

gustafla commented May 31, 2025

Okay, it seems that this "definitely isn't finalized" as you said. The following could be improved:

  • Missing impl TextureSamplerBinding, impl TransferBufferLocation, impl BufferRegion and probably others
  • Minor style issue: I would prefer enum variant names be like TextureUsage::ColorTarget not TextureUsage::COLOR_TARGET but it's fine if implementation is simpler this way
  • Multithreaded use. I know this is a tough one, but the current API allows me to hackily use the Device in two threads simultaneously, where one keeps drawing stuff and the other refreshes some resources periodically. This is probably unsound, though, but I'd like to keep doing something like that.

Neutral: lifetime-heavy API. Doesn't bother me.

I like: everything else! No unnecessary Arcs, Iterator::collects, and the render_pass/copy_pass API with the closures is also much to my liking.

Let's work on this and finalize it?

@gustafla
Copy link
Contributor

gustafla commented May 31, 2025

Well, I was able to work around the multithreading stuff by keeping all of the dynamic resource data on the CPU side and making the main thread submit copy passes instead of the resource updater thread. My engine compiles and runs flawlessly against this PR now (I just had to add the missing impls).

The (trivial) changes are in my fork.

@gustafla
Copy link
Contributor

So should its Drop auto-submit?

I'm not a safety expert but I think that's a reasonable thing to do. I prefer to manually submit/cancel, though.

Any thoughts on how resource management should work with this crate more generally?

I found that in general, everything I wanted to do with the API was already there, with the exception of some of the binding structure impls which are trivial to implement. I can create stuctures which own resources and then borrow said resources, that's all you really need.

@gustafla
Copy link
Contributor

gustafla commented May 31, 2025

Also I'm pretty sure that TextureUsageFlags was still in the enum format with bit operations as mentioned in #147. Those need to be double checked. Edit: It's not.

@quaternic
Copy link
Contributor Author

quaternic commented May 31, 2025

Now that I looked back into this after six weeks of not looking at it, I realized that I had a handful of local changes not pushed. I rebased and included those in commit 1bf2dd6

Given that it's been a while, I've mostly lost the train of thought that went into this, but one notable thing was adding some static assertions in 1bf2dd6#diff-4c5ab03d4ea64c98c160693e928196ddb844dcb27dce9804605c1506dda0f8ef for which auto-traits the various SDL-types should implement, in particular the thread-safety aspect.

Multithreaded use. I know this is a tough one, but the current API allows me to hackily use the Device in two threads simultaneously, where one keeps drawing stuff and the other refreshes some resources periodically. This is probably unsound, though, but I'd like to keep doing something like that.

As noted in the comments in the preceding link, Device should indeed be usable across threads in that way, and I'm also doing that in the toy project I'm using it for. For some of the other things, what is and isn't thread-safe isn't always entirely clear in SDL's documentation.

@gustafla
Copy link
Contributor

gustafla commented Jun 1, 2025

Hey @quaternic one more thing. I edited info_struct.rs with missing impls that my application needs and also added documentation with relevant links and Rust-specific changes from SDL Wiki.

Edit: I opened a PR in your forked repo.

Would you mind merging those changes from https://github.com/gustafla/sdl3-rs/tree/gpurefactor to this PR? Or is it better workflow-wise if I open a new PR that depends on this one?

@revmischa
Copy link
Collaborator

Please let me know when you're ready to merge this

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