diff --git a/crates/eqoxide-renderer/src/pass.rs b/crates/eqoxide-renderer/src/pass.rs index aa861d1..7643949 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 @@ -223,6 +224,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 +760,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 +1012,86 @@ 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, + // 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>, + } + 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 @@ -1776,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, @@ -1797,42 +2162,77 @@ 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: 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 { + 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 0000000..9dccf2d --- /dev/null +++ b/crates/eqoxide-renderer/tests/character_shadow_routing.rs @@ -0,0 +1,285 @@ +//! **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 +//! +//! - **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 +//! `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/shadow_routing.rs b/crates/eqoxide-renderer/tests/shadow_routing.rs index 26cd154..e0f020d 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 new file mode 100644 index 0000000..6108ed8 --- /dev/null +++ b/crates/eqoxide-renderer/tests/zone_pass_routing.rs @@ -0,0 +1,388 @@ +//! **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 +//! +//! - **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 +//! 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. +/// 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), + ("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), + ] +} + +#[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 +/// 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 — 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. + 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"); +}