Fix what reviewing the primitive rework turned up

Three defects, two of them invisible to every case I had run.

An image alone in a layer failed validation. The texture pipeline never
bound group 1, and every earlier case happened to have a rect in the
same layer, which left one bound from the primitive draw -- so the bug
was hidden by the tests passing.

A `min_binding_size: None` binding takes its minimum from the first
pipeline built against that bind group layout, so one shared group 1
layout held every primitive to the largest. Rect and glyph coexisted
only because glyph is the bigger of the two; the texture slots, at four
bytes, did not. Each primitive now gets its own layout with its entry
size stated, which is also why `PrimitiveRegistry` records the stride.
Because the pipeline layouts now differ per primitive, a pipeline change
drops the bound groups, so group 2 moves after `set_pipeline`.

An empty list still built a bind group over a buffer too small for one
entry, which the stated minimum would now reject. It gets no bind group,
and nothing draws it.

Also from the read-through: `UiRenderNode` kept a `Device` beside the
one `update` is handed, `PrimitiveRegistry::default` registered inside
an `assert_eq!`, and `mask_idx` was an unqualified integer varying where
`idx` beside it was `flat`.

core/src/render/mod.rs

@@ -29,17 +29,15 @@ const PRELUDE: &str = include_str!("./shader/prelude.wgsl");
 const TEXTURE_SHADER: &str = include_str!("./shader/texture.wgsl");
 
 pub struct UiRenderNode {
-    device: Device,
     shared_layout: BindGroupLayout,
     shared_group: BindGroup,
-    data_layout: BindGroupLayout,
+    texture_data_layout: BindGroupLayout,
     texture_layout: BindGroupLayout,
-    pipeline_layout: PipelineLayout,
     format: TextureFormat,
 
     /// One per registered primitive, in id order. The texture pipeline is
     /// apart because a texture binds group 2 per instance, not per draw.
-    pipelines: Vec<RenderPipeline>,
+    primitives: Vec<PrimitivePipeline>,
     texture_pipeline: RenderPipeline,
 
     layers: HashMap<usize, RenderLayer>,
@@ -59,6 +57,13 @@ struct RenderLayer {
     texture_slots: Vec<u32>,
 }
 
+/// What draws one registered primitive. The group 1 layout is its own rather
+/// than shared, so its entry size is the minimum only for it.
+struct PrimitivePipeline {
+    data_layout: BindGroupLayout,
+    pipeline: RenderPipeline,
+}
+
 /// One list's vertex buffer and the data its shader reads at group 1.
 struct ListBuffers {
     instance: ArrBuf<PrimitiveInstance>,
@@ -74,18 +79,26 @@ impl UiRenderNode {
             // Types run in registration order, so a rect is under a glyph is
             // under a texture. Ordering beyond that is what `Layers` is for --
             // freeing an instance swaps another into its place.
-            pass.set_bind_group(2, self.pages.group(), &[]);
             for (id, list) in layer.primitives.iter().enumerate() {
-                let (Some(group), true) = (&list.group, list.instance.len() > 0) else {
+                let Some(group) = &list.group else {
                     continue;
                 };
-                pass.set_pipeline(&self.pipelines[id]);
+                pass.set_pipeline(&self.primitives[id].pipeline);
+                // Both after the pipeline: each primitive has its own pipeline
+                // layout, and a change drops the groups from where they differ.
                 pass.set_bind_group(1, group, &[]);
+                pass.set_bind_group(2, self.pages.group(), &[]);
                 pass.set_vertex_buffer(0, list.instance.buffer.slice(..));
                 pass.draw(0..4, 0..list.instance.len() as u32);
             }
             if !layer.texture_slots.is_empty() {
+                // Group 1 too, unread as it is: a layer holding only an image
+                // never ran the loop above, so nothing is bound there.
+                let Some(data) = &layer.textures.group else {
+                    continue;
+                };
                 pass.set_pipeline(&self.texture_pipeline);
+                pass.set_bind_group(1, data, &[]);
                 pass.set_vertex_buffer(0, layer.textures.instance.buffer.slice(..));
                 for (i, &slot) in layer.texture_slots.iter().enumerate() {
                     let Some(group) = self.textures.group(slot) else {
@@ -126,12 +139,17 @@ impl UiRenderNode {
                 rlayer
                     .primitives
                     .resize_with(draws.primitives().len(), || ListBuffers::new(device));
-                for (list, draws) in rlayer.primitives.iter_mut().zip(draws.primitives()) {
-                    list.update(device, queue, &self.data_layout, draws);
+                for ((list, draws), pipeline) in rlayer
+                    .primitives
+                    .iter_mut()
+                    .zip(draws.primitives())
+                    .zip(&self.primitives)
+                {
+                    list.update(device, queue, &pipeline.data_layout, draws);
                 }
                 rlayer
                     .textures
-                    .update(device, queue, &self.data_layout, &draws.textures);
+                    .update(device, queue, &self.texture_data_layout, &draws.textures);
                 rlayer.texture_slots.clear();
                 // Read rather than cast: the payload is a byte vec, so it
                 // carries no alignment a `u32` slice could borrow.
@@ -142,7 +160,7 @@ impl UiRenderNode {
                 draws.updated = false;
             }
         }
-        self.build_pipelines(&ui.primitives);
+        self.build_pipelines(device, &ui.primitives);
         if ui.masks.changed {
             ui.masks.changed = false;
             if self.masks.update(device, queue, &ui.masks[..]) {
@@ -181,7 +199,6 @@ impl UiRenderNode {
         });
 
         let shared_layout = Self::shared_layout(device);
-        let data_layout = Self::data_layout(device);
         let texture_layout = Self::texture_layout(device);
 
         let masks = ArrBuf::new(
@@ -195,30 +212,29 @@ impl UiRenderNode {
         let pages = GpuPages::new(device, queue, &texture_layout, &sampler);
         let textures = GpuTextures::new(device, queue);
 
-        // One layout for every pipeline, so a primitive that does not sample
-        // or read a buffer binds what is there instead of needing its own.
-        let pipeline_layout = device.create_pipeline_layout(&PipelineLayoutDescriptor {
-            label: Some("ui"),
-            bind_group_layouts: &[&shared_layout, &data_layout, &texture_layout],
-            immediate_size: 0,
-        });
+        // A texture instance's data is the slot it binds, which its shader
+        // never reads -- but group 1 is in the layout, so it is bound anyway.
+        let texture_data_layout = Self::data_layout(device, size_of::<u32>() as u64);
         let texture_pipeline = Self::pipeline(
             device,
-            &pipeline_layout,
+            &Self::pipeline_layout(
+                device,
+                &shared_layout,
+                &texture_data_layout,
+                &texture_layout,
+            ),
             config.format,
             TEXTURE_SHADER,
             "texture",
         );
 
         Self {
-            device: device.clone(),
             shared_layout,
             shared_group,
-            data_layout,
+            texture_data_layout,
             texture_layout,
-            pipeline_layout,
             format: config.format,
-            pipelines: Vec::new(),
+            primitives: Vec::new(),
             texture_pipeline,
             window_buffer,
             layers: HashMap::default(),
@@ -232,18 +248,36 @@ impl UiRenderNode {
 
     /// Compiles a pipeline for every primitive registered since the last call.
     /// Sources only ever arrive at the end, so an id keeps its pipeline.
-    fn build_pipelines(&mut self, registry: &PrimitiveRegistry) {
-        for source in &registry.sources()[self.pipelines.len()..] {
-            self.pipelines.push(Self::pipeline(
-                &self.device,
-                &self.pipeline_layout,
-                self.format,
-                source.wgsl,
-                source.label,
-            ));
+    fn build_pipelines(&mut self, device: &Device, registry: &PrimitiveRegistry) {
+        for source in &registry.sources()[self.primitives.len()..] {
+            let data_layout = Self::data_layout(device, source.stride);
+            let layout = Self::pipeline_layout(
+                device,
+                &self.shared_layout,
+                &data_layout,
+                &self.texture_layout,
+            );
+            let pipeline = Self::pipeline(device, &layout, self.format, source.wgsl, source.label);
+            self.primitives.push(PrimitivePipeline {
+                data_layout,
+                pipeline,
+            });
         }
     }
 
+    fn pipeline_layout(
+        device: &Device,
+        shared: &BindGroupLayout,
+        data: &BindGroupLayout,
+        texture: &BindGroupLayout,
+    ) -> PipelineLayout {
+        device.create_pipeline_layout(&PipelineLayoutDescriptor {
+            label: Some("ui"),
+            bind_group_layouts: &[shared, data, texture],
+            immediate_size: 0,
+        })
+    }
+
     fn pipeline(
         device: &Device,
         layout: &PipelineLayout,
@@ -346,7 +380,11 @@ impl UiRenderNode {
     }
 
     /// Group 1: one list's per-instance data, whatever its shader reads it as.
-    fn data_layout(device: &Device) -> BindGroupLayout {
+    ///
+    /// One per primitive with `stride` stated, because a `None` minimum is
+    /// filled in from the first pipeline built against the layout -- sharing
+    /// one would hold every primitive to the largest.
+    fn data_layout(device: &Device, stride: u64) -> BindGroupLayout {
         device.create_bind_group_layout(&BindGroupLayoutDescriptor {
             entries: &[BindGroupLayoutEntry {
                 binding: 0,
@@ -354,7 +392,7 @@ impl UiRenderNode {
                 ty: BindingType::Buffer {
                     ty: BufferBindingType::Storage { read_only: true },
                     has_dynamic_offset: false,
-                    min_binding_size: None,
+                    min_binding_size: std::num::NonZero::new(stride),
                 },
                 count: None,
             }],
@@ -428,7 +466,12 @@ impl ListBuffers {
         list: &InstanceList,
     ) {
         self.instance.update(device, queue, list.instances());
-        if self.data.update(device, queue, list.data()) || self.group.is_none() {
+        let resized = self.data.update(device, queue, list.data());
+        // An empty list has no buffer big enough for one entry, and nothing
+        // draws it, so it has no bind group either.
+        if list.instances().is_empty() {
+            self.group = None;
+        } else if resized || self.group.is_none() {
             self.group = Some(device.create_bind_group(&BindGroupDescriptor {
                 layout,
                 entries: &[BindGroupEntry {

core/src/render/primitive.rs

@@ -51,24 +51,20 @@ pub struct PrimitiveRegistry {
 pub struct PrimitiveSource {
     pub wgsl: &'static str,
     pub label: &'static str,
+    /// Size of one instance's entry, which the renderer states as the group 1
+    /// binding's minimum rather than leaving it to be inferred.
+    pub stride: u64,
 }
 
 impl Default for PrimitiveRegistry {
     fn default() -> Self {
-        let mut kinds = Self { kinds: Vec::new() };
-        assert_eq!(
-            kinds
-                .register::<RectPrimitive>(include_str!("shader/rect.wgsl"), "rect")
-                .id(),
-            RECT.id()
-        );
-        assert_eq!(
-            kinds
-                .register::<GlyphPrimitive>(include_str!("shader/glyph.wgsl"), "glyph")
-                .id(),
-            GLYPH.id()
-        );
-        kinds
+        let mut registry = Self { kinds: Vec::new() };
+        let rect = registry.register::<RectPrimitive>(include_str!("shader/rect.wgsl"), "rect");
+        let glyph = registry.register::<GlyphPrimitive>(include_str!("shader/glyph.wgsl"), "glyph");
+        // The built-ins have constant ids so a widget can name one without the
+        // registry; registering them first is what makes those constants true.
+        assert_eq!((rect.id(), glyph.id()), (RECT.id(), GLYPH.id()));
+        registry
     }
 }
 
@@ -78,7 +74,11 @@ impl PrimitiveRegistry {
         wgsl: &'static str,
         label: &'static str,
     ) -> PrimitiveKind<P> {
-        self.kinds.push(PrimitiveSource { wgsl, label });
+        self.kinds.push(PrimitiveSource {
+            wgsl,
+            label,
+            stride: size_of::<P>() as u64,
+        });
         PrimitiveKind::new(self.kinds.len() as u32 - 1)
     }
 

core/src/render/shader/prelude.wgsl

@@ -44,7 +44,7 @@ struct VertexOutput {
     @location(0) top_left: vec2<f32>,
     @location(1) bot_right: vec2<f32>,
     @location(2) uv: vec2<f32>,
-    @location(3) mask_idx: u32,
+    @location(3) @interpolate(flat) mask_idx: u32,
     // The instance's own index, which is also where its data sits in group 1.
     @location(4) @interpolate(flat) idx: u32,
     @builtin(position) clip_position: vec4<f32>,