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

[Francisco Casas]

> 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.
> """

Okay, thanks for the tip!


>> 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.

Okay, I will work on the same patch but using this strategy.


>> 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.

I like the name change to avoid regressions, it forces to pay attention to all related parts of the code.

The only options for names I can think of are "resource_arrangement" (resource_argmt) or "resource_layout".
Probably "resource_dim" is better.

Best regards,
Francisco