Replace sampler arrays with constant sampler elements, simplify and reuse code for all shaders

[ChibiDenDen]

This changes simplifies the material_samplers arrays with constant elements and a single shared definition for all shaders.

Benefits:
reduces code duplication
removes sampler arrays in shaders (which are not supported by all devices)

bugsquad edit: fixes: #70013

[clayjohn]

I'm quite concerned about this change. I'm not sure if it is safe to arbitrarily set a binding index so high (will we run into binding slot limits on some devices? Will we create a future headache for other APIs)). Further, I'm not even sure that this is solving a problem. There are no linked bug reports, so it looks like the old way of using a sampler array wasn't causing any issues.

You mention that sampler arrays aren't supported on all devices, could you elaborate on that? Have you tested on a device where master doesn't work, but this PR does? Alternatively, is this a limitation mentioned in the spec, or one we can look at in the GPUInfo database?

[ChibiDenDen]

I'm quite concerned about this change. I'm not sure if it is safe to arbitrarily set a binding index so high (will we run into binding slot limits on some devices? Will we create a future headache for other APIs)). Further, I'm not even sure that this is solving a problem. There are no linked bug reports, so it looks like the old way of using a sampler array wasn't causing any issues.

You mention that sampler arrays aren't supported on all devices, could you elaborate on that? Have you tested on a device where master doesn't work, but this PR does? Alternatively, is this a limitation mentioned in the spec, or one we can look at in the GPUInfo database?

Hey @clayjohn
To answer your questions

according to the API documentation (https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkDescriptorSetLayoutBinding.html#_description)
The API allows using sparse binding numbers but claims that it "MAY" consume additional memory for the unused range.

Alternative options I thought of:

• Create a separate binding set for shared bindings (now it seems like only the samplers are shared by all shaders), but this will require updating the sets for all other bindings (which I have no issue doing if necessary).
• Create the sampler bindings dynamically in shader_compiler.cpp

Im trying to run Godot4 vulkan on low end iPhone (iPhone 6 and iPhone simulator).
While GLES3 works, the performance on iOS is very bad compared to vulkan (which is based on metal), and awful on the simulator which uses software simulation for GLES3 and hardware acceleration for Metal.
Both have some limitations that can be seen here:
https://developer.apple.com/metal/Metal-Feature-Set-Tables.pdf

This changes resolves the limitation under "Array of samplers"

With this change and removing a single line in "vulkan_context.cpp"

	ERR_FAIL_COND_V_MSG(!physical_device_features.imageCubeArray, ERR_CANT_CREATE, "Your GPU doesn't support image cube arrays which are required to use the Vulkan-based renderers in Godot.");

Running 2D projects works on both devices with the vulkan renderer.

I would like to have even simple 3D projects working on the iPhone simulator if possible

I did not open a bug about this but I would gladly open one if necessary.

Regarding other devices which does not support "Array of samplers", I could not find any information, and I suspect that there are no issues with vulkan-native devices.

[ChibiDenDen]

The last update replaces the high binding index with a separate binding set.
The set number is dynamic and done similarly to the render_forward_mobile.cpp MATERIAL_UNIFORM_SET define.

So at the cost of some additional management in every shader that requires the samplers

• adding the define
• maintaining an additional uniform set member variable
• binding the additional unifrom set

I removed the high index values.

I think that in general, using the defines to align the shader binding sets and the code values is a good practice.

[clayjohn]

That is a neat idea. Unfortunately, most android devices only support binding up to 4 sets at once https://vulkan.gpuinfo.org/displaydevicelimit.php?name=maxBoundDescriptorSets

This is why we have kept the number of sets so low in all of our shaders. It looks like iOS supports at least 8, so this approach would work there. But it will break on Android

[ChibiDenDen]

That is a neat idea. Unfortunately, most android devices only support binding up to 4 sets at once https://vulkan.gpuinfo.org/displaydevicelimit.php?name=maxBoundDescriptorSets

This is why we have kept the number of sets so low in all of our shaders. It looks like iOS supports at least 8, so this approach would work there. But it will break on Android

Ohh wow, I did not know theres a limit on that.
Thanks for pointing this out.
I will rethink the approach here.

[ChibiDenDen]

Updated the code to not use an additional binding set, instead using a dynamic index assigned by each shader via a define.
Tested using multiple demo projects ( lights_and_shadows, material_testers, particles, volumetric_fog) to cover the affected shaders, all works similarly with and without this change. (all tested on windows)

[clayjohn]

This looks great now! I just left a small nitpick on one comment. but other than that, everything looks good. I am very happy with this approach now.

I tested locally on a few test scenes I have and on GDQuests TPS demo and I didn't encounter any errors.

This should be good to merge right after we release 4.1 so that it can get lots of testing before 4.2

[YuriSizov]

This file is missing an empty new line at the end.

[YuriSizov]

Oh, I merged this a bit too early by accident. It was still good, aside from a small formatting issue. But more importantly, I forgot to thank you for your contribution because of that. So thanks :)