From 477459e08759abbe735b5b8805efe3c02a7815f9 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 07:27:17 -0400 Subject: [PATCH 1/2] test(#739,#741): device-free routing coverage for the zone pass and the character shadow sub-passes Both are routing-coverage gaps in pass.rs, behind the same wall: encode_zone_pass and encode_shadow_pass take &EqRenderer + &mut wgpu::CommandEncoder, neither of which a test can build, so every sub-pass decision inside them was unreachable from the suite. Applies the #721/#737/#740 shape to both: a pure planner over a device-free trait, a sink trait holding the wgpu handles, and an executor that production calls. Each new test file transcribes the pre-refactor loops verbatim at 3d60bfd and asserts the real executor emits an identical command stream, so the "no rendering behaviour change" claim is discharged differentially rather than asserted. #739 asked whether the character sub-passes share #721's vocabulary. They do not: #721 picks its pipeline from a mesh RenderMode and binds a diffuse texture at group 1; these pick from whether a model is skinned and bind pool slots, with group 2 for skinned casters only. Separate planners, with the comparison written down at the code. CharacterShadowBind::Skinned carries the joint slot and ::Static does not, so a static caster carrying a joint palette is unrepresentable rather than merely untested. What that does NOT prevent is stated next to it. No source-text pins: routing is observable, so both files assert which path was taken. The two pipeline lookups inside the sink impls are consequently NOT covered, and each file says so. Ten mutations, both arms measured. Nine turn the relevant file red. The tenth (a `*started &&` guard on the zone texture cache) stayed green because the scan seed is re-evaluated per sub-pass and the guard was unreachable; it is deleted here, and the limit is recorded at the test that relies on the boundary reset. Closes #739 Closes #741 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-renderer/src/pass.rs | 580 +++++++++++++++--- .../tests/character_shadow_routing.rs | 269 ++++++++ .../tests/zone_pass_routing.rs | 340 ++++++++++ 3 files changed, 1087 insertions(+), 102 deletions(-) create mode 100644 crates/eqoxide-renderer/tests/character_shadow_routing.rs create mode 100644 crates/eqoxide-renderer/tests/zone_pass_routing.rs diff --git a/crates/eqoxide-renderer/src/pass.rs b/crates/eqoxide-renderer/src/pass.rs index aa861d1e..cb9cd132 100644 --- a/crates/eqoxide-renderer/src/pass.rs +++ b/crates/eqoxide-renderer/src/pass.rs @@ -223,6 +223,143 @@ pub fn execute_instanced_shadow_plan CharacterShadowKind { + match self { + Self::Skinned { .. } => CharacterShadowKind::Skinned, + Self::Static { .. } => CharacterShadowKind::Static, + } + } +} + +/// One planned character shadow draw. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CharacterShadowStep { + /// Index into the caster slice the plan was built from. + pub caster: usize, + /// True on the first step of each non-empty sub-pass: set the pipeline and bind group 0. + pub set_pipeline: bool, + pub bind: CharacterShadowBind, +} + +/// The device-free slice of a selected shadow caster the ROUTING reads. The real caster owns a +/// `&GpuSkinnedModel`/`&GpuStaticModel` (both unconstructible in a test — they own `wgpu::Buffer`s); +/// routing only ever asks which pipeline draws it and which pool slots it binds. +pub trait CharacterShadowCaster { + fn shadow_bind(&self) -> CharacterShadowBind; +} + +/// Plans every character shadow draw for one frame: which sub-passes run, in which order, which +/// caster is in which, and what each binds. +/// +/// Returns a **lazy iterator** — no allocation — because `encode_shadow_pass` runs every frame. +/// Steps come out in exactly the order `encode_shadow_pass` issues them, so the sub-pass split is +/// observable: a caster's position in this stream is its position in its sub-pass, not its position +/// in `casters`. +pub fn plan_character_shadow_draws( + casters: &[C], +) -> impl Iterator + '_ { + CHARACTER_SHADOW_SUBPASSES.into_iter().flat_map(move |kind| { + casters + .iter() + .enumerate() + .filter(move |(_, c)| c.shadow_bind().kind() == kind) + .scan(false, move |started, (caster, c)| { + let set_pipeline = !*started; + *started = true; + Some(CharacterShadowStep { caster, set_pipeline, bind: c.shadow_bind() }) + }) + }) +} + +/// The device-bound half of a character shadow draw, expressed entirely in **plan vocabulary** — +/// a [`CharacterShadowKind`] and pool slot *indices*, never `wgpu` handles. +/// +/// `encode_shadow_pass` supplies an impl closing over the renderer and the live `wgpu::RenderPass`; +/// `tests/character_shadow_routing.rs` supplies one that appends to a `Vec`. Both run the *same* +/// executor. +pub trait CharacterShadowSink { + /// Start a sub-pass: select the pipeline this kind names. + fn set_pipeline(&mut self, kind: CharacterShadowKind); + /// Bind group 0 (the shadow light's view-projection uniform). + fn bind_light_depth(&mut self); + /// Bind group 1 to `shadow_uniform_pool[u_slot]` (this caster's model matrix). + fn bind_model_uniform(&mut self, u_slot: usize); + /// Bind group 2 to `shadow_joint_pool[j_slot]`. Called for skinned casters only. + fn bind_joints(&mut self, j_slot: usize); + /// Issue the draws for `casters[caster]` (one per mesh of its model). + fn draw(&mut self, caster: usize); +} + +/// Turns a [`plan_character_shadow_draws`] plan into sink calls. **This is the real executor** — +/// `encode_shadow_pass` calls exactly this function, so the command sequence it emits is graded +/// device-free in `tests/character_shadow_routing.rs` against a transcription of the pre-#739 loops. +pub fn execute_character_shadow_plan( + casters: &[C], + sink: &mut S, +) { + for step in plan_character_shadow_draws(casters) { + if step.set_pipeline { + sink.set_pipeline(step.bind.kind()); + sink.bind_light_depth(); + } + match step.bind { + CharacterShadowBind::Skinned { u_slot, j_slot } => { + sink.bind_model_uniform(u_slot); + sink.bind_joints(j_slot); + } + CharacterShadowBind::Static { u_slot } => { + sink.bind_model_uniform(u_slot); + } + } + sink.draw(step.caster); + } +} + /// Entity draw distance (EQ units, measured from the player). Beyond this an NPC's 3D /// model is not drawn — it's a distant speck. Combined with a frustum test, this caps /// the per-frame entity work in densely-populated zones (e.g. gfaydark, ~400 spawns). @@ -622,6 +759,229 @@ pub fn encode_sky_pass( pass.draw(0..6, 0..1); } +// ── Zone colour pass ROUTING (#741) ────────────────────────────────────────────────────────────── +// +// `encode_zone_pass` issues six sub-passes: {static, instanced} × {opaque+masked, blend, additive}. +// Which mesh list each reads, which pipeline it selects, which render modes it accepts, the order +// they run in, and when group 1 is rebound were two hand-written closures called six times, inside a +// function that takes `&EqRenderer` + `&mut wgpu::CommandEncoder` — arguments no test can build +// (neither `wgpu::Device` nor `wgpu::Queue` has a non-adapter constructor, and wgpu 22 has no `noop` +// backend). A flipped source, a dropped sub-pass or a widened mode filter was therefore invisible. +// The split below is the same shape #721 used for the instanced shadow draws: a pure planner over a +// device-free trait plus a sink for the `wgpu` handles, graded in tests/zone_pass_routing.rs. + +/// Which mesh list a zone sub-pass draws from. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum ZoneMeshSource { + /// `EqRenderer::gpu_meshes` — one draw per mesh, one instance. + Static, + /// `EqRenderer::gpu_instanced` — one draw per mesh over `instance_count` instances. + Instanced, +} + +/// The blend class of a zone sub-pass. **This is the mode filter**: a sub-pass does not carry a +/// free-form list of [`eqoxide_assets::RenderMode`]s, so "the opaque sub-pass silently started +/// accepting `Blend`" is not expressible in the table below — only in this one function. +/// +/// `Opaque` and `Masked` share a class because they share a pipeline: masked discards in-shader with +/// depth-write still on, so both belong in the depth-writing prepass. `Blend` and `Additive` run +/// after, each with its own depth-write-off pipeline. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum ZoneBlendClass { + OpaqueMasked, + Blend, + Additive, +} + +impl ZoneBlendClass { + /// Whether a mesh in this render mode is drawn by this class of sub-pass. Every mode is accepted + /// by exactly one class — a mesh is drawn once, never twice and never zero times, which is what + /// `zone_pass_routing.rs::every_render_mode_is_drawn_exactly_once` grades. + pub fn accepts(self, mode: eqoxide_assets::RenderMode) -> bool { + use eqoxide_assets::RenderMode; + matches!( + (self, mode), + (Self::OpaqueMasked, RenderMode::Opaque | RenderMode::Masked) + | (Self::Blend, RenderMode::Blend) + | (Self::Additive, RenderMode::Additive) + ) + } +} + +/// One zone sub-pass: a mesh list and a blend class. The pipeline is a pure function of the pair +/// (`ZoneDrawSink::set_pipeline`'s six arms), so there is no third field to get wrong. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct ZoneSubpass { + pub source: ZoneMeshSource, + pub class: ZoneBlendClass, +} + +/// The zone sub-passes, **in draw order**. Owning the order here rather than as six call statements +/// is what makes "the additive instanced sub-pass got dropped" a single testable edit. +/// +/// The order matters twice over: all depth-writing geometry must precede all depth-write-off +/// geometry, and within a class the static terrain is drawn before the placed objects standing on +/// it. +pub const ZONE_SUBPASSES: [ZoneSubpass; 6] = [ + ZoneSubpass { source: ZoneMeshSource::Static, class: ZoneBlendClass::OpaqueMasked }, + ZoneSubpass { source: ZoneMeshSource::Instanced, class: ZoneBlendClass::OpaqueMasked }, + ZoneSubpass { source: ZoneMeshSource::Static, class: ZoneBlendClass::Blend }, + ZoneSubpass { source: ZoneMeshSource::Instanced, class: ZoneBlendClass::Blend }, + ZoneSubpass { source: ZoneMeshSource::Static, class: ZoneBlendClass::Additive }, + ZoneSubpass { source: ZoneMeshSource::Instanced, class: ZoneBlendClass::Additive }, +]; + +/// Group-1 action for a planned zone draw. There is no `NotSampled` arm (unlike +/// [`ShadowTexBind`]): every zone pipeline has a fragment stage and samples its diffuse texture. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ZoneTexBind { + /// Bind group 1 to this texture index (`None` → the renderer's fallback texture). + Set(Option), + /// Group 1 already holds the right texture from an earlier step **in this sub-pass** — skip the + /// redundant `set_bind_group`. The cache does not survive a sub-pass boundary. + Keep, +} + +/// One planned zone draw. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ZoneDrawStep { + /// Index into [`ZONE_SUBPASSES`]. + pub subpass: usize, + pub source: ZoneMeshSource, + pub class: ZoneBlendClass, + /// Index into the mesh list named by `source` — NOT a position within the sub-pass. + pub mesh: usize, + /// True on the first step of each non-empty sub-pass: set the pipeline and bind groups 0 and 2. + pub set_pipeline: bool, + pub bind: ZoneTexBind, +} + +/// The device-free slice of a mesh the zone pass reads. +/// +/// Structurally identical to [`InstancedShadowCaster`], and deliberately NOT the same trait: that +/// one names the instanced *shadow* casters and is implemented only for +/// [`crate::gpu::GpuInstancedMesh`], whereas this is implemented for both zone mesh lists. Unifying +/// them would rename #721's public trait for no coverage gain, which is out of scope for a coverage +/// issue. +pub trait ZoneDrawMesh { + fn render_mode(&self) -> eqoxide_assets::RenderMode; + fn texture_idx(&self) -> Option; + fn anim(&self) -> Option<&(u32, Vec)>; +} + +impl ZoneDrawMesh for crate::gpu::GpuMesh { + fn render_mode(&self) -> eqoxide_assets::RenderMode { self.render_mode } + fn texture_idx(&self) -> Option { self.texture_idx } + fn anim(&self) -> Option<&(u32, Vec)> { self.anim.as_ref() } +} + +impl ZoneDrawMesh for crate::gpu::GpuInstancedMesh { + fn render_mode(&self) -> eqoxide_assets::RenderMode { self.render_mode } + fn texture_idx(&self) -> Option { self.texture_idx } + fn anim(&self) -> Option<&(u32, Vec)> { self.anim.as_ref() } +} + +/// Plans every zone colour draw for one frame: which sub-passes run, in which order, which mesh is +/// in which, and which texture each step must bind. +/// +/// Returns a **lazy iterator** — no allocation — because `encode_zone_pass` runs every frame. Steps +/// come out in exactly the order `encode_zone_pass` issues them. +pub fn plan_zone_draws<'a, S: ZoneDrawMesh, I: ZoneDrawMesh>( + statics: &'a [S], + instanced: &'a [I], + now_ms: u64, +) -> impl Iterator + 'a { + let len_of = move |src: ZoneMeshSource| match src { + ZoneMeshSource::Static => statics.len(), + ZoneMeshSource::Instanced => instanced.len(), + }; + let mode_of = move |src: ZoneMeshSource, i: usize| match src { + ZoneMeshSource::Static => statics[i].render_mode(), + ZoneMeshSource::Instanced => instanced[i].render_mode(), + }; + let tex_of = move |src: ZoneMeshSource, i: usize| match src { + ZoneMeshSource::Static => + animated_frame_texture(statics[i].texture_idx(), statics[i].anim(), now_ms), + ZoneMeshSource::Instanced => + animated_frame_texture(instanced[i].texture_idx(), instanced[i].anim(), now_ms), + }; + + ZONE_SUBPASSES.into_iter().enumerate().flat_map(move |(subpass, sp)| { + (0..len_of(sp.source)) + .filter(move |&i| sp.class.accepts(mode_of(sp.source, i))) + // The `scan` seed is re-evaluated per sub-pass, so `current_tex` starts as `None` inside + // every sub-pass and the first step of each necessarily takes the `Set` arm. The + // sub-pass boundary reset is therefore STRUCTURAL, not a condition anyone can flip: an + // added `!started` guard here measured green against the whole suite (mutation Z5, + // #739/#741 PR body) precisely because it was unreachable. + .scan((false, None::>), move |(started, current_tex), mesh| { + let tex = tex_of(sp.source, mesh); + let bind = if *current_tex == Some(tex) { + ZoneTexBind::Keep + } else { + *current_tex = Some(tex); + ZoneTexBind::Set(tex) + }; + let set_pipeline = !*started; + *started = true; + Some(ZoneDrawStep { + subpass, source: sp.source, class: sp.class, mesh, set_pipeline, bind, + }) + }) + }) +} + +/// The device-bound half of a zone draw, expressed entirely in **plan vocabulary** — +/// [`ZoneMeshSource`]/[`ZoneBlendClass`] and texture *indices*, never `wgpu` handles. +/// +/// `encode_zone_pass` supplies an impl closing over the renderer and the live `wgpu::RenderPass`; +/// `tests/zone_pass_routing.rs` supplies one that appends to a `Vec`. Both run the *same* executor, +/// so a regression in the plan→command translation is a test failure rather than something only a +/// GPU could notice. +/// +/// [`Self::bind_texture`] receives an index and has no mesh in scope, so "bind the mesh's base +/// `texture_idx` instead of the resolved animation frame" — #718's N2 bug, which shipped once +/// already — is not expressible in the executor at all. +pub trait ZoneDrawSink { + /// Start a sub-pass: select the pipeline for this (source, class) pair. + fn set_pipeline(&mut self, source: ZoneMeshSource, class: ZoneBlendClass); + /// Bind group 0 (the camera uniform). + fn bind_camera(&mut self); + /// Bind group 1 to the *already-resolved* texture index (`None` → fallback texture). + fn bind_texture(&mut self, idx: Option); + /// Bind group 2 (the sun shadow map, #518). + fn bind_shadow_sample(&mut self); + /// Issue the draw for mesh `mesh` of the list named by `source`. + fn draw(&mut self, source: ZoneMeshSource, mesh: usize); +} + +/// Turns a [`plan_zone_draws`] plan into sink calls. **This is the real executor** — +/// `encode_zone_pass` calls exactly this function. +/// +/// The call order within a first step (pipeline, group 0, group 1, group 2) is the pre-#741 order, +/// preserved because `tests/zone_pass_routing.rs` grades this stream against a verbatim +/// transcription of the two pre-#741 closures. +pub fn execute_zone_draw_plan( + statics: &[S], + instanced: &[I], + now_ms: u64, + sink: &mut K, +) { + for step in plan_zone_draws(statics, instanced, now_ms) { + if step.set_pipeline { + sink.set_pipeline(step.source, step.class); + sink.bind_camera(); + } + if let ZoneTexBind::Set(tex) = step.bind { + sink.bind_texture(tex); + } + if step.set_pipeline { + sink.bind_shadow_sample(); + } + sink.draw(step.source, step.mesh); + } +} + /// Zone geometry pass. Clears depth to 1.0; preserves sky color from sky pass. pub fn encode_zone_pass( r: &EqRenderer, @@ -651,83 +1011,71 @@ pub fn encode_zone_pass( occlusion_query_set: None, }); - use eqoxide_assets::RenderMode; - let tex_bg = |idx: Option| -> &wgpu::BindGroup { - match idx { - Some(i) if i < r.texture_bind_groups.len() => &r.texture_bind_groups[i], - _ => &r.fallback_texture_bg, + // Everything decided for this pass — the six sub-passes and their order, which mesh list and + // which render modes each one draws, which animated texture frame a mesh binds, and when a + // group-1 rebind is actually needed — comes from `plan_zone_draws` (#741), which is device-free + // and unit tested in tests/zone_pass_routing.rs (with a differential pin against the pre-#741 + // closures in the same file). All that is left here is `ZoneSink`: five bodies that turn plan + // vocabulary into `wgpu` handles. Nothing in this impl decides anything — if you find yourself + // adding a condition to it, it belongs in the planner. + struct ZoneSink<'r, 'p, 'e> { + r: &'r EqRenderer, + pass: &'p mut wgpu::RenderPass<'e>, + } + impl ZoneDrawSink for ZoneSink<'_, '_, '_> { + fn set_pipeline(&mut self, source: ZoneMeshSource, class: ZoneBlendClass) { + let p = &self.r.pipelines; + self.pass.set_pipeline(match (source, class) { + (ZoneMeshSource::Static, ZoneBlendClass::OpaqueMasked) => &p.zone, + (ZoneMeshSource::Static, ZoneBlendClass::Blend) => &p.zone_blend, + (ZoneMeshSource::Static, ZoneBlendClass::Additive) => &p.zone_additive, + (ZoneMeshSource::Instanced, ZoneBlendClass::OpaqueMasked) => &p.zone_instanced, + (ZoneMeshSource::Instanced, ZoneBlendClass::Blend) => &p.zone_instanced_blend, + (ZoneMeshSource::Instanced, ZoneBlendClass::Additive) => &p.zone_instanced_additive, + }); } - }; - // Wall-clock ms since process start, for cycling animated textures (fire/water/lava). - let now_ms = anim_now_ms(); - // The texture a mesh should bind this frame: its current animation frame if animated, - // else its static texture. The per-loop texture cache naturally rebinds when it advances. - // This is the SAME function the masked shadow sub-pass resolves its frame with (#721) — the - // color and shadow silhouettes must agree on the texel, or #718's N2 mismatch comes back. It - // was a byte-identical copy of `animated_frame_texture` until #721's review round 2; do not - // re-inline it. - let frame_tex = |tex: Option, anim: &Option<(u32, Vec)>| -> Option { - animated_frame_texture(tex, anim.as_ref(), now_ms) - }; - - // Draw the static terrain meshes whose render_mode is in `modes`, with `pipeline`. - // Opaque + masked share the opaque pipeline (masked discards in-shader, depth-write - // on); blend/additive run after with their own depth-write-off pipelines. - let draw_static = |pass: &mut wgpu::RenderPass<'_>, pipeline, modes: &[RenderMode]| { - let mut bound = false; - let mut current_tex: Option = None; - for mesh in &r.gpu_meshes { - if !modes.contains(&mesh.render_mode) { continue; } - let etex = frame_tex(mesh.texture_idx, &mesh.anim); - if !bound { - pass.set_pipeline(pipeline); - pass.set_bind_group(0, &r.camera_uniform.bind_group, &[]); - pass.set_bind_group(1, tex_bg(etex), &[]); - pass.set_bind_group(2, &r.shadow_sample_bg, &[]); // sun shadow map (#518) - current_tex = etex; - bound = true; - } else if etex != current_tex { - current_tex = etex; - pass.set_bind_group(1, tex_bg(current_tex), &[]); - } - pass.set_vertex_buffer(0, mesh.vertex_buf.slice(..)); - pass.set_index_buffer(mesh.index_buf.slice(..), wgpu::IndexFormat::Uint32); - pass.draw_indexed(0..mesh.index_count, 0, 0..1); + fn bind_camera(&mut self) { + let r = self.r; + self.pass.set_bind_group(0, &r.camera_uniform.bind_group, &[]); } - }; - draw_static(&mut pass, &r.pipelines.zone, &[RenderMode::Opaque, RenderMode::Masked]); - - // ── GPU-instanced placed objects ─────────────────────────────────────── - let draw_instanced = |pass: &mut wgpu::RenderPass<'_>, pipeline, modes: &[RenderMode]| { - let mut bound = false; - let mut current_tex: Option = None; - for mesh in &r.gpu_instanced { - if !modes.contains(&mesh.render_mode) { continue; } - let etex = frame_tex(mesh.texture_idx, &mesh.anim); - if !bound { - pass.set_pipeline(pipeline); - pass.set_bind_group(0, &r.camera_uniform.bind_group, &[]); - pass.set_bind_group(1, tex_bg(etex), &[]); - pass.set_bind_group(2, &r.shadow_sample_bg, &[]); // sun shadow map (#518) - current_tex = etex; - bound = true; - } else if etex != current_tex { - current_tex = etex; - pass.set_bind_group(1, tex_bg(current_tex), &[]); + fn bind_texture(&mut self, idx: Option) { + let r = self.r; + let bg = match idx { + Some(i) if i < r.texture_bind_groups.len() => &r.texture_bind_groups[i], + _ => &r.fallback_texture_bg, + }; + self.pass.set_bind_group(1, bg, &[]); + } + fn bind_shadow_sample(&mut self) { + let r = self.r; + self.pass.set_bind_group(2, &r.shadow_sample_bg, &[]); // sun shadow map (#518) + } + fn draw(&mut self, source: ZoneMeshSource, mesh: usize) { + match source { + ZoneMeshSource::Static => { + let m = &self.r.gpu_meshes[mesh]; + self.pass.set_vertex_buffer(0, m.vertex_buf.slice(..)); + self.pass.set_index_buffer(m.index_buf.slice(..), wgpu::IndexFormat::Uint32); + self.pass.draw_indexed(0..m.index_count, 0, 0..1); + } + ZoneMeshSource::Instanced => { + let m = &self.r.gpu_instanced[mesh]; + self.pass.set_vertex_buffer(0, m.vertex_buf.slice(..)); + self.pass.set_vertex_buffer(1, m.instance_buf.slice(..)); + self.pass.set_index_buffer(m.index_buf.slice(..), wgpu::IndexFormat::Uint32); + self.pass.draw_indexed(0..m.index_count, 0, 0..m.instance_count); + } } - pass.set_vertex_buffer(0, mesh.vertex_buf.slice(..)); - pass.set_vertex_buffer(1, mesh.instance_buf.slice(..)); - pass.set_index_buffer(mesh.index_buf.slice(..), wgpu::IndexFormat::Uint32); - pass.draw_indexed(0..mesh.index_count, 0, 0..mesh.instance_count); } - }; - draw_instanced(&mut pass, &r.pipelines.zone_instanced, &[RenderMode::Opaque, RenderMode::Masked]); + } - // ── Transparent passes (after all opaque/masked, depth-write off) ─────── - draw_static(&mut pass, &r.pipelines.zone_blend, &[RenderMode::Blend]); - draw_instanced(&mut pass, &r.pipelines.zone_instanced_blend, &[RenderMode::Blend]); - draw_static(&mut pass, &r.pipelines.zone_additive, &[RenderMode::Additive]); - draw_instanced(&mut pass, &r.pipelines.zone_instanced_additive, &[RenderMode::Additive]); + // Wall-clock ms since process start, for cycling animated textures (fire/water/lava). The frame + // a mesh resolves to is `animated_frame_texture`, the SAME function the masked shadow sub-pass + // uses (#721) — the colour and shadow silhouettes must agree on the texel, or #718's N2 mismatch + // comes back. The planner calls it; do not re-inline a copy here. + let now_ms = anim_now_ms(); + let mut sink = ZoneSink { r, pass: &mut pass }; + execute_zone_draw_plan(&r.gpu_meshes, &r.gpu_instanced, now_ms, &mut sink); } /// Draw the zone's doors (closed state). Each door uses its object model if loaded, else a @@ -1797,42 +2145,70 @@ pub fn encode_shadow_pass( execute_instanced_shadow_plan(&r.gpu_instanced, now_ms, &mut sink); } - // Skinned casters. - let mut skinned_bound = false; - for c in &casters { - if let Caster::Skinned { model, u_slot, j_slot } = c { - if !skinned_bound { - pass.set_pipeline(&r.pipelines.shadow_skinned); - pass.set_bind_group(0, &r.light_depth_bg, &[]); - skinned_bound = true; - } - pass.set_bind_group(1, &r.shadow_uniform_pool[*u_slot].1, &[]); - pass.set_bind_group(2, &r.shadow_joint_pool[*j_slot].1, &[]); - for mesh in &model.meshes { - pass.set_vertex_buffer(0, mesh.vertex_buf.slice(..)); - pass.set_index_buffer(mesh.index_buf.slice(..), wgpu::IndexFormat::Uint32); - pass.draw_indexed(0..mesh.index_count, 0, 0..1); + // Character casters, skinned sub-pass then static sub-pass. Which sub-pass a caster lands in, + // the one-time pipeline/group-0 bind per sub-pass, and the fact that only skinned casters bind a + // joint palette all come from `plan_character_shadow_draws` (#739), device-free and unit tested + // in tests/character_shadow_routing.rs (with a differential pin against the pre-#739 loops). + // `CharacterSink` below is the `wgpu`-handle translation and decides nothing. + impl CharacterShadowCaster for Caster<'_> { + fn shadow_bind(&self) -> CharacterShadowBind { + match *self { + Caster::Skinned { u_slot, j_slot, .. } => + CharacterShadowBind::Skinned { u_slot, j_slot }, + Caster::Static { u_slot, .. } => CharacterShadowBind::Static { u_slot }, } } } - - // Static casters. - let mut static_bound = false; - for c in &casters { - if let Caster::Static { model, u_slot } = c { - if !static_bound { - pass.set_pipeline(&r.pipelines.shadow_static); - pass.set_bind_group(0, &r.light_depth_bg, &[]); - static_bound = true; - } - pass.set_bind_group(1, &r.shadow_uniform_pool[*u_slot].1, &[]); - for mesh in &model.meshes { - pass.set_vertex_buffer(0, mesh.vertex_buf.slice(..)); - pass.set_index_buffer(mesh.index_buf.slice(..), wgpu::IndexFormat::Uint32); - pass.draw_indexed(0..mesh.index_count, 0, 0..1); + struct CharacterSink<'r, 'p, 'e, 'c> { + r: &'r EqRenderer, + pass: &'p mut wgpu::RenderPass<'e>, + casters: &'c [Caster<'c>], + } + impl CharacterShadowSink for CharacterSink<'_, '_, '_, '_> { + fn set_pipeline(&mut self, kind: CharacterShadowKind) { + let r = self.r; + self.pass.set_pipeline(match kind { + CharacterShadowKind::Skinned => &r.pipelines.shadow_skinned, + CharacterShadowKind::Static => &r.pipelines.shadow_static, + }); + } + fn bind_light_depth(&mut self) { + let r = self.r; + self.pass.set_bind_group(0, &r.light_depth_bg, &[]); + } + fn bind_model_uniform(&mut self, u_slot: usize) { + let r = self.r; + self.pass.set_bind_group(1, &r.shadow_uniform_pool[u_slot].1, &[]); + } + fn bind_joints(&mut self, j_slot: usize) { + let r = self.r; + self.pass.set_bind_group(2, &r.shadow_joint_pool[j_slot].1, &[]); + } + fn draw(&mut self, caster: usize) { + // One draw per mesh of the caster's model. Mesh count is not a routing decision, so it + // is not in the plan. + match &self.casters[caster] { + Caster::Skinned { model, .. } => { + for mesh in &model.meshes { + self.pass.set_vertex_buffer(0, mesh.vertex_buf.slice(..)); + self.pass.set_index_buffer(mesh.index_buf.slice(..), wgpu::IndexFormat::Uint32); + self.pass.draw_indexed(0..mesh.index_count, 0, 0..1); + } + } + Caster::Static { model, .. } => { + for mesh in &model.meshes { + self.pass.set_vertex_buffer(0, mesh.vertex_buf.slice(..)); + self.pass.set_index_buffer(mesh.index_buf.slice(..), wgpu::IndexFormat::Uint32); + self.pass.draw_indexed(0..mesh.index_count, 0, 0..1); + } + } } } } + { + let mut sink = CharacterSink { r, pass: &mut pass, casters: &casters }; + execute_character_shadow_plan(&casters, &mut sink); + } } /// Weather precipitation pass (eqoxide#542). Draws an instanced billboard particle field (rain diff --git a/crates/eqoxide-renderer/tests/character_shadow_routing.rs b/crates/eqoxide-renderer/tests/character_shadow_routing.rs new file mode 100644 index 00000000..0262fb79 --- /dev/null +++ b/crates/eqoxide-renderer/tests/character_shadow_routing.rs @@ -0,0 +1,269 @@ +//! **eqoxide#739** — coverage for the skinned and static character shadow sub-passes. +//! +//! #737 built a device-free draw planner for the **instanced** shadow sub-pass and proved it has +//! teeth. The skinned and static sub-passes that follow it in `encode_shadow_pass` made the same +//! kind of pipeline/bind decision with no equivalent coverage: two hand-written loops with a `bool` +//! latch each, inside a function taking `&EqRenderer` + `&mut wgpu::CommandEncoder`, which no test +//! can build (no non-adapter constructor for `wgpu::Device`/`Queue`, no `noop` backend in wgpu 22). +//! A flipped pipeline, a deleted sub-pass, or a static caster that bound a joint palette was +//! invisible to the whole suite. +//! +//! ## #739 asked whether this shares #737's vocabulary. It does not. +//! +//! The issue warned explicitly against assuming the sub-passes unify, citing a #721 correction where +//! exactly that assumption was wrong. Checked rather than assumed, and they do not: +//! +//! | | instanced (#721) | character (#739) | +//! |---|---|---| +//! | what picks the pipeline | the mesh's `RenderMode` | whether the *model* is skinned | +//! | what group 1 holds | a diffuse texture index | a `shadow_uniform_pool` slot | +//! | group 2 | never bound | a `shadow_joint_pool` slot, skinned only | +//! | per-step elision | redundant texture rebinds elided | none — every caster binds its own slot | +//! +//! `ShadowTexBind` cannot name "bind pool slot 3", and `ShadowPipelineKind::for_render_mode` has no +//! meaning for a character. The one genuinely shared shape is "one latch per sub-pass", three lines. +//! So `plan_character_shadow_draws` is a separate planner with its own vocabulary, and this file is +//! separate from `shadow_routing.rs`. +//! +//! ## What is a type here rather than a test +//! +//! `CharacterShadowBind::Skinned` carries the joint slot and `::Static` does not, so a static step +//! **cannot** carry a joint palette — that bad state is unrepresentable, not merely untested. The +//! tests below cover what the type cannot: that each caster lands in the right sub-pass, in the +//! right order, and that the executor calls `bind_joints` exactly for the skinned ones. +//! +//! ## What this file does NOT cover +//! +//! - **The two-arm pipeline lookup in `encode_shadow_pass`'s `CharacterSink::set_pipeline`.** +//! Swapping `shadow_skinned` and `shadow_static` *there* is not observable from here, because this +//! file's sink maps the same `CharacterShadowKind` to a string. Left open deliberately rather than +//! pinned as source text: a source-text pin has been measured on this crate to be evadable by +//! shadowing a local (#773 review, evasions E1b/E2), so it would buy a claim it cannot keep. +//! - **wgpu semantics**, and **whether a flip is visible in play** — #739 listed the second as not +//! established, and this PR does not settle it. No client was run. +//! - **Which entities are selected at all** — that is `plan_shadow_casters` (#740), graded in +//! `shadow_caster_selection.rs`. This file starts from an already-selected caster list. + +use eqoxide_renderer::pass::{ + execute_character_shadow_plan, plan_character_shadow_draws, CharacterShadowBind, + CharacterShadowCaster, CharacterShadowKind, CharacterShadowSink, CHARACTER_SHADOW_SUBPASSES, +}; + +#[derive(Debug, PartialEq, Eq, Clone)] +enum Cmd { + SetPipeline(&'static str), + BindGroup0, + BindGroup1(usize), + BindGroup2(usize), + Draw(usize), +} + +/// A selected caster, reduced to what ROUTING reads — the same reduction `encode_shadow_pass`'s +/// `Caster` enum implements against real `GpuSkinnedModel`/`GpuStaticModel` references. +enum Caster { + Skinned { u_slot: usize, j_slot: usize }, + Static { u_slot: usize }, +} + +impl CharacterShadowCaster for Caster { + fn shadow_bind(&self) -> CharacterShadowBind { + match *self { + Caster::Skinned { u_slot, j_slot } => CharacterShadowBind::Skinned { u_slot, j_slot }, + Caster::Static { u_slot } => CharacterShadowBind::Static { u_slot }, + } + } +} + +/// Verbatim transcription of `encode_shadow_pass`'s two character loops at `3d60bfd` (the merge-base +/// of this branch), with `wgpu` handles replaced by the names/indices of what they were read from. +/// The per-mesh inner loop is collapsed to one `Draw` because mesh count is not a routing decision +/// and is not in the plan; both sides collapse it identically. +fn old(casters: &[Caster]) -> Vec { + let mut out = Vec::new(); + + let mut skinned_bound = false; + for (i, c) in casters.iter().enumerate() { + if let Caster::Skinned { u_slot, j_slot } = c { + if !skinned_bound { + out.push(Cmd::SetPipeline("shadow_skinned")); + out.push(Cmd::BindGroup0); + skinned_bound = true; + } + out.push(Cmd::BindGroup1(*u_slot)); + out.push(Cmd::BindGroup2(*j_slot)); + out.push(Cmd::Draw(i)); + } + } + + let mut static_bound = false; + for (i, c) in casters.iter().enumerate() { + if let Caster::Static { u_slot } = c { + if !static_bound { + out.push(Cmd::SetPipeline("shadow_static")); + out.push(Cmd::BindGroup0); + static_bound = true; + } + out.push(Cmd::BindGroup1(*u_slot)); + out.push(Cmd::Draw(i)); + } + } + + out +} + +/// A device-free [`CharacterShadowSink`]: the same five methods `encode_shadow_pass`'s +/// `CharacterSink` implements against a live `wgpu::RenderPass`, recording into a `Vec` instead. +#[derive(Default)] +struct Recorder { + out: Vec, +} + +impl CharacterShadowSink for Recorder { + fn set_pipeline(&mut self, kind: CharacterShadowKind) { + self.out.push(Cmd::SetPipeline(match kind { + CharacterShadowKind::Skinned => "shadow_skinned", + CharacterShadowKind::Static => "shadow_static", + })); + } + fn bind_light_depth(&mut self) { self.out.push(Cmd::BindGroup0); } + fn bind_model_uniform(&mut self, u_slot: usize) { self.out.push(Cmd::BindGroup1(u_slot)); } + fn bind_joints(&mut self, j_slot: usize) { self.out.push(Cmd::BindGroup2(j_slot)); } + fn draw(&mut self, caster: usize) { self.out.push(Cmd::Draw(caster)); } +} + +fn new(casters: &[Caster]) -> Vec { + let mut rec = Recorder::default(); + execute_character_shadow_plan(casters, &mut rec); + rec.out +} + +/// Casters in the interleaved order `plan_shadow_casters` actually produces — the player first, then +/// nearby characters nearest-first, so skinned and static are mixed rather than grouped. +fn mixed() -> Vec { + vec![ + Caster::Skinned { u_slot: 0, j_slot: 0 }, + Caster::Static { u_slot: 1 }, + Caster::Skinned { u_slot: 2, j_slot: 1 }, + Caster::Skinned { u_slot: 3, j_slot: 2 }, + Caster::Static { u_slot: 4 }, + ] +} + +// ── Differential pin ───────────────────────────────────────────────────────────────────────────── + +/// The extracted planner must emit the byte-identical command stream the pre-#739 loops emitted. +/// +/// This grades the new code against the OLD implementation, not against itself. **When to change +/// this test**: it freezes pre-#739 behaviour, so it is what should fail if someone later *intends* +/// to change character shadow draw order. Update it as part of that change; do not weaken it. +#[test] +fn the_extracted_plan_emits_the_pre_739_command_stream() { + let cases: Vec<(&str, Vec)> = vec![ + ("no casters", vec![]), + ("skinned only", vec![Caster::Skinned { u_slot: 0, j_slot: 0 }, + Caster::Skinned { u_slot: 1, j_slot: 1 }]), + ("static only", vec![Caster::Static { u_slot: 0 }, Caster::Static { u_slot: 1 }]), + ("static first, then skinned", vec![Caster::Static { u_slot: 9 }, + Caster::Skinned { u_slot: 3, j_slot: 7 }]), + ("interleaved", mixed()), + ("repeated slots", vec![Caster::Skinned { u_slot: 2, j_slot: 2 }, + Caster::Skinned { u_slot: 2, j_slot: 2 }, + Caster::Static { u_slot: 2 }]), + ]; + for (name, casters) in &cases { + assert_eq!(new(casters), old(casters), + "#739: the extracted character shadow plan diverged from the pre-#739 loops on case \ + {name:?}"); + } +} + +// ── The routing itself ─────────────────────────────────────────────────────────────────────────── + +/// **Only skinned casters bind a joint palette.** A static caster that bound group 2 would point the +/// static pipeline at a joint buffer it does not declare; a skinned caster that did not would pose +/// against whatever palette the previous caster left in slot 2 — the whole model drawn in another +/// character's pose. Asserted over the executor's command stream, which is where the decision is +/// made, not over the step type (where it is already unrepresentable). +#[test] +fn group_two_is_bound_exactly_for_the_skinned_casters() { + let casters = mixed(); + let cmds = new(&casters); + + let joints: Vec = cmds.iter() + .filter_map(|c| if let Cmd::BindGroup2(j) = c { Some(*j) } else { None }).collect(); + assert_eq!(joints, vec![0, 1, 2], + "each skinned caster binds its own joint slot, in sub-pass order; got {joints:?}"); + + // Every group-2 bind is immediately preceded by that caster's group-1 bind and followed by its + // draw — i.e. no static caster is between a skinned caster's binds and its draw. + for (i, c) in cmds.iter().enumerate() { + if matches!(c, Cmd::BindGroup2(_)) { + assert!(matches!(cmds[i - 1], Cmd::BindGroup1(_)), + "a joint bind must follow the same caster's uniform bind, got {:?}", cmds[i - 1]); + assert!(matches!(cmds[i + 1], Cmd::Draw(_)), + "a joint bind must be followed by that caster's draw, got {:?}", cmds[i + 1]); + } + } + + let statics_drawn: Vec = plan_character_shadow_draws(&casters) + .filter(|s| s.bind.kind() == CharacterShadowKind::Static).map(|s| s.caster).collect(); + assert_eq!(statics_drawn, vec![1, 4], "the two static casters must both be drawn"); +} + +/// **Sub-pass order and grouping.** Every skinned caster is drawn before every static one, whatever +/// order they were selected in, and each sub-pass sets its pipeline exactly once. Drawing in +/// selection order instead would switch pipeline per caster — the cost the two-loop split exists to +/// avoid. +#[test] +fn all_skinned_casters_draw_before_all_static_ones_with_one_pipeline_switch_each() { + assert_eq!(CHARACTER_SHADOW_SUBPASSES, + [CharacterShadowKind::Skinned, CharacterShadowKind::Static], + "#739: the character shadow sub-pass order changed"); + + let casters = mixed(); + let cmds = new(&casters); + + let pipelines: Vec<&str> = cmds.iter() + .filter_map(|c| if let Cmd::SetPipeline(p) = c { Some(*p) } else { None }).collect(); + assert_eq!(pipelines, vec!["shadow_skinned", "shadow_static"], + "exactly one pipeline switch per non-empty sub-pass, skinned first; got {pipelines:?}"); + assert_eq!(cmds.iter().filter(|c| **c == Cmd::BindGroup0).count(), 2, + "group 0 is bound once per non-empty sub-pass"); + + let draws: Vec = cmds.iter() + .filter_map(|c| if let Cmd::Draw(i) = c { Some(*i) } else { None }).collect(); + assert_eq!(draws, vec![0, 2, 3, 1, 4], + "draws must come out grouped by sub-pass (skinned 0,2,3 then static 1,4), not in \ + selection order; got {draws:?}"); +} + +/// **An empty sub-pass emits nothing** — no pipeline switch, no group-0 bind. A zone where every +/// caster is skinned must not pay for the static pipeline, and vice versa. +#[test] +fn an_empty_subpass_emits_no_pipeline_switch() { + let skinned_only = vec![Caster::Skinned { u_slot: 0, j_slot: 0 }]; + let cmds = new(&skinned_only); + assert_eq!(cmds.iter().filter(|c| matches!(c, Cmd::SetPipeline(_))).count(), 1, + "one non-empty sub-pass, one pipeline switch; got {cmds:?}"); + assert!(!cmds.contains(&Cmd::SetPipeline("shadow_static"))); + + let static_only = vec![Caster::Static { u_slot: 4 }]; + let cmds = new(&static_only); + assert_eq!(cmds, vec![Cmd::SetPipeline("shadow_static"), Cmd::BindGroup0, + Cmd::BindGroup1(4), Cmd::Draw(0)], + "a static-only frame must not switch to the skinned pipeline at all"); + + assert!(new(&[]).is_empty(), "no casters must emit no commands"); +} + +/// **A step names the caster's index in the caster list**, not its position in the sub-pass — the +/// sink uses it to index the caster slice, so an off-by-one draws another character's model. +#[test] +fn a_step_carries_the_casters_index_in_the_selection_list() { + let casters = mixed(); + let steps: Vec<_> = plan_character_shadow_draws(&casters).collect(); + assert_eq!(steps.iter().map(|s| s.caster).collect::>(), vec![0, 2, 3, 1, 4]); + assert_eq!(steps[0].bind, CharacterShadowBind::Skinned { u_slot: 0, j_slot: 0 }); + assert_eq!(steps[3].bind, CharacterShadowBind::Static { u_slot: 1 }, + "the first static step must carry caster 1's uniform slot, not the 4th caster's"); +} diff --git a/crates/eqoxide-renderer/tests/zone_pass_routing.rs b/crates/eqoxide-renderer/tests/zone_pass_routing.rs new file mode 100644 index 00000000..ca90af07 --- /dev/null +++ b/crates/eqoxide-renderer/tests/zone_pass_routing.rs @@ -0,0 +1,340 @@ +//! **eqoxide#741** — coverage for `encode_zone_pass`'s `draw_static`/`draw_instanced` routing. +//! +//! The colour pass issues six sub-passes: {static terrain, instanced placed objects} × {opaque and +//! masked, blend, additive}. Before #741 those were two closures called six times inside a function +//! taking `&EqRenderer` + `&mut wgpu::CommandEncoder`, so **no test could reach them**: neither +//! `wgpu::Device` nor `wgpu::Queue` has a non-adapter constructor and wgpu 22 has no `noop` backend, +//! so an integration test cannot build the arguments at all. A flipped mesh list, a dropped +//! sub-pass, a widened mode filter or a lost texture rebind was invisible to the whole suite. +//! +//! What is graded here is the **command sequence** `encode_zone_pass` emits, produced by the real +//! executor (`pass::execute_zone_draw_plan` — the exact function the pass calls) driven into a +//! recording sink. Two kinds of assertion: +//! +//! - a **differential pin** (`old()` below is a verbatim transcription of the pre-#741 closures at +//! merge-base `3d60bfd`) that discharges "#741 changed no rendering behaviour", and +//! - **behavioural** tests of the routing itself: the mode partition, the sub-pass order, and the +//! texture-cache reset at a sub-pass boundary. +//! +//! ## What this file does NOT cover +//! +//! - **The six-arm pipeline lookup in `encode_zone_pass`'s `ZoneSink::set_pipeline`.** That match +//! turns a `(ZoneMeshSource, ZoneBlendClass)` pair into a `&wgpu::RenderPipeline`, and this file's +//! sink turns the same pair into a `&'static str`. Swapping two arms *in the production sink* is +//! not observable from here. That is a real hole and it is left open deliberately: pinning it +//! would mean asserting on the source text of `pass.rs`, and a source-text pin has now been +//! measured on this crate to be evadable by shadowing a local (#773 review, evasions E1b/E2). The +//! honest boundary is that everything which *decides* is graded and the handle lookup is not. +//! - **wgpu semantics.** Nothing here creates a `wgpu::RenderPass`, so a command sequence that is +//! correct as a sequence but invalid as wgpu (wrong group index for a layout, say) still passes. +//! - **Whether a flip is visible in play.** #741 asked; it is still not established. A wrong +//! pipeline for a blend mesh would render it opaque or not at all, but nobody has looked, and +//! there is no live before/after in this PR. + +use eqoxide_assets::RenderMode; +use eqoxide_renderer::pass::{ + execute_zone_draw_plan, plan_zone_draws, ZoneBlendClass, ZoneDrawMesh, ZoneDrawSink, + ZoneMeshSource, ZoneTexBind, ZONE_SUBPASSES, +}; + +#[derive(Debug, PartialEq, Eq, Clone)] +enum Cmd { + SetPipeline(&'static str), + BindCamera, + BindTex(Option), + BindShadow, + DrawStatic(usize), + DrawInstanced(usize), +} + +struct Mesh { + mode: RenderMode, + tex: Option, + anim: Option<(u32, Vec)>, +} + +impl Mesh { + fn new(mode: RenderMode, tex: Option) -> Self { + Mesh { mode, tex, anim: None } + } + fn animated(mode: RenderMode, ms: u32, frames: &[usize]) -> Self { + Mesh { mode, tex: Some(999), anim: Some((ms, frames.to_vec())) } + } +} + +impl ZoneDrawMesh for Mesh { + fn render_mode(&self) -> RenderMode { self.mode } + fn texture_idx(&self) -> Option { self.tex } + fn anim(&self) -> Option<&(u32, Vec)> { self.anim.as_ref() } +} + +/// Verbatim transcription of `encode_zone_pass`'s two closures and their six call sites at +/// `3d60bfd` (the merge-base of this branch), with `wgpu` handles replaced by the names of the +/// fields they were read from. Nothing here is shared with the production code under test. +fn old(statics: &[Mesh], instanced: &[Mesh], now_ms: u64) -> Vec { + let frame_tex = |tex: Option, anim: &Option<(u32, Vec)>| -> Option { + match anim { + Some((ms, frames)) if !frames.is_empty() => { + Some(frames[(now_ms / (*ms).max(1) as u64) as usize % frames.len()]) + } + _ => tex, + } + }; + fn draw_static( + out: &mut Vec, meshes: &[Mesh], pipeline: &'static str, modes: &[RenderMode], + frame_tex: &dyn Fn(Option, &Option<(u32, Vec)>) -> Option, + ) { + let mut bound = false; + let mut current_tex: Option = None; + for (i, mesh) in meshes.iter().enumerate() { + if !modes.contains(&mesh.mode) { continue; } + let etex = frame_tex(mesh.tex, &mesh.anim); + if !bound { + out.push(Cmd::SetPipeline(pipeline)); + out.push(Cmd::BindCamera); + out.push(Cmd::BindTex(etex)); + out.push(Cmd::BindShadow); + current_tex = etex; + bound = true; + } else if etex != current_tex { + current_tex = etex; + out.push(Cmd::BindTex(current_tex)); + } + out.push(Cmd::DrawStatic(i)); + } + } + fn draw_instanced( + out: &mut Vec, meshes: &[Mesh], pipeline: &'static str, modes: &[RenderMode], + frame_tex: &dyn Fn(Option, &Option<(u32, Vec)>) -> Option, + ) { + let mut bound = false; + let mut current_tex: Option = None; + for (i, mesh) in meshes.iter().enumerate() { + if !modes.contains(&mesh.mode) { continue; } + let etex = frame_tex(mesh.tex, &mesh.anim); + if !bound { + out.push(Cmd::SetPipeline(pipeline)); + out.push(Cmd::BindCamera); + out.push(Cmd::BindTex(etex)); + out.push(Cmd::BindShadow); + current_tex = etex; + bound = true; + } else if etex != current_tex { + current_tex = etex; + out.push(Cmd::BindTex(current_tex)); + } + out.push(Cmd::DrawInstanced(i)); + } + } + + let mut out = Vec::new(); + let om = [RenderMode::Opaque, RenderMode::Masked]; + draw_static(&mut out, statics, "zone", &om, &frame_tex); + draw_instanced(&mut out, instanced, "zone_instanced", &om, &frame_tex); + draw_static(&mut out, statics, "zone_blend", &[RenderMode::Blend], &frame_tex); + draw_instanced(&mut out, instanced, "zone_instanced_blend", &[RenderMode::Blend], &frame_tex); + draw_static(&mut out, statics, "zone_additive", &[RenderMode::Additive], &frame_tex); + draw_instanced(&mut out, instanced, "zone_instanced_additive", &[RenderMode::Additive], + &frame_tex); + out +} + +/// A device-free [`ZoneDrawSink`]: the same five methods `encode_zone_pass`'s `ZoneSink` implements +/// against a live `wgpu::RenderPass`, recording into a `Vec` instead. +#[derive(Default)] +struct Recorder { + out: Vec, +} + +impl ZoneDrawSink for Recorder { + fn set_pipeline(&mut self, source: ZoneMeshSource, class: ZoneBlendClass) { + self.out.push(Cmd::SetPipeline(match (source, class) { + (ZoneMeshSource::Static, ZoneBlendClass::OpaqueMasked) => "zone", + (ZoneMeshSource::Static, ZoneBlendClass::Blend) => "zone_blend", + (ZoneMeshSource::Static, ZoneBlendClass::Additive) => "zone_additive", + (ZoneMeshSource::Instanced, ZoneBlendClass::OpaqueMasked) => "zone_instanced", + (ZoneMeshSource::Instanced, ZoneBlendClass::Blend) => "zone_instanced_blend", + (ZoneMeshSource::Instanced, ZoneBlendClass::Additive) => "zone_instanced_additive", + })); + } + fn bind_camera(&mut self) { self.out.push(Cmd::BindCamera); } + fn bind_texture(&mut self, idx: Option) { self.out.push(Cmd::BindTex(idx)); } + fn bind_shadow_sample(&mut self) { self.out.push(Cmd::BindShadow); } + fn draw(&mut self, source: ZoneMeshSource, mesh: usize) { + self.out.push(match source { + ZoneMeshSource::Static => Cmd::DrawStatic(mesh), + ZoneMeshSource::Instanced => Cmd::DrawInstanced(mesh), + }); + } +} + +fn new(statics: &[Mesh], instanced: &[Mesh], now_ms: u64) -> Vec { + let mut rec = Recorder::default(); + execute_zone_draw_plan(statics, instanced, now_ms, &mut rec); + rec.out +} + +const ALL_MODES: [RenderMode; 4] = + [RenderMode::Opaque, RenderMode::Masked, RenderMode::Blend, RenderMode::Additive]; + +// ── Differential pin ───────────────────────────────────────────────────────────────────────────── + +/// The extracted planner must emit the byte-identical command stream the pre-#741 closures emitted, +/// over a corpus that exercises every mode in both lists, texture runs, animated meshes and empty +/// sub-passes. +/// +/// This grades the new code against the OLD implementation, not against itself. **When to change +/// this test**: it deliberately freezes pre-#741 behaviour, so it is exactly what should fail if +/// someone later *intends* to change the zone draw order (say, sorting meshes by texture to cut +/// rebinds). Update it as part of that change; do not weaken it to make a diff go green. +#[test] +fn the_extracted_plan_emits_the_pre_741_command_stream() { + let cases: Vec<(&str, Vec, Vec, u64)> = vec![ + ("empty", vec![], vec![], 0), + ("statics only, one of each mode", + ALL_MODES.iter().map(|&m| Mesh::new(m, Some(1))).collect(), vec![], 0), + ("instanced only, one of each mode", + vec![], ALL_MODES.iter().map(|&m| Mesh::new(m, Some(1))).collect(), 0), + ("both lists, interleaved modes", + vec![Mesh::new(RenderMode::Blend, Some(4)), Mesh::new(RenderMode::Opaque, Some(1)), + Mesh::new(RenderMode::Additive, Some(7)), Mesh::new(RenderMode::Masked, Some(1))], + vec![Mesh::new(RenderMode::Additive, Some(2)), Mesh::new(RenderMode::Opaque, None), + Mesh::new(RenderMode::Opaque, Some(3)), Mesh::new(RenderMode::Blend, Some(3))], + 0), + ("texture runs, so the Keep elision is exercised", + vec![Mesh::new(RenderMode::Opaque, Some(5)), Mesh::new(RenderMode::Opaque, Some(5)), + Mesh::new(RenderMode::Masked, Some(5)), Mesh::new(RenderMode::Opaque, Some(6)), + Mesh::new(RenderMode::Opaque, Some(5))], + vec![], 0), + ("animated meshes at t=0", + vec![Mesh::animated(RenderMode::Opaque, 100, &[3, 4, 5])], + vec![Mesh::animated(RenderMode::Blend, 250, &[8, 9])], 0), + ("animated meshes mid-cycle", + vec![Mesh::animated(RenderMode::Opaque, 100, &[3, 4, 5])], + vec![Mesh::animated(RenderMode::Blend, 250, &[8, 9])], 1_234), + ("animated meshes far into the cycle", + vec![Mesh::animated(RenderMode::Masked, 40, &[1, 2, 3, 4])], + vec![Mesh::animated(RenderMode::Additive, 7, &[0, 1])], 987_654_321), + ("degenerate animation (zero ms, empty frame list)", + vec![Mesh::animated(RenderMode::Opaque, 0, &[2, 2]), + Mesh { mode: RenderMode::Opaque, tex: Some(11), anim: Some((50, vec![])) }], + vec![], 5_000), + ]; + + for (name, statics, instanced, now_ms) in &cases { + assert_eq!(new(statics, instanced, *now_ms), old(statics, instanced, *now_ms), + "#741: the extracted zone plan diverged from the pre-#741 closures on case {name:?}"); + } +} + +// ── The routing itself ─────────────────────────────────────────────────────────────────────────── + +/// **The mode partition.** Every render mode is drawn by exactly one sub-pass of each source — never +/// twice (double-draw, wrong blending) and never zero times (invisible geometry). This is the +/// property the pre-#741 `modes: &[RenderMode]` lists had to satisfy by hand at six call sites. +#[test] +fn every_render_mode_is_drawn_exactly_once_per_source() { + for mode in ALL_MODES { + let statics = vec![Mesh::new(mode, Some(1))]; + let instanced = vec![Mesh::new(mode, Some(1))]; + let steps: Vec<_> = plan_zone_draws(&statics, &instanced, 0).collect(); + let s = steps.iter().filter(|s| s.source == ZoneMeshSource::Static).count(); + let i = steps.iter().filter(|s| s.source == ZoneMeshSource::Instanced).count(); + assert_eq!((s, i), (1, 1), + "{mode:?} must be drawn exactly once from each mesh list, got \ + {s} static / {i} instanced draws"); + } + // and the classes themselves partition the mode space + for mode in ALL_MODES { + let accepting = [ZoneBlendClass::OpaqueMasked, ZoneBlendClass::Blend, + ZoneBlendClass::Additive].iter().filter(|c| c.accepts(mode)).count(); + assert_eq!(accepting, 1, "{mode:?} must be accepted by exactly one blend class"); + } +} + +/// **Sub-pass order.** All depth-writing geometry precedes all depth-write-off geometry, and within +/// a class the static terrain precedes the placed objects standing on it. Asserted on the plan, so a +/// reordering of `ZONE_SUBPASSES` fails here whether or not any mesh happens to be present. +#[test] +fn depth_writing_subpasses_run_before_transparent_ones_static_first() { + let order: Vec<(ZoneMeshSource, ZoneBlendClass)> = + ZONE_SUBPASSES.iter().map(|s| (s.source, s.class)).collect(); + assert_eq!(order, vec![ + (ZoneMeshSource::Static, ZoneBlendClass::OpaqueMasked), + (ZoneMeshSource::Instanced, ZoneBlendClass::OpaqueMasked), + (ZoneMeshSource::Static, ZoneBlendClass::Blend), + (ZoneMeshSource::Instanced, ZoneBlendClass::Blend), + (ZoneMeshSource::Static, ZoneBlendClass::Additive), + (ZoneMeshSource::Instanced, ZoneBlendClass::Additive), + ], "#741: zone sub-pass order changed — transparent geometry must be drawn after every \ + depth-writing sub-pass, or it depth-occludes the opaque world behind it"); + + // and the plan honours that order for real meshes + let meshes: Vec = ALL_MODES.iter().map(|&m| Mesh::new(m, Some(1))).collect(); + let meshes2: Vec = ALL_MODES.iter().map(|&m| Mesh::new(m, Some(1))).collect(); + let subpasses: Vec = + plan_zone_draws(&meshes, &meshes2, 0).map(|s| s.subpass).collect(); + assert!(subpasses.windows(2).all(|w| w[0] <= w[1]), + "plan steps must come out in sub-pass order, got {subpasses:?}"); + assert_eq!(subpasses.len(), 8, "4 modes × 2 lists = 8 draws, got {}", subpasses.len()); +} + +/// **The texture cache is per sub-pass, not per frame.** Group 1 is bound by the pipeline-setting +/// step of each sub-pass, so the same texture used by the last mesh of one sub-pass and the first +/// mesh of the next must be bound again — the pipeline switch in between does not preserve it. +/// Eliding that rebind would leave the second sub-pass sampling whatever the first left behind. +/// +/// **The limit of this test.** The reset is *structural* in `plan_zone_draws` — the `scan` seed is +/// re-evaluated per sub-pass — so no one-line condition controls it, and a guard added in front of +/// it is unreachable. That was measured, not assumed: mutation Z5 (`if *started && …`) left this +/// file and the whole crate green, which is why that dead guard was deleted rather than kept as +/// belt-and-braces. What this test does catch is the state being hoisted out of the `flat_map` so it +/// genuinely spans sub-passes — mutation Z7, which turns this red. +#[test] +fn the_texture_cache_does_not_survive_a_subpass_boundary() { + // One blend mesh and one additive mesh, same texture, adjacent sub-passes of the same source. + let statics = vec![Mesh::new(RenderMode::Blend, Some(7)), Mesh::new(RenderMode::Additive, Some(7))]; + let binds: Vec = plan_zone_draws(&statics, &[] as &[Mesh], 0).map(|s| s.bind).collect(); + assert_eq!(binds, vec![ZoneTexBind::Set(Some(7)), ZoneTexBind::Set(Some(7))], + "each sub-pass must bind group 1 for its first mesh even when the previous sub-pass \ + already had that texture bound"); + + // Within one sub-pass the elision still applies. + let run = vec![Mesh::new(RenderMode::Opaque, Some(7)), Mesh::new(RenderMode::Masked, Some(7)), + Mesh::new(RenderMode::Opaque, Some(8))]; + let binds: Vec = plan_zone_draws(&run, &[] as &[Mesh], 0).map(|s| s.bind).collect(); + assert_eq!(binds, vec![ZoneTexBind::Set(Some(7)), ZoneTexBind::Keep, ZoneTexBind::Set(Some(8))], + "a repeated texture within one sub-pass must not be rebound"); +} + +/// **`set_pipeline` marks the first step of a non-empty sub-pass and only that step.** An empty +/// sub-pass emits nothing at all — no pipeline switch, no binds — which is what keeps a zone with no +/// additive geometry from paying for two pipeline switches every frame. +#[test] +fn an_empty_subpass_emits_nothing_and_a_nonempty_one_sets_its_pipeline_once() { + let statics = vec![Mesh::new(RenderMode::Opaque, Some(1)), Mesh::new(RenderMode::Opaque, Some(2))]; + let cmds = new(&statics, &[] as &[Mesh], 0); + assert_eq!(cmds.iter().filter(|c| matches!(c, Cmd::SetPipeline(_))).count(), 1, + "one non-empty sub-pass must set exactly one pipeline, got {cmds:?}"); + assert_eq!(cmds.iter().filter(|c| *c == &Cmd::BindCamera).count(), 1); + assert_eq!(cmds.iter().filter(|c| *c == &Cmd::BindShadow).count(), 1); + assert!(!cmds.iter().any(|c| matches!(c, Cmd::DrawInstanced(_))), + "an empty instanced list must produce no instanced draw"); + + let steps: Vec<_> = plan_zone_draws(&statics, &[] as &[Mesh], 0).collect(); + assert_eq!(steps.iter().filter(|s| s.set_pipeline).count(), 1); + assert!(steps[0].set_pipeline, "the first step of a sub-pass sets the pipeline"); +} + +/// **A mesh's index is its index in its own list**, not its position within the sub-pass — the sink +/// uses it to index `gpu_meshes`/`gpu_instanced` directly, so an off-by-one here draws the wrong +/// geometry. +#[test] +fn a_step_carries_the_meshs_index_in_its_own_list() { + let statics = vec![Mesh::new(RenderMode::Blend, Some(1)), Mesh::new(RenderMode::Opaque, Some(1)), + Mesh::new(RenderMode::Blend, Some(1))]; + let blend: Vec = plan_zone_draws(&statics, &[] as &[Mesh], 0) + .filter(|s| s.class == ZoneBlendClass::Blend).map(|s| s.mesh).collect(); + assert_eq!(blend, vec![0, 2], + "the blend sub-pass skips mesh 1, but its steps must still name meshes 0 and 2"); +} From 2d3eca00c46ec3c96465602e169def81db16d650 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 10:18:29 -0400 Subject: [PATCH 2/2] docs(#739,#741): correct three claims this PR made false, and disclose the ungraded ZoneSink Review round 2. No code defect was found; every blocking finding was a false or stale claim, and three of them were text this PR wrote. B1: tests/shadow_routing.rs said the other three sub-passes of encode_shadow_pass (skinned casters, static casters, the depth-attachment clear) "remain as untested as they were before #721". Two of those three are what this PR tests, so this PR falsified that sentence without touching it. Only the depth-attachment clear is still uncovered there. The paragraph now records that it has been discharged twice (#740, then here) with the count wrong after each, so the next person treats it as a recurring maintenance point rather than a fact. B3: the zone test file claimed "everything which decides is graded and the handle lookup is not". That was false. Two bodies of encode_zone_pass's ZoneSink do decide and are ungraded: draw picks gpu_meshes vs gpu_instanced, and bind_texture picks a texture bind group vs the fallback. Reproduced both as mutations S1 and S2, both survive crate-wide at 227 passed / 0 failed. Neither is a regression -- both survive identically on the base -- so they are disclosed, not fixed; closing them needs a device or an enum indirection, which is a refactor. Both test files now enumerate their sink's bodies the way shadow_routing.rs already did, and CharacterSink is stated separately because for that one "decides nothing" is true. N4: adds a_pipeline_setting_step_never_elides_its_texture_bind, a property over the whole frame corpus asserting set_pipeline implies bind == Set. That is the invariant the guard deleted in round 1 looked like it was defending; deleting it was right, but the invariant was left true only by construction and asserted nowhere. Mutation Z7 reddens it. N1/N2: two doc comments referenced frame_tex and tex_bg closures that round 1 deleted. N3: "Z7 exists because that test would otherwise have no failing arm at all" was measurably false; Z4 reddens it too. The false workspace baseline is in the PR comments, which is where it was published; the commit body never carried figures. Closes #739 Closes #741 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-renderer/src/pass.rs | 48 +++++++++--- .../tests/character_shadow_routing.rs | 26 +++++-- .../eqoxide-renderer/tests/shadow_routing.rs | 24 ++++-- .../tests/zone_pass_routing.rs | 74 +++++++++++++++---- 4 files changed, 135 insertions(+), 37 deletions(-) diff --git a/crates/eqoxide-renderer/src/pass.rs b/crates/eqoxide-renderer/src/pass.rs index cb9cd132..7643949b 100644 --- a/crates/eqoxide-renderer/src/pass.rs +++ b/crates/eqoxide-renderer/src/pass.rs @@ -113,9 +113,10 @@ impl InstancedShadowCaster for crate::gpu::GpuInstancedMesh { } /// The texture a mesh should bind at `now_ms`: its current animation frame if animated, else its -/// static texture. **The single definition** — `encode_zone_pass`'s `frame_tex` closure delegates -/// here, so the color pass and the masked shadow sub-pass cannot drift apart, and this function's -/// tests grade both. +/// static texture. **The single definition** — both the colour pass and the masked shadow sub-pass +/// reach it through their planners ([`plan_zone_draws`] and [`plan_instanced_shadow_draws`]), so +/// they cannot drift apart, and this function's tests grade both. (Until #741 the colour side went +/// through a `frame_tex` closure in `encode_zone_pass` that delegated here; that closure is gone.) /// /// The masked shadow sub-pass needs this, not the raw `texture_idx`: an animated `RenderMode::Masked` /// caster's shadow must alpha-test against the SAME texel the color pass is sampling, or the two @@ -1011,13 +1012,28 @@ pub fn encode_zone_pass( occlusion_query_set: None, }); - // Everything decided for this pass — the six sub-passes and their order, which mesh list and - // which render modes each one draws, which animated texture frame a mesh binds, and when a - // group-1 rebind is actually needed — comes from `plan_zone_draws` (#741), which is device-free - // and unit tested in tests/zone_pass_routing.rs (with a differential pin against the pre-#741 - // closures in the same file). All that is left here is `ZoneSink`: five bodies that turn plan - // vocabulary into `wgpu` handles. Nothing in this impl decides anything — if you find yourself - // adding a condition to it, it belongs in the planner. + // The pass ROUTING — the six sub-passes and their order, which mesh list and which render modes + // each one draws, which animated texture frame a mesh binds, and when a group-1 rebind is + // actually needed — comes from `plan_zone_draws` (#741), which is device-free and unit tested in + // tests/zone_pass_routing.rs (with a differential pin against the pre-#741 closures there). + // + // `ZoneSink` below is the rest: five bodies that turn plan vocabulary into `wgpu` handles, ALL + // of which need a live device and NONE of which any test can reach. Two of the five make real + // decisions, so "nothing here decides anything" would be false — measured, by the #784 reviewer, + // with two mutations that both left the crate green at 226 passed / 0 failed: + // + // * `draw` picks `gpu_meshes` vs `gpu_instanced` from the `ZoneMeshSource`. Pointing the + // `Static` arm at `gpu_instanced` draws the wrong geometry for every static zone mesh, and + // no test notices (mutation S1). + // * `bind_texture` picks `texture_bind_groups[i]` vs `fallback_texture_bg`. Ignoring the index + // and always binding the fallback untextures the whole zone, and no test notices (S2). + // + // Both predate #741 (they survive identically on the base file) and are recorded rather than + // fixed here; closing them needs a device or a further indirection, not another planner test. + // The other three bodies — the six-arm pipeline lookup, `camera_uniform.bind_group`, + // `shadow_sample_bg` — are straight lookups with no condition in them. + // + // If you add a condition to this impl, it belongs in the planner, where it is testable. struct ZoneSink<'r, 'p, 'e> { r: &'r EqRenderer, pass: &'p mut wgpu::RenderPass<'e>, @@ -2124,7 +2140,8 @@ pub fn encode_shadow_pass( } fn bind_texture(&mut self, idx: Option) { let r = self.r; - // The out-of-range fallback is the same one `encode_zone_pass`'s `tex_bg` applies. + // The out-of-range fallback is the same one `encode_zone_pass`'s `ZoneSink::bind_texture` + // applies. (It named a `tex_bg` closure there until #741 replaced it with that method.) let bg = match idx { Some(i) if i < r.texture_bind_groups.len() => &r.texture_bind_groups[i], _ => &r.fallback_texture_bg, @@ -2149,7 +2166,14 @@ pub fn encode_shadow_pass( // the one-time pipeline/group-0 bind per sub-pass, and the fact that only skinned casters bind a // joint palette all come from `plan_character_shadow_draws` (#739), device-free and unit tested // in tests/character_shadow_routing.rs (with a differential pin against the pre-#739 loops). - // `CharacterSink` below is the `wgpu`-handle translation and decides nothing. + // `CharacterSink` below is the `wgpu`-handle translation: five bodies, none reachable by a test. + // Four are straight lookups (`shadow_skinned`/`shadow_static`, `light_depth_bg`, + // `shadow_uniform_pool[u_slot]`, `shadow_joint_pool[j_slot]`). `draw` re-matches the caster's own + // variant, which is a branch but not a decision: the two arms differ only in the concrete model + // type and are otherwise the same loop, so they cannot be swapped — the compiler rejects it. + // Contrast `encode_zone_pass`'s `ZoneSink`, where two bodies DO decide and are ungraded; see the + // note there. Do not paraphrase this as "decides nothing" — an earlier draft did, and the + // equivalent sentence in the zone pass was measurably false. impl CharacterShadowCaster for Caster<'_> { fn shadow_bind(&self) -> CharacterShadowBind { match *self { diff --git a/crates/eqoxide-renderer/tests/character_shadow_routing.rs b/crates/eqoxide-renderer/tests/character_shadow_routing.rs index 0262fb79..9dccf2dc 100644 --- a/crates/eqoxide-renderer/tests/character_shadow_routing.rs +++ b/crates/eqoxide-renderer/tests/character_shadow_routing.rs @@ -34,11 +34,27 @@ //! //! ## What this file does NOT cover //! -//! - **The two-arm pipeline lookup in `encode_shadow_pass`'s `CharacterSink::set_pipeline`.** -//! Swapping `shadow_skinned` and `shadow_static` *there* is not observable from here, because this -//! file's sink maps the same `CharacterShadowKind` to a string. Left open deliberately rather than -//! pinned as source text: a source-text pin has been measured on this crate to be evadable by -//! shadowing a local (#773 review, evasions E1b/E2), so it would buy a claim it cannot keep. +//! - **All five bodies of `encode_shadow_pass`'s `CharacterSink`.** This file supplies its own sink, +//! so nothing it does can reach the production one. Enumerated, in the style `shadow_routing.rs` +//! already uses for the instanced sub-pass: +//! +//! 1. `set_pipeline` — the two-arm `CharacterShadowKind` → `&wgpu::RenderPipeline` match. Swapping +//! `shadow_skinned` and `shadow_static` there is invisible here, because this file's sink maps +//! the same kind to a string. Left open **deliberately** rather than for cost: pinning it means +//! asserting on the source text of `pass.rs`, and a source-text pin has been measured on this +//! crate to be evadable by shadowing a local (#773 review, evasions E1b/E2). +//! 2. `bind_light_depth` — `light_depth_bg` at group 0. A straight lookup. +//! 3. `bind_model_uniform` — `shadow_uniform_pool[u_slot].1` at group 1. A straight lookup; an +//! out-of-range slot panics rather than falling back. +//! 4. `bind_joints` — `shadow_joint_pool[j_slot].1` at group 2. Same. +//! 5. `draw` — re-matches the caster's own variant to reach `model.meshes`. A branch, but not a +//! decision: the two arms differ only in the concrete model type and are otherwise the same +//! loop, so swapping them does not compile. +//! +//! None of the five decides anything a mutation could silently flip. That is **not** true of the +//! sibling `ZoneSink` in `encode_zone_pass`, where two bodies do decide and are ungraded — see +//! `zone_pass_routing.rs`'s list. The distinction is stated because an earlier draft of both files +//! used one blanket sentence for both sinks, and for the zone one it was measurably false. //! - **wgpu semantics**, and **whether a flip is visible in play** — #739 listed the second as not //! established, and this PR does not settle it. No client was run. //! - **Which entities are selected at all** — that is `plan_shadow_casters` (#740), graded in diff --git a/crates/eqoxide-renderer/tests/shadow_routing.rs b/crates/eqoxide-renderer/tests/shadow_routing.rs index 26cd1542..e0f020dd 100644 --- a/crates/eqoxide-renderer/tests/shadow_routing.rs +++ b/crates/eqoxide-renderer/tests/shadow_routing.rs @@ -38,13 +38,23 @@ //! `execute_instanced_shadow_plan` call in `encode_shadow_pass` were deleted or wrapped in //! `if false`. (The executor's own logic *is* graded, in `shadow_routing_equivalence.rs`; it is //! the one call site that is not.) -//! - **Anything about the other three sub-passes** in `encode_shadow_pass` (skinned casters, static -//! casters, the depth-attachment clear). Those remain as untested as they were before #721. The -//! caster-selection/culling that fills `casters` was a *fourth* item in this list until #740, -//! which extracted it to `pass::plan_shadow_casters` and graded it in -//! `shadow_caster_selection.rs`; what selection *turns into* (matrices, buffer writes) is still -//! uncovered. (**Corrected:** #740's first draft dropped the item but left the count reading -//! "four" — an off-by-one this file introduced, not an inherited one.) +//! - **The depth-attachment clear** in `encode_shadow_pass`. That is the last of the three items +//! this bullet originally listed and the only one still uncovered here. +//! +//! This bullet is a **recurring maintenance point** — it has now been discharged twice, and the +//! count has been wrong after each discharge, so check it rather than trusting it. It read "the +//! other three sub-passes (skinned casters, static casters, the depth-attachment clear) … remain +//! as untested as they were before #721", plus a *fourth* item for the caster selection that +//! fills `casters`. #740 extracted that fourth to `pass::plan_shadow_casters` and graded it in +//! `shadow_caster_selection.rs` (leaving the count reading "four" — an off-by-one this file +//! introduced). #739 then extracted the **skinned and static** sub-passes to +//! `pass::plan_character_shadow_draws` and graded them in `character_shadow_routing.rs`, which +//! falsified two more of the three without touching this sentence. +//! +//! What is *still* uncovered from those discharges, and is not restated by the files that took +//! them over: what caster selection turns into (matrices, buffer writes), and — exactly as in the +//! first bullet above — `encode_shadow_pass`'s `CharacterSink` handle lookups, which +//! `character_shadow_routing.rs` enumerates for itself. use eqoxide_assets::RenderMode; use eqoxide_renderer::pass::{ diff --git a/crates/eqoxide-renderer/tests/zone_pass_routing.rs b/crates/eqoxide-renderer/tests/zone_pass_routing.rs index ca90af07..6108ed8a 100644 --- a/crates/eqoxide-renderer/tests/zone_pass_routing.rs +++ b/crates/eqoxide-renderer/tests/zone_pass_routing.rs @@ -18,13 +18,32 @@ //! //! ## What this file does NOT cover //! -//! - **The six-arm pipeline lookup in `encode_zone_pass`'s `ZoneSink::set_pipeline`.** That match -//! turns a `(ZoneMeshSource, ZoneBlendClass)` pair into a `&wgpu::RenderPipeline`, and this file's -//! sink turns the same pair into a `&'static str`. Swapping two arms *in the production sink* is -//! not observable from here. That is a real hole and it is left open deliberately: pinning it -//! would mean asserting on the source text of `pass.rs`, and a source-text pin has now been -//! measured on this crate to be evadable by shadowing a local (#773 review, evasions E1b/E2). The -//! honest boundary is that everything which *decides* is graded and the handle lookup is not. +//! - **All five bodies of `encode_zone_pass`'s `ZoneSink`.** This file supplies its own sink, so +//! nothing it does can reach the production one. Enumerated, in the style `shadow_routing.rs` +//! already uses for the instanced sub-pass: +//! +//! 1. `set_pipeline` — the six-arm `(ZoneMeshSource, ZoneBlendClass)` → `&wgpu::RenderPipeline` +//! match. Swapping two arms there is invisible here, because this file's sink maps the same +//! pair to a `&'static str`. +//! 2. `bind_texture` — `texture_bind_groups[i]` vs `fallback_texture_bg`. **This one decides.** +//! Ignoring the index and always binding the fallback untextures the whole zone (mutation S2). +//! 3. `draw` — `gpu_meshes` vs `gpu_instanced`, and the static vs instanced buffer/draw form. +//! **This one decides too.** Pointing the `Static` arm at `gpu_instanced` draws the wrong +//! geometry for every static zone mesh (mutation S1). +//! 4. `bind_camera` — `camera_uniform.bind_group` at group 0. A straight lookup. +//! 5. `bind_shadow_sample` — `shadow_sample_bg` at group 2. A straight lookup. +//! +//! **An earlier draft of this paragraph claimed "everything which *decides* is graded and the +//! handle lookup is not". That was false**, and items 2 and 3 are why: the #784 reviewer mutated +//! both in the production sink and both left the crate green at 226 passed / 0 failed. Neither is +//! a regression — both survive identically on the base file — but the description was wrong about +//! where the hole is and how wide it is. **They remain ungraded after this PR.** Closing them +//! needs a live device or a further indirection between the plan and the mesh list, which is a +//! refactor rather than the coverage this issue asked for. +//! +//! Item 1 is additionally left open *deliberately* rather than for cost: pinning it would mean +//! asserting on the source text of `pass.rs`, and a source-text pin has now been measured on this +//! crate to be evadable by shadowing a local (#773 review, evasions E1b/E2). //! - **wgpu semantics.** Nothing here creates a `wgpu::RenderPass`, so a command sequence that is //! correct as a sequence but invalid as wgpu (wrong group index for a layout, say) still passes. //! - **Whether a flip is visible in play.** #741 asked; it is still not established. A wrong @@ -187,9 +206,11 @@ const ALL_MODES: [RenderMode; 4] = /// this test**: it deliberately freezes pre-#741 behaviour, so it is exactly what should fail if /// someone later *intends* to change the zone draw order (say, sorting meshes by texture to cut /// rebinds). Update it as part of that change; do not weaken it to make a diff go green. -#[test] -fn the_extracted_plan_emits_the_pre_741_command_stream() { - let cases: Vec<(&str, Vec, Vec, u64)> = vec![ +/// The frame corpus: every mode in both lists, texture runs, animated meshes at three points of +/// their cycle, degenerate animations, and empty sub-passes. Shared by the differential pin and by +/// the `set_pipeline ⟹ Set` property below, so a case added for one is graded by both. +fn corpus() -> Vec<(&'static str, Vec, Vec, u64)> { + vec![ ("empty", vec![], vec![], 0), ("statics only, one of each mode", ALL_MODES.iter().map(|&m| Mesh::new(m, Some(1))).collect(), vec![], 0), @@ -219,14 +240,40 @@ fn the_extracted_plan_emits_the_pre_741_command_stream() { vec![Mesh::animated(RenderMode::Opaque, 0, &[2, 2]), Mesh { mode: RenderMode::Opaque, tex: Some(11), anim: Some((50, vec![])) }], vec![], 5_000), - ]; + ] +} - for (name, statics, instanced, now_ms) in &cases { +#[test] +fn the_extracted_plan_emits_the_pre_741_command_stream() { + for (name, statics, instanced, now_ms) in &corpus() { assert_eq!(new(statics, instanced, *now_ms), old(statics, instanced, *now_ms), "#741: the extracted zone plan diverged from the pre-#741 closures on case {name:?}"); } } +/// **A pipeline-setting step always carries `Set`, never `Keep`.** +/// +/// This is the invariant the deleted `*started &&` guard used to assert by hand. Removing that +/// guard (see `the_texture_cache_does_not_survive_a_subpass_boundary`) was correct — it was +/// unreachable — but it left the invariant true only by construction and asserted nowhere, so a +/// later change to the cache could make a sub-pass start with group 1 holding the *previous* +/// sub-pass's texture and nothing would say so. This turns that reading argument into a check. +/// +/// Universal over the whole corpus rather than one example, because the claim is universal. +#[test] +fn a_pipeline_setting_step_never_elides_its_texture_bind() { + for (name, statics, instanced, now_ms) in &corpus() { + for step in plan_zone_draws(statics, instanced, *now_ms) { + if step.set_pipeline { + assert!(matches!(step.bind, ZoneTexBind::Set(_)), + "case {name:?}: sub-pass {} starts with {:?} — the first step of a sub-pass must \ + bind group 1, since the pipeline switch does not preserve it", + step.subpass, step.bind); + } + } + } +} + // ── The routing itself ─────────────────────────────────────────────────────────────────────────── /// **The mode partition.** Every render mode is drawn by exactly one sub-pass of each source — never @@ -289,7 +336,8 @@ fn depth_writing_subpasses_run_before_transparent_ones_static_first() { /// it is unreachable. That was measured, not assumed: mutation Z5 (`if *started && …`) left this /// file and the whole crate green, which is why that dead guard was deleted rather than kept as /// belt-and-braces. What this test does catch is the state being hoisted out of the `flat_map` so it -/// genuinely spans sub-passes — mutation Z7, which turns this red. +/// genuinely spans sub-passes — mutation Z7 — and the elision being removed outright — mutation Z4. +/// Both turn this red; Z7 is the one that targets the boundary specifically. #[test] fn the_texture_cache_does_not_survive_a_subpass_boundary() { // One blend mesh and one additive mesh, same texture, adjacent sub-passes of the same source.