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