[PATCH vkd3d 1/2] vkd3d-shader/hlsl: Separate texture_dim and sampler_dim, add support for texture arrays.

[Giovanni Mascellani]

Hi,

On 03/12/21 18:57, Francisco Casas wrote:
> This patch is a proposal for considering separating texture_dim and
> sampler_dim, it probably will be resent after Matteo's batch is
> reviewed.
> Opinions are appreciated.
> 
> So far, the
> 
> enum hlsl_sampler_dim sampler_dim;
> 
> member of struct hlsl_type has been used to store the dimension type
> of both HLSL_TYPE_SAMPLER and HLSL_TYPE_TEXTURE types.
> 
> However, textures have more possible options, e.g. Texture1DArray and
> Texture2DArray, which are ld and sampled with vectors of 2 and 3
> dimensions respectively. Support for these is added in this patch.
> 
> The new enum hlsl_texture_dim is added to include these cases and the
> 
> enum hlsl_texture_dim texture_dim;
> 
> member is added to struct hlsl_type to store the dimension type of
> HLSL_TYPE_TEXTURE types.
> 
> Nevertheless, sampler_dim is still used on HLSL_TYPE_TEXTURE types to
> know the compatible types for sampling (albeit this could be deducted
> from texture_dim).
> 
> Future support for Texture2DMS and Texture2DMSArray would be affected
> if this strategy is chosen.
> 
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
As a general suggestion, I think the commit message should be already in 
the version that would be committed if the commit were to be accepted. 
I.e., just describe what the patch does and why, not the various other 
process issues like the fact that it will be submitted again after 
another patch set or what does change if one or another strategy is chosen.

If you want to add this kind of content, that in this case and in many 
other is useful and important, separate it from the rest in this way:

"""
foo/bar: Add baz field.

The baz field can be used by downstream consumers to ensure that the 
frobnicator is correctly spingled.

Signed-off-by: John Doe <jdoe at example.com>
---
I think this will be useful when we'll be implementing droobster 
simplification, becase yada yada yada.
"""

> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index e7bdb45e..9d73d979 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -101,6 +101,17 @@ enum hlsl_sampler_dim
>      HLSL_SAMPLER_DIM_MAX = HLSL_SAMPLER_DIM_CUBE
>   };
>   
> +enum hlsl_texture_dim
> +{
> +   HLSL_TEXTURE_DIM_GENERIC = HLSL_SAMPLER_DIM_GENERIC,
> +   HLSL_TEXTURE_DIM_1D      = HLSL_SAMPLER_DIM_1D,
> +   HLSL_TEXTURE_DIM_2D      = HLSL_SAMPLER_DIM_2D,
> +   HLSL_TEXTURE_DIM_3D      = HLSL_SAMPLER_DIM_3D,
> +   HLSL_TEXTURE_DIM_CUBE    = HLSL_SAMPLER_DIM_CUBE,
> +   HLSL_TEXTURE_DIM_1DARRAY,
> +   HLSL_TEXTURE_DIM_2DARRAY,
> +};

Thinking again about this and looking at the patches I have in my queue, 
I now think this looks a bit too much of a duplication. I'd rather unify 
the two enums in a singled "agnostic" one along these lines:

enum hlsl_dimension
{
     HLSL_DIM_GENERIC,
     HLSL_DIM_1D,
     HLSL_DIM_2D,
     HLSL_DIM_3D,
     HLSL_DIM_CUBE,
     HLSL_DIM_MAX_SAMPLER = HLSL_DIM_CUBE,
     HLSL_DIM_1DARRAY,
     HLSL_DIM_2DARRAY,
     HLSL_DIM_2DMS,
     HLSL_DIM_MAX = HLSL_DIM_2DMS,
};

The value HLSL_DIM_MAX_SAMPLER can be used to check whether a dimension 
value is valid in the context of a sampler dimension.

>   enum hlsl_matrix_majority
>   {
>       HLSL_COLUMN_MAJOR,
> @@ -114,6 +125,7 @@ struct hlsl_type
>       enum hlsl_type_class type;
>       enum hlsl_base_type base_type;
>       enum hlsl_sampler_dim sampler_dim;
> +    enum hlsl_texture_dim texture_dim;

Consequently, we'd also have just one field of type hlsl_dimension. As 
for the name, it might be sensible to avoid "dim" or "dimension", which 
might get confused with the fields in hlsl_type. Possibly 
"resource_dim"? I am not sure.

My two cents.

Giovanni.