diff --git a/crates/eqoxide-renderer/src/gpu.rs b/crates/eqoxide-renderer/src/gpu.rs index 625d5d53..bf5cbf1a 100644 --- a/crates/eqoxide-renderer/src/gpu.rs +++ b/crates/eqoxide-renderer/src/gpu.rs @@ -142,7 +142,10 @@ pub struct GpuStaticModel { /// Used to compute the ground lift so models stand at Z=0 instead of floating or sinking. pub y_bottom: f32, /// Vertical extent of the model (max_y - min_y) in buffer vertex space. - /// Used to compute visual_scale: visual_scale = 2 * y_extent * arch_scale. + /// NOT read by this crate's render path: since #768 a static placement takes its whole vertical + /// lift from `y_bottom` (see `models::static_placement`). `visual_scale = 2 * y_extent * + /// arch_scale` survives only in the standalone `render_model` viewer bin, which reads this + /// field at `src/bin/render_model.rs:1266`. /// Separate from y_bottom because chr.s3d models may have vertices far above Y=0 /// (e.g. feet at Y=20), making y_bottom unreliable as a height proxy. pub y_extent: f32, diff --git a/crates/eqoxide-renderer/src/models.rs b/crates/eqoxide-renderer/src/models.rs index 58a745ee..c62b8ded 100644 --- a/crates/eqoxide-renderer/src/models.rs +++ b/crates/eqoxide-renderer/src/models.rs @@ -129,7 +129,10 @@ pub struct ModelAsset { /// raw pre-node-scale positions. Lift = y_bottom × mesh_scale (dominant). pub y_bottom: f32, /// Vertical extent of the model (max_y - min_y) in buffer vertex space. - /// Used for visual_scale: visual_scale = 2 × y_extent × arch_scale. + /// Read here as the `true_height` fallback when the glTF carries no `eq_height` extra. It is + /// NOT part of static placement: since #768 `static_placement` takes the whole lift from + /// `y_bottom`. `visual_scale = 2 × y_extent × arch_scale` survives only in the standalone + /// `render_model` viewer bin (`src/bin/render_model.rs:1097`). pub y_extent: f32, /// Center of the model in the X and Z axes (raw pre-node-scale space, dominant meshes only). /// Used as a centering correction so models are rendered at their entity position rather than @@ -712,12 +715,6 @@ pub fn race_to_archetype(race: &str) -> &'static str { } } -/// Fixed display scale (EQ units) for each glTF archetype. -/// All models with node_scale=100 have that applied during loading, so these -/// values reflect the effective post-scale model height in EQ units. -/// Arch-scale is a multiplier such that `visual_scale = 2 * y_bottom * arch_scale * node_scale` -/// equals the desired total character height in EQ units (feet-to-head). -/// Calibrated from actual GLTF vertex bounds; review after adding new models. /// Per-archetype model-space orientation fix-up applied in `entity_model_matrix_heading` (#149). /// Most models need none (identity). The shared substitute `fish.glb` is authored with its /// nose→tail along the model's Y axis; after the standard Y-up→Z-up conversion that axis points @@ -898,20 +895,44 @@ pub fn skinned_target_height(race: &str, archetype: &str, true_height: f32) -> f /// /// It is NOT the only copy in the repository, and saying otherwise would be false: the standalone /// model-viewer binary (`src/bin/render_model.rs`, in the root `eqoxide` crate — it cannot depend on -/// this crate's pass module) still writes `2.0 * y_extent * arch_scale` and -/// `vscale * 0.5 + y_bottom * arch_scale` by hand at three places. Consequence, stated because it is -/// real: the viewer has no `floating` concept at all, so it still draws `boat.glb` 13.9275u above -/// its turntable while the game now draws it at the stored z. #756 did not change the viewer; -/// converging it is its own change. +/// this crate's pass module) still writes `2.0 * y_extent * arch_scale` (`render_model.rs:1097` +/// and `:1266`) and `vscale * 0.5 + y_bottom * arch_scale` (`:1271`) by hand. Consequence, stated +/// because it is real: the viewer has no `floating` concept at all and did not take #768's +/// correction either, so its static arm still lifts a model by `(y_extent + y_bottom) * arch_scale` +/// — the formula #768 replaced here. How far off that puts a given model on the turntable is not +/// stated, because the viewer's `arch_scale` comes from a CLI-supplied archetype name +/// (`render_model.rs:814`) and no run of the viewer was made for this change. Neither #756 nor #768 +/// changed the viewer; converging it is its own change, and it is not a pure deletion there — the +/// viewer also feeds `visual_scale` to its camera distance (`:1098`), its camera focus height +/// (`:1291`) and its marker placement (`:1471`), so removing the term reframes the turntable. #[derive(Clone, Copy, Debug, PartialEq)] pub struct StaticPlacement { /// Uniform mesh scale fed to `entity_model_matrix_heading`'s `mesh_scale`. pub mesh_scale: f32, - /// Z-lift fed to `entity_model_matrix_heading`'s `visual_scale` (halved there). - pub visual_scale: f32, /// Horizontal recentre fed to `entity_model_matrix_heading`'s `center_xz`. pub center_xz: [f32; 2], - /// Extra lift fed to `entity_model_matrix_heading`'s `y_bottom` (scaled by `mesh_scale` there). + /// The WHOLE vertical lift, fed to `entity_model_matrix_heading`'s `y_bottom` (scaled by + /// `mesh_scale` there). `0.0` on the floating arm. + /// + /// There is deliberately no `visual_scale` field. `entity_model_matrix_heading` lifts by + /// `visual_scale * 0.5 + y_bottom * mesh_scale`, and the static call sites pass a literal `0.0` + /// for `visual_scale`, so this field is the only lift a static *placement* can express. #768 was + /// exactly a second lift term hiding in `visual_scale`. + /// + /// Dropping the field removes that term from this shared helper. It does NOT make the term + /// unwritable, and there are TWO ways back to it, not one — a caller can hand + /// `2.0 * model.y_extent * p.mesh_scale` to `entity_model_matrix_heading` (nothing here + /// constrains that argument), and a caller can hand `model.y_bottom + model.y_extent` to this + /// function's `y_bottom` (nothing here constrains that either — an `f32` is an `f32`). Both were + /// measured to leave every behavioural test in this crate green. + /// + /// What holds them today is a source-text pin over `pass.rs`, + /// `tests/floating_placement.rs::every_static_placement_in_pass_rs_is_written_exactly_as_reviewed`, + /// which requires each of the four call sites to be spelled exactly as reviewed. That bounds the + /// four call sites in that one file. It is NOT a type-level guarantee and it does not reach a + /// caller in another file; making the state unrepresentable would mean this function taking the + /// model's measured bounds as an opaque value only the loader can mint, which is a wider change + /// than #768 and is not done here. pub y_bottom: f32, } @@ -921,30 +942,46 @@ pub struct StaticPlacement { /// different meanings of the entity's stored **z** (#756): /// /// - **grounded** (`false`) — the stored z is eqoxide's FOOT datum: the ingest path subtracted -/// `WIRE_Z_OFFSET` (`crates/eqoxide-core/src/coord.rs:115` `wire_z_to_foot`). The model's local -/// bounding box is arbitrary, so the model is lifted off that z. This arm is byte-for-byte the -/// pre-#756 formula. +/// `WIRE_Z_OFFSET` (`crates/eqoxide-core/src/coord.rs:115` `wire_z_to_foot`). The model is lifted +/// so its **lowest vertex sits on that z**, which is the rule `GpuStaticModel::y_bottom` already +/// states (`gpu.rs:141-142`, "used to compute the ground lift so models stand at Z=0 instead of +/// floating or sinking"). /// -/// It does **not** lift the model to *stand on* that z, and saying so would be false. Measured by -/// building this arm's matrix and mapping `boat.glb`'s bounding box through it (stored z 4, -/// heading 0): drawn origin z `17.927488`, **lowest vertex z `13.945171`** — the hull's bottom -/// lands `9.945171` ABOVE the stored z, which is exactly `y_extent * mesh_scale`, a full rendered -/// model height. Standing on the z would need a lift of `y_bottom * mesh_scale` = `3.982317`, -/// which is what `gpu.rs:180` states the rule to be ("lift = y_bottom × mesh_scale") and what the -/// calibration note at line 718 above assumes (`visual_scale = 2 * y_bottom * arch_scale`, where -/// the code computes `2 * y_extent`). The axis direction is not assumed: +/// The lift is `y_bottom * mesh_scale` and nothing else. Until #768 this arm ALSO passed +/// `visual_scale = 2 * y_extent * mesh_scale`, which `entity_model_matrix_heading` halves and adds +/// on top, so the real lift was `(y_extent + y_bottom) * mesh_scale` and the model's bottom landed +/// a full rendered model height above the stored z. Measured by mapping `boat.glb`'s bounding box +/// through this arm's production matrix (stored z 4, heading 0): drawn origin z was `17.927488` +/// with the lowest vertex at `13.945171` — `9.945171` too high, exactly `y_extent * mesh_scale`. +/// It is now `7.982317` / `4.000000`. The axis direction is not assumed: /// `camera.rs:264` `static_model_y_up_axis_maps_to_world_up` pins that a static model's +Y -/// becomes world +Z. +/// becomes world +Z, and `tests/floating_placement.rs` re-derives it from the matrix's own +/// columns before reading a lowest vertex off it. /// -/// So of the `13.927488` that #756 removes for a floating hull, only `3.982317` is the datum -/// error #756 is about; the other `9.945171` is the grounded arm over-lifting *any* static model. -/// That is a separate defect with a separate blast radius and is NOT fixed here — filed as #768 -/// with the measurement above. Nothing renders wrong today because of it, which follows from two -/// measured facts: `Entity::floating()` is `skips_wire_z_offset(is_boat, flymode)` -/// (`eqoxide-core/src/game_state.rs:192`), true for every boat regardless of flymode; and -/// `boat.glb` is the only model `model_for` can load with no skin. Since the grounded arm is only -/// reached by a static model, the inference is that it has **no live consumer** today. That is -/// also why a false sentence could sit here undetected. +/// Residual, stated because it is real and NOT fixed here: `y_bottom` is `-y_min` only when +/// `y_min < 0` and `0.0` otherwise (see the loader's reduction in this file). For a model whose +/// vertices all sit above its local origin this arm therefore lifts by `0` and the bottom lands +/// `y_min * mesh_scale` above the stored z. No shipped model exercises that clamp — `boat.glb` is +/// the only unskinned model `model_for` can load and its `y_min` is `-3.982317` — so #768 left the +/// clamp alone rather than change a datum the skinned path also reads. +/// +/// Nothing rendered wrong because of the over-lift — true as measured on 2026-07-28, and stated +/// with the mechanism that actually decides it, because an earlier version of this paragraph got +/// that mechanism wrong. A model takes the static arm when +/// `!(0 < joint_count <= 128)` (`renderer.rs:663-664`): no skin at all, **or** a skin with zero +/// joints, **or** a skin with MORE than 128 joints. "Unskinned" is sufficient, not necessary. +/// Scanned every name `model_for` can ask for (18 archetypes + 29 `race_*` + the 3 `_f` variants +/// that exist = 50 files): exactly one lands on the static arm, `boat.glb`, at `skins == 0`. +/// Nothing is over the 128 cap. `Entity::floating()` is `skips_wire_z_offset(is_boat, flymode)` +/// (`eqoxide-core/src/game_state.rs:192`), true for every boat regardless of flymode, so the +/// grounded arm has no live consumer today. +/// +/// **The margin is one joint, and it is not ours to hold.** `race_pcfroglok.glb` has 127 joints +/// against the 128 cap; 11 rigs are at 109 or more. The model directory is an externally rebaked +/// sync target. A two-joint rebake of that one file would put a PC race on the grounded arm, +/// where `floating()` is false — which is why this arm has to be correct rather than merely +/// unreached. The absence of a live consumer is also why a false sentence could sit here +/// undetected, and why #768 has no live before/after to show. /// - **floating** (`true`) — two separate steps, held to different standards: /// /// 1. *That the current lift is wrong* is certain from the code alone. `wire_z_to_foot` @@ -981,16 +1018,20 @@ pub struct StaticPlacement { /// real and unresolved; it is not what #756 is about. /// /// Magnitude of the grounded arm applied to a floating hull, from the measured `boat.glb` bounds -/// (`y_extent = 9.9452`, `y_bottom = 3.9823`, `archetype_scale("boat") = 1.0`): -/// `2*9.9452*1.0*0.5 + 3.9823*1.0 = 13.9275` units of lift on a 9.9452-unit-tall model. +/// (`y_extent = 9.9452`, `y_bottom = 3.9823`, `archetype_scale("boat") = 1.0`): before #768 the two +/// arms differed by `2*9.9452*1.0*0.5 + 3.9823*1.0 = 13.9275` units of lift on a 9.9452-unit-tall +/// model; after it they differ by `3.9823`, the grounded arm's whole lift. +/// +/// `y_extent` is no longer a parameter. It was only ever read to build the `visual_scale` term #768 +/// removed, so keeping it would have left the over-lift one edit away from returning. pub fn static_placement( - archetype: &str, y_extent: f32, y_bottom: f32, center_xz: [f32; 2], floating: bool, + archetype: &str, y_bottom: f32, center_xz: [f32; 2], floating: bool, ) -> StaticPlacement { let mesh_scale = archetype_scale(archetype); if floating { - StaticPlacement { mesh_scale, visual_scale: 0.0, center_xz, y_bottom: 0.0 } + StaticPlacement { mesh_scale, center_xz, y_bottom: 0.0 } } else { - StaticPlacement { mesh_scale, visual_scale: 2.0 * y_extent * mesh_scale, center_xz, y_bottom } + StaticPlacement { mesh_scale, center_xz, y_bottom } } } diff --git a/crates/eqoxide-renderer/src/pass.rs b/crates/eqoxide-renderer/src/pass.rs index b0c6bc6d..aa861d1e 100644 --- a/crates/eqoxide-renderer/src/pass.rs +++ b/crates/eqoxide-renderer/src/pass.rs @@ -1107,10 +1107,10 @@ pub fn encode_player_pass( // `floating: false` — the player's z is the CharacterController's FOOT datum, not // a wire passthrough, so the player is never a model-origin placement (#756). let p = crate::models::static_placement( - archetype, model.y_extent, model.y_bottom, + archetype, model.y_bottom, [model.x_center, model.z_center], false); let mat = crate::camera::entity_model_matrix_heading( - scene.player_pos, scene.player_heading, p.visual_scale, p.mesh_scale, + scene.player_pos, scene.player_heading, 0.0, p.mesh_scale, p.center_xz, true, p.y_bottom, crate::models::archetype_correction(archetype), ); @@ -1266,8 +1266,8 @@ pub fn encode_entity_pass( // so `static_placement` drops the grounding lift for it (#756). The horizontal // recentre is unchanged — that datum was not established; see the fn's doc. let p = crate::models::static_placement( - archetype, model.y_extent, model.y_bottom, [model.x_center, model.z_center], b.floating); - let mat = crate::camera::entity_model_matrix_heading(b.pos, b.heading, p.visual_scale, p.mesh_scale, + archetype, model.y_bottom, [model.x_center, model.z_center], b.floating); + let mat = crate::camera::entity_model_matrix_heading(b.pos, b.heading, 0.0, p.mesh_scale, p.center_xz, true, p.y_bottom, crate::models::archetype_correction(archetype)); for (mesh_idx, mesh) in model.meshes.iter().enumerate() { if slot >= slot_end { break; } @@ -1677,10 +1677,10 @@ pub fn encode_shadow_pass( (Some(GpuModel::Static(model)), ShadowCasterDraw::Static) => { // Same placement call as the color pass, so the shadow tracks the body // (see this fn's doc). `floating: false` — the player is never one (#756). - let p = static_placement(archetype, model.y_extent, model.y_bottom, + let p = static_placement(archetype, model.y_bottom, [model.x_center, model.z_center], false); let mat = crate::camera::entity_model_matrix_heading( - scene.player_pos, scene.player_heading, p.visual_scale, p.mesh_scale, + scene.player_pos, scene.player_heading, 0.0, p.mesh_scale, p.center_xz, true, p.y_bottom, archetype_correction(archetype)); write_model(step.u_slot, mat); @@ -1711,11 +1711,12 @@ pub fn encode_shadow_pass( } (Some(GpuModel::Static(model)), ShadowCasterDraw::Static) => { // Same placement call as the color pass, so a floating hull's shadow - // tracks the hull instead of staying 13.9275u above it (#756). - let p = static_placement(archetype, model.y_extent, model.y_bottom, + // tracks the hull instead of staying above it (#756; the gap between the two + // arms is 3.9823u for `boat.glb` since #768 corrected the grounded lift). + let p = static_placement(archetype, model.y_bottom, [model.x_center, model.z_center], b.floating); let mat = crate::camera::entity_model_matrix_heading( - b.pos, b.heading, p.visual_scale, p.mesh_scale, + b.pos, b.heading, 0.0, p.mesh_scale, p.center_xz, true, p.y_bottom, archetype_correction(archetype)); write_model(step.u_slot, mat); diff --git a/crates/eqoxide-renderer/tests/floating_placement.rs b/crates/eqoxide-renderer/tests/floating_placement.rs index ba975a34..071b89fe 100644 --- a/crates/eqoxide-renderer/tests/floating_placement.rs +++ b/crates/eqoxide-renderer/tests/floating_placement.rs @@ -1,4 +1,6 @@ //! #756 — the grounding lift must not be applied to a floating spawn's z. +//! #768 — and when it IS applied, it must put the model's lowest vertex ON the stored z rather than +//! a full rendered model height above it. //! //! ## What is certain here, and what is inferred //! @@ -47,15 +49,27 @@ //! same day counted 130: the model directory is a live sync target, so the *total* moves. The //! loadable subset below and the result did not. //! -//! 1. **`boat.glb` is the only model `model_for` can load with `skins == 0`.** The loadable names -//! are the 18 distinct archetypes `race_to_archetype` can return (`humanoid`, `elf`, `dwarf`, -//! `gnoll`, `skeleton`, `zombie`, `creature`, `rat`, `snake`, `frog`, `wasp`, `wolf`, `bat`, -//! `bird`, `worm`, `fish`, `bear`, `boat`) plus the 29 `race_*` player models — 47 names, all 47 -//! present. Every one reported `skins == 1` except `boat.glb`, at `skins == 0`. The remaining -//! files are zone/door/weapon assets `model_for` never names. Also scanned, because the first -//! pass missed them: the `_f.glb` female variants `model_for` prefers for `gender == 1` -//! (`renderer.rs:610`). Three exist on disk — `humanoid_f`, `elf_f`, `dwarf_f` — and all three -//! reported `skins == 1`, so they do not change the conclusion. +//! 1. **`boat.glb` is the only model `model_for` can load that lands on the STATIC arm.** State the +//! gate first, because an earlier version of this note stated the wrong one: `renderer.rs:663-664` +//! picks the skinned path when `0 < joint_count <= 128`, so the static arm is +//! `!(0 < joint_count <= 128)` — no skin, a skin with zero joints, **or a skin with more than 128 +//! joints**. `skins == 0` is therefore *sufficient but not necessary*, and a scan that only reads +//! `len(skins)` does not establish which arm a model takes. +//! +//! Re-scanned on 2026-07-28 reading `len(skins[0].joints)` as well. Loadable names: the 18 +//! distinct archetypes `race_to_archetype` can return (`humanoid`, `elf`, `dwarf`, `gnoll`, +//! `skeleton`, `zombie`, `creature`, `rat`, `snake`, `frog`, `wasp`, `wolf`, `bat`, `bird`, +//! `worm`, `fish`, `bear`, `boat`) plus the 29 `race_*` player models plus the 3 `_f.glb` +//! female variants that exist (`humanoid_f`, `elf_f`, `dwarf_f`; `model_for` prefers them for +//! `gender == 1`, `renderer.rs:610`) — **50 files, all present**. Exactly one lands on the static +//! arm: `boat.glb`, `skins == 0`, 0 joints. Nothing is above the cap. The remaining files on disk +//! are zone/door/weapon assets `model_for` never names. +//! +//! **This is a margin of one joint on a directory rebaked outside this repo.** +//! `race_pcfroglok.glb` sits at **127** joints against the 128 cap, and 11 rigs are at ≥ 109 +//! (next highest 110). A two-joint rebake of that file moves a PC race — for which +//! `Entity::floating()` is false — onto the grounded arm. The grounded arm being unreached today +//! is a fact about the current asset bake, not a property of the code. //! 2. **A skinned rig's raw vertex origin is not a stable EQ datum.** `race_hum.glb` reported //! `y_min = -10.430519`; `race_huf.glb` reported `y_min = -3.688780`. Both are nominally //! 6.0-foot humans (`race_target_height("HUM") == 6.0`), so their raw origins sit at wildly @@ -82,8 +96,20 @@ fn boat_center_xz() -> [f32; 2] { [(BOAT_X_MIN + BOAT_X_MAX) * 0.5, (BOAT_Z_MIN + BOAT_Z_MAX) * 0.5] } -/// Build the production model matrix for a static entity and return its translation column — -/// where the model's local origin (0,0,0) is drawn in world space. +/// Build the production model matrix for a static `boat.glb` entity — the exact calls the render +/// passes make (`models::static_placement` fed into `camera::entity_model_matrix_heading`), with the +/// literal `0.0` the static call sites pass for `visual_scale` since #768. +/// +/// Column-major (`glam`'s `to_cols_array_2d`), so `m[col][row]`. +fn boat_matrix(pos: [f32; 3], heading: f32, floating: bool) -> [[f32; 4]; 4] { + let p = static_placement("boat", boat_y_bottom(), boat_center_xz(), floating); + entity_model_matrix_heading( + pos, heading, 0.0, p.mesh_scale, p.center_xz, true, p.y_bottom, + archetype_correction("boat"), + ) +} + +/// Where the model's local origin (0,0,0) is drawn in world space — the matrix's translation column. /// /// Only the **z** of this is asserted on below. The horizontal recentre survives into the x/y of /// this column, and #756 deliberately did not touch it: the wire *xy* datum was never established @@ -91,15 +117,39 @@ fn boat_center_xz() -> [f32; 2] { /// `(-c0, 0, -c1)` translate that the `+90°` X rotation maps to `(-c0, c1, 0)`, after which only /// the heading yaw (a Z rotation) is applied — so the z assertion is unaffected by it either way. fn drawn_origin(pos: [f32; 3], heading: f32, floating: bool) -> [f32; 3] { - let p = static_placement( - "boat", boat_y_extent(), boat_y_bottom(), boat_center_xz(), floating); - let m = entity_model_matrix_heading( - pos, heading, p.visual_scale, p.mesh_scale, p.center_xz, true, p.y_bottom, - archetype_correction("boat"), - ); + let m = boat_matrix(pos, heading, floating); [m[3][0], m[3][1], m[3][2]] } +/// World z of the lowest point of the drawn hull, by mapping `boat.glb`'s measured bounding box +/// through the production matrix. +/// +/// **Why a bounding-box corner is a real vertex here, checked rather than assumed.** The AABB's +/// minimum world z is only the mesh's minimum world z if world z depends on exactly one local axis +/// — otherwise the lowest AABB corner can be a point no vertex occupies. So this asserts the +/// matrix's own third row first: the x and z columns must contribute 0 to world z, leaving +/// `world_z = m[1][2] * local_y + m[3][2]`. A real vertex attains `local_y == BOAT_Y_MIN` (that is +/// where the accessor's `min` comes from), so the corner IS a vertex. That check also re-derives, +/// from the matrix rather than from a second test's name, the fact `camera.rs`'s +/// `static_model_y_up_axis_maps_to_world_up` pins: local +Y becomes world +Z. +fn drawn_lowest_vertex_z(pos: [f32; 3], heading: f32, floating: bool) -> f32 { + let m = boat_matrix(pos, heading, floating); + assert!(m[0][2].abs() < 1e-5 && m[2][2].abs() < 1e-5, + "world z must depend only on the model's local Y for a bbox corner to be a real vertex; \ + x-col {} z-col {} (heading {heading})", m[0][2], m[2][2]); + + let mut lowest = f32::INFINITY; + for x in [BOAT_X_MIN, BOAT_X_MAX] { + for y in [BOAT_Y_MIN, BOAT_Y_MAX] { + for z in [BOAT_Z_MIN, BOAT_Z_MAX] { + let wz = m[0][2] * x + m[1][2] * y + m[2][2] * z + m[3][2]; + if wz < lowest { lowest = wz; } + } + } + } + lowest +} + /// THE regression: no grounding lift is applied to a floating spawn's stored z, which leaves the /// hull's authored origin on that z. /// @@ -121,36 +171,67 @@ fn a_floating_spawn_gets_no_grounding_lift_so_its_origin_stays_on_the_stored_z() ); } -/// The size of the bug, stated as the difference between the two arms rather than as a bare -/// constant: with `boat.glb`'s measured bounds the grounded arm lifts the hull 13.9275u — more -/// than the model's own 9.9452u height — and slides it 3.9849u along its length axis. +/// **#768's regression.** The grounded arm puts the model's LOWEST VERTEX on the stored z. +/// +/// Graded on the lowest vertex, not on the lift scalar, because that is how the bug hid: the lift +/// was `(y_extent + y_bottom) * mesh_scale` instead of `y_bottom * mesh_scale`, and every assertion +/// #756 shipped read either the lift magnitude or the drawn ORIGIN — both of which the buggy arm +/// satisfies just as happily as the correct one. Mapping the bounding box through is what shows the +/// hull sitting `9.945171` in the air. +/// +/// Swept over headings because the lift and the horizontal recentre are applied on opposite sides of +/// the heading yaw; a lift that leaked into the rotated part would land on the stored z at heading 0 +/// and drift elsewhere. #[test] -fn the_grounded_arm_would_lift_a_boat_clear_of_its_own_height() { - let grounded = static_placement( - "boat", boat_y_extent(), boat_y_bottom(), boat_center_xz(), false); +fn a_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_z() { + let pos = [1200.0_f32, -640.0, 4.0]; + for heading in [0.0_f32, 90.0, 137.0, 180.0, 270.0, 359.5] { + let lowest = drawn_lowest_vertex_z(pos, heading, false); + assert!((lowest - pos[2]).abs() < 1e-3, + "a grounded static model's lowest vertex must be drawn on its stored z; heading \ + {heading}: lowest vertex z {lowest}, stored z {} (delta {}). Before #768 this delta \ + was 9.945171 — `y_extent * mesh_scale`, a full rendered model height.", + pos[2], lowest - pos[2]); + } +} + +/// The size of the correction, stated as the difference between the two arms rather than as a bare +/// constant: with `boat.glb`'s measured bounds the grounded arm lifts the hull 3.9823u — its +/// `y_bottom`, LESS than the model's own 9.9452u height — and slides it 3.9849u along its length +/// axis. +/// +/// The `lift < height` direction is the half that would have caught #768. It is not a general law +/// about static models (a model whose origin sits above its own top would legitimately exceed it); +/// it is a statement about `boat.glb`'s measured bounds, which is why the constants are named here. +#[test] +fn the_grounded_arm_lifts_a_boat_by_its_y_bottom_not_by_its_whole_height() { + let grounded = static_placement("boat", boat_y_bottom(), boat_center_xz(), false); let arch = archetype_scale("boat"); - // `entity_model_matrix_heading` lifts by `visual_scale * 0.5 + y_bottom * mesh_scale`. - let lift = grounded.visual_scale * 0.5 + grounded.y_bottom * grounded.mesh_scale; - assert!((lift - 13.9275).abs() < 1e-3, "grounded lift for boat.glb: expected 13.9275, got {lift}"); - assert!(lift > boat_y_extent() * arch, - "the defect: lift {lift} exceeds the model's whole rendered height {}", + // `entity_model_matrix_heading` lifts by `visual_scale * 0.5 + y_bottom * mesh_scale`, and the + // static call sites pass a literal `0.0` for `visual_scale` (there is no longer a field for it). + let lift = grounded.y_bottom * grounded.mesh_scale; + assert!((lift - 3.9823).abs() < 1e-3, "grounded lift for boat.glb: expected 3.9823, got {lift}"); + assert!(lift < boat_y_extent() * arch, + "#768: the lift {lift} must not reach the model's whole rendered height {} — that is the \ + over-lift that put the hull in the air", boat_y_extent() * arch); - // And with the same bounds, the floating arm removes both lift terms — and ONLY those. The - // horizontal recentre is passed through untouched, because #756 established the z datum and - // not the xy one; pinning that here keeps a later change from quietly widening the exemption - // into an axis nobody measured. - let floating = static_placement( - "boat", boat_y_extent(), boat_y_bottom(), boat_center_xz(), true); - assert_eq!(floating.visual_scale, 0.0, "floating placement must carry no lift"); - assert_eq!(floating.y_bottom, 0.0, "floating placement must carry no y_bottom lift"); + // And with the same bounds, the floating arm removes the lift — and ONLY that. The horizontal + // recentre is passed through untouched, because #756 established the z datum and not the xy + // one; pinning that here keeps a later change from quietly widening the exemption into an axis + // nobody measured. + let floating = static_placement("boat", boat_y_bottom(), boat_center_xz(), true); + assert_eq!(floating.y_bottom, 0.0, "floating placement must carry no lift"); assert_eq!(floating.center_xz, grounded.center_xz, "the floating arm must not change the horizontal recentre — that datum is unestablished"); } /// The exemption must not become "nothing is ever grounded". A grounded spawn with the *same* -/// bounds still gets the full lift — without this, deleting the whole formula would pass. +/// bounds still has its ORIGIN lifted off the stored z — without this, deleting the whole formula +/// would pass. `boat.glb`'s origin ends up `y_bottom * mesh_scale` = 3.9823u up, so the `> 1.0` +/// threshold is loose on purpose: this test grades "some lift survives", and +/// `a_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_z` grades how much. #[test] fn a_grounded_spawn_with_the_same_bounds_is_still_lifted() { let pos = [1200.0_f32, -640.0, 4.0]; @@ -165,8 +246,7 @@ fn a_grounded_spawn_with_the_same_bounds_is_still_lifted() { #[test] fn the_floating_exemption_changes_placement_only_not_scale() { for floating in [false, true] { - let p = static_placement( - "boat", boat_y_extent(), boat_y_bottom(), boat_center_xz(), floating); + let p = static_placement("boat", boat_y_bottom(), boat_center_xz(), floating); assert_eq!(p.mesh_scale, archetype_scale("boat"), "mesh_scale must stay archetype_scale for floating={floating}"); } @@ -256,6 +336,25 @@ const MODELS_RS: &str = include_str!("../src/models.rs"); /// So the list is pinned set-equal to the archetypes `race_to_archetype` can actually return, /// parsed from its source. Same technique and same caveat as the call-site pin below: source text, /// not semantics. +/// +/// ## The parser's own escape hatches, closed (#769) +/// +/// The parse is literally "arrow, space, double-quote". Four match-arm spellings return an +/// archetype it cannot see, and `cargo fmt` normalizes only two of them — and this repo runs no +/// `cargo fmt` job in CI at all (`.github/workflows/test.yml` has `no-local-detail` and `test`, and +/// no fmt or clippy step), so even that mitigation is a convention rather than a gate: +/// +/// | spelling | `cargo fmt` normalizes | +/// |---|---| +/// | `"AIR"=>"airship"` | yes | +/// | `"AIR" =>`⏎`"airship"` | yes | +/// | `"AIR" => { "airship" }` | no | +/// | `"AIR" => AIRSHIP` (named const) | no | +/// +/// The arrow-count assert below reddens all four: it requires every `=>` in the parsed region to be +/// the start of a `=> "` string-literal arm, so an arm the extractor skips shows up as a bare arrow +/// with no matching literal. Each of the four was applied to `models.rs` and measured RED +/// individually — see the PR for #769 for the counts. #[test] fn all_archetypes_is_every_archetype_race_to_archetype_can_return() { let body = MODELS_RS @@ -263,6 +362,19 @@ fn all_archetypes_is_every_archetype_race_to_archetype_can_return() { .expect("race_to_archetype not found in models.rs").1; let body = body.split_once("\npub fn ").map(|(b, _)| b).unwrap_or(body); + // Anti-escape: the extractor below only sees `=> "…"`. Any other arm spelling would be silently + // dropped from `from_source` and the set-equality would then compare a short list against a + // short list and pass. Requiring the two counts to match means an unparseable arm cannot hide. + // 19/19 today. (This assumes no `=>` appears in a comment or string inside the parsed region — + // which the equality itself enforces, since such an arrow would have no matching `=> "`.) + assert_eq!( + body.matches("=>").count(), + body.matches("=> \"").count(), + "every arm of race_to_archetype must return a string literal spelled `=> \"…\"` — the \ + archetype parser below only sees that spelling, so an arm written `=> {{ \"x\" }}` or \ + `=> NAMED_CONST` would be invisible to it and to the property it feeds", + ); + let mut from_source: Vec<&str> = body .match_indices("=> \"") .map(|(i, _)| { @@ -300,8 +412,43 @@ fn all_archetypes_is_every_archetype_race_to_archetype_can_return() { /// the "must pass `b.floating`" half alone would miss. Measured: rewriting the entity sites to /// `b.floating && false` — which still mentions `b.floating`, so `from_spawn` stays at 2 — trips the /// `player` half instead, at `found 4 of 2`, because those call sites now end in `false` too. +/// +/// ## This parser examined for #769's escape class, and what is left open +/// +/// #769 asked whether this sibling has the same "one keystroke away from blind" hole. It has one, +/// and it is closed below: `match_indices("static_placement(")` requires the name and the paren to +/// be adjacent, so a call written `static_placement (…)` is invisible — `calls.len()` stays 4 and +/// both halves stay 2, i.e. green with an unreviewed call site. `cargo fmt` would remove that space, +/// but this repo runs no fmt job in CI, so the assert is the guard. +/// +/// Four residual holes are NOT closed, stated rather than papered over: +/// - **A call site in another file.** This test reads only `pass.rs`. A `static_placement` call +/// added to a new render module is invisible to it. What still forces a decision there is the +/// API, not this test: `floating` is a required argument with no default (#756). "Four call +/// sites" is a count of `pass.rs` call sites; it is not a bound on callers elsewhere. +/// - **A fifth producer of the flag, outside `pass.rs`.** `Billboard.floating` is set in two +/// places: `Scene::from_game_state` derives it (`src/scene.rs:337`, `e.floating()`), and +/// `Scene::inject_test_billboards` hardcodes `floating: false` (`src/scene.rs:248`). The +/// hardcode is not currently reachable by a static-arm model: it is called only when +/// `self.scene.zone == "testzone"` (`src/app.rs:1267-1269`), and its race table +/// (`src/scene.rs:189-205`) lists 16 character races with no `SHP` entry, so the one model that +/// takes the static arm today (`boat.glb`) is never injected. That chain holds by asset and +/// zone facts, not by construction — adding `SHP` to that table would grade a grounded static +/// model with a hardcoded flag, and nothing here would go red. +/// - **A call whose argument list contains `);` before its own close** — the slice would end early +/// and the two halves would read a truncated call. No such call exists today (that needs a +/// statement inside an argument, e.g. a block or closure body). +/// - **A trailing comma after `false`.** `player` matches on `ends_with("false")`, so +/// `…, false,\n)` would count 0 and this test would go RED on correct code. That direction is +/// loud rather than silent, so it is left as-is. #[test] fn every_static_placement_call_site_in_pass_rs_decides_floating_explicitly() { + // Anti-escape for the extractor immediately below (#769): it only sees the name and the paren + // adjacent, so a space between them would drop a whole call site out of every count here. + assert!(!PASS_RS.contains("static_placement ("), + "a `static_placement (…)` call site (space before the paren) is invisible to the parser \ + below, which would leave this pin green with an unreviewed call site"); + let calls: Vec<&str> = PASS_RS .match_indices("static_placement(") .map(|(i, _)| { @@ -320,9 +467,110 @@ fn every_static_placement_call_site_in_pass_rs_decides_floating_explicitly() { assert_eq!(from_spawn, 2, "the entity pass and the nearby shadow-caster arm must pass `b.floating`, not a literal — \ - passing `false` there restores the pre-#756 rendering (a boat 13.9275u above the water) \ - and no other test in this crate would notice; found {from_spawn} of 2"); + passing `false` there puts the hull above the water (13.9275u before #768 corrected the \ + grounded lift, 3.9823u after) and no other test in this crate would notice; \ + found {from_spawn} of 2"); assert_eq!(player, 2, "the player pass and the player shadow-caster arm must pass a literal `false`; \ found {player} of 2"); } + +/// **#768's lift, pinned at both source-text channels through which `pass.rs` can re-create it.** +/// The behavioural tests above cannot reach either one: they call `static_placement` with their own +/// arguments and build the matrix with a literal `0.0`, so they grade the helper, never the call. +/// +/// There are exactly two channels, and both were measured green before being closed: +/// +/// 1. **Out of the placement, into the matrix.** `model` is in scope at all four call sites and +/// `GpuStaticModel::y_extent` is public, so `2.0 * model.y_extent * p.mesh_scale` can be handed +/// to `entity_model_matrix_heading`'s `visual_scale`. Measured with this test present, that edit +/// at the entity call site reddens this test and nothing else in the crate: +/// `--no-fail-fast` over the whole crate gave **214 passed / 1 failed / 11 ignored**, the one +/// failure being this test. (Without `--no-fail-fast` cargo stops after the failing binary and +/// only 2 of the 10 binaries report — a run that reddens here is not evidence about the rest.) +/// 2. **Into the placement.** Nothing about dropping the `visual_scale` FIELD constrains what is +/// passed as `y_bottom`. `static_placement(archetype, model.y_bottom + model.y_extent, …)` +/// restores the identical pre-#768 lift `(y_extent + y_bottom) * mesh_scale`. Measured by the +/// round-1 reviewer of PR #773 and reproduced here before writing anything down: with only the +/// channel-1 half present, the crate stayed **green at 215 passed / 0 failed / 11 ignored**, this +/// test included — it read the matrix call, not the placement call. That is the finding this test +/// was extended for; with the `REVIEWED_ARGS` check below, the same mutation now fails on the +/// `REVIEWED_ARGS` assert (10 passed / 1 failed in this binary), printing the offending call. +/// +/// Channel 2 is closed by pinning the whole argument list, not one argument: an expression is not +/// something source text can bound (`model.y_bottom` vs `model.y_bottom + 0.0` vs a helper call are +/// all "the second argument"), so the test instead requires each call to be written EXACTLY as one +/// of two reviewed spellings. **The consequence is deliberate and is the loud direction**: renaming +/// the `model` binding, or adding a fifth site with any other argument, turns this RED on correct +/// code and asks for a review. That is the trade this repo's verification hierarchy prefers over a +/// comment. +/// +/// **What this still does not do**, stated because the previous version of this doc overstated it: +/// - It is source text, not semantics. It proves the argument is *written* that way, not that the +/// call is reached, and not that `model.y_bottom` itself holds what the loader intended. +/// - It only reads `pass.rs`. A static placement built in another file is invisible to it, and +/// "four call sites" is a count of `pass.rs` call sites — it is not a bound on callers of +/// `static_placement` anywhere else in the workspace. +/// - Whitespace is normalized, so a re-wrapped argument list is not a violation; a renamed +/// *variable* is. +#[test] +fn every_static_placement_in_pass_rs_is_written_exactly_as_reviewed() { + // Anti-escape, same class as #769's: the extractor needs the name and the paren adjacent. + assert!(!PASS_RS.contains("entity_model_matrix_heading ("), + "an `entity_model_matrix_heading (…)` call (space before the paren) is invisible to the \ + parser below"); + + // ── Channel 2: what goes IN to static_placement ────────────────────────────────────────── + // (the sibling test above grades the `floating` argument's provenance and counts the sites; + // this grades the exact spelling of all four arguments, which is what bounds the lift.) + const REVIEWED_ARGS: [&str; 2] = [ + "archetype, model.y_bottom, [model.x_center, model.z_center], false", + "archetype, model.y_bottom, [model.x_center, model.z_center], b.floating", + ]; + let placements: Vec = PASS_RS + .match_indices("static_placement(") + .map(|(i, _)| { + let rest = &PASS_RS[i..]; + let call = &rest[..rest.find(");").expect("unterminated static_placement( call")]; + call.split_whitespace().collect::>().join(" ") + }) + .collect(); + assert_eq!(placements.len(), 4, + "pass.rs must call static_placement at exactly the 4 known sites; found {}", + placements.len()); + for call in &placements { + let args = call.split_once('(').expect("no argument list").1.trim(); + assert!(REVIEWED_ARGS.contains(&args), + "#768: a static model's whole vertical lift is the `y_bottom` argument, so each call \ + site must pass the model's OWN measured bounds and nothing derived from them — \ + `model.y_bottom + model.y_extent` here restores the exact pre-#768 over-lift and no \ + behavioural test in this crate would fail. Expected one of {REVIEWED_ARGS:?}, found: \ + {args}"); + } + + // ── Channel 1: what comes OUT of the placement, into the matrix ────────────────────────── + let statics: Vec = PASS_RS + .match_indices("entity_model_matrix_heading(") + .map(|(i, _)| { + let rest = &PASS_RS[i..]; + let call = &rest[..rest.find(");").expect("unterminated matrix call in pass.rs")]; + call.split_whitespace().collect::>().join(" ") + }) + // A static placement is the only thing that feeds `p.y_bottom` into this call; the skinned + // and non-entity sites do not. + .filter(|c| c.contains("p.y_bottom")) + .collect(); + + assert_eq!(statics.len(), 4, + "pass.rs must build exactly the 4 known static-model matrices (entity, player, and both \ + static shadow-caster arms); found {}", statics.len()); + + for call in &statics { + assert!(call.contains(", 0.0, p.mesh_scale"), + "#768: a static model's whole vertical lift comes from `p.y_bottom`, so every static \ + call site must pass the literal `0.0` for `visual_scale` (the argument immediately \ + before `mesh_scale`). `entity_model_matrix_heading` adds `visual_scale * 0.5` on top \ + of `y_bottom * mesh_scale`, so anything else here re-creates the over-lift that put \ + the model a full rendered height above its stored z. Offending call: {call}"); + } +}