diff --git a/crates/eqoxide-core/src/region_map.rs b/crates/eqoxide-core/src/region_map.rs index c7f0d229..e4bc22ff 100644 --- a/crates/eqoxide-core/src/region_map.rs +++ b/crates/eqoxide-core/src/region_map.rs @@ -23,8 +23,8 @@ use std::path::Path; /// EQEmu region type for a zone-line region. const REGION_ZONE_LINE: i32 = 3; -/// **Why a zone's `.wtr` region data is NOT available** — the fact [`RegionMap::load`]'s `Option` -/// throws away (#762). +/// **Why a zone's `.wtr` region data is NOT available** — the fact the old lossy +/// `RegionMap::load`'s `Option` threw away (#762; that function is now deleted, #803). /// /// A `None` from the old loader collapsed five different facts into one value: *there is no file*, /// *the file is there but couldn't be read* (permission error, bad mount, a directory in its @@ -73,6 +73,59 @@ impl std::fmt::Display for RegionLoadError { } } +/// **Why a collision grid has no region data to answer from** — the fact a `None` water slot used +/// to throw away (#803). +/// +/// [`RegionLoadError`] says why a `.wtr` *load* failed. This says why a grid cannot answer a +/// water/zone-line question at all, which is strictly larger: it also covers the grid that was +/// never handed any region data in the first place (a synthetic test scene, or a grid read before +/// the zone's `.wtr` was consulted). Both are **absences**, and neither is a fact about the world — +/// but the code they feed used to turn them into one, because +/// `Collision::zone_line_indices()` answered an empty `Vec` in every case and +/// `/v1/observe/zone_exits` published that as `[]` with 200 OK. +/// +/// `[]` is also the true, common answer for a zone that genuinely has no zone lines, so the agent +/// could not tell the two apart — and exits are the only way out of a zone. Carrying the reason +/// here is what lets the endpoint refuse instead of inventing an answer. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum RegionDataAbsent { + /// No region data was ever attached to this collision grid — nothing was loaded and nothing + /// failed. A grid in this state has not been *asked* about the zone's `.wtr`, so any water or + /// zone-line answer off it is about nothing. + NotAttached, + /// The zone's `.wtr` was consulted and did not load. Carries the loader's own verdict. + LoadFailed(RegionLoadError), +} + +impl RegionDataAbsent { + /// The machine-readable `reason` an agent reads off a refusal. Distinct per cause on purpose: + /// "the file isn't there" and "the file is truncated" call for different operator action + /// (re-sync the asset pack vs. re-bake it), and collapsing them would repeat the substitution + /// this type exists to kill one level up. + pub fn as_str(&self) -> &'static str { + match self { + Self::NotAttached => "region_data_not_attached", + Self::LoadFailed(e) => match e { + RegionLoadError::Missing => "region_data_missing", + RegionLoadError::Unreadable(_) => "region_data_unreadable", + RegionLoadError::NotRegionData => "region_data_not_region_data", + RegionLoadError::UnsupportedVersion(_) => "region_data_unsupported_version", + RegionLoadError::Truncated { .. } => "region_data_truncated", + }, + } + } +} + +impl std::fmt::Display for RegionDataAbsent { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::NotAttached => + write!(f, "no region data has been attached to this collision grid"), + Self::LoadFailed(e) => write!(f, "{e}"), + } + } +} + #[derive(Clone, Copy)] struct BspNode { normal: [f32; 3], @@ -301,8 +354,9 @@ impl RegionMap { } /// Load `/.wtr` (v1 or v2 BSP), **keeping the failure** ([`RegionLoadError`]) instead - /// of collapsing it to a bare absence. Prefer this over [`RegionMap::load`] everywhere the - /// caller's answer would otherwise depend on data it never read (#762). + /// of collapsing it to a bare absence (#762). This is the ONLY loader — the lossy + /// `Option`-returning wrapper it used to share this impl with was deleted in #803, so a caller + /// can no longer answer off data it never read without first writing the discard by hand. pub fn try_load(dir: &Path, zone: &str) -> Result { let path = dir.join(format!("{zone}.wtr")); let d = std::fs::read(&path).map_err(|e| if e.kind() == std::io::ErrorKind::NotFound { @@ -345,17 +399,15 @@ impl RegionMap { Ok(RegionMap { nodes }) } - /// [`RegionMap::try_load`] with the failure discarded. - /// - /// ⚠️ **LOSSY, and lossy in the direction that lies (#762).** A `None` here is - /// indistinguishable from "loaded, and this zone simply has no water and no zone lines", and - /// callers that store it as `set_water(None)` publish exactly that reading. Only use it where a - /// caller genuinely does not care WHY there is no region data; anything that will report a - /// number, a count, or an emptiness derived from water/zone-line state must use `try_load` (or - /// `eqoxide_nav::water_grid::ZoneWater`, which carries the failure for you). - pub fn load(dir: &Path, zone: &str) -> Option { - Self::try_load(dir, zone).ok() - } + // `RegionMap::load` — `try_load` with the failure discarded — WAS here. It is gone (#803), not + // merely unused: while it existed, `load(..).map(Arc::new)` remained the shortest way to get a + // region map onto a collision grid, and it is lossy in the direction that lies. Its `None` was + // indistinguishable from "loaded, and this zone simply has no water and no zone lines", and + // every caller stored it where that absence stops being an absence and becomes an ANSWER — the + // zone reads as having no water anywhere and, worse, **no zone-line regions anywhere**, which + // `/v1/observe/zone_exits` published as a bare `[]` with 200 OK. Exits are the only way out of a + // zone, so an agent read that as "sealed in" (#803). Use `try_load` and keep the + // [`RegionLoadError`]; `Collision::set_region_data` takes exactly that `Result`. /// Walk the BSP from node 1 to the leaf containing the server-coord point. The swap to (y,x,z) /// matches EQEmu's WaterMapV1::ReturnRegionType. Returns `None` if the tree is empty or the walk @@ -773,18 +825,27 @@ mod tests { assert_eq!(unique.len(), msgs.len(), "reasons must be distinguishable: {msgs:?}"); } - /// The lossy `load` is exactly `try_load(..).ok()` — one loader, so the two can never drift into - /// disagreeing about what a valid `.wtr` is. + /// Every absence an agent can be shown must be a DIFFERENT machine-readable `reason` — the same + /// rule `region_load_error_messages_are_distinguishable` applies to the loader verdicts, carried + /// one level up to the value `/v1/observe/zone_exits` refuses with (#803). If two collapsed, an + /// agent handed one could not tell "re-sync the asset pack" from "re-bake it". #[test] - fn load_is_try_load_with_the_reason_thrown_away() { - let dir = tempfile::tempdir().unwrap(); - assert!(RegionMap::load(dir.path(), "absent").is_none()); - std::fs::write(dir.path().join("ok.wtr"), wtr_v2(&[ - ([0.0, 0.0, 1.0], -2.0, 0, 2, 3, 0), - ([0.0; 3], 0.0, 0, 0, 0, 0), - ([0.0; 3], 0.0, 1, 0, 0, 0), - ])).unwrap(); - assert!(RegionMap::load(dir.path(), "ok").is_some()); + fn region_data_absent_reasons_are_distinguishable() { + let all = [ + RegionDataAbsent::NotAttached, + RegionDataAbsent::LoadFailed(RegionLoadError::Missing), + RegionDataAbsent::LoadFailed(RegionLoadError::Unreadable(std::io::ErrorKind::PermissionDenied)), + RegionDataAbsent::LoadFailed(RegionLoadError::NotRegionData), + RegionDataAbsent::LoadFailed(RegionLoadError::UnsupportedVersion(9)), + RegionDataAbsent::LoadFailed(RegionLoadError::Truncated { declared_nodes: 1, bytes: 40 }), + ]; + let reasons: std::collections::HashSet<&str> = all.iter().map(|a| a.as_str()).collect(); + assert_eq!(reasons.len(), all.len(), "reasons must be distinguishable: {reasons:?}"); + // And none of them may read as an empty answer: the whole point is that they are NOT `[]`. + for a in &all { assert!(!a.as_str().is_empty() && !a.to_string().is_empty()); } + // The load failures must carry the loader's own sentence through, not a flattened summary. + assert_eq!(RegionDataAbsent::LoadFailed(RegionLoadError::Missing).to_string(), + RegionLoadError::Missing.to_string()); } #[test] @@ -797,7 +858,7 @@ mod tests { ]); let dir = tempfile::tempdir().unwrap(); std::fs::write(dir.path().join("z.wtr"), &blob).unwrap(); - let rm = RegionMap::load(dir.path(), "z").expect("v2 loads"); + let rm = RegionMap::try_load(dir.path(), "z").expect("v2 loads"); // A point below the split is in the zone-line region carrying index 1. assert_eq!(rm.zone_line_at(10.0, 20.0, -5.0), Some(1)); // A point above is not a zone line. @@ -829,7 +890,7 @@ mod tests { } let dir = tempfile::tempdir().unwrap(); std::fs::write(dir.path().join("z.wtr"), &out).unwrap(); - let rm = RegionMap::load(dir.path(), "z").expect("v1 still loads"); + let rm = RegionMap::try_load(dir.path(), "z").expect("v1 still loads"); assert!(rm.is_water(10.0, 20.0, -5.0)); assert_eq!(rm.zone_line_at(10.0, 20.0, -5.0), None); // v1 carries no index } @@ -873,7 +934,7 @@ mod tests { ]); let dir = tempfile::tempdir().unwrap(); std::fs::write(dir.path().join("z.wtr"), &blob).unwrap(); - let loaded = RegionMap::load(dir.path(), "z").expect("v2 loads"); + let loaded = RegionMap::try_load(dir.path(), "z").expect("v2 loads"); assert_eq!(loaded.zone_line_at(10.0, 20.0, -5.0), Some(0), "a loaded special=3/index=0 leaf must classify as a zone line"); assert_eq!(loaded.zone_line_indices(), vec![0]); diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 71f88e07..86267ae7 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -65,18 +65,74 @@ fn zone_assets_not_ready(s: &HttpState) -> Option { let st = eqoxide_nav::zone_assets::lock_state(&s.zone_assets).clone(); let player_zone = s.player().zone; let verdict = eqoxide_nav::zone_assets::usability(&st, &player_zone)?; - Some(( + Some(zone_assets_refusal(verdict, &st, &player_zone)) +} + +/// The body of that refusal, split out so a caller that obtained its verdict from +/// [`eqoxide_nav::zone_assets::usable_collision`] (which hands back the grid as well) serves the +/// byte-identical 503 rather than a second, drifting spelling of it (#821 review round 2, B4). +fn zone_assets_refusal( + verdict: eqoxide_nav::zone_assets::NotUsable, + st: &eqoxide_nav::zone_assets::ZoneAssetState, + player_zone: &str, +) -> Response { + ( StatusCode::SERVICE_UNAVAILABLE, Json(serde_json::json!({ "error": "zone_assets_not_ready", "reason": verdict.as_str(), - "zone_assets": zone_assets_json_of(&st, &player_zone), + "zone_assets": zone_assets_json_of(st, player_zone), "message": "the loaded zone assets cannot describe the zone this character is in, \ so this endpoint cannot answer without inventing a world. Poll GET \ /v1/observe/debug until `zone_assets.state` is \"ready\" (or handle \ \"failed\", which will never become ready).", })), - ).into_response()) + ).into_response() +} + +/// **503 for a question that can only be answered from the zone's `.wtr` region map when that map +/// is not there (#803).** +/// +/// The `zone_assets_not_ready` gate above covers the terrain GLB and the collision grid built from +/// it. It does **not** cover the `.wtr`: a zone whose GLB loaded fine and whose region map did not +/// is fully `ready`, and used to answer `/v1/observe/zone_exits` with a bare `[]` and 200 OK. That +/// is indistinguishable from the true, common reading "this zone has no zone lines" — and since +/// exits are the only way out of a zone, an agent reads it as *sealed in*, off a success response. +/// +/// Deliberately NOT folded into `NotUsable`/`zone_assets_not_ready`. **`usability()` has FOUR +/// non-test consumers** (#821 review round 2, B2 — an earlier revision of this comment said three +/// and omitted the fourth, which made this argument *understate* the blast radius). By call site: +/// * `observe.rs:38` / `:67` / `:1554` — every `/observe/*` endpoint, plus `/debug`'s zone block; +/// * `move_api.rs` — `POST /v1/move/goto`, which turns the verdict into its `zone_assets_pending` +/// note (**not** an `/observe/*` route, which is why it was missed); +/// * `walker.rs` — the nav path-walker's `drive_walk` gate; and +/// * `action_loop.rs` — `ActionLoop::drain_zone_cross`. +/// +/// So a new `NotUsable` variant would stop routing, stop `/v1/move/goto`, stop zone-crossing AND +/// stop rendering frames in any zone with a missing `.wtr` — far past what a region-map failure +/// actually invalidates. The refusal belongs to the questions whose answer really does come out of +/// that file. (Confirmed live in the PR's forced-failure run: with the `.wtr` broken and the zone +/// otherwise `ready`, `/frame`, `/zone_entrances` and `/debug` all still answered `200`.) +/// +/// `reason` is [`eqoxide_core::region_map::RegionDataAbsent::as_str`] — distinct per cause, so an +/// agent (or an operator reading its log) can tell "the asset pack never delivered this file" from +/// "the file is truncated" from "this build cannot read that version". +fn region_data_unavailable(absent: &eqoxide_core::region_map::RegionDataAbsent) -> Response { + ( + StatusCode::SERVICE_UNAVAILABLE, + Json(serde_json::json!({ + "error": "zone_region_data_unavailable", + "reason": absent.as_str(), + "detail": absent.to_string(), + "message": "this zone's region map (its `.wtr`) is not loaded, and the answer to this \ + question comes out of that file. An empty list here would say \"this zone \ + has no exits\" — a claim this client cannot make, and one you could not \ + tell apart from the truth. This does NOT resolve itself by polling: it is a \ + missing/unusable asset, not a load in progress. Re-sync the zone's asset \ + pack, or use `/v1/observe/zone_entrances` (server-advertised, independent of \ + the `.wtr`) to reason about where this zone connects.", + })), + ).into_response() } /// #646 (split out of #634/#647): the read-time freshness age every `/v1/observe/*` endpoint now @@ -1910,8 +1966,25 @@ async fn get_zone_entrances(State(s): State) -> Response { /// `/v1/move/zone_cross` walks to. Per exit: `location` `[x,y,z]` (a point inside the region nearest /// the player — position-relative), `zone_id` (destination, or `null` if the WLD region's index /// isn't advertised in the entrance list), and `index` (the link to the matching entrance's -/// `iterator`). Advertised entrances with no WLD region are omitted. Empty when the zone has no -/// region map (no `.wtr` / v1 map). +/// `iterator`). Advertised entrances with no WLD region are omitted. +/// +/// **`[]` means exactly one thing: this zone's region map loaded and contains no zone-line regions +/// (#803).** It never means "the file that would list them did not load" — that is a +/// **503 `zone_region_data_unavailable`** with a machine-readable `reason` +/// (`region_data_missing` / `_unreadable` / `_not_region_data` / `_unsupported_version` / +/// `_truncated`). Before #803 both produced `[]` with 200 OK, and because exits +/// are the only way out of a zone, an agent read a failed read as "sealed in". Unlike the +/// `zone_assets_not_ready` 503 below, this one does **not** clear by polling — the asset is missing +/// or unusable, not loading. +/// +/// **What makes that "never" true is the shape of the code, not this sentence** (#821 review round +/// 2, B4). This handler used to take permission from the zone-assets verdict and then read the grid +/// out of the separate `shared_collision` slot behind an `if let Some(col)` with **no `else`** — so +/// a `None` there returned `200 []` having consulted no region map at all, and nothing coupled the +/// two slots. It now gets verdict AND grid from one call +/// ([`eqoxide_nav::zone_assets::usable_collision`]), whose `Ok` arm carries the `Arc` the +/// `Ready` state owns. There is no longer a branch that can reach the response builder without a +/// grid, so every `[]` this endpoint emits has been through `Collision::zone_line_indices()`. /// /// `gated` (#713 item 3) is `true` when this exit's destination is unadvertised **and** this ZONE's /// #679/#683 unresolved-cross gate refuses server-resolved crossings. It is a **zone-level** verdict: @@ -1929,7 +2002,16 @@ async fn get_zone_entrances(State(s): State) -> Response { /// has no exits at all". That is a falsehood an agent cannot detect; an explicit refusal is the /// honest answer. Poll `/v1/observe/debug` → `zone_assets` until it reads `ready`. async fn get_zone_exits(State(s): State) -> Response { - if let Some(refusal) = zone_assets_not_ready(&s) { return refusal; } + // The #579 gate AND the grid it vouches for, from ONE read of ONE slot (#821 review round 2, + // B4). Deliberately not `zone_assets_not_ready(&s)` followed by `s.shared_collision`: those are + // two slots, and the fall-through when the second was `None` answered `[]` — see this + // function's doc comment. + let st = eqoxide_nav::zone_assets::lock_state(&s.zone_assets).clone(); + let player_zone = s.player().zone; + let col = match eqoxide_nav::zone_assets::usable_collision(&st, &player_zone) { + Ok(col) => col.clone(), + Err(verdict) => return zone_assets_refusal(verdict, &st, &player_zone), + }; // #646: bare array body (backward-compatible shape) — freshness rides `SNAPSHOT_AGE_HEADER` // instead of a JSON key. See that const's doc for the clock this reuses. let snapshot_age_ms = s.health().snapshot_age_ms; @@ -1976,22 +2058,28 @@ async fn get_zone_exits(State(s): State) -> Response { let unresolved_gated = eqoxide_core::zone_cross::classify_unresolved_cross( &zone_points, s.game_state.load().world.zone_id, ) == eqoxide_core::zone_cross::UnresolvedCross::Ignore; + // #803: `[]` here must only ever mean "this zone's region map loaded and has no zone-line + // regions". When the `.wtr` did NOT load, the exits are UNKNOWN, and publishing the empty + // list would be a confident falsehood the agent cannot detect — and the falsehood is + // specifically "there is no way out of this zone". Refuse, and name the cause. + let indices = match col.zone_line_indices() { + Ok(ix) => ix, + Err(absent) => return region_data_unavailable(&absent), + }; let mut exits = Vec::new(); - if let Some(col) = s.shared_collision.read().unwrap().as_ref() { - for index in col.zone_line_indices() { - let location = col - .find_zone_line_near(Some(index), pos) - .map(|(_, p)| serde_json::json!([p[0], p[1], p[2]])); - // An exit with a known destination is crossed by `perform_cross`, which never consults - // the unresolved gate — so the gate can only ever close a `zone_id: null` exit. - let gated = dest_of.get(&index).is_none() && unresolved_gated; - exits.push(serde_json::json!({ - "index": index, - "zone_id": dest_of.get(&index), - "location": location, - "gated": gated, - })); - } + for index in indices { + let location = col + .find_zone_line_near(Some(index), pos) + .map(|(_, p)| serde_json::json!([p[0], p[1], p[2]])); + // An exit with a known destination is crossed by `perform_cross`, which never consults + // the unresolved gate — so the gate can only ever close a `zone_id: null` exit. + let gated = dest_of.get(&index).is_none() && unresolved_gated; + exits.push(serde_json::json!({ + "index": index, + "zone_id": dest_of.get(&index), + "location": location, + "gated": gated, + })); } with_snapshot_age(Json(serde_json::json!(exits)).into_response(), snapshot_age_ms) } @@ -4320,6 +4408,16 @@ mod tests { // `test_ready()`'s `"testfixture"` so this loop exercises the header on a genuinely-served // 200, the same way `zone_asset_gate_tests` does. set_gs(&state, |gs| gs.world.zone_name = "testfixture".to_string()); + // …and give that grid a region map that LOADED and simply has no zone-line regions, so + // `/zone_exits` serves an honest `[]`/200 (#821 review round 2, B4). `empty_state()`'s + // `test_ready()` grid has no region data attached at all, which since B4 is a + // `503 zone_region_data_unavailable` — correctly, because the endpoint now reads the grid + // the `Ready` state OWNS instead of falling through an unset `shared_collision` slot and + // answering `[]` off nothing. This test is about the freshness header on a 200, so it needs + // a state that genuinely earns one. + *eqoxide_nav::zone_assets::lock_state(&state.zone_assets) = + eqoxide_nav::zone_assets::ZoneAssetState::test_ready_with_water(Some( + std::sync::Arc::new(eqoxide_core::region_map::RegionMap::flat_below(-10.0)))); for uri in ["/entities", "/doors", "/zone_entrances", "/zone_points", "/zone_exits"] { let resp = get(state.clone(), uri).await; @@ -4865,10 +4963,19 @@ mod zone_asset_gate_tests { const FIXTURE_ZONE: &str = "testfixture"; /// A state whose assets are loaded AND belong to the zone the character is standing in. + /// + /// The region map is `Ok` (a loaded map with no zone-line regions) rather than + /// `test_ready()`'s never-attached grid, because a `ready` zone in production has ALWAYS had + /// `set_region_data` called on it — with an `Ok` or with the loader's `Err`, never with nothing + /// (#821 review round 2, B4). Before that round `/zone_exits` read the *other* slot, + /// `shared_collision`, which `empty_state()` leaves `None`, so this fixture's grid was never + /// consulted at all and the endpoint answered `[]` off a fall-through. It is consulted now. fn ready_state() -> HttpState { let s = empty_state(); set_gs(&s, |gs| gs.world.zone_name = FIXTURE_ZONE.to_string()); - *eqoxide_nav::zone_assets::lock_state(&s.zone_assets) = ZoneAssetState::test_ready(); + *eqoxide_nav::zone_assets::lock_state(&s.zone_assets) = + ZoneAssetState::test_ready_with_water(Some(std::sync::Arc::new( + eqoxide_core::region_map::RegionMap::flat_below(-10.0)))); s } @@ -5306,6 +5413,163 @@ mod zone_cross_observables_713 { "and it must say what the caller actually risks"); } } +/// **#803 — `/v1/observe/zone_exits` must not publish a failed file read as "this zone has no way +/// out".** +/// +/// The endpoint answers out of the zone's region map (`maps/water/.wtr`), whose DRNTP +/// zone-line regions ARE the exits. When that file was missing, truncated, of an unsupported +/// version, or simply never attached, the loader's failure was discarded into a `None`, +/// `Collision::zone_line_indices()` `.unwrap_or_default()`-ed it to an empty vec, and this endpoint +/// served `[]` with **200 OK** — byte-identical to the true, common answer for a zone that +/// genuinely has no zone lines. Exits are the only way out of a zone, so the agent concluded it was +/// sealed in, from a success response it had no way to doubt. +/// +/// The two halves below are deliberately separate tests with different names: one pins that the +/// honest `[]` is STILL served (so the fix cannot degenerate into "refuse whenever the list is +/// empty"), the other that a load failure is a 503 naming its cause. Neither alone is the property. +#[cfg(test)] +mod zone_exits_never_publishes_a_failed_read_as_empty_803 { + use super::*; + use crate::testkit::{empty_state, set_gs}; + use axum::body::Body; + use axum::http::Request; + use eqoxide_core::region_map::{RegionLoadError, RegionMap}; + use eqoxide_nav::zone_assets::ZoneAssetState; + use tower::ServiceExt; + + async fn get(state: HttpState, uri: &str) -> (StatusCode, serde_json::Value) { + let app = router().with_state(state); + let resp = app.oneshot(Request::get(uri).body(Body::empty()).unwrap()).await.unwrap(); + let code = resp.status(); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX).await.unwrap(); + (code, serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null)) + } + + /// A `Ready` zone — terrain loaded, so the #579 gate is open and cannot be what refuses — whose + /// grid carries the given region-data outcome. This is the combination the bug lives in: the + /// zone is genuinely usable, and only the `.wtr` is not. + fn state_with_region_data( + data: Result, RegionLoadError>, + ) -> HttpState { + let s = empty_state(); + set_gs(&s, |gs| gs.world.zone_name = "testfixture".to_string()); + let ready = ZoneAssetState::test_ready_with_region_data(data); + *s.shared_collision.write().unwrap() = ready.collision().cloned(); + *eqoxide_nav::zone_assets::lock_state(&s.zone_assets) = ready; + s + } + + /// **Half one: the honest empty stays green.** A zone whose region map LOADED and contains no + /// zone-line regions still gets `[]` with 200 — that is a real reading of the world and the + /// overwhelmingly common one. A "fix" that refused on an empty list would break every zone. + #[tokio::test] + async fn a_zone_that_genuinely_has_no_zone_lines_still_answers_the_empty_list() { + let (code, j) = get( + state_with_region_data(Ok(std::sync::Arc::new(RegionMap::flat_below(-10.0)))), + "/zone_exits", + ).await; + assert_eq!(code, StatusCode::OK, + "a loaded map with no zone-line regions is an ANSWER; refusing here would break the \ + common case the empty list legitimately describes"); + assert_eq!(j, serde_json::json!([])); + } + + /// …and a loaded map WITH an exit still lists it, so half one is not passing because the + /// endpoint stopped reporting exits altogether. + #[tokio::test] + async fn a_zone_with_a_zone_line_still_lists_it() { + let (code, j) = get( + state_with_region_data(Ok(std::sync::Arc::new( + RegionMap::zone_line_box(-4.0, 4.0, -4.0, 4.0, -2.0, 2.0, 7)))), + "/zone_exits", + ).await; + assert_eq!(code, StatusCode::OK); + assert_eq!(j.as_array().map(|a| a.len()), Some(1), "the fixture bakes exactly one exit: {j}"); + assert_eq!(j[0]["index"], 7); + } + + /// **Half two: THE #803 FALSEHOOD.** Every way the `.wtr` can fail to load, plus the + /// never-attached case, must come back as an explicit refusal naming its cause — never `[]`, + /// never 200. All five are looped because the old code collapsed them into one `None`; a fix + /// that only handled the failure in the issue title would rebuild the same lie for the rest. + #[tokio::test] + async fn a_region_map_that_did_not_load_refuses_instead_of_reporting_no_exits() { + let cases: Vec<(Result, RegionLoadError>, &str)> = vec![ + (Err(RegionLoadError::Missing), "region_data_missing"), + (Err(RegionLoadError::Unreadable(std::io::ErrorKind::PermissionDenied)), + "region_data_unreadable"), + (Err(RegionLoadError::NotRegionData), "region_data_not_region_data"), + (Err(RegionLoadError::UnsupportedVersion(99)), "region_data_unsupported_version"), + (Err(RegionLoadError::Truncated { declared_nodes: 400, bytes: 12 }), + "region_data_truncated"), + ]; + for (data, want_reason) in cases { + let (code, j) = get(state_with_region_data(data), "/zone_exits").await; + assert_eq!(code, StatusCode::SERVICE_UNAVAILABLE, + "{want_reason}: a failed read served as `[]`/200 tells the agent this zone has no \ + way out — a confident falsehood it cannot detect (#803). Got {code}: {j}"); + assert_eq!(j["error"], "zone_region_data_unavailable", "body: {j}"); + assert_eq!(j["reason"], want_reason, + "the reason must name the ACTUAL failure — 'the file is absent' and 'the file is \ + truncated' call for different operator action"); + assert!(!j["detail"].as_str().unwrap_or("").is_empty(), "body: {j}"); + } + } + + /// **#821 review round 2, B4: the `shared_collision` slot is no longer an input, so an unset one + /// cannot produce `[]`.** + /// + /// The handler used to take permission from the zone-assets verdict and then read the grid from + /// the separate `shared_collision` slot behind `if let Some(col)` with no `else`. A `None` there + /// returned `200 []` — "this zone has no way out" — having consulted no region map at all. That + /// path was live in-tree and green: the HTTP testkit builds exactly that combination. + /// + /// Both directions are asserted from ONE fixture, so this cannot pass by the endpoint having + /// stopped reading exits: with the slot empty and the region map holding an exit, the exit is + /// still listed; with the slot empty and the region map failed, it is still a 503. + #[tokio::test] + async fn an_unset_shared_collision_slot_can_no_longer_produce_an_empty_exit_list() { + for (label, data, want) in [ + ("a loaded map WITH an exit", + Ok(std::sync::Arc::new(RegionMap::zone_line_box(-4.0, 4.0, -4.0, 4.0, -2.0, 2.0, 7))), + StatusCode::OK), + ("a map that did not load", + Err(RegionLoadError::Missing), + StatusCode::SERVICE_UNAVAILABLE), + ] { + let s = state_with_region_data(data); + // The exact fixture shape the old fall-through fired on: `Ready` for this zone, and + // NOTHING in the shared slot. + *s.shared_collision.write().unwrap() = None; + let (code, j) = get(s, "/zone_exits").await; + assert_eq!(code, want, + "{label}: with the shared slot empty the endpoint must still answer out of the \ + grid the `Ready` state OWNS, never out of a fall-through. Got {code}: {j}"); + assert_ne!(j, serde_json::json!([]), + "{label}: `[]` off an unread region map is the #803 falsehood with a new cause"); + } + } + + /// The refusal is deliberately NOT the `zone_assets_not_ready` verdict: that one is also read by + /// the nav walker's drive gate and the net thread's zone-cross drain, and a missing `.wtr` does + /// not invalidate the terrain those consume. Pinned so a later "simplification" that folds the + /// two together has to change this line and say why. + /// + /// **Asserted positively** (#821 review round 2, minor M1). This used to be only + /// `assert_ne!(j["error"], "zone_assets_not_ready")`, which is vacuously true whenever the body + /// is an ARRAY — `j["error"]` is then `Value::Null` — so the endpoint restored to `200 []` (the + /// exact regression this test is named after) passed it. Measured, not reasoned: the reviewer + /// ran that mutation and this test survived it. + #[tokio::test] + async fn the_refusal_is_distinct_from_the_zone_assets_gate() { + let (code, j) = get(state_with_region_data(Err(RegionLoadError::Missing)), "/zone_exits").await; + assert_eq!(code, StatusCode::SERVICE_UNAVAILABLE, "body: {j}"); + assert_eq!(j["error"], "zone_region_data_unavailable", + "region data is a separate readiness question from the zone's terrain assets, and the \ + refusal has to SAY so — an array body would make a bare `assert_ne!` pass: {j}"); + assert_ne!(j["error"], "zone_assets_not_ready", "body: {j}"); + } +} /// Reach control for `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it` (#760/C1). /// diff --git a/crates/eqoxide-nav/src/collision.rs b/crates/eqoxide-nav/src/collision.rs index 67ed9f28..b5220e08 100644 --- a/crates/eqoxide-nav/src/collision.rs +++ b/crates/eqoxide-nav/src/collision.rs @@ -507,9 +507,19 @@ pub struct Collision { /// narrow door or a tight bridge with no margin to spare. Surfaced as `nav_tight` so an agent is /// never silently handed a riskier path than it thinks (`search_tiered`). tight_plans: std::sync::atomic::AtomicU64, - /// Optional water-region map (from the zone's `.wtr`). When present, find_path may DESCEND - /// through water (swim down a canal/shaft) to a lower floor that has no walkable connection. - water: Option>, + /// The zone's region map (from its `.wtr`) — **or the reason there isn't one** (#803). + /// + /// `Ok`: find_path may DESCEND through water (swim down a canal/shaft) to a lower floor that has + /// no walkable connection, and [`Collision::zone_line_indices`] enumerates the zone's real exits. + /// + /// `Err`: this grid has NO region data, and here is why. It is a `Result` rather than an + /// `Option` because the absence used to be laundered into an answer: `zone_line_indices` handed + /// back an empty `Vec` and `/v1/observe/zone_exits` published it as `[]` with 200 OK, which is + /// also the true reading for a zone that genuinely has no zone lines. Exits are the only way out + /// of a zone, so an agent read a failed READ as "sealed in". Written only by + /// [`Collision::set_region_data`]; read through [`Collision::region_map`]. + water: Result, + eqoxide_core::region_map::RegionDataAbsent>, /// True when the terrain triangles came from a dedicated `__collision__` mesh (SOLID + /// INVIS faces, PASSABLE excluded). False for legacy zones with no baked collision mesh, /// where the rendered terrain is used as a fallback. Diagnostic/provenance only. @@ -757,7 +767,8 @@ impl Collision { z_min: 0.0, z_max: 0.0, facing_blind_hits: Default::default(), tight_plans: Default::default(), clearance: Default::default(), water_grid: None, - water: None, from_collision_mesh, zone_line_regions: Vec::new() }; + water: Err(eqoxide_core::region_map::RegionDataAbsent::NotAttached), + from_collision_mesh, zone_line_regions: Vec::new() }; } let cols = (((max[0] - min[0]) / cell_size).ceil() as usize + 1).max(1); let rows = (((max[1] - min[1]) / cell_size).ceil() as usize + 1).max(1); @@ -782,7 +793,9 @@ impl Collision { #[cfg(test)] z_min, z_max, - facing_blind_hits: Default::default(), tight_plans: Default::default(), water: None, from_collision_mesh, zone_line_regions: Vec::new(), + facing_blind_hits: Default::default(), tight_plans: Default::default(), + water: Err(eqoxide_core::region_map::RegionDataAbsent::NotAttached), + from_collision_mesh, zone_line_regions: Vec::new(), clearance: Default::default(), water_grid: None } } @@ -803,19 +816,60 @@ impl Collision { self.facing_blind_hits.load(std::sync::atomic::Ordering::Relaxed) } - /// Attach a zone water map so find_path can route swim descents. Call after `build`. - pub fn set_water(&mut self, water: Option>) { - self.water = water; + /// **The one writer of this grid's region data**: attach the zone's `.wtr` map, or record WHY + /// there is none. Call after `build`. + /// + /// Takes the loader's own `Result` (`RegionMap::try_load`) rather than an `Option` on purpose + /// (#803). The production caller no longer *has* a way to drop the reason on the floor without + /// writing the discard out by hand — `RegionMap::load`, the wrapper that used to do it for you, + /// is deleted. + pub fn set_region_data( + &mut self, + data: Result, + eqoxide_core::region_map::RegionLoadError>, + ) { + self.water = data.map_err(eqoxide_core::region_map::RegionDataAbsent::LoadFailed); // Precompute zone-line region points now (zone load, off the net thread) so the runtime // find_zone_line_near is an O(1) cache read (#204). self.zone_line_regions = self.precompute_zone_line_regions(); } + /// Fixture shorthand for synthetic scenes: attach a hand-authored region map, or (with `None`) + /// leave the grid with no region data at all. + /// + /// **Test-only, and gated so on purpose.** For a synthetic scene there is no `.wtr` and so no + /// load that could have failed — `None` genuinely means [`RegionDataAbsent::NotAttached`], which + /// is still an explicit absence, not an empty answer. Because this is + /// `#[cfg(any(test, feature = "test-fixtures"))]` it does not exist in a release build, so no + /// production path can reach the reason-free spelling. Same gating as the `RegionMap::flat_below` + /// / `water_slab` constructors it exists to accept. + /// + /// [`RegionDataAbsent::NotAttached`]: eqoxide_core::region_map::RegionDataAbsent::NotAttached + #[cfg(any(test, feature = "test-fixtures"))] + pub fn set_water(&mut self, water: Option>) { + self.water = water.ok_or(eqoxide_core::region_map::RegionDataAbsent::NotAttached); + self.zone_line_regions = self.precompute_zone_line_regions(); + } + + /// This grid's region map when it has one — the read path for every water/zone-line query below. + /// `None` here is only ever consumed by a query whose "no data ⇒ no water here" answer is + /// *physically* the safe one; anything an agent reads back as a fact must go through + /// [`Collision::zone_line_indices`] (or `region_data_absent`) and surface the reason instead. + fn region_map(&self) -> Option<&std::sync::Arc> { + self.water.as_ref().ok() + } + + /// Why this grid has no region data — `None` when it has some. The value an agent-facing + /// endpoint refuses with instead of publishing an empty answer (#803). + pub fn region_data_absent(&self) -> Option<&eqoxide_core::region_map::RegionDataAbsent> { + self.water.as_ref().err() + } + /// Build the zone-line region cache from the water map. Bounded + timed; warns if it runs long /// (nav prep should never be slow enough to matter next to keepalive). Empty when there's no /// water map / no zone-line regions. fn precompute_zone_line_regions(&self) -> Vec<(i32, [f32; 3])> { - let Some(water) = self.water.as_ref() else { return Vec::new(); }; + let Some(water) = self.region_map() else { return Vec::new(); }; if self.cols == 0 || self.tris.is_empty() { return Vec::new(); } let (mut zmin, mut zmax) = (f32::MAX, f32::MIN); for t in &self.tris { for v in t { zmin = zmin.min(v[2]); zmax = zmax.max(v[2]); } } @@ -854,11 +908,11 @@ impl Collision { /// The first call in a water zone runs [`Self::build_water_grid`] (the measured 357 ms – 1 s /// cost) on whatever thread the plan runs on — which is off the network thread for both the /// coarse (#377) and fine (#382) tiers — and caches it; every later call is a pointer read. The - /// build is gated on `self.water.is_some()` *before* `get_or_init`, so a grid is never cached for + /// build is gated on `self.region_map().is_some()` *before* `get_or_init`, so a grid is never cached for /// a zone whose water map has not been attached yet. fn water_grid_active(&self) -> Option<&crate::water_grid::WaterGrid> { if let Some(g) = self.water_grid.as_ref() { return Some(g); } - self.water.as_ref()?; + self.region_map()?; Some(self.water_grid_lazy.get_or_init(|| { let t0 = std::time::Instant::now(); let g = self.build_water_grid(&crate::traversability::PLAYER_BODY); @@ -901,7 +955,7 @@ impl Collision { pub fn build_water_grid(&self, body: &crate::traversability::Body) -> crate::water_grid::WaterGrid { const COL: f32 = 4.0; // 4u XY water columns (design §5.1 / owner decision #2, locked) let mut grid = crate::water_grid::WaterGrid::new(self.origin, COL); - let Some(water) = self.water.as_ref() else { return grid; }; + let Some(water) = self.region_map() else { return grid; }; if self.cols == 0 || self.tris.is_empty() { return grid; } // Zone z-extent — same source as the zone-line precompute (`precompute_zone_line_regions`). @@ -1070,13 +1124,13 @@ impl Collision { /// True if `pos` = [east, north, z] (server coords) lies in a water region. /// False when the zone has no water map. Used to gate swim (vertical) movement. pub fn in_water(&self, pos: [f32; 3]) -> bool { - self.water.as_ref().is_some_and(|w| w.is_water(pos[0], pos[1], pos[2])) + self.region_map().is_some_and(|w| w.is_water(pos[0], pos[1], pos[2])) } /// Water-surface height above a submerged `pos`, or `None` if not in water / no bounded surface. /// Used by the controller's buoyancy to float toward the surface (#172). pub fn water_surface(&self, pos: [f32; 3]) -> Option { - self.water.as_ref().and_then(|w| w.surface_z(pos[0], pos[1], pos[2])) + self.region_map().and_then(|w| w.surface_z(pos[0], pos[1], pos[2])) } /// If `pos` = [east, north, z] (server coords) lies in a zone-line (`DRNTP`) region, the @@ -1085,7 +1139,7 @@ impl Collision { /// This is how the native client triggers a crossing: it detects the region from zone geometry /// rather than a coordinate list. pub fn zone_line_at(&self, pos: [f32; 3]) -> Option { - self.water.as_ref().and_then(|w| w.zone_line_at(pos[0], pos[1], pos[2])) + self.region_map().and_then(|w| w.zone_line_at(pos[0], pos[1], pos[2])) } /// The zone-line index a **standing** character occupies at `(east, north, feet_z)`, probing the @@ -1114,7 +1168,7 @@ impl Collision { /// zone-line slab thinner than the vertical sample step is the only miss, far below any shipped /// DRNTP thickness. pub fn zone_line_at_standing(&self, pos: [f32; 3]) -> Option { - let w = self.water.as_ref()?; + let w = self.region_map()?; const STEP: f32 = 1.0; let feet = pos[2]; let head = feet + crate::traversability::PLAYER_BODY.height; @@ -1127,11 +1181,33 @@ impl Collision { w.zone_line_at(pos[0], pos[1], head) } - /// Distinct zone-point indices of every zone-line region in this zone — the set of exits. Each - /// links to an entrance via the `OP_SendZonepoints` `iterator`. Empty when the zone has no - /// region map (or a v1 map with no indices). - pub fn zone_line_indices(&self) -> Vec { - self.water.as_ref().map(|w| w.zone_line_indices()).unwrap_or_default() + /// Distinct zone-point indices of every zone-line region in this zone — **the set of exits**. + /// Each links to an entrance via the `OP_SendZonepoints` `iterator`. + /// + /// `Ok(vec![])` is a real reading: this zone's region map loaded and contains no zone-line + /// regions. `Err` means the question could not be answered at all, and names why. + /// + /// **A v1 map is NOT an exception to that** (#821 review round 2, B1 — an earlier revision of + /// this sentence said it was). v1 records carry no `zone_line_index` field, so every v1 zone + /// line reads as index **0**, and index 0 is *included*: this returns `[0]`, not `[]`. That is + /// the whole point of #683 — gating recognition on a nonzero index locked a character in qrg + /// forever. See `RegionMap::zone_line_indices` and + /// `a_zero_index_zone_line_region_is_still_recognized_683`. + /// + /// **The `Result` is the fix for #803, and it is here rather than in a caller-side guard because + /// the guard is what kept failing.** This used to be `Vec` with `.unwrap_or_default()`, so a + /// `.wtr` that did not load produced an empty vec that `/v1/observe/zone_exits` published as `[]` + /// with 200 OK — byte-identical to the true, common answer for a zone with no zone lines. Exits + /// are the only way out of a zone, so the agent concluded it was sealed in, from a success + /// response it had no way to doubt. Now the two cases have different TYPES, and every caller has + /// to say out loud what it does with the second. + pub fn zone_line_indices(&self) + -> Result, eqoxide_core::region_map::RegionDataAbsent> + { + match self.water.as_ref() { + Ok(w) => Ok(w.zone_line_indices()), + Err(absent) => Err(absent.clone()), + } } /// Find a point inside a zone-line region nearest to `near` (= [east, north, z]), returning @@ -1186,7 +1262,7 @@ impl Collision { let fz = self.floor_beneath(p[0], p[1], p[2], 2.0, REGION_DROP)?; // Standing on that floor must still be INSIDE the region — else we'd walk the char to a spot // that never fires the auto-cross. - let inside = self.water.as_ref() + let inside = self.region_map() .and_then(|w| w.zone_line_at(p[0], p[1], fz + 1.0)) == Some(index); inside.then_some([p[0], p[1], fz]) } @@ -1304,7 +1380,7 @@ impl Collision { const STEP_H: f32 = 20.0; const REGION_DROP: f32 = 400.0; self.column_floors(p[0], p[1], p[2], STEP_H, REGION_DROP).into_iter() - .find(|&fz| self.water.as_ref() + .find(|&fz| self.region_map() .and_then(|w| w.zone_line_at(p[0], p[1], fz + 1.0)) == Some(index)) .map(|fz| [p[0], p[1], fz]) } @@ -2345,7 +2421,7 @@ impl Collision { /// is UNBOUNDED (water 200u+ up, `surface_z` = `None`): there is no surface to anchor to, and /// the caller must fall through to the ordinary dry resolution, never unwrap. pub fn floating_goal_surface(&self, goal: [f32; 3]) -> Option { - let w = self.water.as_ref()?; + let w = self.region_map()?; let (x, y, z) = (goal[0], goal[1], goal[2]); // A point IN water (checked at z and a hair under, for a goal exactly at the waterline) — // or ABOVE water found by a short downward probe, the same shape as the WATER SURFACE @@ -2382,7 +2458,7 @@ impl Collision { // (`surface_z` is `None` unless `f + 1` is actually in water, and `None` for an // unbounded column — both read as "nothing to report" here.) if let Some(f) = self.nearest_floor(goal[0], goal[1], goal[2], GOAL_TIER_TOL, GOAL_TIER_TOL) { - if let Some(s) = self.water.as_ref().and_then(|w| w.surface_z(goal[0], goal[1], f + 1.0)) { + if let Some(s) = self.region_map().and_then(|w| w.surface_z(goal[0], goal[1], f + 1.0)) { if s - f > GOAL_TIER_TOL { return Some(GoalSnap::ToWaterSurface { surface_z: s }); } @@ -2433,7 +2509,7 @@ impl Collision { // is one the walker floats above, so arrival is at the SURFACE (mirrors clause 1 of // `goal_z_was_snapped`). `surface_z` is `None` for a dry tier or an unbounded column. if let Some(f) = self.nearest_floor(goal[0], goal[1], goal[2], GOAL_TIER_TOL, GOAL_TIER_TOL) { - if let Some(s) = self.water.as_ref().and_then(|w| w.surface_z(goal[0], goal[1], f + 1.0)) { + if let Some(s) = self.region_map().and_then(|w| w.surface_z(goal[0], goal[1], f + 1.0)) { if s - f > GOAL_TIER_TOL { return Some(s); } } return Some(f); @@ -2992,7 +3068,7 @@ impl Collision { // actually floating on — which is exactly the tier the WATER SURFACE TRAVERSAL edges connect. // `FOOTING` (module const): a floor this close under the feet = standing (wading), not // floating — shared with the floating GOAL anchor (`floating_goal_surface`, design §4d). - let floating_surface = self.water.as_ref().and_then(|w| { + let floating_surface = self.region_map().and_then(|w| { let (x, y, z) = (start[0], start[1], start[2]); let wet = w.is_water(x, y, z) || w.is_water(x, y, z - 1.0); let footed = self.nearest_floor(x, y, z, 2.0, FOOTING).is_some(); @@ -3066,7 +3142,7 @@ impl Collision { let cell_has_start_floor = |c: i32, r: i32| -> bool { let ctr = center(c, r); if floating_surface.is_some() { - return self.water.as_ref().is_some_and(|w| w.is_water(ctr[0], ctr[1], start_floor - 1.0)); + return self.region_map().is_some_and(|w| w.is_water(ctr[0], ctr[1], start_floor - 1.0)); } self.column_floors(ctr[0], ctr[1], start_floor, STEP_H, MAX_STEP_DOWN) .into_iter().any(|z| (z - start_floor).abs() <= GOAL_TIER_TOL) @@ -3227,7 +3303,7 @@ impl Collision { // to ask of it. Ask the right one instead: am I STANDING INSIDE the region? That is // exactly the predicate the native auto-cross fires on. Only tested near the goal cell, // so it costs a handful of BSP walks per plan, not one per node. - if let (Some(want), Some(water)) = (ctx.goal_region, self.water.as_ref()) { + if let (Some(want), Some(water)) = (ctx.goal_region, self.region_map()) { if h(c, r) <= 2.0 * cell { let p = center(c, r); if water.zone_line_at(p[0], p[1], fz + 1.0) == Some(want) { @@ -3418,7 +3494,7 @@ impl Collision { // controller can execute. (The legacy WATER ASCENT family still serves land- // anchored floaters; it is unreachable from a water node, which `continue`s below.) if cur_col.top_node_z().is_some_and(|t| (t - cz).abs() < 0.5) { - if let Some(water) = &self.water { + if let Some(water) = self.region_map() { if let Some(surface) = water.surface_z(a[0], a[1], cz).map(|s| s.max(cz)) { let haul_out_up = crate::traversability::PLAYER_BODY.haul_out_up; for nf in self.column_floors(b[0], b[1], surface, STEP_H, surface - cz) { @@ -3635,7 +3711,7 @@ impl Collision { // dropping/swimming down to the floor beneath it even past MAX_STEP_DOWN and without // a clear chest-height walking segment — you fall into the water and sink/swim. This // connects an upper walkway to a flooded lower level (e.g. qcat's canal → sewer). - if let Some(water) = &self.water { + if let Some(water) = self.region_map() { // Is there water somewhere in the column between here and far below? let has_water = (1..=12).any(|k| water.is_water(b[0], b[1], cz - k as f32 * 8.0)); if has_water { @@ -3694,7 +3770,7 @@ impl Collision { // and haul out onto a neighbor floor at or below surface + STEP_H. Without // this, flooded pits (qeynos2's moat) are one-way traps: descent gets you in, // and the normal climb's chest ray hits the pit wall on the way out. - if let Some(water) = &self.water { + if let Some(water) = self.region_map() { // Submerged (water ABOVE us), or floating AT the surface — a start anchored to // the water surface (#329/#197p2) has air above it, so the old submerged-only // test never fired for it and a floating swimmer had no way OUT of the water. @@ -3772,7 +3848,7 @@ impl Collision { // back (which fights the controller's buoyancy toward the surface). This makes a // surface pool (e.g. the Halas central pool on the way to the Everfrost line) a // crossable swim rather than a drop to the pool floor the fall-guard refuses. - if let Some(water) = &self.water { + if let Some(water) = self.region_map() { // Probe downward for the first swimmable water within a step of the current // floor — a pool's surface often sits a little BELOW the shore you wade in from // (Halas's central pool surface is ~5u under the ice), so a 1u probe would miss @@ -7657,7 +7733,7 @@ mod tests { crate::water_grid::ZoneWater::load(&std::path::Path::new(&dir).join("maps/water"), &zone) .install(&mut col) .unwrap_or_else(|e| panic!("{zone}: {e} — every water answer below would be fabricated (#762)")); - println!("zone={zone} water loaded: {}", col.water.is_some()); + println!("zone={zone} water loaded: {}", col.region_map().is_some()); println!("grid: origin={:?} cols={} rows={} cell={} z=[{:.1},{:.1}] extent e=[{:.0},{:.0}] n=[{:.0},{:.0}]", col.origin, col.cols, col.rows, col.cell_size, col.z_min, col.z_max, col.origin[0], col.origin[0] + col.cols as f32 * col.cell_size, @@ -8281,13 +8357,13 @@ mod tests { eprintln!(" moat → street x={gx}: {}", r.map(|p| format!("{} waypoints", p.len())).unwrap_or_else(|| "NONE".into())); } - eprintln!(" zone has a water volume: {}", col.water.is_some()); + eprintln!(" zone has a water volume: {}", col.region_map().is_some()); // Moat exit scan: walk the whole moat water region and report every column where a // haul-out is geometrically possible (a neighbor floor within STEP_H of the water // surface and a clear chest ray from swim height). If this prints nothing, the // collision genuinely has no exit and the swim-up nav edge can't help. - if let Some(w) = col.water.clone() { + if let Some(w) = col.region_map().cloned() { let mut found = 0; let mut y = -260.0f32; while y < -20.0 { @@ -8416,3 +8492,94 @@ mod tests { "footprint on open floor should be clear"); } } + +/// **#803 — "this zone has no exits" and "I could not read this zone's exits" must not be the same +/// value.** +/// +/// `zone_line_indices` used to return `Vec` and reached for `.unwrap_or_default()` when there +/// was no region map, so a `.wtr` that was missing, truncated, of an unsupported version, or simply +/// never attached produced the byte-identical empty vec that a healthy zone with no zone-line +/// regions produces. `/v1/observe/zone_exits` published that as `[]` with 200 OK. Exits are the only +/// way out of a zone, so an agent read a failed FILE READ as a fact about the world: sealed in. +/// +/// These four cases are one table on purpose. The pairing is the property — an assertion that a +/// failure is `Err` is worth nothing unless the true empty answer is, at the same time, still `Ok`. +#[cfg(test)] +mod zone_line_indices_is_not_lossy_803 { + use super::*; + use eqoxide_assets::{MeshData, RenderMode, ZoneAssets}; + use eqoxide_core::region_map::{RegionDataAbsent, RegionLoadError, RegionMap}; + + fn grid() -> Collision { + let floor = MeshData { + positions: vec![[-100.0, 0.0, -100.0], [100.0, 0.0, -100.0], + [100.0, 0.0, 100.0], [-100.0, 0.0, 100.0]], + normals: vec![[0.0, 1.0, 0.0]; 4], uvs: vec![[0.0, 0.0]; 4], + indices: vec![0, 1, 2, 0, 2, 3], texture_name: None, base_color: [1.0; 4], + center: [0.0; 3], render_mode: RenderMode::Opaque, anim: None, + }; + Collision::build(&ZoneAssets { terrain: vec![floor], objects: vec![], textures: vec![] }, 8.0) + } + + /// **The honest empty, which must stay green.** A region map that LOADED and genuinely contains + /// no zone-line regions still answers `Ok(vec![])` — `/zone_exits` must keep serving `[]`/200 + /// for the common real zone that has no exits baked. If the fix had been "refuse whenever the + /// list is empty", this is the test that would have caught it. + #[test] + fn a_loaded_map_with_no_zone_lines_still_answers_the_empty_list() { + let mut c = grid(); + c.set_region_data(Ok(std::sync::Arc::new(RegionMap::flat_below(-10.0)))); + assert_eq!(c.zone_line_indices(), Ok(vec![]), + "a map that loaded and has no zone-line regions is an ANSWER, not a refusal"); + } + + /// A loaded map WITH a zone line still enumerates it — the `Ok` arm is not a stub. + #[test] + fn a_loaded_map_with_a_zone_line_enumerates_it() { + let mut c = grid(); + c.set_region_data(Ok(std::sync::Arc::new( + RegionMap::zone_line_box(-4.0, 4.0, -4.0, 4.0, -2.0, 2.0, 7)))); + assert_eq!(c.zone_line_indices(), Ok(vec![7])); + } + + /// **The falsehood, as a test.** Every way the `.wtr` can fail to load must come back as `Err` + /// carrying WHICH way — never as the empty list. The four `RegionLoadError`s are looped rather + /// than represented by one, because the old code collapsed all of them into a single `None` and + /// a fix that only handled the one in the issue title would rebuild the same lie for the others. + #[test] + fn every_load_failure_is_an_error_not_an_empty_list() { + for e in [ + RegionLoadError::Missing, + RegionLoadError::NotRegionData, + RegionLoadError::UnsupportedVersion(99), + RegionLoadError::Truncated { declared_nodes: 400, bytes: 12 }, + ] { + let mut c = grid(); + c.set_region_data(Err(e.clone())); + assert_eq!(c.zone_line_indices(), Err(RegionDataAbsent::LoadFailed(e.clone())), + "{e:?}: a failed READ published as an empty exit list tells the agent it is sealed \ + in a zone, from a response it has no way to doubt (#803)"); + } + } + + /// The fourth case, distinct from all of the above: nothing was ever attached to this grid (a + /// synthetic scene, or a zone loaded before the region map). Also not an empty exit list. + #[test] + fn a_grid_with_no_region_data_attached_is_an_error_too() { + assert_eq!(grid().zone_line_indices(), Err(RegionDataAbsent::NotAttached)); + assert_eq!(grid().region_data_absent(), Some(&RegionDataAbsent::NotAttached)); + } + + /// The reason has to survive the trip to the caller INTACT: `/zone_exits` reports + /// `absent.as_str()`, so a `set_region_data` that stored one canned failure for all of them + /// would leave the endpoint naming the wrong cause while still looking "explicit". + #[test] + fn the_reported_reason_names_the_actual_failure() { + let mut c = grid(); + c.set_region_data(Err(RegionLoadError::Truncated { declared_nodes: 400, bytes: 12 })); + assert_eq!(c.region_data_absent().unwrap().as_str(), "region_data_truncated"); + let mut c = grid(); + c.set_region_data(Err(RegionLoadError::Missing)); + assert_eq!(c.region_data_absent().unwrap().as_str(), "region_data_missing"); + } +} diff --git a/crates/eqoxide-nav/src/water_grid.rs b/crates/eqoxide-nav/src/water_grid.rs index 513256a5..bac63371 100644 --- a/crates/eqoxide-nav/src/water_grid.rs +++ b/crates/eqoxide-nav/src/water_grid.rs @@ -298,9 +298,14 @@ impl ZoneWater { fabricated dry (#762). Report it as unmeasured or refuse to score the zone."] pub fn install(&self, col: &mut crate::collision::Collision) -> Result<(), &eqoxide_core::region_map::RegionLoadError> { + // #803: BOTH arms now write the grid. An `Unmeasured` zone used to leave the grid's region + // slot untouched, so the grid could not tell "nobody asked" from "we asked and the `.wtr` + // did not load" — and `zone_line_indices` answered `[]` for both. Recording the reason keeps + // the `Err` return (the corpus's must-use contract is unchanged) AND makes the grid itself + // able to refuse honestly if some later reader asks it about exits. match self { - Self::Measured(m) => { col.set_water(Some(m.clone())); Ok(()) } - Self::Unmeasured(e) => Err(e), + Self::Measured(m) => { col.set_region_data(Ok(m.clone())); Ok(()) } + Self::Unmeasured(e) => { col.set_region_data(Err(e.clone())); Err(e) } } } @@ -909,9 +914,17 @@ mod tests { "the round-2 clean-looking empty string must not be producible: {line}"); } - /// Installing an unmeasured zone onto a collision grid is an error the caller must handle: the - /// grid keeps `water: None`, and a scorer that ignored the `Result` would read fabricated dry - /// answers out of it. `#[must_use]` makes ignoring it a compiler warning. + /// Installing an unmeasured zone onto a collision grid is an error the caller must handle: a + /// scorer that ignored the `Result` would read fabricated dry answers out of it. + /// `#[must_use]` makes ignoring it a compiler warning. + /// + /// **#821 review round 2, N1.** Since #803 `install` also RECORDS the reason on the grid, so the + /// grid itself can refuse instead of answering `[]`. That behaviour change was stated in + /// `install`'s comment ("BOTH arms now write the grid") and pinned by nothing: the reviewer + /// severed the `Unmeasured` arm's `col.set_region_data(Err(e.clone()))` and three crates stayed + /// green. The before/after assertions below are the missing pin — before the install the grid is + /// `NotAttached` (nobody asked), after it is `LoadFailed(Missing)` (we asked and the file was + /// not there), and those are DIFFERENT states with different `reason` strings on the wire. #[test] fn installing_an_unmeasured_zone_is_a_handled_failure_762() { use crate::collision::Collision; @@ -925,13 +938,30 @@ mod tests { let mut col = Collision::build( &ZoneAssets { terrain: vec![floor], objects: vec![], textures: vec![] }, 32.0); + // Nobody has installed anything yet, so the grid says exactly that — NOT "the file failed". + assert_eq!(col.region_data_absent().map(|a| a.as_str()), Some("region_data_not_attached"), + "a freshly built grid has had no region data handed to it"); + let err = absent().install(&mut col).unwrap_err().clone(); assert_eq!(err, RegionLoadError::Missing); assert!(!col.in_water([0.0, 0.0, 1.0]), "the grid is dry — but that dry is FABRICATED, which is why install() reported Err"); + // …and the GRID now carries the reason too (#803), so a later reader that never sees this + // `Result` still cannot mistake the failure for an answer. N1: severing the `Unmeasured` + // arm's `set_region_data` leaves the grid on `region_data_not_attached` and both of these + // go RED. + assert_eq!(col.region_data_absent().map(|a| a.as_str()), Some("region_data_missing"), + "the Unmeasured arm must RECORD why, not leave the grid looking un-asked"); + assert!(matches!(col.zone_line_indices(), Err(ref a) if a.as_str() == "region_data_missing"), + "…so the exits question refuses with the real cause instead of answering `[]`: {:?}", + col.zone_line_indices()); dry_but_loaded().install(&mut col).expect("a loaded map installs"); assert!(!col.in_water([0.0, 0.0, 1.0]), "loaded and genuinely dry"); + // The Measured arm's write clears the recorded failure rather than leaving it latched. + assert_eq!(col.region_data_absent().map(|a| a.as_str()), None, "a loaded map is present"); + assert_eq!(col.zone_line_indices().as_deref(), Ok(&[7][..]), + "and the loaded map's zone line is enumerable"); ZoneWater::from_map(RegionMap::water_slab(-40.0, 0.0)).install(&mut col).expect("installs"); assert!(col.in_water([0.0, 0.0, -10.0]), "a real water map really answers wet"); } diff --git a/crates/eqoxide-nav/src/zone_assets.rs b/crates/eqoxide-nav/src/zone_assets.rs index ccd38436..b6187cc3 100644 --- a/crates/eqoxide-nav/src/zone_assets.rs +++ b/crates/eqoxide-nav/src/zone_assets.rs @@ -154,9 +154,35 @@ impl ZoneAssetState { /// [`Self::test_ready`], with an optional region map (`.wtr` BSP) installed on the grid — for /// tests in crates that cannot build a zone but need zone-line-region behaviour (e.g. the #683 - /// unresolved-index auto-cross in `eqoxide-net`). Test-only. + /// unresolved-index auto-cross in `eqoxide-net`). `None` leaves the grid with NO region data + /// (the honest "nobody attached any", not a load failure). Test-only. #[cfg(any(test, feature = "test-fixtures"))] pub fn test_ready_with_water(water: Option>) -> Self { + let mut col = Self::fixture_grid(); + col.set_water(water); + Self::ready("testfixture", 1, Arc::new(col)) + } + + /// [`Self::test_ready`], with the zone's region data **or the reason it failed to load** + /// installed on the grid (#803). The case this adds over `test_ready_with_water` is + /// `Err(RegionLoadError::…)`: a zone whose terrain GLB loaded fine — so the state genuinely IS + /// `Ready` — and whose `.wtr` did not. That combination is exactly the one the + /// `zone_assets_not_ready` gate does not cover, and the one `/v1/observe/zone_exits` used to + /// answer `[]` with 200 OK for. Test-only. + #[cfg(any(test, feature = "test-fixtures"))] + pub fn test_ready_with_region_data( + data: Result, + eqoxide_core::region_map::RegionLoadError>, + ) -> Self { + let mut col = Self::fixture_grid(); + col.set_region_data(data); + Self::ready("testfixture", 1, Arc::new(col)) + } + + /// The trivial 200×200 flat floor both `test_ready_*` fixtures build on — a grid that really + /// does have geometry, so `ready()` will not downgrade it. + #[cfg(any(test, feature = "test-fixtures"))] + fn fixture_grid() -> Collision { use eqoxide_assets::{MeshData, RenderMode, ZoneAssets}; let mesh = MeshData { positions: vec![ @@ -167,10 +193,7 @@ impl ZoneAssetState { texture_name: None, base_color: [1.0; 4], center: [0.0; 3], render_mode: RenderMode::Opaque, anim: None, }; - let mut col = Collision::build( - &ZoneAssets { terrain: vec![mesh], objects: vec![], textures: vec![] }, 32.0); - if water.is_some() { col.set_water(water); } - Self::ready("testfixture", 1, Arc::new(col)) + Collision::build(&ZoneAssets { terrain: vec![mesh], objects: vec![], textures: vec![] }, 32.0) } /// The live progress line while `Pending`, or the failure reason while `Failed`. @@ -268,18 +291,23 @@ impl NotUsable { /// forgotten by a caller that only had the state handy — and so the universal claim ("a `ready` /// observation is never about a zone you are not in") is a property test, not a live run. /// -/// **Which consumers actually go through it (verified by grep, not asserted — #600).** Three, and -/// they are the three that make a world-answering claim (a route/geometry answer or an -/// agent-observable `nav_state`) off the shared collision grid: +/// **Which consumers actually go through it (verified by grep, not asserted — #600; recounted in +/// the #821 review round 2, B2, which found this list one short).** **FOUR**, and they are the four +/// that make a world-answering claim (a route/geometry answer or an agent-observable `nav_state`) +/// off the shared collision grid: /// * the HTTP world-observation endpoints — `/observe/frame`, `/observe/zone_exits`, /// `/observe/zone_entrances`, `/observe/debug`'s zone block, etc. — via `zone_assets_not_ready` /// (`crates/eqoxide-http/src/observe.rs`), which early-returns a 503 before any collision read; +/// * `POST /v1/move/goto` (`crates/eqoxide-http/src/move_api.rs`, #579), which accepts the goal +/// but reports `zone_assets_pending` so `"status": "navigating"` is not read as "a walkable +/// route was found". **This one is NOT an `/observe/*` route**, which is exactly how it went +/// uncounted here for four issues; /// * the nav path-walker's `drive_walk` gate (`crate::walker`, #600), which refuses to route in the /// stale/loading window instead of steering on the previous zone's grid; and /// * `ActionLoop::drain_zone_cross` (`crates/eqoxide-net/src/action_loop.rs`, #600 review round 2), /// which resolves `/v1/move/zone_cross` off the grid and publishes `nav_state` — it answers the /// honest transient `zone_loading` while not usable instead of a definitive `no_path`. -/// A `None` here therefore guarantees, for ALL THREE, that the collision grid they go on to read is +/// A `None` here therefore guarantees, for ALL FOUR, that the collision grid they go on to read is /// the current zone's. What does NOT go through it, deliberately (each verified to make no /// route/geometry claim about the current zone): /// * the two per-zone DIAGNOSTIC COUNTERS `nav_support` and `nav_tight` read `shared_collision` @@ -303,6 +331,31 @@ pub fn usability(state: &ZoneAssetState, player_zone: &str) -> Option None } +/// [`usability`] **and the grid it blesses, in one call** — the verdict and the collision it vouches +/// for can never come from two different places (#821 review round 2, B4). +/// +/// The bug this closes: a reader used to ask `usability` (or `zone_assets_not_ready`) for permission +/// and then fetch the grid from [`crate::collision::SharedCollision`], a *separate* slot. Nothing — +/// not a type, not a test — coupled them. `/v1/observe/zone_exits` guarded that slot with a bare +/// `if let Some(col)` and no `else`, so a `None` there produced `200 []`, i.e. "this zone has no way +/// out", **having consulted no region map at all**. Production writes both slots together +/// ([`finish_zone_load`] literally stores `verdict.collision().cloned()`, the same `Arc`), but that +/// is a convention, and the HTTP testkit already violated it. With this, the `None` case is not +/// reachable to write: `Ready` OWNS its `Arc`, so a blessed verdict comes with a grid. +/// +/// Delegates to [`usability`] rather than re-deriving the rule, so the two can never disagree about +/// staleness, zone-name case, or an empty `player_zone`. +pub fn usable_collision<'a>(state: &'a ZoneAssetState, player_zone: &str) + -> Result<&'a Arc, NotUsable> +{ + if let Some(why) = usability(state, player_zone) { return Err(why); } + // `usability` returns `None` for `Ready` and for nothing else, and `Ready`'s `collision` field is + // NOT an `Option` — so this fallback is unreachable. It is spelled as a REFUSAL rather than an + // `unwrap`/`expect` because the honest answer to "we cannot find the grid" is never an empty + // world; `usable_collision_agrees_with_usability_for_every_state` asserts it never fires. + state.collision().ok_or(NotUsable::Idle) +} + /// Lock a [`ZoneAssetStateShared`], **recovering from poisoning**. /// /// A panic in the zone-asset loader while it holds this lock must not turn every later read into a @@ -557,6 +610,45 @@ mod tests { } } + /// [`usable_collision`] must agree with [`usability`] on EVERY state × zone combination, and + /// must hand back a grid whenever it agrees — never the unreachable `Idle` fallback. + /// + /// This is what makes the `if let Some(col)` fall-through in `/v1/observe/zone_exits` + /// unrepresentable rather than merely documented (#821 review round 2, B4): "the verdict says + /// yes" and "here is a grid" are now the same value, so there is no `None` branch left for a + /// caller to answer `[]` from. + #[test] + fn usable_collision_agrees_with_usability_for_every_state() { + let zones = ["qeynos", "freporte", "FREPORTE", "gfaydark", ""]; + let states: Vec<(&str, ZoneAssetState)> = vec![ + ("idle", ZoneAssetState::Idle), + ("pendA", ZoneAssetState::pending("qeynos", "loading…")), + ("failA", ZoneAssetState::failed("qeynos", "boom")), + ("readyA", ZoneAssetState::ready("qeynos", 3, floor_collision())), + ("readyB", ZoneAssetState::ready("freporte", 3, floor_collision())), + ]; + for (name, st) in &states { + for pz in zones { + match (usability(st, pz), usable_collision(st, pz)) { + (Some(why), Err(e)) => assert_eq!( + why, e, "{name}/{pz:?}: the two must give the SAME refusal"), + (None, Ok(col)) => assert!( + std::sync::Arc::ptr_eq(col, st.collision().unwrap()), + "{name}/{pz:?}: must hand back the state's OWN grid, not a copy"), + (u, c) => panic!( + "{name}/{pz:?}: disagreement — usability={u:?}, usable_collision ok={}", + c.is_ok()), + } + // …and the unreachable fallback never fires: a refusal is never `Idle` unless + // `usability` genuinely said `Idle`. + if let Err(NotUsable::Idle) = usable_collision(st, pz) { + assert_eq!(usability(st, pz), Some(NotUsable::Idle), + "{name}/{pz:?}: `Idle` came from the unreachable fallback, not the verdict"); + } + } + } + } + /// The specific F1 capture, as an assertion: standing in qeynos while the previous zone's /// assets are still fully `Ready` must NOT read as ready — and must name the reason, so an /// agent can tell "wrong world" from "no world". diff --git a/crates/eqoxide-net/src/action_loop.rs b/crates/eqoxide-net/src/action_loop.rs index d6194bb0..1f36b319 100644 --- a/crates/eqoxide-net/src/action_loop.rs +++ b/crates/eqoxide-net/src/action_loop.rs @@ -1769,7 +1769,19 @@ impl ActionLoop { .map(|zp| zp.iterator as i32).collect()) }; if !allowed { return None; } - c.zone_line_indices().into_iter() + // #803: `zone_line_indices` is now a `Result`. This fallback + // only ever ASKS for candidates, so an `Err` (no region data) + // yields none to offer, exactly as the old empty vec did — + // BEHAVIOUR HERE IS UNCHANGED, deliberately, because this file + // belongs to another change in flight. + // + // What is NOT fixed here: the `None` arm below then reports + // `zone_line_not_in_map`, whose documented meaning is "a + // client-side `.wtr` map-data GAP". When the `.wtr` did not + // load at all that reason names the wrong cause — the same + // substitution #803 kills on `/observe/zone_exits`, one caller + // over. Filed as #815 rather than fixed in this PR. + c.zone_line_indices().unwrap_or_default().into_iter() .filter(|idx| !resolvable.contains(idx)) .filter_map(|idx| c.find_zone_line_near(Some(idx), pos)) .min_by(|a, b| { diff --git a/docs/http-api.md b/docs/http-api.md index 9fc13cda..561ab6c3 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -545,6 +545,9 @@ which a later load on the same code refuted. | `GET /v1/observe/zone_exits` | Exits come out of the collision grid; before it exists this returned a confident `[]` — "this zone has no exits at all" — and during `stale` it returned the *previous* zone's exits. | | `GET /v1/observe/frame` | A PNG of the placeholder ground plane is indistinguishable from a genuinely empty zone, and a `stale` frame shows the zone you left. Pass **`?allow_pending=1`** if the loading screen is what you actually want. | +`zone_exits` has a **second, unrelated 503** that this readiness verdict does not cover — see +[Zone exits: `[]` means exactly one thing (#803)](#zone-exits--means-exactly-one-thing-803). + Every `200` from `/v1/observe/frame` also carries **`X-Zone-Assets-State:`** with the same word as `zone_assets.state`, so a PNG fetched with `?allow_pending=1` cannot be mistaken downstream for one of the real zone. Only `ready` means the image shows the zone the character is in. It also carries @@ -562,6 +565,50 @@ character is moving through a world the client has not built, so prefer waiting **`zone_assets_pending`** note while the assets are missing, and `nav_state` reads `zone_loading` until they land. +### Zone exits: `[]` means exactly one thing (#803) + +`GET /v1/observe/zone_exits` answers out of the zone's **region map** (`maps/water/.wtr`), +whose baked zone-line regions *are* the exits. That file is separate from the terrain the +[`zone_assets`](#zone_assets--is-the-world-this-response-describes-actually-loaded-579) verdict +tracks, and it can fail on its own: absent, unreadable, not region data, a format version this build +cannot read, or truncated. Until #803 all of those were discarded and the endpoint served **`[]` with +`200 OK`** — the same bytes as the true, common answer "this zone has no zone lines". Exits are the +only way out of a zone, so a failed *file read* read as a fact about the world: sealed in. + +Now: + +* **`200` with `[]`** means, and only means, *this zone's region map loaded and contains no + zone-line regions.* It is a real reading of the world. +* **`503 {"error": "zone_region_data_unavailable", "reason": …, "detail": …, "message": …}`** means + the question could not be answered. Unlike `zone_assets_not_ready`, **this does not clear by + polling** — the asset is missing or unusable, not loading. Re-sync or re-bake the zone's assets. + `reason` is the machine-readable cause below, `detail` is that cause rendered with its specifics + (e.g. the declared node count), and `message` is prose for a human reading the log. + +| `reason` | What happened | +|---|---| +| `region_data_missing` | No `.wtr` for this zone. | +| `region_data_unreadable` | The file exists but could not be read (permissions, a bad mount, a directory in its place). | +| `region_data_not_region_data` | Present, but not a region map (wrong magic, or shorter than the header). | +| `region_data_unsupported_version` | A `.wtr` format version this build cannot read. | +| `region_data_truncated` | The header declares more BSP nodes than the file carries. | + +**A sixth `reason` exists in the code and is NOT in that table on purpose: `region_data_not_attached`** +(#821 review round 2, B3 — a previous revision listed it as an outcome you might receive). It is the +state a freshly built collision grid is in before any region data is handed to it. **No release +build can serve it**: the client has exactly one production construction of a collision grid +(`build_zone_collision` in `src/app.rs`), which builds and attaches in a single call, and the only +reason-free way to write the slot (`Collision::set_water`) is `#[cfg(any(test, feature = +"test-fixtures"))]` and so does not exist in a release binary. It is listed here only so that an +agent that somehow *does* receive it knows what it means: **a client bug, not an asset problem** — +re-syncing assets will not help; file it. (This is an argument from enumerating every construction +site, not a type-level guarantee — nothing stops a future non-test grid from reaching a reader +un-attached.) + +**Not covered by this:** `POST /v1/move/zone_cross` still reports `zone_line_not_in_map` (documented +as a map-data *gap*) when the region map failed to load rather than merely lacking the region — the +same substitution, one caller over, tracked as #815. + ### Camera override for `/observe/frame` (#422) The live/persistent gameplay camera is often at an unhelpful angle for judging nav or collision diff --git a/src/app.rs b/src/app.rs index 1ec1381b..78be4cb1 100644 --- a/src/app.rs +++ b/src/app.rs @@ -52,6 +52,41 @@ fn publish_load(slot: &Arc>>, gen: u64, load: PendingL } } +/// **The client's ONE production construction of a zone's collision grid**: build it from the +/// terrain, then attach the zone's region map (`/water/.wtr`) — water volumes for +/// swim/descend routing, AND the DRNTP zone-line regions that ARE this zone's exits. +/// +/// #803: the loader's `Err` is kept and installed on the grid. A discarded failure used to read as +/// "this zone has no water and no exits", which `/v1/observe/zone_exits` published as `[]` with 200 +/// OK — a confident "there is no way out of here" that the agent had no way to doubt. +/// +/// **Why this is a named function and not four lines inside the loader closure** (#821 review round +/// 2, N2): while it lived inline in a thread closure it could not be called from a test, and the +/// reviewer proved the consequence by deleting the `set_region_data` line — the whole root crate +/// stayed green, though every zone's grid would then carry `Err(NotAttached)` and `zone_exits` would +/// 503 in **every zone in the game**. The type change in #803 forced all the *library* call sites to +/// speak up; the line that actually feeds them in the shipped client was pinned by nothing. It is +/// pinned by `zone_load_wiring_803` below. +/// +/// One deliberate behaviour change from the inline version: the `.wtr` load (and its `warn!`) now +/// happens only when the terrain assets loaded. When they did not, no grid is built at all and the +/// zone publishes `Failed` — a warning about the region data of a world that does not exist was +/// noise, and there is no grid for it to be about. +pub(crate) fn build_zone_collision( + za: &assets::ZoneAssets, + maps_dir: &std::path::Path, + zone_name: &str, +) -> collision::Collision { + let water = crate::region_map::RegionMap::try_load(&maps_dir.join("water"), zone_name) + .map(Arc::new); + if let Err(e) = &water { + tracing::warn!("region_map: zone '{}' has no usable region data: {}", zone_name, e); + } + let mut c = collision::Collision::build(za, 32.0); + c.set_region_data(water); + c +} + /// The `watch_for_lost_load` decision, pure so it can be tested (#595 review F3). `Some(zone)` means /// the state is stuck `Pending` for `zone` and no loader is left that could ever report it — a /// panic, or a reply clobbered in the handoff slot. `None` means leave it alone: either a loader is @@ -786,14 +821,8 @@ impl App { }; set_status("Building collision grid…"); - // Load the zone's water regions (maps/water/.wtr) so find_path can swim/descend - // through water where there's no walkable connection. None if the zone has no .wtr. - let water = crate::region_map::RegionMap::load(&maps_dir.join("water"), &zone_name).map(Arc::new); - let collision = opt_assets.as_ref().map(|za| { - let mut c = collision::Collision::build(za, 32.0); - c.set_water(water); - Arc::new(c) - }); + let collision = opt_assets.as_ref() + .map(|za| Arc::new(build_zone_collision(za, &maps_dir, &zone_name))); set_status("Loading minimap…"); let zone_map = zone_map::ZoneMap::load(&maps_dir, &zone_name); @@ -3338,3 +3367,102 @@ mod worker_panic_protection_tests { assert_eq!(dead.lock().unwrap().as_deref(), Some("login failed: boom")); } } + +/// **#821 review round 2, N2 — the production zone-load wiring, pinned.** +/// +/// #803 made "no exits" and "could not read the exits" different TYPES, and the compiler forced +/// every library call site to say which it meant. But the one line in the shipped client that +/// actually feeds region data onto the grid — [`build_zone_collision`]'s `set_region_data` — lived +/// inside a spawned thread closure, so no test could reach it. The reviewer deleted it: the whole +/// root crate stayed green, while a real client would have carried `Err(NotAttached)` on every +/// zone's grid and answered `503` from `/v1/observe/zone_exits` in **every zone in the game**. +/// +/// These tests call the production function directly, against a real `.wtr` on disk, and assert the +/// grid can answer the exits question afterwards. They cover the whole wiring, not just the write: +/// the `maps/water/.wtr` path convention, the loader's `Ok`/`Err`, and the attach. +#[cfg(test)] +mod zone_load_wiring_803 { + use super::build_zone_collision; + use crate::assets::{MeshData, RenderMode, ZoneAssets}; + + /// A minimal but non-degenerate zone: one floor quad. `Collision::build` needs real triangles + /// (an empty grid takes a different constructor branch), and `ZoneAssetState::ready` refuses a + /// grid with none — so this is the shape a production `Ready` zone actually has. + fn one_floor() -> ZoneAssets { + let floor = MeshData { + positions: vec![[-100.0, -20.0, -100.0], [100.0, -20.0, -100.0], + [100.0, -20.0, 100.0], [-100.0, -20.0, 100.0]], + normals: vec![[0.0, 1.0, 0.0]; 4], uvs: vec![[0.0, 0.0]; 4], + indices: vec![0, 1, 2, 0, 2, 3], texture_name: None, base_color: [1.0; 4], + center: [0.0; 3], render_mode: RenderMode::Opaque, anim: None, + }; + ZoneAssets { terrain: vec![floor], objects: vec![], textures: vec![] } + } + + /// A v2 `.wtr` byte blob whose whole lower half is one zone-line region carrying `index`. + /// + /// Written as BYTES on purpose: the point of this test is the production path from a FILE at + /// `/water/.wtr` to an answerable grid. Handing `build_zone_collision` an + /// already-parsed `RegionMap` would skip `try_load`, the directory join and the filename + /// convention — three of the four things that have to be right. Layout is the one documented at + /// the top of `eqoxide_core::region_map`: magic, u32 version, u32 node count, then per node + /// `i32 index, 3×f32 normal, f32 split, i32 region, i32 special, i32 left, i32 right, + /// i32 zone_line_index`. + fn wtr_with_one_zone_line(index: i32) -> Vec { + // (normal, split, special, left, right, zone_line_index). `leaf_at` walks LEFT when + // `dot(normal, p) + split > 0`, so z > 0 lands on the dry leaf and z < 0 on the zone line. + // `special == 3` is `REGION_ZONE_LINE` (private to region_map, hence the literal). + let nodes: &[([f32; 3], f32, i32, i32, i32, i32)] = &[ + ([0.0, 0.0, 1.0], 0.0, 0, 2, 3, 0), // split at z == 0 + ([0.0; 3], 0.0, 0, 0, 0, 0), // 2: dry leaf (above) + ([0.0; 3], 0.0, 3, 0, 0, index), // 3: ZONE LINE leaf (below) + ]; + let mut out = b"EQEMUWATER".to_vec(); + out.extend_from_slice(&2u32.to_le_bytes()); + out.extend_from_slice(&(nodes.len() as u32).to_le_bytes()); + for (i, (normal, split, special, left, right, zli)) in nodes.iter().enumerate() { + out.extend_from_slice(&(i as i32).to_le_bytes()); + for c in normal { out.extend_from_slice(&c.to_le_bytes()); } + out.extend_from_slice(&split.to_le_bytes()); + out.extend_from_slice(&0i32.to_le_bytes()); // region ordinal, unused by the reader + out.extend_from_slice(&special.to_le_bytes()); + out.extend_from_slice(&left.to_le_bytes()); + out.extend_from_slice(&right.to_le_bytes()); + out.extend_from_slice(&zli.to_le_bytes()); + } + out + } + + /// **The healthy path.** A zone whose `.wtr` is where production looks for it must produce a + /// grid that ENUMERATES its exits. Delete the `set_region_data` call and this reads + /// `Err(NotAttached)` instead of `Ok([7])`. + #[test] + fn a_zone_with_a_wtr_gets_a_grid_that_can_answer_its_exits() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join("water")).unwrap(); + std::fs::write(dir.path().join("water/testzone.wtr"), wtr_with_one_zone_line(7)).unwrap(); + + let col = build_zone_collision(&one_floor(), dir.path(), "testzone"); + assert_eq!(col.zone_line_indices().as_deref(), Ok(&[7][..]), + "the loaded region map's zone line must reach the grid — this is what \ + /v1/observe/zone_exits reports and the ONLY production write that puts it there"); + assert_eq!(col.region_data_absent(), None, "the region data IS attached"); + } + + /// **The failure path.** A zone with no `.wtr` must leave the grid able to say WHY — not + /// `Ok([])` ("this zone has no way out", the #803 falsehood) and not `NotAttached` ("nobody + /// ever asked", which is a client bug, not a missing asset). + #[test] + fn a_zone_with_no_wtr_gets_a_grid_that_refuses_with_the_real_cause() { + let dir = tempfile::tempdir().unwrap(); + let col = build_zone_collision(&one_floor(), dir.path(), "testzone"); + + let absent = col.region_data_absent().expect("a missing .wtr is an absence, not an answer"); + assert_eq!(absent.as_str(), "region_data_missing", + "the grid must name the LOAD failure. `region_data_not_attached` here would mean the \ + production write was skipped entirely (N2), which reads to an operator as a client \ + bug rather than a missing asset"); + assert!(col.zone_line_indices().is_err(), + "and the exits question refuses rather than answering the empty list"); + } +} diff --git a/src/movement.rs b/src/movement.rs index cc0f48f3..1801fae3 100644 --- a/src/movement.rs +++ b/src/movement.rs @@ -2610,7 +2610,7 @@ mod tests { let Ok(za) = crate::assets::ZoneAssets::from_glb(&dir.join(format!("{name}.glb"))) else { continue }; let mut col = Collision::build(&za, 32.0); if col.cols == 0 { continue; } - col.set_water(crate::region_map::RegionMap::load(&dir.join("maps/water"), name) + col.set_region_data(crate::region_map::RegionMap::try_load(&dir.join("maps/water"), name) .map(std::sync::Arc::new)); t_zones += 1; let mut seed: u64 = 0x9E37_79B9_7F4A_7C15; diff --git a/tests/water_capability.rs b/tests/water_capability.rs index 2edb7ab2..df09ab18 100644 --- a/tests/water_capability.rs +++ b/tests/water_capability.rs @@ -44,8 +44,8 @@ fn zone(name: &str) -> Collision { let za = ZoneAssets::from_glb(&dir.join(format!("{name}.glb"))) .unwrap_or_else(|e| panic!("baked {name}.glb required at $EQZONES: {e:?}")); let mut c = Collision::build(&za, 32.0); - c.set_water(Some(std::sync::Arc::new( - RegionMap::load(&dir.join("maps/water"), name).expect("baked .wtr required")))); + c.set_region_data(Ok(std::sync::Arc::new( + RegionMap::try_load(&dir.join("maps/water"), name).expect("baked .wtr required")))); c }