Address review comments

filament/src/Engine.cpp

@@ -468,7 +468,7 @@ Handle<HwProgram> FEngine::createPostProcessProgram(MaterialParser& parser,
                 map.getSamplerBinding(bindingPoint, (uint8_t)i, &binding);
                 samplers[i] = { uniformName, binding };
             }
-            pb.addSamplerGroup(bindingPoint, samplers.data(), samplers.size());
+            pb.setSamplerGroup(bindingPoint, samplers.data(), samplers.size());
         }
     };
 

filament/src/Material.cpp

@@ -337,7 +337,7 @@ Handle<HwProgram> FMaterial::getProgramSlow(uint8_t variantKey) const noexcept {
                 map.getSamplerBinding(bindingPoint, (uint8_t)i, &binding);
                 samplers[i] = { uniformName, binding };
             }
-            pb.addSamplerGroup(bindingPoint, samplers.data(), samplers.size());
+            pb.setSamplerGroup(bindingPoint, samplers.data(), samplers.size());
         }
     };
 

filament/src/driver/Program.cpp

@@ -53,7 +53,8 @@ Program& Program::addUniformBlock(size_t index, const UniformInterfaceBlock* ib)
     return *this;
 }
 
-Program& Program::addSamplerGroup(size_t bindingPoint, const Program::Sampler* samplers, size_t count) {
+Program& Program::setSamplerGroup(size_t bindingPoint,
+        const Program::Sampler* samplers, size_t count) {
     auto& samplerList = mSamplerGroups[bindingPoint];
     samplerList.clear();
     samplerList.insert(samplerList.begin(), samplers, samplers + count);

filament/src/driver/Program.h

@@ -67,10 +67,11 @@ public:
     // The lifetime of UniformInterfaceBlock* must be longer than Program's
     Program& addUniformBlock(size_t index, const UniformInterfaceBlock* ib);
 
-    // sets the 'index' sampler group descriptor for this program.
+    // sets the 'bindingPoint' sampler group descriptor for this program.
     // 'samplers' can be destroyed after this call.
-    // This effectively associates a set of (BindingPoints, index) to a (shader-binding)
-    Program& addSamplerGroup(size_t bindingPoint, Sampler const* samplers, size_t count);
+    // This effectively associates a set of (BindingPoints, index) to a texture unit in the shader.
+    // Or more precisely, what layout(binding=) is set to in GLSL.
+    Program& setSamplerGroup(size_t bindingPoint, Sampler const* samplers, size_t count);
 
     Program& withVertexShader(void const* data, size_t size) {
         return shader(Shader::VERTEX, data, size);

filament/src/driver/metal/MetalDriver.mm

@@ -758,7 +758,7 @@ void MetalDriver::enumerateSamplerGroups(
                 continue;
             }
 
-            if (!program->samplerBindings[bufferIdx].empty()) {
+            if (samplerIdx < program->samplerBindings[bufferIdx].size()) {
                 uint8_t binding = (uint8_t)program->samplerBindings[bufferIdx][samplerIdx].binding;
                 f(sampler, binding);
             }

filament/src/driver/vulkan/VulkanDriver.cpp

@@ -963,10 +963,9 @@ void VulkanDriver::draw(Driver::PipelineState pipelineState, Driver::RenderPrimi
             // Obtain the global sampler binding index and pass this to VulkanBinder. Note that
             // "binding" is an offset that is global to the shader, whereas "samplerIndex" is an
             // offset into the virtual sampler buffer.
-            uint8_t binding;
 
-            if (!program->samplerBindings[bufferIdx].empty()) {
-                binding = (uint8_t)program->samplerBindings[bufferIdx][samplerIndex].binding;
+            if (samplerIndex < program->samplerBindings[bufferIdx].size()) {
+                uint8_t binding = (uint8_t)program->samplerBindings[bufferIdx][samplerIndex].binding;
                 const SamplerParams& samplerParams = sampler->s;
                 VkSampler vksampler = mSamplerCache.getSampler(samplerParams);
                 const auto* tex = handle_const_cast<VulkanTexture>(mHandleMap, sampler->t);

libs/filabridge/include/private/filament/SamplerBindingMap.h

@@ -53,8 +53,8 @@ public:
             const SamplerInterfaceBlock* perMaterialSib = nullptr,
             const char* materialName = nullptr);
 
-    // Given a valid Filament binding point and an offset with the block, returns true and sets
-    // the output arguments: (1) the globally unique binding index, and (2) the grouping index.
+    // Given a valid Filament binding point and an offset within the block, returns true and sets
+    // the output argument 'globalOffset' to the globally unique binding index.
     bool getSamplerBinding(uint8_t blockIndex, uint8_t localOffset, uint8_t* globalOffset) const {
         assert(globalOffset);
         auto iter = mBindingMap.find(getBindingKey(blockIndex, localOffset));

libs/filabridge/include/private/filament/SibGenerator.h

@@ -56,13 +56,12 @@ struct PostProcessSib {
 inline constexpr uint8_t getSamplerBindingsStart(driver::Backend api) noexcept {
     switch (api) {
         default:
-        case driver::Backend::OPENGL:
-        case driver::Backend::VULKAN: {
-            // Vulkan and OpenGL have a single namespace for uniforms and samplers.
+        case driver::Backend::VULKAN:
+            // The Vulkan backend has single namespace for uniforms and samplers.
             // To avoid collision, the sampler bindings start after the last UBO binding.
             return filament::BindingPoints::COUNT;
-        }
 
+        case driver::Backend::OPENGL:
         case driver::Backend::METAL:
             // Metal has a separate namespace for uniforms and samplers- collisions aren't an issue.
             return 0;