It is not clear to me why you have to break the eGPUSamplerState enum into smaller ones. I don't think being able to reuse the enums in nodes is enough to justify this. To me it just adds complexity.
I don't really like ImageTextureExtensionType being a DNA member referenced in GPU . The name makes it confusing. Maybe there should be a corresponding eGPUSamplerExtensionType, but again I don't find this.

If you would introduce repeat mirror extent mode, you could just add a new bit to the eGPUSamplerState enum that modifies the behavior of the extent flag just like GPU_SAMPLER_CLAMP_BORDER changes the clamp behavior.
But this would prevent having one dimension being repeat normal and the other repeat mirror. So I can see the benefit of your approach. However, there is still the concern that it creates a lot of samplers variations which can become a problem.

Another approach would be to have a separate Texture sampler list that does not include all the variations we dont need. I would keep eGPUSamplerState as the only state definition for API simplicity. Maybe you could have something like this:

enum eGPUSamplerState {
   /* General Purpose States */
   ...
   /* Image samplers
     * These samplers do not have GPU_SAMPLER_COMPARE and GPU_SAMPLER_ANISO usage is automatic.
     */
   GPU_SAMPLER_IMAGE_WRAP_S_REPEAT = (1 << 16)
   GPU_SAMPLER_IMAGE_WRAP_S_REPEAT_MIRROR = (2 << 16)
   ....
};

General purpose GPU_SAMPLER_* would be incompatible to mix with the GPU_SAMPLER_IMAGE_*. In the bind function, you could then use the image sampler array if sampler_state > 0xFFFF.

I would prefer to have the GPU changes (and their side effects) in another patch.

@Clément Foucault (fclem) First of all, thank you for your time.

In D14237#380386, @Clément Foucault (fclem) wrote:

It is not clear to me why you have to break the eGPUSamplerState enum into smaller ones. I don't think being able to reuse the enums in nodes is enough to justify this. To me it just adds complexity.

The reason this is because the extension types are mutually exclusive. I don't think of GPU_SAMPLER_CLAMP_BORDER as changing the clamp behavior. I think of it as one of (currently) three separate modes. This is how it is portrayed in the various graphics library specifications.
I find it is easier to assign the settings if they are separated like this. Mapping the settings to a sampler index is also easier for me to comprehend and I was able to reduce the total number of samplers.

The eGPUSamplerState enum had me asking questions like:
What does GPU_SAMPLER_CLAMP_BORDER bit flag do when the extension type is set to repeat?
What do all of the bits mean when the GPU_SAMPLER_ICON flag is set?

I had to look deeper into the code to figure out which setting overrides another.

The method GPU_texture_wrap_mode was quite confusing to me. Not just for the questions raised above but also because the name use_clamp does not indicate if the sampler will use clamp to border or clamp to edge.

I don't really like ImageTextureExtensionType being a DNA member referenced in GPU . The name makes it confusing. Maybe there should be a corresponding eGPUSamplerExtensionType

I can agree to this. I couldn't quite make up my mind on what to do, but using the same enum isn't worth the confusion. Having a separate enum for this would Indicate the context change better. If you recall, the old texture system had extension settings (checker for example) that do not correspond to OpenGL or other graphics libraries' extension settings. I don't know if something like that would ever be added to the image texture nodes, but regardless it makes sense to have separate enums for the higher level extension settings and lower level graphics library compatible extension settings.

If you would introduce repeat mirror extent mode, you could just add a new bit to the eGPUSamplerState enum that modifies the behavior of the extent flag just like GPU_SAMPLER_CLAMP_BORDER changes the clamp behavior.
But this would prevent having one dimension being repeat normal and the other repeat mirror. So I can see the benefit of your approach.

Yeah, that's basically it. In my opinion, adding these features would overcomplicate the eGPUSamplerState enum, and that is why I chose to break it apart.

However, there is still the concern that it creates a lot of samplers variations which can become a problem.

Perhaps I am wrong about this, but in the current implementation we are initializing 257 samplers.
In my implementation, we initialize 145 samplers.
If we add Mirrored Repeat to my implementation we would be initializing 257 samplers.
It looks like the samplers are only bound as needed and the array to store bound samplers can store 64 unique samplers.

Another approach would be to have a separate Texture sampler list that does not include all the variations we don't need. I would keep eGPUSamplerState as the only state definition for API simplicity. Maybe you could have something like this:

enum eGPUSamplerState {
   /* General Purpose States */
   ...
   /* Image samplers
     * These samplers do not have GPU_SAMPLER_COMPARE and GPU_SAMPLER_ANISO usage is automatic.
     */
   GPU_SAMPLER_IMAGE_WRAP_S_REPEAT = (1 << 16)
   GPU_SAMPLER_IMAGE_WRAP_S_REPEAT_MIRROR = (2 << 16)
   ....
};

General purpose GPU_SAMPLER_* would be incompatible to mix with the GPU_SAMPLER_IMAGE_*. In the bind function, you could then use the image sampler array if sampler_state > 0xFFFF.

I'm not exactly sure what you mean by General Purpose States. How many samplers would be included? Is a General Purpose State made of a combination of flags, or is it more like the GPU_SAMPLER_ICON type that I called a Custom Sampler.
I don't favor this approach for a couple of reasons:
It doesn't distinguish the mutually exclusive settings from the other settings.
It seems to be more complicated, but that could be because I don't know what the General Purpose States would encompass.

In my mind, having more variables carry the state is simpler because each component is very simple. The flags enum is used only as flags and every paramutation of bits should be a valid set of flags. Having one variable to carry the state that is split in such a way seems more complicated and less clear.

If GPU_SAMPLER_COMPARE or non-ANISO samplers are not used much, then it would be better just to make a few Custom Samplers via my implementation.
This way the programmer always knows where to look. If they need a GPU_SAMPLER_COMPARE, they can find it in the Custom Samplers enum list or define a new Custom Sampler.

From what I saw, there appears to be only one unique sampler state that has compare mode enabled.