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

Improve clarity of TextureCreateInfo #67

Closed
TheSpydog opened this issue Jul 2, 2024 · 4 comments
Closed

Improve clarity of TextureCreateInfo #67

TheSpydog opened this issue Jul 2, 2024 · 4 comments
Assignees

Comments

@TheSpydog
Copy link
Collaborator

While the current SDL_GpuTextureCreateInfo struct design gets the job done, I think it could be improved for usability and clarity. What we have currently is this:

typedef struct SDL_GpuTextureCreateInfo
{
    Uint32 width;
    Uint32 height;
    Uint32 depth;
    SDL_bool isCube;
    Uint32 layerCount;
    Uint32 levelCount;
    SDL_GpuSampleCount sampleCount;
    SDL_GpuTextureFormat format;
    SDL_GpuTextureUsageFlags usageFlags;
} SDL_GpuTextureCreateInfo;

This is a collection of texture properties that covers all possible scenarios (depth, 2D, arrays, etc.), but it's not clear which properties are compatible with one another, or what the "default" values should be. It raises questions like:

  • If you're creating a 2D texture, what should the depth value be? 0? 1?
  • If you're creating a cube texture, what should the layer count be? 0? 1? 6? Are cube layers the same as texture array layers? Can you have a texture cube array?
  • If you create a cube texture with a sample count > 1 is that valid? What about a depth value of > 1, or mismatched width/height?
  • If you create a 3D texture with a sample count > 1 is that valid? What about a layer count > 1? Are 3D depth slices the same as layers?
  • If you create a multisample 2D texture with a level count > 1 is that valid?

Having this many individual, incompatible settings all presented in one grab-bag is clearly less than ideal. A potentially better option would be to follow the D3D11_VIEW_DESC approach, where we have a tagged union of texture types that only contain the options actually applicable for that type. Something like:

typedef struct SDL_GpuTextureCreateInfo
{
	SDL_GpuTextureType type; // 2D, 2D_MULTISAMPLE, 2D_ARRAY, 3D, CUBE
	SDL_GpuTextureFormat format;
	SDL_GpuTextureUsageFlags usageFlags;
	union
	{
		struct Texture2D
		{
			Uint32 width;
			Uint32 height;
			Uint32 levelCount;
		};
		struct Texture2DMultisample
		{
			Uint32 width;
			Uint32 height;
			Uint32 sampleCount;
		};
		struct Texture2DArray
		{
			Uint32 width;
			Uint32 height;
			Uint32 levelCount;
			Uint32 layerCount; // could also rename to arraySize or something for clarity?
		};
		struct Texture3D
		{
			Uint32 width;
			Uint32 height;
			Uint32 depth;
			Uint32 levelCount;
		};
		struct TextureCube
		{
			Uint32 size;
			Uint32 levelCount;
		};
	};
} SDL_GpuTextureCreateInfo;

This isn't 100% perfect since you can still have incompatible type/format/usage pairings, but I think it substantially lightens the mental load of having to remember all the rules about which properties work with which types.

@flibitijibibo
Copy link
Collaborator

I moved this to Ship but probably shouldn't have since it could possibly change the ABI. With the texture rework and the blit stuff it may not be as scary as it was when we first filed it?

@thatcosmonaut
Copy link
Owner

thatcosmonaut commented Aug 18, 2024

I'm not thrilled about unions in the header in the first place, and I think that our recent texture creation rework reduced the need for this a lot. I think once we consolidate layer and depth into one field I would be pretty happy with where this is. Willing to discuss if others feel strongly about it.

@TheSpydog
Copy link
Collaborator Author

I agree, once layer and depth are consolidated I think that's sufficient.

@flibitijibibo
Copy link
Collaborator

Resolved by #205

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