From a08f61a80c97b040970df51ccb2c043d72666988 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 13 Sep 2026 14:27:09 -0400 Subject: [PATCH] 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 | 115 +++++++++++++++++++--------- core/src/render/primitive.rs | 30 ++++---- core/src/render/shader/prelude.wgsl | 2 +- 3 files changed, 95 insertions(+), 52 deletions(-) diff --git a/core/src/render/mod.rs b/core/src/render/mod.rs index d161560..37e61b5 100644 --- a/core/src/render/mod.rs +++ b/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, + primitives: Vec, texture_pipeline: RenderPipeline, layers: HashMap, @@ -59,6 +57,13 @@ struct RenderLayer { texture_slots: Vec, } +/// 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, @@ -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::() 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 ®istry.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 ®istry.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 { diff --git a/core/src/render/primitive.rs b/core/src/render/primitive.rs index 5cc13c3..048ebea 100644 --- a/core/src/render/primitive.rs +++ b/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::(include_str!("shader/rect.wgsl"), "rect") - .id(), - RECT.id() - ); - assert_eq!( - kinds - .register::(include_str!("shader/glyph.wgsl"), "glyph") - .id(), - GLYPH.id() - ); - kinds + let mut registry = Self { kinds: Vec::new() }; + let rect = registry.register::(include_str!("shader/rect.wgsl"), "rect"); + let glyph = registry.register::(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

{ - self.kinds.push(PrimitiveSource { wgsl, label }); + self.kinds.push(PrimitiveSource { + wgsl, + label, + stride: size_of::

() as u64, + }); PrimitiveKind::new(self.kinds.len() as u32 - 1) } diff --git a/core/src/render/shader/prelude.wgsl b/core/src/render/shader/prelude.wgsl index 67f1e32..766ef9d 100644 --- a/core/src/render/shader/prelude.wgsl +++ b/core/src/render/shader/prelude.wgsl @@ -44,7 +44,7 @@ struct VertexOutput { @location(0) top_left: vec2, @location(1) bot_right: vec2, @location(2) uv: vec2, - @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,