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

Parameter consistency for textures/buffers #36

Closed
flibitijibibo opened this issue Jun 16, 2024 · 5 comments
Closed

Parameter consistency for textures/buffers #36

flibitijibibo opened this issue Jun 16, 2024 · 5 comments
Assignees

Comments

@flibitijibibo
Copy link
Collaborator

From the API PR:

Functions that operate on buffer take a buffer and a struct for parameters. Functions that operate on texture only take a struct that includes both texture and parameters. I think it would be better to have functions with similar signature.

@thatcosmonaut
Copy link
Owner

thatcosmonaut commented Jun 19, 2024

Breaking this down a bit...

TextureSlice is a struct which contains a texture pointer, a layer, and mip level. This is used by functions which need to bind a specific resource view, like BeginRenderPass and BindFragmentStorageTextures.

TextureRegion is a struct which encompasses a TextureSlice and 3D offset/dimensions. This is used by functions which write data to a specific region of a texture, like UploadToTexture and CopyTextureToTexture. You cannot have a TextureRegion without also needing a TextureSlice, so it makes sense for TextureRegion to contain TextureSlice.

Let's compare the signatures of UploadToTexture and UploadToBuffer.

extern SDL_DECLSPEC void SDLCALL SDL_GpuUploadToTexture(
    SDL_GpuCopyPass *copyPass,
    SDL_GpuTransferBuffer *transferBuffer,
    SDL_GpuTextureRegion *textureRegion,
    SDL_GpuBufferImageCopy *copyParams,
    SDL_bool cycle);

extern SDL_DECLSPEC void SDLCALL SDL_GpuUploadToBuffer(
    SDL_GpuCopyPass *copyPass,
    SDL_GpuTransferBuffer *transferBuffer,
    SDL_GpuBuffer *buffer,
    SDL_GpuBufferCopy *copyParams,
    SDL_bool cycle);

These have a similar shape. You have a copy pass, the transfer buffer, the resource being uploaded to, some parameters particular to the copy operation, and a cycle bool. The main difference is that TextureRegion contains data about where the data is being copied to, whereas offset data for buffers is contained in the BufferCopy struct.

If we were to change the definition of TextureRegion, we would have to add an extra TextureSlice parameter to every struct and function that uses it. I don't think that is really an improvement.

We could create structs like TransferBufferRegion and BufferRegion that contain a pointer, an offset, and a size. In that case the BufferCopy param would be unnecessary, but then the client would have to duplicate the size fields, and UploadToTexture and DownloadToTexture would still require inputs about stride and pitch. In a way the size duplication would be similar to how CopyTextureToTexture requires that the TextureRegion dimensions match, but I am again not sure if this is a clear improvement.

I don't really see a strong reason for changing the parameter structure. It's a reasonable design as-is and I think this is mostly a matter of personal preference.

@flibitijibibo
Copy link
Collaborator Author

It does seem like it would just add layers of indirection without actually changing how it's called or implemented - @meyraud705, thoughts?

@meyraud705
Copy link

UploadToTexture and UploadToBuffer have similar signature but different call: order of copy parameters for UploadToTexture look reversed if you use compound literal.

SDL_GpuUploadToTexture(copypass,
    transfer_buffer,  // source
    &(SDL_GpuTextureRegion){{texture, mipmap, 0}, // destination
                            0, 0, 0, w, h, 1}, // destination parameters
    &(SDL_GpuBufferImageCopy){0, pitch, s}, // source parameters
    SDL_FALSE);
SDL_GpuUploadToBuffer(copypass,
    transfer_buffer, // source
    buffer, // destination
    &(SDL_GpuBufferCopy){src_offset, // source parameter
                         dst_offset, // destination parameter
                         size}, // global parameter
    SDL_FALSE);

And SDL_GpuDownloadFromTexture() is also different: src, src params, dst, dst params.

By moving TextureSlice out of TextureRegion all functions have same parameter order when calling them: src, dst, src_params, dst_params. I think it doesn't really add a parameter, it flatten them.

SDL_GpuUploadToTexture(copypass,
    transfer_buffer,  // source
    &(SDL_GpuTextureSlice){texture, mipmap, 0}, // destination
    &(SDL_GpuBufferImageCopy){0, pitch, s}, // source parameters
    &(SDL_GpuTextureRegion){0, 0, 0, w, h, 1}, // destination parameters
    SDL_FALSE);

@flibitijibibo
Copy link
Collaborator Author

After trying a few different things there is now #51

@flibitijibibo
Copy link
Collaborator Author

This is now in the latest gpu revision, the result is soooo much nicer to read/write so extra thanks to @meyraud705 for the feedback!

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

No branches or pull requests

3 participants