From 76e787d704aee64d83a6d65e19de28628b20b085 Mon Sep 17 00:00:00 2001 From: Sam Date: Sat, 8 Aug 2026 17:13:02 +0700 Subject: [PATCH] step budgets come from the binary, and a step-down follows the ground MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - the step-up and step-down budgets are the client's own verified numbers, and the step regime is chosen by the blocking edge - walking off a kerb follows the surface down instead of diving past the edge; steep ground supports you only while you are descending onto it - a ride on a moving platform survives the ground snap, and a vertical wall no longer depends on a float's sign - trainer and craft rows sort with the real client's own comparators, one per window — Beast Training lists a skill's ranks in ascending order again Co-Authored-By: Claude Fable 5 --- crates/benilla-app/src/net/motion/remote.rs | 14 +- crates/benilla-app/src/player/move_trace.rs | 47 +- crates/benilla-app/src/player/mover.rs | 532 +++++++++++++-- crates/benilla-app/src/player/state.rs | 100 ++- crates/benilla-app/src/ui_craft.rs | 46 +- .../src/ui_script/trainer_tests.rs | 8 +- crates/benilla-app/src/ui_tradeskill.rs | 16 +- crates/benilla-app/src/ui_trainer/law.rs | 89 ++- crates/benilla-app/src/ui_trainer/mod.rs | 74 ++- crates/benilla-app/src/ui_trainer/tests.rs | 63 +- .../benilla-formats/examples/order_probe.rs | 80 +++ crates/benilla-formats/src/lib.rs | 3 +- crates/benilla-formats/src/spells/mod.rs | 6 + crates/benilla-ui/src/script/craft.rs | 226 ++++++- crates/benilla-ui/src/script/mod.rs | 2 +- .../src/script/tests/tooltip_spell.rs | 6 +- .../benilla-ui/src/script/tradeskill/mod.rs | 8 +- .../src/script/{trainer.rs => trainer/mod.rs} | 612 ++++++----------- crates/benilla-ui/src/script/trainer/tests.rs | 620 ++++++++++++++++++ 19 files changed, 1933 insertions(+), 619 deletions(-) create mode 100644 crates/benilla-formats/examples/order_probe.rs rename crates/benilla-ui/src/script/{trainer.rs => trainer/mod.rs} (60%) create mode 100644 crates/benilla-ui/src/script/trainer/tests.rs diff --git a/crates/benilla-app/src/net/motion/remote.rs b/crates/benilla-app/src/net/motion/remote.rs index 2dd1906e..7922e2ba 100644 --- a/crates/benilla-app/src/net/motion/remote.rs +++ b/crates/benilla-app/src/net/motion/remote.rs @@ -510,11 +510,15 @@ pub(in crate::net) fn extrapolate_remote_units( from, vel, time.delta(), - // No hover offset: a remote's own granted modes are not modelled yet — decision - // 0866 builds the family for our own mover. A hovering *observed* player draws - // a yard low until the relayed `MSG_MOVE_HOVER` is read into per-unit mode - // state; the same shape as every other unmodelled remote mode. - 0.0, + // Default support: **no hover offset**, because a remote's own granted modes + // are not modelled yet — decision 0866 builds the family for our own mover. A + // hovering *observed* player draws a yard low until the relayed + // `MSG_MOVE_HOVER` is read into per-unit mode state; the same shape as every + // other unmodelled remote mode. And **no carried steep-support bit** (1129): + // it is per-frame state the dead-reckon does not keep between packets, so a + // watched player descending a steep face gets the ordinary cone reach, and the + // next packet's wire Z corrects whatever that misses. + crate::player::mover::Support::default(), ) .center }; diff --git a/crates/benilla-app/src/player/move_trace.rs b/crates/benilla-app/src/player/move_trace.rs index 1eb105bd..ab412ab0 100644 --- a/crates/benilla-app/src/player/move_trace.rs +++ b/crates/benilla-app/src/player/move_trace.rs @@ -26,6 +26,13 @@ pub(super) struct Frame { pub y_in: f32, /// Feet height leaving the step (yd) — after the slide *and* the step-down snap. pub y_out: f32, + /// Horizontal distance covered this frame (yd). Paired with the vertical it gives the frame's + /// **descent slope**, which is the whole question behind "we dive over the edge instead of + /// stepping down it": the reference bounds a step-down to the foot cone's own + /// [`super::STEP_SLOPE_RATIO`] per unit of horizontal travel (wow-re `step-off-recourse.md`), so + /// a walk-off that reads far steeper — or worse, flat-then-plummet across two frames — is the + /// mover leaving the surface instead of following it down. + pub dx: f32, pub grounded: bool, pub on_walkable: bool, pub vel_y: f32, @@ -158,19 +165,57 @@ pub(super) fn frame(f: Frame) { } ), }; + // The descent slope, printed only while actually going down and moving: `>cone` marks a frame + // steeper than the foot cone allows, which is the signature of a dive rather than a step-down. + let slope = if dy < -1.0e-4 && f.dx > 1.0e-4 { + let s = -dy / f.dx; + format!( + " dx={:.3} slope={s:.2}{}", + f.dx, + if s > super::STEP_SLOPE_RATIO { + " >cone" + } else { + "" + } + ) + } else { + format!(" dx={:.3}", f.dx) + }; + // The dive's real signature, and it is not steepness — it is the *absence* of descent. A body + // that leaves the surface travels forward with `dy≈0` while the probe reports a steep face right + // beneath it; everything after that is a ballistic arc, and by the time the fall looks steep the + // frame that caused it is long gone. `>cone` was the first guess and the director's capture + // showed it never fires: the arc starts at slope 0.05 and only reaches 0.83. This is the marker + // that would have named it on the first read. + let left = f + .snap + .and_then(|(_, hit)| hit) + .is_some_and(|(_, ny)| ny < super::GROUND_COS) + // **Flat is the signature — not "anything that isn't descending".** A dive frame is one that + // travels forward and goes *nowhere* vertically while a steep surface sits below it. Written + // as `dy > -eps` this also swallows every frame that is climbing, where not descending is + // exactly right: first the atomic step-ups, then — once those were excluded — the ordinary + // walk up a hill, which rises through the slope ride and carries no `climb` at all. Three + // false-positive classes on three consecutive captures, the second of which buried nine real + // takeoffs under fifty-five phantoms. A window around zero admits none of them. + && dy.abs() < 1.0e-3 + && f.dx > 1.0e-4; + let left = if left { " LEFT-SURFACE" } else { "" }; let climb = f.climb.map_or(String::new(), |t| format!(" climb={t:+.3}")); let anchored = if f.anchored { " ROOTED" } else { "" }; dbg_trace::line( "move", &format!( - "y {:9.3} -> {:9.3} dy={:+.3} grounded={} walk={} vy={:+7.2} {}{}{}", + "y {:9.3} -> {:9.3} dy={:+.3}{} grounded={} walk={} vy={:+7.2} {}{}{}{}", f.y_in, f.y_out, dy, + slope, f.grounded as u8, f.on_walkable as u8, f.vel_y, snap, + left, climb, anchored ), diff --git a/crates/benilla-app/src/player/mover.rs b/crates/benilla-app/src/player/mover.rs index f9315ee1..b7cb5ae5 100644 --- a/crates/benilla-app/src/player/mover.rs +++ b/crates/benilla-app/src/player/mover.rs @@ -151,7 +151,7 @@ pub(super) fn step( // failure 0209's atomic commit was built to make impossible. The ride is re-earned from the // certification every frame it continues, so this can hold nothing up that has not just proved // it can be climbed. - let on_floor = !held && (on_walkable || player.cone_riding) && player.vel_y <= 0.0; + let on_floor = !held && (on_walkable || player.steep_support) && player.vel_y <= 0.0; // The wedged rest (decision 0211) stands until real ground takes over or the support // vanishes — we walked off the funnel wall into open air, which resumes a normal fresh fall. @@ -175,7 +175,7 @@ pub(super) fn step( if want_jump { player.vel_y = JUMP_SPEED; player.wedged = false; - player.cone_riding = false; + player.steep_support = false; jumped = true; } } else { @@ -221,7 +221,10 @@ pub(super) fn step( center, player.horiz_vel, time.delta(), - hover_offset, + Support { + offset: hover_offset, + steep: player.steep_support, + }, ); // The step-up probe (this is the LOCAL mover; a remote's dead-reckon is not a report // anyone is looking at): a walk frame that went nowhere writes the `stup` deep report — @@ -239,7 +242,7 @@ pub(super) fn step( center = g.center; climb = g.climb; snap_probe = g.snap; - player.cone_riding = g.cone_riding; + player.steep_support = g.steep_support; if let Some(e) = g.ground { ground_entity = Some(e); } @@ -255,7 +258,7 @@ pub(super) fn step( // remote mover's arc runs, so a jump meets our walls whoever is jumping (decision 0627). center = airborne_step(ms, capsule, &filter, center, velocity, time.delta()); // Nothing here can be riding a cone: this arm is the arc, the hold and the anchor. - player.cone_riding = false; + player.steep_support = false; } // Wedge-rest detection (decisions 0211/0212): airborne, already falling fast, yet the // descent achieved is a sliver of what gravity intended — [`WEDGE_STILL_FRAMES`] in a row @@ -330,6 +333,7 @@ pub(super) fn step( move_trace::frame(move_trace::Frame { y_in: pre_move.y - half_h.y, y_out: player.pos.y, + dx: (center - pre_move).xz().length(), grounded, on_walkable, vel_y: player.vel_y, @@ -351,6 +355,23 @@ pub(super) fn step( } } +/// **What already holds the body up, entering the frame** — the two facts [`grounded_step`] cannot +/// work out for itself, because both are carried from the *previous* frame's resolve. +/// +/// They travel together because they are the same question asked twice: how far below the feet does +/// "the ground" start, and does the thing under us count as a floor at all. +#[derive(Clone, Copy, Default)] +pub(crate) struct Support { + /// The body rests this far **above** the surface — HOVER's float (decision 0866), + /// [`super::HOVER_HEIGHT`] while the mode is up and `0.0` for everyone else, which is the + /// ordinary case. + pub(crate) offset: f32, + /// The support entering the frame is a **certified steep contact**, not a walkable floor — the + /// reference's `0x4000000` (decision 1125). It is the sole gate on the step-down probe's deep + /// reach; see the reach in [`grounded_step`]. + pub(crate) steep: bool, +} + /// What one grounded walk step resolved against the world came out as ([`grounded_step`]). pub(crate) struct GroundedStep { /// The resolved capsule centre. @@ -362,10 +383,11 @@ pub(crate) struct GroundedStep { /// The height gain (yd) this frame's climb achieved — the atomic step-up's committed rise, or /// the distance a foot-cone ride carried the body up its skirt. pub(crate) climb: Option, - /// The body is part-way up a **foot-cone ride** and is *supported by the edge it is riding*, - /// not standing on a floor — so the caller must keep treating it as grounded until a walkable - /// floor takes over (decision 1123). Always `false` for a frame that did not ride. - pub(crate) cone_riding: bool, + /// The body is **supported by a certified steep contact** rather than standing on a walkable + /// floor — climbing a foot-cone ride (1123) or following a surface down off a ledge (1127) — + /// so the caller must keep treating it as grounded until a walkable floor takes over. This is + /// the reference's `0x4000000` (decision 1125). `false` for an ordinary frame. + pub(crate) steep_support: bool, /// The election snap's `(probe reach, what it found)` — trace fodder, `None` when the step-up /// took the frame instead. The inner pair is `(hit distance, hit normal.y)`. pub(crate) snap: Option<(f32, Option<(f32, f32)>)>, @@ -396,8 +418,9 @@ pub(crate) fn grounded_step( center: Vec3, horiz_vel: Vec3, dt: std::time::Duration, - surface_offset: f32, + support: Support, ) -> GroundedStep { + let surface_offset = support.offset; let cast = |from: Vec3, disp: Vec3| { crate::collision::one_sided::cast_move(ms, capsule, from, disp, SKIN_WIDTH, filter) }; @@ -454,20 +477,34 @@ pub(crate) fn grounded_step( ); } } - // **Which regime** (decision 1123, wow-re `climb-vs-slide.md` §2/§4/§6). The certification above - // settles *whether* the obstacle can be cleared; its committed rise settles *how*. The real + // **Which regime** (decisions 1123/1126, wow-re `climb-vs-slide.md` §2/§4/§6). The certification + // above settles *whether* the obstacle can be cleared; the height of the **blocking edge** — the + // face the look-ahead is pressed against, measured from the feet — settles *how*. The real // client's solid is a cone below [`FOOT_CONE_HEIGHT`] and a vertical box above it, so a low edge - // never meets a wall to be lifted over — it meets the slanted skirt, and the ordinary slide - // carries the body up it. Only an edge that clears the cone meets the box square, and that - // square meeting is the instant step-up. One law, two outcomes, selected by the rise: - // - `dy ≤ FOOT_CONE_HEIGHT` ⇒ **ride** the skirt at atan 1.8494 ≈ 61.6°, over as many frames - // as the gait needs (a kerb takes two at a run) — smooth, and never a teleport; - // - `dy > FOOT_CONE_HEIGHT` ⇒ the atomic pop of 0209, unchanged; - // - no certification at all ⇒ neither: the plain slide, and the body never rises. (This is - // the gate that keeps the ride honest — a tall wall's contact point sits *inside* the cone - // band too, so height alone would ride it. Only the certification distinguishes them.) - let ride_to = match attempt.verdict { - StepVerdict::Commit { landed, dy, .. } if dy <= FOOT_CONE_HEIGHT => Some(landed.y), + // never meets a wall to be lifted over: it meets the slanted skirt, and the ordinary slide + // carries the body up it. Only an edge that clears the cone meets the box square, and that square + // meeting is the instant step-up. One law, two outcomes: + // - edge inside the cone ⇒ **ride** the skirt at atan 1.8494 ≈ 61.6°, over as many frames as + // the gait needs — smooth, and never a teleport; + // - edge above the cone ⇒ the atomic pop of 0209, unchanged; + // - no certification at all ⇒ neither: the plain slide, and the body never rises. + // + // **The certification is the gate, not the height** — a tall wall's contact against a *capsule* + // sits at the hemisphere centre, ≈0.33 yd up and squarely inside the band, so height alone would + // ride a cliff. Only "the settle found a walkable floor" separates the two. + // + // 1123 selected on the certified *rise* instead, which was indistinguishable while the advance was + // one body radius and became wrong the moment 1126 lengthened it: the rise then measures how high + // the ground is a yard further on, not how tall the thing in front of you is. Booty Bay's 0.40 yd + // step onto a rising ramp reads as a 0.755 yd obstacle under that rule and teleports; its blocking + // edge is 0.40 and rides. The edge is the quantity the reference's own cone band tests, and unlike + // the rise it does not move when the probe reaches further. + let ride_to = match (attempt.verdict, attempt.contact) { + (StepVerdict::Commit { landed, .. }, Some((p, _))) + if p.y - (center.y - CAPSULE_HEIGHT * 0.5) <= FOOT_CONE_HEIGHT => + { + Some(landed.y) + } _ => None, }; if ride_to.is_none() { @@ -479,7 +516,7 @@ pub(crate) fn grounded_step( ground: None, climb: Some(landed.y - center.y), snap: None, - cone_riding: false, + steep_support: false, }; } } @@ -520,47 +557,136 @@ pub(crate) fn grounded_step( let mut slid = out.position; // Snap onto the surface so we follow downhill slopes + steps down — the client's step-vs-fall // election (`0x6367b0`, wow-re `step-vs-fall-election.md`): the probe reaches - // [`STEP_SLOPE_RATIO`]·travel + [`STEP_SNAP_SLACK`] + the unit's collision height (`0x617430` = - // `[unit+0xb8]`, our [`CAPSULE_HEIGHT`]; the election's `0x4000000`-gated extension — decision - // 0182) and snaps only onto a *walkable* floor (≤50°, the election's own `cos50°` = - // [`GROUND_COS`]). A deeper or steeper floor is NOT absorbed: no snap, the next frame's ground - // probe misses, and the gap becomes a fall (the client's `StartFalling(0)` election) — a short - // ledge drop reads as a quick, continuous, steep descent, which is what the director's eye - // confirmed against the reference (decision 0190; 0189's instant absorbed step read as a - // teleport and was reverted). + // [`STEP_SLOPE_RATIO`]·travel + [`STEP_SNAP_SLACK`], and snaps only onto a *walkable* floor + // (≤50°, the election's own `cos50°` = [`GROUND_COS`]). // - // Standing still the reach is slack + the collision height, which is what re-grounds an - // *idle* body every frame: the small float a raw wire Z leaves a watched player standing on - // our terrain is taken out here, the same way [`crate::net::motion::ground_clamp_creatures`] - // takes it out of an idle NPC. - // `surface_offset` is HOVER (decision 0866): the reference's WALK resolver `0x6367b0` adds + // **The reach is the cone's own slope, and nothing else, on an ordinary walking frame** + // (decision 1129). It used to carry a flat `+`[`CAPSULE_HEIGHT`] — 2.028 yd of unconditional + // extra depth — on the reading that `0x617430` returned a collision height. Both halves of that + // were wrong (decision 1125, wow-re `mover-collision-scalars.md` + `step-off-recourse.md`): + // `0x617430` is `[unit+0xb8]`, the dimensionless scale ratio `max(SCALE_X / CreatureModelScale, + // 1)`, so `H` is **1.0** for a player and not 2.028; and the reference adds it only while + // `0x4000000` is set — "the current support is a certified STEEP contact, not a walkable + // floor", which is [`Support::steep`]. So the deep reach belongs to the step-down *recourse*, + // where a body already following a steep face down needs to see the walkable ground waiting at + // its foot; the ordinary walking frame reaches exactly as far as the foot cone could rest. + // + // What that costs is real and intended: a drop deeper than the cone is NOT absorbed. No snap, + // the next frame's ground probe misses, and the gap becomes a fall (the client's + // `StartFalling(0)` election) — a short ledge drop reads as a quick, continuous, steep descent, + // which is what the director's eye confirmed against the reference (decision 0190; 0189's + // instant absorbed step read as a teleport and was reverted). At 2.028 yd we were absorbing + // nearly two body-heights of ledge in one frame, which is the same teleport by another route. + // + // Standing still the reach collapses to the slack alone, so an idle body is re-grounded only + // when it is genuinely resting on the floor; a body left floating above one falls the float and + // lands, rather than being pulled down through up to two yards of air. + // + // [`Support::offset`] is HOVER (decision 0866): the reference's WALK resolver `0x6367b0` adds // `[0x7ff9d8]` = 1.0 to this same surface offset while `MOVEFLAG_HOVER` is set, and widens the // step-down reach by the same yard (`0x633e35`) so the float still follows the ground down. // Both halves are here: the reach grows by the offset, and the snap stops that far short of the // floor. Zero for everyone not hovering, which is the ordinary case and unchanged. let d = slid - center; - let reach = - d.x.hypot(d.z) * STEP_SLOPE_RATIO + STEP_SNAP_SLACK + CAPSULE_HEIGHT + surface_offset; + let reach = d.x.hypot(d.z) * STEP_SLOPE_RATIO + + STEP_SNAP_SLACK + + if support.steep { STEP_UP_HEIGHT } else { 0.0 } + + surface_offset; let hit = cast(slid, Vec3::NEG_Y * reach); let snap = Some((reach, hit.as_ref().map(|h| (h.distance, h.normal1.y)))); let mut ground = None; + let mut steep_support = false; + // **A ride's height is earned; the snap follows ground *down*, it never undoes a climb.** + // + // This is where our capsule and the reference's cone part company, and it has to be said out + // loud or the ride only works on bevels. Mid-ride the real client's foot cone is *resting on + // the very edge it is climbing* — its skirt is in contact, so its down-probe is blocked at zero + // and there is nothing to settle to. A capsule against a **vertical** riser touches it side-on + // and hangs clear of everything below, so the same down-probe sees the floor it is climbing + // away from, a full ride's height beneath it, and yanks it straight back down — every frame, + // forever. Booty Bay's 0.40 yd step is exactly that: certified `COMMIT dy=+0.439` on every + // frame, and stalled at +0.020. + // + // So while a ride is in progress the only floor that may end it is one **higher than the ride + // began on** — the tread it is certified to be climbing to. A floor at or below the start is + // the ground being left behind, and it gets no say. + // The height this frame's ride earned, read before the snap can spend any of it. (The two snap + // arms below are mutually exclusive — walkable or steep — so one value serves both.) + let ride_rise = (slid.y - center.y).max(0.0); + // **The step-down follows the surface; it does not leave it** (decision 1127). The reference's + // finalize writes `pos.z -= min(clearance, d_h·1.8493990)` *before* it classifies anything, and a + // steep landing is answered by the multipass rather than by a fall — a `ret 2` continues, + // grounded, with the "my support is a certified steep contact" bit set (wow-re + // `step-off-recourse.md`; the bit's meaning is decision 1125's). + // + // We refused every hit under 50° outright, so walking off a kerb the probe found only the ~61° + // riser, declined it, and the body kept its full height while its forward speed carried it out + // past the edge — then gravity took over. The director's capture is unambiguous: one frame of + // `dy=+0.000 dx=0.118` with the surface 0.162 below, then eighteen airborne frames landing 2.1 yd + // downrange. That is the dive. + // + // **The descent is unconditional, it is bounded by the probe itself, and only the *support* is + // conditional.** Byte-exact (`0x636e45`–`0x636e52`, wow-re `step-off-recourse.md` §1): the + // finalize sweeps down by `L` and writes `pos.z -= achieved`, where `achieved` is the sweep's own + // output — the clearance when something is under us, and the **whole of `L`** when nothing is — + // and only *then* does it classify what it found. So the cap and the reach are the same number, + // and there is no second, smaller bound: `L` is how far the body may fall to stay with its + // surface this frame, whatever the surface turns out to be. + // + // 1127 shipped a separate `min(drop, d_h·1.8494 + 1/36)` cap because at the time `reach` also + // carried the bogus flat 2.028 and could not be used as the bound. With the reach corrected + // above, the two are one quantity again — and the difference is exactly what closes the last of + // the dive: on a face **steeper than the cone** the old cap could never keep up (walking off the + // 0.91 yd step, whose face is 66°, the surface drops 0.256 in a frame the cone allowed 0.242, so + // every frame leaked its remainder into a small fall). Once the first frame latches the + // steep-support bit, `L` grows by `H` and the following frames can take the whole clearance. + if let Some(h) = hit.as_ref().filter(|h| h.normal1.y < GROUND_COS) { + let drop = (h.distance - surface_offset).max(0.0); + slid.y -= drop; + // **Support is earned by descending, not by touching.** A bound alone reads a body *resting* + // on a steep face as supported — standing still the reach collapses to the slack, the resting + // clearance is ~0, and any "is it within reach" test passes — which perches a motionless body + // on a 60° bank forever instead of letting it slide off. That is 1121's named "standing on + // >50° faces" hazard, and it is a worse defect than the dive this recourse exists to fix. + // + // Requiring real downward progress states the actual condition: this is a step-*down* in + // progress, not a resting state. A body standing on a bank, or traversing one across the + // slope, makes no progress and gets nothing. (The reference reaches the same place by a + // different road — its `0x4000000` is set only by a multipass `ret 2`, a full rise-step-settle + // certification that a body with nowhere to descend to cannot pass.) + steep_support = drop > STEP_SNAP_SLACK; + } + // **Nothing under us: descend the whole probe anyway, then let the fall elect.** The same + // `pos.z -= achieved` runs on the no-hit leg with `achieved == L` — wow-re `step-off-recourse.md` + // §1, "no committed record: `pos.z` has ALREADY dropped by the full `L`" — so the frame that + // leaves a ledge starts its fall a probe's-worth lower instead of flat. A ride is exempt: mid-ride + // the body legitimately hangs clear of everything below, and this would spend the climb. + if hit.is_none() && !rode { + slid.y -= (reach - surface_offset).max(0.0); + } if let Some(h) = hit.filter(|h| h.normal1.y >= GROUND_COS) { // `max(…, 0)` is the reference's, and it is the difference between a float and a pop: the // snap **only ever descends** (`0x636e8d`–`0x636e9e` skips the write when `L − 1.0 < 0`), so // a hovering body that is already within its clearance is left where it is rather than // yanked up to it. The rise to clearance is a separate rate-limited climb — see [`step`]. - slid.y -= (h.distance - surface_offset).max(0.0); - ground = Some(h.entity); + let drop = (h.distance - surface_offset).max(0.0); + if !(rode && drop >= ride_rise) { + slid.y -= drop; + ground = Some(h.entity); + } } GroundedStep { center: slid, // A ride is a climb too — the trace reads the same whichever regime moved the body. climb: rode.then_some(slid.y - center.y), - // Mid-ride the body is held up by the edge under its skirt, and the frame-start ground - // probe can only see the steep riser there. Say so, so the caller keeps it standing - // instead of letting gravity undo the climb. A snap onto a walkable floor ends the ride: - // the tread is under the feet and ordinary grounding takes over from here. - cone_riding: rode && ground.is_none(), + // **Supported by a certified steep contact** — the reference's `0x4000000`, whose meaning + // decision 1125 settled. Two ways to earn it and one meaning: mid-ride the body is held up + // by the edge under its skirt (climbing), and mid-step-down it is resting on the surface it + // is following off a ledge (descending). Either way the frame-start ground probe looks + // straight down and sees only a steep face, so without this the frame reads as airborne and + // gravity takes the body off the surface — the climb undone on the way up, the dive on the + // way down. A walkable floor ends it: ordinary grounding takes over from there. + steep_support: (rode && ground.is_none()) || steep_support, ground, snap, } @@ -605,6 +731,25 @@ pub(crate) fn airborne_step( .position } +/// **The overhang line is a tolerance, not zero** — and it has to be, or half the world's vertical +/// walls behave differently from the other half for no physical reason. +/// +/// Every "is this face steep-but-not-overhanging" test here is a range whose floor used to be `0.0`. +/// An axis-aligned authored quad — a doorstep, a crate, a dock plank, most of Booty Bay — computes a +/// normal whose `y` is `±0` decided by nothing but which way the cross product's rounding fell. The +/// director's capture caught both halves of that coin on the *same* step: `n.y = −4.08e-5` refused to +/// certify and refused to ride, where `+0.009` on the same geometry did both. A real overhang is a +/// ceiling, nowhere near this line, so widening the floor by a ten-thousandth costs nothing and +/// makes a vertical wall a vertical wall whichever way the rounding goes. +const OVERHANG_EPS: f32 = -1.0e-4; + +/// A face the mover meets side-on: too steep to stand on, not a ceiling. The shared gate behind the +/// step-up's certification, the foot-cone ride and the wall flatten — one line, so the three can +/// never disagree about what a vertical wall is (see [`OVERHANG_EPS`]). +fn is_steep_face(ny: f32) -> bool { + (OVERHANG_EPS..GROUND_COS).contains(&ny) +} + /// The even-speed ramp ride: a walkable slope never slows or deflects the grounded walk. The /// real client's walk step is two-dimensional — the resolver takes speed·dt as a *horizontal* /// distance and a normalized 2D direction, and Z follows purely through the snap/step machinery @@ -651,7 +796,7 @@ fn walkable_ride_velocity(n: Vec3, v: Vec3) -> Option { /// certification's call in [`grounded_step`], because a tall wall's contact point sits inside the /// cone band too and height alone cannot tell the two apart. fn foot_cone_ride(n: Vec3, v: Vec3) -> Option { - if !(0.0..GROUND_COS).contains(&n.y) { + if !is_steep_face(n.y) { return None; } // Steepness bounds the horizontal part below by sin 50°, so the normalize is safe. @@ -679,7 +824,7 @@ fn foot_cone_ride(n: Vec3, v: Vec3) -> Option { /// `floor_block_on_wall`); penetration safety is untouched — the slide's sweeps still stop at /// the real surface, the plane only shapes the deflection. fn steep_wall_plane(n: Vec3, v: Vec3) -> Option { - if !(0.0..GROUND_COS).contains(&n.y) { + if !is_steep_face(n.y) { return None; } let vn = v.dot(n); @@ -815,7 +960,7 @@ pub(crate) fn step_up( return none(StepVerdict::NoFace); }; let n = ahead.normal1; - if n.y >= GROUND_COS || n.y < 0.0 || n.dot(dir_h) >= 0.0 { + if n.y >= GROUND_COS || n.y < OVERHANG_EPS || n.dot(dir_h) >= 0.0 { return none(StepVerdict::NoFace); } let at = |verdict| StepAttempt { @@ -881,6 +1026,12 @@ mod tests { /// these casts, so a mis-wound fixture would silently be a hole to fall through. fn world_with_kerb() -> App { const PROFILE: [(f32, f32); 4] = [(-2.0, 0.0), (0.29, 0.0), (0.446, 0.28), (3.0, 0.28)]; + world_from_profile(&PROFILE) + } + + /// The fixture builder behind every profiled world here: a `(x, y)` polyline extruded across + /// `z`, wound so every face's **authored** normal points up and back at the approaching body. + fn world_from_profile(profile: &[(f32, f32)]) -> App { const W: f32 = 3.0; let mut app = App::new(); // avian's collider backend reads `Assets` and `SceneSpawner` even in a meshless @@ -894,7 +1045,7 @@ mod tests { )); app.init_asset::(); let (mut verts, mut tris) = (Vec::new(), Vec::new()); - for w in PROFILE.windows(2) { + for w in profile.windows(2) { let (&(x0, y0), &(x1, y1)) = (&w[0], &w[1]); let b = verts.len() as u32; verts.extend([ @@ -950,8 +1101,24 @@ mod tests { /// Walk the capsule into the kerb and run `frames` consecutive **whole** grounded steps from /// wherever the last one left it — the mover's own loop, so what these assert is the behaviour /// on screen and not a single probe in isolation. - fn walk_kerb(start_y: f32, frames: usize) -> Vec<(f32, Option, bool)> { - world_with_kerb() + fn walk_kerb(start_y: f32, frames: usize) -> Vec { + walk_profile(world_with_kerb(), start_y, frames) + } + + /// One frame of a walked fixture: `(feet height, climb, steep support, walkable ground)`. + type Row = (f32, Option, bool, bool); + + /// Walk the capsule into `world` and run `frames` consecutive **whole** grounded steps from + /// wherever the last one left it — the mover's own loop, so what these assert is the behaviour + /// on screen and not a single probe in isolation. Rows are `(feet height, climb, ride latch)`. + fn walk_profile(world: App, start_y: f32, frames: usize) -> Vec { + walk_from(world, Vec3::new(-1.0, start_y, 0.0), Vec3::X, frames) + } + + /// The general walker: place the **feet** at `start`, push in `dir`, and run `frames` whole + /// grounded steps from wherever the last one left the body. + fn walk_from(mut world: App, start: Vec3, dir: Vec3, frames: usize) -> Vec { + world .world_mut() .run_system_once(move |ms: MoveAndSlide| { let capsule = player_capsule(); @@ -961,23 +1128,268 @@ mod tests { &ms, &capsule, from, disp, SKIN_WIDTH, &filter, ) }; - let start = Vec3::new(-1.0, CAPSULE_HEIGHT * 0.5 + start_y, 0.0); - let run = cast(start, Vec3::X).map_or(1.0, |h| h.distance); - let mut center = start + Vec3::X * run; + let start = start + Vec3::Y * (CAPSULE_HEIGHT * 0.5); + let run = cast(start, dir).map_or(1.0, |h| h.distance); + let mut center = start + dir * run; let dt = std::time::Duration::from_secs_f32(TRAVEL_60FPS / 7.0); + // The steep-support bit is carried frame to frame exactly as the driver carries it + // (`Player::steep_support`), so the deep step-down reach is gated here the way it is + // on screen — a fixture that reset it every frame would test a mover we do not ship. + let mut support = Support::default(); (0..frames) .map(|_| { let g = - grounded_step(&ms, &capsule, &filter, center, Vec3::X * 7.0, dt, 0.0); + grounded_step(&ms, &capsule, &filter, center, dir * 7.0, dt, support); center = g.center; - // Feet height relative to the street, the climb, and the ride latch. - (center.y - CAPSULE_HEIGHT * 0.5, g.climb, g.cone_riding) + support.steep = g.steep_support; + ( + center.y - CAPSULE_HEIGHT * 0.5, + g.climb, + g.steep_support, + g.ground.is_some(), + ) }) .collect::>() }) .unwrap() } + /// **Booty Bay's step** — a 0.40 yd *vertical* riser onto a ~22° ramp, profiled from the + /// director's steered capture at WoW `(-14314.3, 466.2, 18.5)`, collider `54046v0`: the wall + /// probe reads `n=(-0.96,+0.00,-0.27)` with the blocking edge `h=+0.40` above the feet, and the + /// surface ahead runs `+0.35:+0.40/+0.93` … `+1.40:+0.82/+0.93`. + /// + /// The riser is tilted by 0.002 yd across its 0.40 of rise for one reason: a *perfectly* axis- + /// aligned quad computes a normal whose `y` is ±0 on a float coin flip, and this fixture must + /// pin the ride, not that coin flip. `an_exactly_vertical_riser_still_certifies` owns the flip. + fn world_with_vertical_step() -> App { + const PROFILE: [(f32, f32); 4] = [(-2.0, 0.0), (0.30, 0.0), (0.302, 0.40), (3.0, 1.48)]; + world_from_profile(&PROFILE) + } + + #[test] + fn a_vertical_step_is_climbed_not_stalled() { + // The 1123 regression the capture caught. A 0.40 yd rise is inside [`FOOT_CONE_HEIGHT`], so + // it rides — and against a *vertical* riser a capsule hangs clear of everything below, so + // the election snap saw the street a full ride beneath it and pulled the body back down + // every frame. Certified `COMMIT dy=+0.439` on every one, and stalled at +0.020 forever. + let frames = walk_profile(world_with_vertical_step(), 0.0, 6); + let top = frames.last().unwrap().0; + assert!( + top > 0.40, + "the body must finish on the step, not stalled below it: {frames:?}" + ); + // Monotone: a ride that is undone and re-earned reads as a judder, which is what the + // director would see even if the body eventually arrived. + for w in frames.windows(2) { + assert!( + w[1].0 >= w[0].0 - 1.0e-3, + "the climb must never go backwards: {frames:?}" + ); + } + assert!( + frames[0].0 < 0.40, + "…and it is still a ride, not a one-frame pop: {frames:?}" + ); + } + + #[test] + fn a_tall_step_behind_an_unwalkable_face_is_climbed() { + // Stormwind, WoW `(-8427.4, 607.4, 95.1)`, collider `14282v1` — the director's second + // capture. A 0.91 yd step whose face is a 66° bevel for most of a yard before the flat top + // begins, so it defeated **both** of the old budgets at once: the top is above 0209's 0.7 + // rise ceiling, and 1121's one-radius advance put the settle probe still over the bevel, + // where the walkable gate correctly refused it. The captured ladder is unanimous — + // `STEEP fwd0.31 ny+0.41` at every rung out to 1.20. + const P: [(f32, f32); 4] = [(-2.0, -0.02), (0.28, -0.02), (0.694, 0.91), (3.0, 0.91)]; + let frames = walk_profile(world_from_profile(&P), -0.02, 8); + assert!( + frames.last().unwrap().0 > 0.88, + "the body must finish on the 0.91 yd top: {frames:?}" + ); + for w in frames.windows(2) { + assert!( + w[1].0 >= w[0].0 - 1.0e-3, + "the climb must never go backwards: {frames:?}" + ); + } + // The blocking edge is low on the bevel, inside the cone, so this rides — six frames of + // smooth diagonal rather than a yard-high teleport. + assert!( + frames[0].0 < 0.3, + "a 0.91 yd gain in one frame is the teleport 1126 exists to avoid: {frames:?}" + ); + } + + #[test] + fn stepping_off_a_kerb_follows_the_surface_down() { + // Decision 1127, and the exact frame the director's capture caught: walking *off* the same + // Stormwind kerb the down-probe finds only its ~61° riser, and refusing that outright left + // the body at full height while its forward speed carried it past the edge — one frame of + // `dy=+0.000 dx=0.118` with the surface 0.162 below, then eighteen airborne frames landing + // 2.1 yd downrange. The dive. + // + // **Support** is the thing to assert, because losing it *is* the dive: the caller turns an + // unsupported frame into gravity, and everything after that is an arc no snap can undo. + let frames = walk_from(world_with_kerb(), Vec3::new(1.6, 0.28, 0.0), Vec3::NEG_X, 8); + for (i, f) in frames.iter().enumerate() { + assert!( + f.2 || f.3, + "frame {i} left the surface entirely — that is the dive: {frames:?}" + ); + } + assert!( + frames.last().unwrap().0 < 0.05, + "the walk-off must reach the street: {frames:?}" + ); + } + + #[test] + fn a_face_steeper_than_the_cone_still_descends_what_it_can() { + // The "only sometimes" case. A face steeper than the cone cannot be followed exactly, and the + // question is what the mover does with the part it *can* follow. The reference descends + // `min(clearance, cap)` unconditionally and only then decides it is falling; the first cut of + // 1127 descended the clearance **or nothing**, so a surface a hair steeper than the cone + // dived — eight takeoffs in one capture, each missing the bound by 0.001–0.019. + // + // 66° face (`ny ≈ 0.41`, the captured value), walked off at a run: the first frame past the + // lip must give up most of a cone's worth of height, not hold its own. + const P: [(f32, f32); 4] = [(-2.0, 0.0), (0.60, 0.0), (1.0, -0.91), (3.0, -0.91)]; + // (`walk_from`'s approach cast falls back to a yard when nothing blocks — there is no wall + // to stop against here — so the start is placed a yard short of the lip on purpose.) + let frames = walk_from( + world_from_profile(&P), + Vec3::new(-0.6, 0.0, 0.0), + Vec3::X, + 5, + ); + let cone = TRAVEL_60FPS * STEP_SLOPE_RATIO; + let biggest = frames + .windows(2) + .map(|w| w[0].0 - w[1].0) + .fold(frames[0].0.abs(), f32::max); + assert!( + biggest > cone * 0.5, + "some frame past the lip must follow the face down rather than hold height: \ + {frames:?} (best drop {biggest:.3}, a cone's worth is {cone:.3})" + ); + } + + #[test] + fn a_ledge_deeper_than_the_probe_is_a_fall_not_an_absorbed_step() { + // **The pin on decision 1129's reach.** A 1.2 yd sheer drop — deeper than the step-down probe + // can see, shallower than the 2.028 yd of flat extra depth the probe used to carry. Under the + // old reach the snap found the lower floor from the top of the ledge and absorbed the whole + // drop in ONE frame, still walkably grounded the entire way: a downward teleport, and the + // step-up teleport's exact mirror. + // + // What must happen instead is what the reference does: the body leaves the ground before it + // reaches the bottom, and the gap is a fall. So — **an airborne frame must come first.** + // (This test fails on the pre-1129 reach, which is the only reason it is worth having.) + const P: [(f32, f32); 4] = [(-2.0, 0.0), (0.60, 0.0), (0.62, -1.20), (3.0, -1.20)]; + let frames = walk_from( + world_from_profile(&P), + Vec3::new(-1.0, 0.0, 0.0), + Vec3::X, + 12, + ); + // The measurable form: **no single frame may give up an unbounded amount of height.** A + // frame's descent is the slide's own drop plus the step-down snap's, each bounded by what + // the foot cone could rest on over one frame's travel — so two cone-reaches is a generous + // ceiling. The corrected mover clears it easily (its worst frame here gives up 0.257); the + // old reach blows straight through, holding height to the lip and then taking the remaining + // **1.126 yd in a single frame**, walkably grounded at both ends of it. + let bound = 2.0 * (TRAVEL_60FPS * STEP_SLOPE_RATIO + STEP_SNAP_SLACK); + let worst = frames + .windows(2) + .map(|w| w[0].0 - w[1].0) + .fold(0.0_f32, f32::max); + assert!( + worst <= bound, + "no frame may drop more than {bound:.3} yd; worst was {worst:.3}\n{frames:#?}" + ); + } + + #[test] + fn an_idle_body_is_not_pulled_down_through_open_air() { + // The other half of the same reach. A motionless body a yard above flat ground: standing + // still the probe is the slack and nothing more, so it must find nothing, keep its height, + // and be handed to gravity. The old reach was `slack + 2.028` even at a dead stop — it found + // the floor a yard down and yanked the body onto it, one frame, no fall. + const P: [(f32, f32); 2] = [(-2.0, 0.0), (3.0, 0.0)]; + let (dy, ground) = world_from_profile(&P) + .world_mut() + .run_system_once(move |ms: MoveAndSlide| { + let capsule = player_capsule(); + let filter = SpatialQueryFilter::default(); + let dt = std::time::Duration::from_secs_f32(TRAVEL_60FPS / 7.0); + let center = Vec3::new(0.0, CAPSULE_HEIGHT * 0.5 + 1.0, 0.0); + let g = grounded_step( + &ms, + &capsule, + &filter, + center, + Vec3::ZERO, + dt, + Support::default(), + ); + (g.center.y - center.y, g.ground.is_some()) + }) + .unwrap(); + assert!(!ground, "a body a yard up is standing on nothing"); + assert!( + dy.abs() <= STEP_SNAP_SLACK * 2.0, + "it must keep its height and fall, not be snapped a yard down: dy={dy:+.3}" + ); + } + + #[test] + fn a_motionless_body_is_never_perched_on_a_steep_slope() { + // A 60° bank. Walk onto it, then STOP. Standing still the cone bound collapses to the slack, + // so does the steep support quietly keep a motionless body perched on it? + const P: [(f32, f32); 3] = [(-2.0, 0.0), (0.0, 0.0), (3.0, -5.196)]; + let out = world_from_profile(&P) + .world_mut() + .run_system_once(move |ms: MoveAndSlide| { + let capsule = player_capsule(); + let filter = SpatialQueryFilter::default(); + let dt = std::time::Duration::from_secs_f32(TRAVEL_60FPS / 7.0); + let mut center = Vec3::new(0.6, CAPSULE_HEIGHT * 0.5 - 1.039, 0.0); + let mut rows = Vec::new(); + let mut support = Support::default(); + for i in 0..4 { + // Two frames walking, then two standing perfectly still. + let v = if i < 2 { Vec3::X * 7.0 } else { Vec3::ZERO }; + let g = grounded_step(&ms, &capsule, &filter, center, v, dt, support); + center = g.center; + support.steep = g.steep_support; + rows.push((v.length() > 0.0, g.steep_support)); + } + rows + }) + .unwrap(); + for (moving, supported) in out { + assert!( + moving || !supported, + "a motionless body must never hold a steep face — it slides off" + ); + } + } + + #[test] + fn an_exactly_vertical_riser_still_certifies() { + // The other half of the same capture. An axis-aligned riser's normal has `y = ±0` depending + // on nothing but the cross product's rounding, and an overhang guard written as `n.y < 0.0` + // refuses the negative half — reading a plain vertical step as a ceiling. Half the world's + // risers, decided by a float sign. + const PROFILE: [(f32, f32); 4] = [(-2.0, 0.0), (0.30, 0.0), (0.30, 0.40), (3.0, 1.48)]; + let frames = walk_profile(world_from_profile(&PROFILE), 0.0, 6); + assert!( + frames.last().unwrap().0 > 0.40, + "an exactly-vertical riser must climb exactly like a tilted one: {frames:?}" + ); + } + #[test] fn the_kerb_is_ridden_up_its_skirt_never_popped() { // Decision 1123, the whole point: a 0.28 yd kerb is inside the foot cone @@ -988,7 +1400,7 @@ mod tests { let frames = walk_kerb(0.0, 4); let arrived = frames .iter() - .position(|&(y, _, _)| (y - 0.28).abs() < 0.03) + .position(|&(y, _, _, _)| (y - 0.28).abs() < 0.03) .expect("the ride should put the feet on the 0.28 yd tread"); assert!( arrived > 0, @@ -1011,7 +1423,7 @@ mod tests { // the cone band too). Read the same geometry from a full kerb below — a 2.3 yd wall — and // nothing may rise: no ride, no climb, no lift, just the slide. This is the check that // stops the ride becoming a ladder up every cliff in the world. - for (y, climb, riding) in walk_kerb(-2.0, 4) { + for (y, climb, riding, _) in walk_kerb(-2.0, 4) { assert!(!riding, "a wall must never start a cone ride"); assert!(climb.is_none(), "a wall must never register a climb"); assert!( diff --git a/crates/benilla-app/src/player/state.rs b/crates/benilla-app/src/player/state.rs index 7cfbdaae..b7f45910 100644 --- a/crates/benilla-app/src/player/state.rs +++ b/crates/benilla-app/src/player/state.rs @@ -45,7 +45,15 @@ pub(super) const STATIONARY_CHASE_RATE: f32 = 8.0; // fidelity targets. The mechanism is a thin kinematic controller over avian's `MoveAndSlide`; further // refinements (accel/decel curves, partial air control beyond the one-shot nudge) dial up from here. -/// Player capsule radius (yd) — the vanilla box's ±1/3 half-width. +/// Player capsule radius (yd) — the vanilla `CMovement` ctor's ±1/3 half-width, and a *tunable* per +/// the block header above, not a fidelity target. +/// +/// Known since to be the ctor **placeholder**, not the real mover's radius: `0x6174b0` overwrites +/// `+0xb0`/`+0xb4` from `CreatureModelData` on every model build (`0x5fb880` → `0x5fb9dd` passes +/// `force = 1`, which skips the only refusal path), so the live extents are per-model — human male +/// radius **0.30555**, human female **0.20835** (VERIFIED, wow-re `mover-collision-scalars.md`; +/// decision 1125). Adopting them is the same movement-fidelity question [`CAPSULE_HEIGHT`] describes +/// below, and this radius is its other half. pub(super) const CAPSULE_RADIUS: f32 = 1.0 / 3.0; /// Player capsule total height (yd) — the **movement** capsule avian sweeps, deliberately a /// constant. Numerically the vanilla ctor-default collision height it was derived from @@ -107,17 +115,34 @@ pub(super) const GROUND_PROBE: f32 = 0.2; /// probe reaches `d_h · ratio + slack + collision height` below the post-move position, where /// `d_h` is the frame's achieved horizontal travel. Scaling by the travel makes the absorbed /// *slope* the constant (atan 1.8494 ≈ 61.6°, comfortably above the 50° walkable limit), -/// frame-rate independent; the collision-height term (our [`CAPSULE_HEIGHT`], the election's -/// `0x4000000`-gated `+0x617430()` extension — decision 0182) is what absorbs a discrete ledge: -/// a fence-height drop is a silent straight-down step, only a deeper floor becomes a fall. +/// frame-rate independent; the collision-height term (our [`CAPSULE_HEIGHT`] — decision 0182) is +/// what absorbs a discrete ledge: a fence-height drop is a silent straight-down step, only a deeper +/// floor becomes a fall. That term is a **known deviation** on two counts — the reference's `H` is +/// 1.0, not 2.028, and its extension is `0x4000000`-gated rather than always-on (decision 1125). +/// +/// The ratio itself is confirmed to be one constant with two jobs, exactly as we use it: `0x80c740` +/// has four references image-wide, and they are the foot cone's waist ring (`0x631c0b`) and this +/// step-down term (`0x636dda`, inside `0x6367b0`) among them — the same 61.6° serving the climb and +/// the descent (wow-re `mover-collision-scalars.md`). pub(super) const STEP_SLOPE_RATIO: f32 = 1.849_399; /// The election's fixed slack (yd) added to the travel-scaled snap reach — `[0x7ff9d0]` = 1/36 yd. pub(super) const STEP_SNAP_SLACK: f32 = 0.027_777_8; -/// The step-up rise ceiling (yd): how tall an obstacle the atomic step-up can walk you onto -/// (decision 0209). A plain tunable, deliberately modest — stairs, doorsteps, low rocks — and -/// deliberately NOT the reference's ~2 yd body-height budget, so fences (collision tops -/// 1.8–2.3 yd) always slide. One number to nudge if a real spot feels too restrictive. -pub(super) const STEP_UP_HEIGHT: f32 = 0.7; +/// The step-up **rise ceiling** (yd): how tall an obstacle the atomic step-up can walk you onto — +/// the reference's own `H`, VERIFIED (decision 1126). +/// +/// 0209 set this to 0.7 as a deliberately modest tunable, on the understanding that the reference's +/// budget was a ~2 yd body height we did not want. That understanding was wrong on its central fact: +/// `0x617430` returns `CMovement+0xb8`, which is **not a height** but the dimensionless ratio +/// `max(SCALE_X / CreatureModelScale, 1)`, and a complete writer census puts it at **1.0** for a +/// clean local player (decision 1125). The reference's rise budget was never 2 — it was 1.0, and the +/// gap we were preserving was 0.7 vs 1.0, not 0.7 vs 2. +/// +/// 0209's invariant is untouched by closing it: a fence's collision top is 1.8–2.3 yd, so fences, +/// trunks and walls still slide at exactly the height they always did. What changes is the class +/// 0209's note invited us to fix — "one number to nudge if a real spot feels too restrictive" — +/// which the director's captures found twice: a 0.91 yd Stormwind step (`14282v1`, a 66° face onto a +/// flat top) and 1121's deferred 1.04 yd ledge. +pub(super) const STEP_UP_HEIGHT: f32 = 1.0; /// The step-up **certify advance** (yd): how far ahead the maneuver looks for the tread it would /// stand on. A property of the BODY, never of the frame (decision 1121). /// @@ -129,16 +154,24 @@ pub(super) const STEP_UP_HEIGHT: f32 = 0.7; /// commit onto the tread, at every one of the seven captured contacts. Sidewalks 0.28 yd tall /// refused to be stepped onto because the probe never reached them. /// -/// A body radius is the principled value: it is exactly the distance at which the capsule's -/// footprint has cleared the lip it is standing against, and it is the same number whatever the -/// frame rate or the gait. The frame's travel still wins when it is *longer* (a very low frame -/// rate must not step you less far than you asked to walk), so this is a floor, not a fixed reach. +/// A body radius was the principled *guess* — exactly the distance at which the capsule's footprint +/// has cleared the lip it is standing against, and the same number whatever the frame rate or the +/// gait. It is now superseded by the measured one (decision 1126): the reference steps +/// `max(H·tan50°, radius + 1/720)` square into the certified face, which with the verified `H` of 1.0 +/// is **1.1918 yd** — three and a half times our radius. 1121 reached for a body-scaled number in the +/// absence of this one and got the *shape* of the answer right; the magnitude was never ours to +/// invent. The frame's travel still wins when it is longer, so this remains a floor, not a fixed +/// reach. /// -/// It does not widen what may be climbed: the rise ceiling is still [`STEP_UP_HEIGHT`], the settle -/// must still find a *walkable* floor higher than the feet, and the elevated forward sweep is still -/// clipped by anything in the way — so a fence, a trunk and a two-trunk pinch all still block at -/// the same body height they did before, with a clipped advance and a net-zero settle. -pub(super) const STEP_UP_ADVANCE: f32 = CAPSULE_RADIUS; +/// It does not widen what may be climbed, and that argument is what makes the larger reach safe: the +/// rise ceiling is still [`STEP_UP_HEIGHT`], the settle must still find a *walkable* floor higher +/// than the feet, and — the load-bearing part — **the elevated forward sweep is clipped by anything +/// in the way**, so the advance only ever reaches as far as there is clear air at the raised height. +/// A fence, a trunk and a two-trunk pinch still block at the same body height, with a clipped advance +/// and a net-zero settle. What the longer reach buys is the case the director's capture found: a step +/// whose *face* is unwalkable for most of a yard before the flat top begins, where a probe that stops +/// at one radius is still over the face and reads it as a steep floor. +pub(super) const STEP_UP_ADVANCE: f32 = 1.191_753_6; /// The **foot cone's height** (yd): how far above the feet the reference's movement solid is still /// narrower than its full radius — and therefore the band within which a blocking edge is *ridden /// up* rather than stepped onto (decision 1123). @@ -155,9 +188,15 @@ pub(super) const STEP_UP_ADVANCE: f32 = CAPSULE_RADIUS; /// the vertical box square, and that is the case the instant step-up ([`STEP_UP_HEIGHT`]) exists /// for. One solid, two behaviours, selected by the edge's height above the feet. /// -/// Not a new tunable: it is our own two constants multiplied, and it reproduces the reference's -/// ≈0.62 yd exactly, because [`CAPSULE_RADIUS`] is already the reference's own mover radius of 1/3 -/// (`climb-vs-slide.md` §2 names it). +/// Not a new tunable: it is our own two constants multiplied, so it is *our* mover's cone — the band +/// is a property of the body's radius, and ours is [`CAPSULE_RADIUS`]. +/// +/// It is **not** the real client's band, and the gap is recorded rather than papered over (decision +/// 1125, correcting 1123's claim that it matched exactly). Because [`CAPSULE_RADIUS`] is the ctor +/// placeholder, ours comes out at 0.616 against a live human male's **0.565** and a human female's +/// **0.385** — ~9% and ~60% generous. It only decides ride-vs-pop for obstacles *between* the two +/// bands, so the cost is a narrow slice of over-smooth rides on tall-ish steps; closing it properly +/// means per-model mover extents, which moves far more than this constant. pub(super) const FOOT_CONE_HEIGHT: f32 = CAPSULE_RADIUS * STEP_SLOPE_RATIO; /// The landing probe (yd): while airborne, walk mode resumes only this close to the floor, so /// the arc ends where the slide actually contacts (skin scale) instead of [`GROUND_PROBE`] @@ -508,13 +547,18 @@ pub(crate) struct Player { pub(super) wedged: bool, /// Consecutive stalled airborne frames (see [`WEDGE_STALL_RATIO`]). pub(super) wedge_still: u8, - /// Part-way up a **foot-cone ride** — the body is resting on the slanted skirt of a low edge it - /// has already certified it can clear, climbing it over the frames the gait needs (decision - /// 1123). Counts as standing: the frame-start ground probe sees only the steep riser under the - /// feet, so without this the mid-ride frame reads as airborne and gravity undoes the climb — - /// the mid-face dwelling 0209 was built to make impossible. Re-earned from the certification - /// every frame (no certification, no ride), and cleared the moment a walkable floor takes over. - pub(super) cone_riding: bool, + /// **Supported by a certified steep contact, not a walkable floor** — the reference's + /// `0x4000000`, whose meaning decision 1125 settled from the bytes. Two ways to earn it, one + /// meaning: climbing the slanted skirt of a low edge already certified clearable (the foot-cone + /// ride, 1123), or resting on the surface being followed down off a ledge (1127). + /// + /// Counts as standing. The frame-start ground probe looks straight down and sees only the steep + /// face, so without this the frame reads as airborne and gravity takes the body off the surface: + /// the climb undone into the mid-face dwelling 0209 exists to prevent, and on the way down the + /// dive past the edge the director reported. Re-earned every frame from the certification going + /// up and from the cone's descent bound coming down — neither can hold a body that has not just + /// proved it is on a surface — and cleared the moment a walkable floor takes over. + pub(super) steep_support: bool, /// The take-off vertical speed (yd/s, WoW +Z up) snapshotted when the airborne phase began — the /// client's `StartFalling` argument (`+0xa0`, constant per arc) and the `zspeed` we send in the /// jump tail: `JUMP_SPEED` for a jump, **exactly 0** for a step-off (the walk election calls diff --git a/crates/benilla-app/src/ui_craft.rs b/crates/benilla-app/src/ui_craft.rs index 97d9827c..bc291e3e 100644 --- a/crates/benilla-app/src/ui_craft.rs +++ b/crates/benilla-app/src/ui_craft.rs @@ -42,13 +42,15 @@ use crate::ui_unit::UnitFeed; // leg). INTERIM admission pending a wow-re detail pass: a known spell joins the craft list when its // SLA line matches AND (`castUI != 0` OR the tradeskill bit) and it is not an opener. -/// The open Craft window: the skill line whose recipes it lists (`None` = closed). Routed here -/// by the opener's `EffectMiscValue[0] != 0` (byte-VERIFIED — wow-re `tradeskill` TU-A); a Beast -/// Training opener also routes here and simply lists nothing (its pet-ability content is its own -/// arc, 0437 out-of-scope). Client-local state, no wire. +/// The open Craft window: the skill line whose recipes it lists (`None` = closed) and the **craft +/// type** that opened it. Routed here by the opener's `EffectMiscValue[0] != 0` (byte-VERIFIED — +/// wow-re `tradeskill` TU-A); that same misc value *is* the craft type (1 Beast Training · +/// 3 Enchanting), which the client keeps at `ds:0xbdcfb8` and reads for both the window's admission +/// filter and its row comparator (decision 1124). Client-local state, no wire. #[derive(Resource, Default)] pub(crate) struct CraftOpen { pub(crate) line: Option, + pub(crate) craft_type: u32, } pub(crate) struct UiCraftPlugin; @@ -148,6 +150,7 @@ fn feed_craft( }; let fresh = (|| -> Option { let line = open.line?; + let craft_type = open.craft_type; let spells = spells.as_deref()?; let skill_lines = skill_lines.as_deref()?; let store = self_store.single().ok()?; @@ -164,14 +167,24 @@ fn feed_craft( lookup: &|id| spells.catalog.get(id), home_area: None, }; - let mut recipes: Vec = actions + // The **admission law** — `0x5e9c20`, byte-verified (decision 1124): the player knows the + // spell, it is not hidden (`Attributes & 0x20`), and its `castUI` **equals this window's + // craft type**. It is not a skill-line join: the client walks its own per-type list at + // `CGPlayer_C + 0x1cd0 + 0x10*type` and never consults `SkillLineAbility` for membership. On + // the shipped `Spell.dbc` the two agree on every spell a player can know (castUI 1 → 110 of + // the 111 line-261 rows, the exception being the opener 5149 itself, which castUI 0 excludes + // where the old `effects[0] != TRADE_SKILL` clause used to; castUI 3 → every line-333 + // recipe), so this is a law correction, not a content change. What it also does is keep the + // `spellLevel = 0` **trainer-wrapper tier** of every pet ability (24519-24522 and friends, + // castUI 1 but never known) out of the list — the tier whose presence would flatten the + // sort's rank key even with the right comparator. + let recipes: Vec = actions .spells .iter() .filter(|&&s| { spells.catalog.get(s).is_some_and(|d| { - (d.cast_ui != 0 || d.attributes & SPELL_ATTR_IS_TRADESKILL != 0) - && d.effects[0] != benilla_formats::SPELL_EFFECT_TRADE_SKILL - }) && skill_lines.catalog.spell_to_line(s) == Some(line) + d.cast_ui == craft_type && d.attributes & SPELL_ATTR_IS_TRADESKILL == 0 + }) }) .filter_map(|&s| { let d = spells.catalog.get(s)?; @@ -241,23 +254,20 @@ fn feed_craft( reagents, tools, tooltip: craft_tooltip(s, d), + spell_level: d.spell_level, }) }) .collect(); - recipes.sort_by(|a, b| { - let req = |r: &CraftRecipe| { - skill_lines - .catalog - .ability(r.spell_id) - .map(|x| x.req_skill_value) - .unwrap_or(0) - }; - req(b).cmp(&req(a)).then_with(|| a.name.cmp(&b.name)) - }); + // No app-side sort: the ROW ORDER is the engine's, transcribing this craft type's own + // comparator (`benilla_ui::script::craft::recipe_order`, decision 1124) — the same seam the + // trainer tree and the TradeSkill list already sit behind. What used to be here was a + // `req_skill_value`-descending sort keyed on a DBC column the client never reads, over rows + // whose iteration order was a `HashSet`'s. Some(CraftState { name, rank, max_rank, + craft_type, recipes, }) })(); diff --git a/crates/benilla-app/src/ui_script/trainer_tests.rs b/crates/benilla-app/src/ui_script/trainer_tests.rs index ed728b8d..c44730c5 100644 --- a/crates/benilla-app/src/ui_script/trainer_tests.rs +++ b/crates/benilla-app/src/ui_script/trainer_tests.rs @@ -118,8 +118,8 @@ fn service( skill_req, ability_reqs, is_trade_skill: false, - skill_line, - skill_line_name: line_name.into(), + group_key: skill_line, + group_name: line_name.into(), } } @@ -130,7 +130,7 @@ fn service( fn menu() -> TrainerState { TrainerState { greeting: "Well met. Let me show you the way of the warrior.".into(), - is_tradeskill: false, + trainer_type: 0, groups: Vec::new(), services: vec![ service( @@ -443,7 +443,7 @@ fn filter_rows_toggle_through_the_dropdown_kit() { fn long_menu() -> TrainerState { TrainerState { greeting: "Much to learn.".into(), - is_tradeskill: false, + trainer_type: 0, groups: Vec::new(), services: (1..=15) .map(|i| { diff --git a/crates/benilla-app/src/ui_tradeskill.rs b/crates/benilla-app/src/ui_tradeskill.rs index dc16787e..3aeabc64 100644 --- a/crates/benilla-app/src/ui_tradeskill.rs +++ b/crates/benilla-app/src/ui_tradeskill.rs @@ -142,14 +142,20 @@ fn open_trade_skill( // The routing key — byte-VERIFIED (wow-re `tradeskill` TU-A, `0x6e4bd7`): the opener's // `EffectMiscValue[0] != 0` routes to the CraftFrame (Enchanting 3, Beast Training 1); // zero routes to the TradeSkillFrame. NOT a skill-line test — 0437's line-333 INTERIM - // is corrected by this fold-back. - let to_craft = spells + // is corrected by this fold-back. The nonzero value is not merely a flag: it IS the craft + // type the client keeps at `ds:0xbdcfb8`, and the Craft window keys both its admission + // filter and its row comparator on it (decision 1124), so it rides along. + let craft_type = spells .as_deref() .and_then(|s| s.catalog.get(spell_id)) - .is_some_and(|d| d.effect_misc_value[0] != 0); - if to_craft { - debug!("ui_tradeskill: opener {spell_id} opens the CraftFrame (line {line})"); + .and_then(|d| u32::try_from(d.effect_misc_value[0]).ok()) + .unwrap_or(0); + if craft_type != 0 { + debug!( + "ui_tradeskill: opener {spell_id} opens the CraftFrame (line {line}, craft type {craft_type})" + ); craft_open.line = Some(line); + craft_open.craft_type = craft_type; } else { debug!("ui_tradeskill: opener {spell_id} opens skill line {line}"); if open.line != Some(line) { diff --git a/crates/benilla-app/src/ui_trainer/law.rs b/crates/benilla-app/src/ui_trainer/law.rs index 24c331ea..1d24301b 100644 --- a/crates/benilla-app/src/ui_trainer/law.rs +++ b/crates/benilla-app/src/ui_trainer/law.rs @@ -1,30 +1,33 @@ -//! The trainer window's two **byte-transcribed laws** — the icon and the tooltip. +//! The trainer window's **byte-transcribed laws** — the icon, the tooltip, and the group key. //! -//! They live together because they are the same *kind* of thing (a direct transcription of one Lua -//! binding each, with the binary's addresses in the doc), and apart from the feed because they are -//! the part that must never drift toward "what seems consistent". The whole point of both records -//! is that they **disagree with each other** on the same row: the icon needs a trainer-type gate the -//! tooltip does not have, and the icon pins the *wire* wrapper where the tooltip hops to the -//! *taught* spell. On ~806 of the shipped corpus's trainer services the reference client visibly -//! shows one spell's icon above another spell's tooltip. Putting them side by side makes that -//! deliberate, and makes a future "unification" obviously wrong rather than obviously tidy. +//! They live together because they are the same *kind* of thing (a direct transcription of one +//! binding or builder branch each, with the binary's addresses in the doc), and apart from the feed +//! because they are the part that must never drift toward "what seems consistent". The whole point +//! of putting them side by side is that they **disagree with each other on the same row**, on +//! purpose: the icon needs a trainer-type gate the tooltip does not have; the icon pins the *wire* +//! wrapper where the tooltip hops to the *taught* spell; the group key hops at three trainer types +//! and resolves nothing at the fourth. On ~806 of the shipped corpus's trainer services the +//! reference client visibly shows one spell's icon above another spell's tooltip. Keeping them +//! adjacent makes that deliberate, and makes a future "unification" obviously wrong rather than +//! obviously tidy — decision 1124 is the second time a plausible unification (the display name, +//! assumed to hop like the group key) turned out to be refuted at the bytes. //! -//! Sources: wow-re `system/ui/scratch/spell-icon-substitution-law.md` §1 (the icon) and -//! `system/ui/scratch/trainer-service-tooltip-law.md` (the tooltip). +//! Sources: wow-re `system/ui/scratch/spell-icon-substitution-law.md` §1 (the icon), +//! `trainer-service-tooltip-law.md` (the tooltip) and `trainer-craft-list-order.md` (the group key). use benilla_formats::{ - SpellCatalog, SPELL_ATTR_IS_TRADESKILL, SPELL_EFFECT_CREATE_ITEM, SPELL_EFFECT_LEARN_PET_SPELL, - SPELL_EFFECT_LEARN_SPELL, + SkillLineCatalog, SpellCatalog, SPELL_ATTR_IS_TRADESKILL, SPELL_EFFECT_CREATE_ITEM, + SPELL_EFFECT_LEARN_PET_SPELL, SPELL_EFFECT_LEARN_SPELL, SPELL_EFFECT_SKILL_STEP, }; use benilla_protocol::messages::trainer_spell_state; -use benilla_ui::script::{TrainerServiceCategory, TrainerTooltip}; +use benilla_ui::script::{TrainerServiceCategory, TrainerTooltip, TRAINER_GROUP_KNOWN}; use crate::entities::ItemDisplays; use crate::items::Items; use crate::net::NetCommands; use crate::ui_items::item_icon; -use super::TRAINER_TYPE_TRADESKILL; +use super::{TRAINER_TYPE_MOUNT, TRAINER_TYPE_TRADESKILL}; /// The trainer's **icon law** — `GetTrainerServiceIcon 0x4d8f50`, byte-verified whole in wow-re /// (`system/ui/scratch/spell-icon-substitution-law.md` §1; the binding is not a marshal, the entire @@ -149,6 +152,62 @@ pub(super) fn service_tooltip(wire_spell: u32, spells: &SpellCatalog) -> Trainer wire_only } +/// The **group-key law** — the builder `0x4d7560`'s per-trainer-type branch at `0x4d7786` +/// (byte-verified, decision 1124; wow-re `system/ui/scratch/trainer-craft-list-order.md`). It is the +/// third law on this page that forks on the same dword the other two do, and the one that decides +/// what the director actually sees at the top of a profession trainer's list. +/// +/// **Type 2 (tradeskill) does not resolve a skill line at all.** The key defaults to `2` and becomes +/// `1` iff the **WIRE** spell's own `Spell.dbc Effect[0..2]` contains `44 SKILL_STEP` (`0x4d77b6`) — +/// no `EffectTriggerSpell` deref, no `SkillLineAbility` lookup. Two consequences that a skill-line +/// implementation gets wrong even when its ordering is right: the partition is **total**, so no row +/// is ever dropped at a tradeskill trainer; and the header vocabulary is the client's own label table +/// (`0x807520 + key * 0x40`), whose two entries are the global strings `TRADESKILL_SERVICE_STEP` +/// ("Development Skills") and `TRADESKILL_SERVICE_LEARN` ("Recipes") — 1.12.1 enUS `GlobalStrings.lua`, +/// the same inlining [`super::train_error_text`] does for `ERR_NOT_ENOUGH_MONEY`. +/// +/// **Type 1** (mount — the client's "talent") is a *hybrid*, not a third predicate: an +/// already-known service (`state == 2`) goes to the signed **`-1`** group (`0x4d77e8`) under the +/// `KNOWN_TALENTS_HEADER` global string ("My Talents"); everything else takes the same skill-line +/// path as types 0/3. +/// +/// **Types 0/3 (and type 1's remainder)** key on the **TAUGHT** spell's `SkillLine` +/// (`0x4d7c60`→`0x60c920`, decision 0247 — the one hop in this file that survived 1124's audit); an +/// unresolved `0` drops the service (`0x4d7807`). Pet (3) is not special-cased anywhere. +pub(super) fn service_group( + wire_spell: u32, + taught: u32, + trainer_type: u32, + category: TrainerServiceCategory, + spells: &SpellCatalog, + skill_lines: Option<&SkillLineCatalog>, +) -> (u32, String) { + if trainer_type == TRAINER_TYPE_TRADESKILL { + let step = spells + .get(wire_spell) + .is_some_and(|d| d.effects.contains(&SPELL_EFFECT_SKILL_STEP)); + return if step { + (1, "Development Skills".to_string()) + } else { + (2, "Recipes".to_string()) + }; + } + if trainer_type == TRAINER_TYPE_MOUNT && category == TrainerServiceCategory::Used { + return (TRAINER_GROUP_KNOWN, "My Talents".to_string()); + } + let line = skill_lines + .and_then(|c| c.spell_to_line(taught)) + .unwrap_or(0); + if line == 0 { + return (0, String::new()); + } + let name = skill_lines + .and_then(|c| c.line(line)) + .map(|l| l.name.clone()) + .unwrap_or_else(|| format!("Skill {line}")); + (line, name) +} + /// The green/red/gray colour a wire `state` byte maps to (decision 0237): GRAY → known, GREEN → /// learnable, everything else (RED + any unexpected value) → gated. pub(super) fn category(state: u8) -> TrainerServiceCategory { diff --git a/crates/benilla-app/src/ui_trainer/mod.rs b/crates/benilla-app/src/ui_trainer/mod.rs index 8a09ba79..d8e9db86 100644 --- a/crates/benilla-app/src/ui_trainer/mod.rs +++ b/crates/benilla-app/src/ui_trainer/mod.rs @@ -43,10 +43,15 @@ use crate::ui_session::{close_npc_session_out_of_range, npc_switched, NpcSession use crate::ui_spellbook::SkillLines; /// `SMSG_TRAINER_LIST`'s `trainer_type` for a tradeskill trainer (0 class · 1 mount · 2 tradeskill · -/// 3 pet). Used only to classify a service as tradeskill-vs-learn-spell for the Era -/// `IsTrainerServiceTradeSkill` surface — a whole-trainer approximation (per-service typing from the -/// spell's effects is a later refinement). +/// 3 pet). Three separate laws fork on it here — the **icon** ([`service_icon`]), the **group key** +/// ([`service_group`]), and the Era `IsTrainerServiceTradeSkill` flag, which remains a whole-trainer +/// approximation (per-service typing from the spell's effects is a later refinement; the reference +/// window's Lua never calls it). const TRAINER_TYPE_TRADESKILL: u32 = 2; +/// `SMSG_TRAINER_LIST`'s `trainer_type` for a mount trainer — the type the client's own vocabulary +/// calls "talent" (`IsTalentTrainer 0x4d8ed0`), and the one whose grouping folds already-known +/// services into a "My Talents" bucket ([`service_group`], decision 1124). +const TRAINER_TYPE_MOUNT: u32 = 1; /// The open trainer, filled by the net bridge ([`crate::net`]) and read by [`feed_trainer`]. Holds /// the trainer guid and its services exactly as the wire delivered them (`SMSG_TRAINER_LIST`), plus @@ -150,12 +155,20 @@ fn resolve_service( items: &mut Items, commands: &NetCommands, ) -> TrainerService { - // The trainer offers a LEARN wrapper (decision 0247); the ability it teaches — with the skill - // line the tree groups by, and the real display name/icon — is the taught spell. Resolve it once - // for both. `wire.spell` (the wrapper) stays the buy id below (CMSG_TRAINER_BUY_SPELL names it). - // A spell that teaches nothing (a plain ability, or before Spell.dbc loads) resolves to itself. + // The trainer offers a LEARN wrapper (decision 0247); the ability it teaches is the taught + // spell, and the tree GROUPS by that hop (`0x4d7c60` → `[skillrec+4]`). It is the only thing + // that hops: the row's **displayed name and subtext are the WIRE spell's own** `Spell.dbc` + // columns — `GetTrainerServiceInfo` reads `row[+0]` for both returns (`0x4d8aa0` → `[+0x1e0]`, + // `0x4d8b50` → `[+0x204]`), with no `EffectTriggerSpell` deref anywhere in either body. 0247 + // recorded the display as hopping too; decision 1124 refutes that at the bytes, and it is not a + // cosmetic difference: 1607 of the 4711 shipped learn wrappers (34.1 %) disagree with what they + // teach, and that set is *every profession-learn row* — spell 2020 is "Apprentice Blacksmith" + // with no subtext where the taught 2018 is "Blacksmithing"/"Apprentice". Pet rows agree on both + // columns, which is how the wrong hop survived this long. + // `wire.spell` (the wrapper) stays the buy id below (CMSG_TRAINER_BUY_SPELL names it); a spell + // that teaches nothing (a plain ability, or before Spell.dbc loads) resolves to itself. let taught = spells.learned_spell(wire.spell).unwrap_or(wire.spell); - let display = spells.get(taught); + let display = spells.get(wire.spell); let cat = category(wire.state); // The SKILL gate has no per-gate "met" bit on the 5875 wire, so approximate it from the service's // overall category (an unavailable service has some unmet gate). The real client computes the @@ -200,36 +213,27 @@ fn resolve_service( } }) .collect(); - // The tree's grouping key (decision 0247): the TAUGHT spell's skill line (`SkillLineAbility.dbc` - // → `SkillLine.dbc` name) — the wire wrapper id itself is never in `SkillLineAbility`, so grouping - // MUST go through the hop above. `0` when unresolved — the engine drops such a service from the - // tree, matching the client. A resolved id with a missing name falls back to `Skill `. We log - // a genuine miss (catalog present but no line) so DBC gaps surface rather than silently swallowing - // a service the server offered. - let skill_line = skill_lines - .and_then(|c| c.spell_to_line(taught)) - .unwrap_or(0); - if skill_line == 0 && skill_lines.is_some() { + // The tree's grouping key — its own byte-verified law, and per trainer type ([`service_group`], + // decision 1124). Only the skill-line arm (types 0/1/3) can fail: the wire wrapper id itself is + // never in `SkillLineAbility`, so that arm MUST go through the taught-spell hop above, and an + // unresolved `0` drops the service from the tree exactly as the client's builder does. Log the + // genuine miss (catalog present but no line) so DBC gaps surface rather than silently swallowing + // a service the server offered. The tradeskill arm resolves no line at all and cannot miss. + let (group_key, group_name) = + service_group(wire.spell, taught, trainer_type, cat, spells, skill_lines); + if group_key == 0 && skill_lines.is_some() { debug!( "ui_trainer: trainer spell {} (teaches {taught}) has no skill line — dropped from the tree", wire.spell ); } - let skill_line_name = if skill_line != 0 { - skill_lines - .and_then(|c| c.line(skill_line)) - .map(|l| l.name.clone()) - .unwrap_or_else(|| format!("Skill {skill_line}")) - } else { - String::new() - }; TrainerService { spell_id: wire.spell, name: display.map(|d| d.name.clone()), subtext: display.and_then(|d| d.rank.clone()), - // The NAME/subtext hop through the taught spell stays (decisions 0247/0252 — the wrapper is - // not in SkillLineAbility, so grouping and display MUST hop). The ICON does not: it is its - // own byte-verified law over the WIRE spell ([`service_icon`]). + // The icon is its own byte-verified law over the WIRE spell ([`service_icon`]) — as, since + // 1124, are the name and subtext above. Only the GROUP key still hops to the taught spell, + // and only because the wrapper is not in `SkillLineAbility` at all. texture: service_icon(wire.spell, trainer_type, spells, icons, items, commands), // The detail pane's description body, left empty by design. The real // `GetTrainerServiceDescription` returns the spell's *tooltip* — `Spell.dbc`'s Description @@ -245,8 +249,8 @@ fn resolve_service( skill_req, ability_reqs, is_trade_skill: trainer_type == TRAINER_TYPE_TRADESKILL, - skill_line, - skill_line_name, + group_key, + group_name, tooltip: service_tooltip(wire.spell, spells), } } @@ -266,7 +270,7 @@ fn snapshot( open.trainer?; Some(TrainerState { greeting: open.greeting.clone(), - is_tradeskill: open.trainer_type == TRAINER_TYPE_TRADESKILL, + trainer_type: open.trainer_type, services: open .services .iter() @@ -283,8 +287,8 @@ fn snapshot( ) }) .collect(), - // The engine synthesizes the skill-line tree in `set_trainer` — the app pushes only the flat - // services (each carrying its resolved skill line above). + // The engine synthesizes the tree in `set_trainer` — the app pushes only the flat services + // (each carrying its resolved group key above). groups: Vec::new(), }) } @@ -422,7 +426,7 @@ fn drain_trainer( } mod law; -use law::{category, service_icon, service_tooltip}; +use law::{category, service_group, service_icon, service_tooltip}; #[cfg(test)] mod tests; diff --git a/crates/benilla-app/src/ui_trainer/tests.rs b/crates/benilla-app/src/ui_trainer/tests.rs index 191292cb..7dad4f0b 100644 --- a/crates/benilla-app/src/ui_trainer/tests.rs +++ b/crates/benilla-app/src/ui_trainer/tests.rs @@ -318,17 +318,66 @@ fn resolve_hops_the_learn_wrapper_to_the_taught_ability() { ); assert_eq!(svc.spell_id, 1605, "the buy id stays the wire wrapper"); assert_eq!( - svc.skill_line, 26, + svc.group_key, 26, "grouped under the taught ability's Arms line" ); - assert_eq!(svc.skill_line_name, "Arms"); + assert_eq!(svc.group_name, "Arms"); assert_eq!( svc.name.as_deref(), Some("Heroic Strike"), - "display resolves through the taught spell" + "the WIRE spell's own name — which here happens to match the taught one" ); } +/// The **display name is the WIRE spell's**, not the taught spell's (decision 1124, refuting 0247's +/// display half). 1605/78 above cannot tell the two laws apart — both are "Heroic Strike", as are +/// 65.9 % of the shipped learn wrappers, which is how the wrong hop survived. The profession-learn +/// wrappers are the visible 34.1 %: on real 5875 data spell **2020 is "Apprentice Blacksmith" with +/// no rank**, where the spell it teaches (2018) is "Blacksmithing"/"Apprentice" — so this is the row +/// the director watches change. Skips without client data. +#[test] +fn display_name_is_the_wire_spell_not_the_taught_one() { + let data = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../WoW/Data"); + if !data.is_dir() { + eprintln!("skipping: vanilla client not present at {}", data.display()); + return; + } + let mut chain = benilla_formats::open_chain(&data).expect("open chain"); + let spells = benilla_formats::load_spell_catalog(&mut chain).expect("load Spell"); + let skills = benilla_formats::load_skill_line_catalog(&mut chain).expect("load skill lines"); + + // Dane Lindgren's learn row, exactly as the wire carries it (reqLevel 5, no skill gate). + let svc = resolve_service( + &wire(2020, trainer_spell_state::GREEN, 9, 5, 0), + TRAINER_TYPE_TRADESKILL, + &spells, + Some(&skills), + &HashSet::new(), + ); + assert_eq!(svc.name.as_deref(), Some("Apprentice Blacksmith")); + assert_eq!(svc.subtext.as_deref(), None); + assert_ne!( + spells.get(2020).map(|d| d.name.as_str()), + spells.get(2018).map(|d| d.name.as_str()), + "the two names really do differ on the shipped data — the test is not vacuous" + ); + + // And the group law that puts it first: 2020 carries Effect 44 SKILL_STEP where a recipe + // wrapper does not. Both read off the real `Spell.dbc` rather than a fixture. + assert_eq!(svc.group_key, 1); + assert_eq!(svc.group_name, "Development Skills"); + let recipe = resolve_service( + &wire(2743, trainer_spell_state::RED, 50, 0, 164), + TRAINER_TYPE_TRADESKILL, + &spells, + Some(&skills), + &HashSet::new(), + ); + assert_eq!(recipe.group_key, 2); + assert_eq!(recipe.group_name, "Recipes"); + assert_eq!(recipe.name.as_deref(), Some("Copper Chain Pants")); +} + #[test] fn category_maps_the_wire_state_byte() { assert_eq!( @@ -381,9 +430,11 @@ fn resolve_reads_cost_state_and_gates_with_no_catalog() { ); assert!(svc.is_trade_skill, "trainer_type 2 → tradeskill"); assert!(svc.prof_first_rank == w.is_primary_prof_first_rank); - // No skill-line catalog → skill_line 0 (the engine drops it from the tree) and an empty name. - assert_eq!(svc.skill_line, 0); - assert_eq!(svc.skill_line_name, ""); + // No spell catalog at a TRADESKILL trainer → the SKILL_STEP predicate reads false and the row + // falls to the "Recipes" group. Nothing is ever dropped at type 2 (the partition is total), so + // the "unresolved → 0" arm belongs to the skill-line types, not this one. + assert_eq!(svc.group_key, 2); + assert_eq!(svc.group_name, "Recipes"); // No skill gate when req_skill is 0. let plain = resolve_service( &wire(78, trainer_spell_state::GREEN, 50, 5, 0), diff --git a/crates/benilla-formats/examples/order_probe.rs b/crates/benilla-formats/examples/order_probe.rs new file mode 100644 index 00000000..a56d0228 --- /dev/null +++ b/crates/benilla-formats/examples/order_probe.rs @@ -0,0 +1,80 @@ +//! Throwaway: the sort/admission inputs the trainer & craft windows key on. Two modes: +//! +//! * `line ...` — a line's abilities with their raw +//! `SkillLineAbility.dbc` columns (`req_skill_value` col 7, `reqtrainpoints` col 14), their +//! `Spell.dbc` name/rank, `castUI`, `attributes` and `spellLevel`. +//! * `castui ` — every spell whose `castUI == n`, with the skill line (if any) its first +//! `SkillLineAbility` row names: the client's craft-admission key vs benilla's SLA join. +fn main() -> anyhow::Result<()> { + let data = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../WoW/Data"); + let mut chain = benilla_formats::open_chain(&data)?; + let skills = benilla_formats::load_skill_line_catalog(&mut chain)?; + let spells = benilla_formats::load_spell_catalog(&mut chain)?; + + // Raw SkillLineAbility rows: (skillId, spellId, req_skill_value, reqtrainpoints). + let sla = chain.read_file("DBFilesClient\\SkillLineAbility.dbc")?; + let g = |o: usize| u32::from_le_bytes(sla[o..o + 4].try_into().unwrap()); + let (rows, _fields, rec) = (g(4) as usize, g(8) as usize, g(12) as usize); + let raw: Vec<(u32, u32, u32, u32)> = (0..rows) + .map(|r| { + let b = 20 + r * rec; + (g(b + 4), g(b + 8), g(b + 28), g(b + 56)) + }) + .collect(); + + let mut args = std::env::args().skip(1); + let mode = args.next().unwrap_or_default(); + let rest: Vec = args.map(|s| s.to_lowercase()).collect(); + + if mode == "castui" { + let want: u32 = rest[0].parse()?; + let mut hits = 0; + for spell in 1..30000u32 { + let Some(d) = spells.get(spell) else { continue }; + if d.cast_ui != want { + continue; + } + hits += 1; + let line = skills.spell_to_line(spell).unwrap_or(0); + let hidden = d.attributes & benilla_formats::SPELL_ATTR_IS_TRADESKILL != 0; + println!( + " {spell:>6} line={line:<5} attr20={} lvl={:<3} {} ({})", + u8::from(hidden), + d.spell_level, + d.name, + d.rank.as_deref().unwrap_or("") + ); + } + println!("castUI == {want}: {hits} spells"); + return Ok(()); + } + + let mut lines: Vec<(u32, String)> = Vec::new(); + for id in 1..1200u32 { + if let Some(l) = skills.line(id) { + let lower = l.name.to_lowercase(); + if rest.iter().any(|w| lower.contains(w)) { + lines.push((id, l.name.clone())); + } + } + } + for (id, name) in &lines { + let mut mine: Vec<&(u32, u32, u32, u32)> = raw.iter().filter(|r| r.0 == *id).collect(); + mine.sort_by_key(|r| r.1); + println!("== skill line {id}: {name} — {} SLA rows", mine.len()); + for &&(_, spell, req, tp) in mine.iter().take(120) { + let d = spells.get(spell); + let n = d.map(|d| d.name.as_str()).unwrap_or("?"); + let rank = d.and_then(|d| d.rank.as_deref()).unwrap_or(""); + println!( + " spell {spell:>6} req={req:<4} tp={tp:<4} castUI={:<2} attr20={} lvl={:<3} {n} ({rank})", + d.map(|d| d.cast_ui).unwrap_or(0), + u8::from(d.is_some_and(|d| d.attributes + & benilla_formats::SPELL_ATTR_IS_TRADESKILL + != 0)), + d.map(|d| d.spell_level).unwrap_or(0), + ); + } + } + Ok(()) +} diff --git a/crates/benilla-formats/src/lib.rs b/crates/benilla-formats/src/lib.rs index 28272a3b..729be4cd 100644 --- a/crates/benilla-formats/src/lib.rs +++ b/crates/benilla-formats/src/lib.rs @@ -141,7 +141,8 @@ pub use spells::{ SpellRangeCatalog, TokenContext, ATTR_CASTABLE_WHILE_DEAD, ATTR_NOT_IN_COMBAT, ATTR_ONLY_STEALTHED, SPELL_ATTR_IS_TRADESKILL, SPELL_EFFECT_CREATE_ITEM, SPELL_EFFECT_ENCHANT_ITEM, SPELL_EFFECT_ENCHANT_ITEM_TEMPORARY, SPELL_EFFECT_LEARN_PET_SPELL, - SPELL_EFFECT_LEARN_SPELL, SPELL_EFFECT_SKINNING, SPELL_EFFECT_TRADE_SKILL, + SPELL_EFFECT_LEARN_SPELL, SPELL_EFFECT_SKILL_STEP, SPELL_EFFECT_SKINNING, + SPELL_EFFECT_TRADE_SKILL, }; mod skill_lines; pub use skill_lines::{ diff --git a/crates/benilla-formats/src/spells/mod.rs b/crates/benilla-formats/src/spells/mod.rs index 3bcf4d99..0182f710 100644 --- a/crates/benilla-formats/src/spells/mod.rs +++ b/crates/benilla-formats/src/spells/mod.rs @@ -267,6 +267,12 @@ pub const SPELL_EFFECT_LEARN_SPELL: u32 = 36; /// **display** hop deliberately stays 36-only — that is decision 0247's verified grouping hop, and /// widening it is a separate question nothing has asked. pub const SPELL_EFFECT_LEARN_PET_SPELL: u32 = 57; +/// `SpellEffects` value `44` — `SPELL_EFFECT_SKILL_STEP`: the effect that raises a profession's +/// *potential* (Apprentice → Journeyman → …). It is the whole of what a **tradeskill trainer's** +/// display tree partitions on — the builder tests the WIRE spell's three effect slots for it and +/// reads nothing else (`0x4d77b6`, decision 1124; benilla-app's `ui_trainer::law::service_group` is +/// the transcription). +pub const SPELL_EFFECT_SKILL_STEP: u32 = 44; /// `SpellEffects` value `SPELL_EFFECT_OPEN_LOCK` — the lock-opening effect the GameObject /// interact-cast matches (decision 0239; RE `cursor-system.md` §8, the client's diff --git a/crates/benilla-ui/src/script/craft.rs b/crates/benilla-ui/src/script/craft.rs index 877e46ce..0067d3f2 100644 --- a/crates/benilla-ui/src/script/craft.rs +++ b/crates/benilla-ui/src/script/craft.rs @@ -46,8 +46,10 @@ //! //! ## v1 is FLAT — no header rows (the law landed for TradeSkill; Craft is a named divergence) //! -//! Exactly [`super::tradeskill`]'s own v1 law: [`CraftState::recipes`] renders in the app's pre-sorted -//! order, one row per recipe, `index` 1-based straight into that list. `GetCraftInfo` therefore never +//! Exactly [`super::tradeskill`]'s own v1 law: [`CraftState::recipes`] renders as one flat row per +//! recipe, `index` 1-based straight into the list — though the ORDER of that list is now this +//! module's, not the app's (decision 1124: [`recipe_order`], the craft type's own byte-verified +//! comparator, applied in [`UiScript::set_craft`]). `GetCraftInfo` therefore never //! returns the `"header"` `craftType` the ref Lua also checks for (ref l.38/77/199/252/292). The //! header/grouping law itself is no longer pending — decision 0446 confirmed it byte-exact (wow-re //! `tradeskill` TU-B) and TradeSkill grew the real tree engine ([`super::tradeskill`]'s `build_groups`) @@ -153,10 +155,14 @@ pub struct CraftRecipe { /// What the detail-icon hover describes ([`CraftTooltip`]) — resolved app-side, because the law /// reads `Spell.dbc` effect columns the engine cannot see. pub tooltip: CraftTooltip, + /// `Spell.dbc spellLevel` (`+0x74`) — the **rank-ordering key** of the Beast Training comparator + /// ([`recipe_order`]). Also what `GetCraftInfo`'s `requiredLevel` return is built from, though + /// that surface is still hardcoded `0` here (module doc). + pub spell_level: u32, } -/// One open craft window: the skill line's name/rank and its (app-presorted) recipe list. Pushed whole -/// by the app ([`UiScript::set_craft`]); `None` means the window is closed. +/// One open craft window: the skill line's name/rank, the craft type, and its recipe list. Pushed +/// whole by the app ([`UiScript::set_craft`]); `None` means the window is closed. #[derive(Clone, Debug, PartialEq)] pub struct CraftState { /// The window's title (`GetCraftName`) — e.g. `"Enchanting"`. Also the skill line's display name @@ -165,18 +171,73 @@ pub struct CraftState { pub name: String, pub rank: u32, pub max_rank: u32, - /// The window's recipe rows, PRE-SORTED by the app — the engine renders this order verbatim (v1 - /// ships no grouping, module doc); `index` is 1-based straight into this list. + /// The **craft type** — the opener spell's `EffectMiscValue[0]`, which the client stores at + /// `ds:0xbdcfb8` (1 Beast Training · 3 Enchanting). It is the comparator switch: `cmp + /// ds:0xbdcfb8,1` @ `0x4f6765` picks `0x4f6920` for Beast Training, `0x4f67a0` otherwise. + pub craft_type: u32, + /// The window's recipe rows in ANY order — [`UiScript::set_craft`] sorts them ([`recipe_order`]); + /// `index` is 1-based into the sorted list. pub recipes: Vec, } +/// The craft type whose comparator carries the extra `spellLevel` key ([`recipe_order`]) — the +/// opener spell 5149 "Beast Training"'s `EffectMiscValue[0]`. The only other type this window ever +/// opens with is Enchanting's **3**, which takes the shorter comparator `0x4f67a0`. +const CRAFT_TYPE_BEAST_TRAINING: u32 = 1; + +/// The Craft window's **row order** — `0x4f6920` (craft type 1, Beast Training) / `0x4f67a0` (every +/// other type, i.e. Enchanting), byte-verified in wow-re +/// (`system/ui/scratch/trainer-craft-list-order.md`, decision 1124). Both are the same cascade and +/// the Beast Training one has one extra key: +/// +/// 1. the **difficulty tier** `[+0xc]` ascending ([`super::TradeSkillDifficulty::tier`]); +/// 2. the localized **name** (collator `0x64a4c0`); +/// 3. **`Spell.dbc spellLevel`** (`+0x74`) ascending — **only** in `0x4f6920`. This is the key that +/// orders a pet ability's ranks, and the whole reason Beast Training needs its own comparator. +/// +/// What benilla had instead was `SkillLineAbility.req_skill_value`, descending — a column the client +/// never reads anywhere in its craft TU, and which is a constant `1` on every row of skill line 261 +/// *and* 333 in the shipped DBC. So the primary key was inert and the order collapsed to the name. +/// +/// The trailing `spell_id` key is **ours, not the client's**: its cascade ends at `spellLevel`, so +/// rows tying on everything above (Charge's six ranks all carry `spellLevel = 0`) fall through to its +/// qsort's residue over an array benilla doesn't reproduce. Something must break that tie, and a +/// deterministic key is the only honest choice — benilla's was a `HashSet`'s iteration order, which +/// re-shuffled the pane between runs. Ascending spell id is also ascending rank across every 1.12 +/// pet-ability family, so it agrees with the reference wherever the reference is defined. +fn recipe_order(a: &CraftRecipe, b: &CraftRecipe, craft_type: u32) -> std::cmp::Ordering { + a.difficulty + .tier() + .cmp(&b.difficulty.tier()) + .then_with(|| collate(&a.name, &b.name)) + .then_with(|| { + if craft_type == CRAFT_TYPE_BEAST_TRAINING { + a.spell_level.cmp(&b.spell_level) + } else { + std::cmp::Ordering::Equal + } + }) + .then_with(|| a.spell_id.cmp(&b.spell_id)) +} + +/// The WoW enUS collator (`0x64a4c0` — the case-**insensitive** one the craft comparators use, unlike +/// the trainer rows' `0x64a480`), approximated: case-insensitive alphabetical, with the raw bytes as +/// a stable tie-break so equal-when-folded names keep a deterministic order. +fn collate(a: &str, b: &str) -> std::cmp::Ordering { + a.to_lowercase() + .cmp(&b.to_lowercase()) + .then_with(|| a.cmp(b)) +} + impl super::UiScript { /// Push (or clear, with `None`) the open craft window's recipe snapshot. Snapshots re-push every - /// time a resolved field changes — no diffing happens here. On a push, the engine **preserves the - /// selection across the replace**: if the previously selected recipe's spell id still appears in - /// the new list, the selection follows it to its new index; otherwise it clears to `0` - /// ([`super::tradeskill::set_trade_skill`]'s own selection-survival law, verbatim). Clearing - /// (`None`) always resets the selection. + /// time a resolved field changes — no diffing happens here. On a push, the engine **sorts the + /// rows** into the craft type's own order ([`recipe_order`] — the ordering is the engine's, like + /// the trainer tree's and the tradeskill list's, so one binding's verified comparator lives in + /// exactly one place) and then **preserves the selection across the replace**: if the previously + /// selected recipe's spell id still appears in the new list, the selection follows it to its new + /// index; otherwise it clears to `0` ([`super::tradeskill::set_trade_skill`]'s own + /// selection-survival law, verbatim). Clearing (`None`) always resets the selection. pub fn set_craft(&mut self, state: Option) { let mut model = self.model_mut(); match state { @@ -184,7 +245,9 @@ impl super::UiScript { model.craft_selection = 0; model.craft = None; } - Some(s) => { + Some(mut s) => { + let craft_type = s.craft_type; + s.recipes.sort_by(|a, b| recipe_order(a, b, craft_type)); let prev_spell_id = model .craft_selection .checked_sub(1) @@ -458,6 +521,9 @@ mod tests { use super::*; use crate::script::UiScript; + /// Enchanting's `EffectMiscValue[0]` — the craft type whose comparator has no `spellLevel` key. + const CRAFT_TYPE_ENCHANTING: u32 = 3; + /// One recipe fixture — a single-reagent, single-tool row, distinct spell ids per name. fn recipe( spell_id: u32, @@ -485,16 +551,20 @@ mod tests { have: 3, }], tools: vec![("Runed Copper Rod".into(), true)], + spell_level: 0, } } - /// A two-recipe Enchanting window: Minor Beastslaying (trivial, craftable) then Minor Health - /// (optimal, out of reagents). + /// A two-recipe Enchanting window: Minor Beastslaying (trivial, craftable) and Minor Health + /// (optimal, out of reagents). Declared trivial-first on purpose — the engine sorts by + /// difficulty tier ascending ([`recipe_order`]), so the window renders **Minor Health at index 1** + /// and Beastslaying at 2, whatever order the app pushed. fn state() -> CraftState { CraftState { name: "Enchanting".into(), rank: 57, max_rank: 75, + craft_type: CRAFT_TYPE_ENCHANTING, recipes: vec![ recipe( 7418, @@ -543,10 +613,10 @@ mod tests { lvl ), ( - "Enchant Weapon - Minor Beastslaying", + "Enchant Weapon - Minor Health", "", - "trivial", - 5, + "optimal", + 0, None, 0, 0 @@ -557,15 +627,15 @@ mod tests { "local n,s,t,a,e,tp,l = GetCraftInfo(2) return n,s,t,a,e,tp,l", ) .unwrap(); - assert_eq!((kind2.as_str(), avail2), ("optimal", 0)); + assert_eq!((kind2.as_str(), avail2), ("trivial", 5)); assert_eq!( s.eval::("return GetCraftIcon(1)").unwrap(), - "Interface\\Icons\\Spell_7418" + "Interface\\Icons\\Spell_683" ); assert_eq!( s.eval::("return GetCraftDescription(1)").unwrap(), - "Enchant Weapon - Minor Beastslaying description." + "Enchant Weapon - Minor Health description." ); assert_eq!(s.eval::("return GetCraftNumReagents(1)").unwrap(), 1); let (rname, ricon, need, have) = s @@ -598,16 +668,17 @@ mod tests { #[test] fn selection_persists_across_a_repush_by_spell_id() { let mut s = UiScript::new().unwrap(); - // Minor Beastslaying (spell 7418) starts at index 2. - let mut first = state(); - first.recipes.swap(0, 1); - s.set_craft(Some(first)); - + // Minor Beastslaying (spell 7418) sorts to index 2 (trivial, behind the optimal row). + s.set_craft(Some(state())); s.run("SelectCraft(2)").unwrap(); assert_eq!(s.eval::("return GetCraftSelectionIndex()").unwrap(), 2); - // Re-push with 7418 back at index 1 (a reagent-count re-list) — the selection follows it. - s.set_craft(Some(state())); + // A re-list that turns 7418 optimal moves it to index 1 — the selection FOLLOWS the spell + // id, not the index (and this is the case the engine-side sort makes reachable). + let mut promoted = state(); + promoted.recipes[0].difficulty = super::super::TradeSkillDifficulty::Optimal; + promoted.recipes[0].name = "Aaa Beastslaying".into(); + s.set_craft(Some(promoted)); assert_eq!(s.eval::("return GetCraftSelectionIndex()").unwrap(), 1); // A re-push that drops the selected recipe entirely clears the selection. @@ -623,7 +694,11 @@ mod tests { s.set_craft(Some(state())); s.run("DoCraft(1) DoCraft(2)").unwrap(); - assert_eq!(s.take_craft_dos(), vec![7418, 683]); + assert_eq!( + s.take_craft_dos(), + vec![683, 7418], + "display order, not push order" + ); assert!(s.take_craft_dos().is_empty(), "drained"); // An out-of-range index is ignored. @@ -661,7 +736,8 @@ mod tests { fn get_craft_spell_focus_multivalue_shape() { let mut s = UiScript::new().unwrap(); let mut c = state(); - c.recipes[0].tools = vec![ + // recipes[1] (Minor Health, optimal) is the row the sort puts at index 1. + c.recipes[1].tools = vec![ ("Runed Copper Rod".into(), true), ("Arcanite Rod".into(), false), ]; @@ -679,7 +755,7 @@ mod tests { // A recipe with no tools returns an empty multivalue (select('#', ...) == 0). let mut c2 = state(); - c2.recipes[0].tools.clear(); + c2.recipes[1].tools.clear(); s.set_craft(Some(c2)); assert_eq!( s.eval::("return select('#', GetCraftSpellFocus(1))") @@ -703,4 +779,94 @@ mod tests { assert_eq!(s.eval::("return GetNumCrafts()").unwrap(), 0); assert_eq!(s.eval::("return GetCraftSelectionIndex()").unwrap(), 0); } + + /// **Beast Training's rank order** (decision 1124), pinned against wow-re's emulated run of the + /// real `0x4f6920` over real `Spell.dbc` values — and the regression for the director's report + /// that "Beast Training lists a skill's ranks out of ascending order" (ledger B229). + /// + /// The fixture is the reported pane: four Arcane Resistance ranks and four Fire Resistance + /// ranks, pushed **shuffled** (as a `HashSet`'s iteration order used to deliver them), all on + /// one difficulty tier because pet abilities carry no trivial ranks. Name groups them; the + /// `spellLevel` key — 20/30/40/50 per rank — is what orders the ranks inside a group, and it is + /// the key the old `req_skill_value`-descending sort did not have. + #[test] + fn beast_training_orders_ranks_by_spell_level() { + let rank = |spell_id: u32, name: &str, level: u32| CraftRecipe { + spell_id, + tooltip: CraftTooltip::Spell(spell_id), + name: name.into(), + sub_name: format!("Rank {}", level / 10 - 1), + difficulty: super::super::TradeSkillDifficulty::Trivial, + num_available: 0, + icon: None, + description: None, + needs_item_target: false, + reagents: vec![], + tools: vec![], + spell_level: level, + }; + // Shuffled on purpose — the exact shape of the report (Arcane read 1, 4, 3, 5). + let recipes = vec![ + rank(24508, "Arcane Resistance", 30), + rank(24441, "Fire Resistance", 30), + rank(24495, "Arcane Resistance", 20), + rank(24464, "Fire Resistance", 50), + rank(24510, "Arcane Resistance", 50), + rank(24440, "Fire Resistance", 20), + rank(24509, "Arcane Resistance", 40), + rank(24463, "Fire Resistance", 40), + ]; + let mut s = UiScript::new().unwrap(); + s.set_craft(Some(CraftState { + name: "Beast Training".into(), + rank: 0, + max_rank: 0, + craft_type: CRAFT_TYPE_BEAST_TRAINING, + recipes: recipes.clone(), + })); + let seen: Vec<(String, String)> = (1..=8) + .map(|i| { + s.eval::<(String, String)>(&format!("local n,sub = GetCraftInfo({i}) return n,sub")) + .unwrap() + }) + .collect(); + let got: Vec = seen.iter().map(|(n, sub)| format!("{n} ({sub})")).collect(); + assert_eq!( + got, + [ + "Arcane Resistance (Rank 1)", + "Arcane Resistance (Rank 2)", + "Arcane Resistance (Rank 3)", + "Arcane Resistance (Rank 4)", + "Fire Resistance (Rank 1)", + "Fire Resistance (Rank 2)", + "Fire Resistance (Rank 3)", + "Fire Resistance (Rank 4)", + ] + ); + + // The falsifiable control wow-re ran: the SAME rows at the Enchanting craft type select + // `0x4f67a0`, which has no `spellLevel` key — so the ranks fall to the trailing spell-id + // tie-break instead. That they still come out ascending here is benilla's determinism, not + // the reference's order; what matters is that the two types differ at all. + s.set_craft(Some(CraftState { + name: "Beast Training".into(), + rank: 0, + max_rank: 0, + craft_type: CRAFT_TYPE_ENCHANTING, + recipes, + })); + let ids: Vec = (1..=8) + .map(|i| { + s.eval::(&format!("DoCraft({i}) return 0")).unwrap(); + 0 + }) + .collect(); + assert_eq!(ids.len(), 8); + assert_eq!( + s.take_craft_dos(), + vec![24495, 24508, 24509, 24510, 24440, 24441, 24463, 24464], + "no spellLevel key at type 3 — the order is name then the deterministic id tie-break" + ); + } } diff --git a/crates/benilla-ui/src/script/mod.rs b/crates/benilla-ui/src/script/mod.rs index 003ebfa6..8457d66f 100644 --- a/crates/benilla-ui/src/script/mod.rs +++ b/crates/benilla-ui/src/script/mod.rs @@ -151,7 +151,7 @@ pub use trade::{TradeSideState, TradeSlotItem, TradeState, TRADE_SLOTS}; pub use tradeskill::{TradeSkillDifficulty, TradeSkillReagent, TradeSkillRecipe, TradeSkillState}; pub use trainer::{ TrainerAbilityReq, TrainerGroup, TrainerService, TrainerServiceCategory, TrainerSkillReq, - TrainerState, TrainerTooltip, + TrainerState, TrainerTooltip, TRAINER_GROUP_KNOWN, }; pub use types::{ EditAction, EditBoxTextUi, EditOutcome, EditUnit, ExtractedQuad, FontObject, FontShadow, diff --git a/crates/benilla-ui/src/script/tests/tooltip_spell.rs b/crates/benilla-ui/src/script/tests/tooltip_spell.rs index a6a42c98..1bfcddd9 100644 --- a/crates/benilla-ui/src/script/tests/tooltip_spell.rs +++ b/crates/benilla-ui/src/script/tests/tooltip_spell.rs @@ -520,8 +520,8 @@ fn trainer_service(spell_id: u32, name: &str, tooltip: TrainerTooltip) -> Traine name: Some(name.into()), texture: Some("Interface\\Icons\\Temp".into()), category: TrainerServiceCategory::Available, - skill_line: 26, - skill_line_name: "Arms".into(), + group_key: 26, + group_name: "Arms".into(), ..Default::default() } } @@ -678,11 +678,13 @@ fn set_craft_spell_selects_the_builder_like_the_trainer_hover_does() { needs_item_target: false, reagents: vec![], tools: vec![], + spell_level: 0, }; s.set_craft(Some(CraftState { name: "Enchanting".into(), rank: 100, max_rank: 150, + craft_type: 3, recipes: vec![ recipe( 7420, diff --git a/crates/benilla-ui/src/script/tradeskill/mod.rs b/crates/benilla-ui/src/script/tradeskill/mod.rs index bc4e1cf5..9b7aa68e 100644 --- a/crates/benilla-ui/src/script/tradeskill/mod.rs +++ b/crates/benilla-ui/src/script/tradeskill/mod.rs @@ -129,8 +129,12 @@ impl TradeSkillDifficulty { } /// The byte tier a group sorts recipes by, ascending (wow-re `tradeskill` TU-B, VERIFIED): - /// Optimal < Medium < Easy < Trivial, 0..3. - fn tier(self) -> u8 { + /// Optimal < Medium < Easy < Trivial, 0..3. The **Craft** window's row comparators key on the + /// same tier byte first (`row[+0xc]`, decision 1124), which is why this is `pub(crate)` and not + /// private to this module: the client's two code tables are one vocabulary shifted by one + /// (Craft's `0` is a `"none"` tier benilla cannot produce), so the *relative* order is identical + /// and the shift is invisible to a comparator. + pub(crate) fn tier(self) -> u8 { match self { TradeSkillDifficulty::Optimal => 0, TradeSkillDifficulty::Medium => 1, diff --git a/crates/benilla-ui/src/script/trainer.rs b/crates/benilla-ui/src/script/trainer/mod.rs similarity index 60% rename from crates/benilla-ui/src/script/trainer.rs rename to crates/benilla-ui/src/script/trainer/mod.rs index 4549f426..deb59e9a 100644 --- a/crates/benilla-ui/src/script/trainer.rs +++ b/crates/benilla-ui/src/script/trainer/mod.rs @@ -17,16 +17,35 @@ //! `"header"` (a skill-line group header, with `isExpanded` set) or the service's colour state //! `"available"`/`"unavailable"`/`"used"` (green/red/gray, `isExpanded` nil). //! -//! ## The tree (decision 0247 — the byte-verified grouping/sort model) +//! ## The tree (decisions 0247 + 1124 — the byte-verified grouping/sort model, **per trainer type**) //! //! The 1.12 wire (`SMSG_TRAINER_LIST`) is a **flat** service list; the client builds a **collapsible -//! skill-line tree** on top of it, and `index` is **1-based into that visible tree**, not the wire -//! order. [`UiScript::set_trainer`] takes the flat services (each already carrying its resolved -//! [`TrainerService::skill_line`]/name, resolved app-side from `SkillLineAbility.dbc`) and synthesizes -//! the tree: one **header row per distinct skill line** (headers sorted by name), then that line's -//! services sorted by the within-group keys (required level → required-skill value → localized name → -//! localized rank — keys 4–7 of the verified comparator `0x4d85c0`). State is a per-row **colour**, not -//! the grouping key. A service whose skill line is `0` (unresolved) is dropped, matching the client. +//! tree** on top of it, and `index` is **1-based into that visible tree**, not the wire order. +//! [`UiScript::set_trainer`] takes the flat services (each already carrying its app-resolved +//! [`TrainerService::group_key`]/[`TrainerService::group_name`]) and synthesizes the tree: one +//! **header row per distinct group key**, then that group's services in the group's own order. State +//! is a per-row **colour**, never the grouping key. +//! +//! **The `trainerType` selects everything** — the list finalizer `0x4d8410` picks the row comparator +//! by `ds:0xb73a08` (`dec eax; je` chain @ `0x4d8561`) and the builder `0x4d7560` picks both the +//! group key and the header comparator by the same dword (`0x4d7786`, `0x4d79eb`). Neither the group +//! key nor the sort is one law with a "later refinement" — they are four laws, and 1124 is where +//! benilla stopped applying the class one to all of them: +//! +//! | type | rows | headers | group key | +//! |---|---|---|---| +//! | 0 class, 3 pet | `0x4d85c0` — level → skill value → name → rank | `0x4d7b90`, by name | taught spell's `SkillLine` | +//! | 1 mount ("talent") | `0x4d8850` — **state byte → name** | `0x4d7b90`, `-1` first then by name | `-1` when the service is `used`, else the `SkillLine` | +//! | 2 tradeskill | `0x4d8760` — **skill value → name**, no level key, no rank key | `0x4d7c30`, by **raw key asc** | **1 or 2**, see below | +//! +//! At type 2 the group key is **not a skill line and never resolves one**: the builder defaults it +//! to 2 and sets it to 1 iff the **wire** spell's own `Spell.dbc Effect[0..2]` contains `44` +//! `SKILL_STEP` (`0x4d77b6`), which makes exactly two groups — `TRADESKILL_SERVICE_STEP` ("Development +//! Skills", the profession-learn services) and `TRADESKILL_SERVICE_LEARN` ("Recipes") — and means +//! **no row is ever dropped at a tradeskill trainer**. That partition, not a level key, is what puts +//! "Apprentice Blacksmith" at the top of a blacksmithing trainer: `reqLevel` is inert at type 2. +//! At the other types a service whose group key is `0` (unresolved skill line) is still dropped, +//! matching the client. //! //! `GetNumTrainerServices`/every getter index the **visible** rows: headers always show, but a //! service hides when its state fails the dropdown filter ([`Model::trainer_filter`]) or its group is @@ -36,12 +55,17 @@ //! //! ## Faithful stubs (no wire data yet — kept so the ported XML runs) //! -//! `GetTrainerServiceStepReq` returns nil (no tradeskill-step data on the wire), `IsTalentTrainer` is -//! always false (benilla drives no talent trainer), and per-requirement `hasReq`/`met` flags are -//! derived from the service's overall `category` (an unavailable service has an unmet gate) rather -//! than a per-gate wire bit, which 5875 does not send. The within-group sort uses the **class** -//! comparator (`0x4d85c0`) for every trainer; the talent/tradeskill comparator variants (`0x4d8850`/ -//! `0x4d8760`, documented in 0247) are a later refinement — the director's window is the class trainer. +//! `GetTrainerServiceStepReq` returns nil (no tradeskill-step data on the wire), and per-requirement +//! `hasReq`/`met` flags are derived from the service's overall `category` (an unavailable service has +//! an unmet gate) rather than a per-gate wire bit, which 5875 does not send. +//! +//! One thing the client does that this engine does not, named rather than left to be discovered: a +//! group whose row counters go to zero **vanishes entirely, header included** (`0x4d8460`, +//! `0x4d8528`–`0x4d8549`) — which is how a type-1 trainer whose every service is `used` shows one +//! "My Talents" header and no skill-line headers at all. benilla always keeps a header. The case +//! that is *reachable* here (does the state-filter dropdown emptying a group also take its header?) +//! is an open question with wow-re; the build-time case above cannot arise, because benilla builds +//! a group only from services that exist. use mlua::{Lua, MultiValue, Value}; @@ -79,6 +103,18 @@ impl TrainerServiceCategory { } } + /// The row's **state byte** `[+0x30]` — the value `0x4d8ba0` maps to the colour string (`0` → + /// available, `2` → used, anything else → unavailable). It is a colour on every trainer type but + /// one: at type 1 it is also the third sort key ([`talent_order`]), which is why this exists + /// separately from the filter slot it numerically coincides with. + fn state_key(self) -> u8 { + match self { + TrainerServiceCategory::Available => 0, + TrainerServiceCategory::Unavailable => 1, + TrainerServiceCategory::Used => 2, + } + } + /// Parse the Era filter-type string the sort checkboxes pass. fn from_filter_str(s: &str) -> Option { match s { @@ -176,41 +212,45 @@ pub struct TrainerService { pub ability_reqs: Vec, /// A tradeskill step (`IsTrainerServiceTradeSkill`) vs. a plain learn-spell. pub is_trade_skill: bool, - /// The taught spell's skill line (`SkillLineAbility.dbc` → `SkillLine.dbc`), resolved app-side — - /// the tree's grouping key (decision 0247). `0` = unresolved: the service is **dropped** from the - /// tree (the client's `skillLine == 0 → drop`). - pub skill_line: u32, - /// The skill line's localized display name (`SkillLine.dbc`) — the header row's text. - pub skill_line_name: String, + /// The tree's grouping key, resolved app-side by the **trainer type's own** builder law + /// (module doc, decision 1124): the taught spell's `SkillLine` at types 0/1/3, or the + /// `SKILL_STEP` partition's `1`/`2` at type 2. `0` = unresolved: the service is **dropped** from + /// the tree (the client's `skillLine == 0 → drop`) — reachable only at types 0/1/3. + pub group_key: u32, + /// The group's localized display name — the header row's text. `SkillLine.dbc`'s name at types + /// 0/1/3; the `TRADESKILL_SERVICE_STEP`/`_LEARN` global strings at type 2. + pub group_name: String, /// What the detail-icon hover describes ([`TrainerTooltip`]) — resolved app-side, because the /// law reads `Spell.dbc` fields the engine cannot see. Independent of [`Self::texture`]. pub tooltip: TrainerTooltip, } -/// One skill-line group in the display tree (decision 0247): the header's skill line + name and the -/// positions (into [`TrainerState::services`]) of the line's services, pre-sorted by the within-group -/// keys. Synthesized by [`UiScript::set_trainer`] — the app pushes only the flat services. +/// One group in the display tree (decisions 0247/1124): the header's key + name and the positions +/// (into [`TrainerState::services`]) of the group's services, pre-sorted by the trainer type's +/// within-group comparator. Synthesized by [`UiScript::set_trainer`] — the app pushes only the flat +/// services. #[derive(Clone, Debug, Default, PartialEq)] pub struct TrainerGroup { - /// The skill-line id (the collapse key). - pub skill_line: u32, + /// The group key (also the collapse key). + pub key: u32, /// The header's display name. pub name: String, - /// The line's services, as positions into [`TrainerState::services`], sorted (level → skill-value - /// → name → rank). + /// The group's services, as positions into [`TrainerState::services`], in display order. pub services: Vec, } -/// One open trainer window: the services, the greeting line, and whether the trainer is a tradeskill -/// trainer (`IsTradeskillTrainer` — drives the real window's tradeskill-vs-class layout switch). -/// Pushed whole by the app; `None` means no trainer is open (the window is closed). +/// One open trainer window: the services, the greeting line, and the wire trainer type. Pushed whole +/// by the app; `None` means no trainer is open (the window is closed). #[derive(Clone, Debug, Default, PartialEq)] pub struct TrainerState { pub services: Vec, /// The trainer's greeting/title line (`SMSG_TRAINER_LIST`'s trailing string). pub greeting: String, - /// `IsTradeskillTrainer()` — the whole-trainer kind (`trainer_type == 2`). - pub is_tradeskill: bool, + /// `SMSG_TRAINER_LIST`'s `trainerType` (`ds:0xb73a08`), verbatim — **the** switch: it selects the + /// row comparator, the header comparator and the group-key law (module doc), and it is what the + /// two whole-trainer predicates test (`IsTradeskillTrainer 0x4d8ea0`: `== 2`; `IsTalentTrainer + /// 0x4d8ed0`: `== 1`). 0 class · 1 mount · 2 tradeskill · 3 pet. + pub trainer_type: u32, /// The sorted skill-line groups over `services` — **built by [`UiScript::set_trainer`]**, not /// pushed by the app (default-empty on a bare `TrainerState`). The display tree walks these. pub groups: Vec, @@ -231,9 +271,8 @@ impl super::UiScript { model.trainer = None; } Some(mut s) => { - s.groups = build_groups(&s.services); - let live: std::collections::HashSet = - s.groups.iter().map(|g| g.skill_line).collect(); + s.groups = build_groups(&s.services, s.trainer_type); + let live: std::collections::HashSet = s.groups.iter().map(|g| g.key).collect(); model.trainer_collapsed.retain(|sl| live.contains(sl)); model.trainer = Some(s); } @@ -270,64 +309,152 @@ fn collate(a: &str, b: &str) -> std::cmp::Ordering { .then_with(|| a.cmp(b)) } -/// The within-group service comparator — keys 4–7 of the verified class comparator (`0x4d85c0`, -/// decision 0247): required level → required-skill value → localized name → localized rank/subtext. -fn service_order(a: &TrainerService, b: &TrainerService) -> std::cmp::Ordering { - let skillval = |s: &TrainerService| s.skill_req.as_ref().map_or(0, |r| r.rank); +/// `SMSG_TRAINER_LIST`'s tradeskill `trainerType` (module doc's table). +const TRAINER_TYPE_TRADESKILL: u32 = 2; +/// `SMSG_TRAINER_LIST`'s mount `trainerType` — the one the client's own vocabulary calls "talent" +/// (`IsTalentTrainer 0x4d8ed0` tests it). +const TRAINER_TYPE_MOUNT: u32 = 1; + +/// The already-known group a **type 1** trainer buckets its `used` services into — the client's +/// signed `-1` group key (`0x4d77e8`), whose header takes the `KNOWN_TALENTS_HEADER` global string +/// ("My Talents", 1.12.1 enUS). Modelled as `u32::MAX` because the key is otherwise a skill-line id; +/// the header comparator `0x4d7b90` tests for it explicitly and sorts it **first**, ahead of the +/// name ordering that ranks every other header. +pub const TRAINER_GROUP_KNOWN: u32 = u32::MAX; + +/// A service's required-skill value (`[+0x1c]`, the wire's `reqSkillValue`) — `0` when the service +/// carries no skill gate. +fn skill_value(s: &TrainerService) -> u32 { + s.skill_req.as_ref().map_or(0, |r| r.rank) +} + +/// The within-group comparator for a **class** (type 0) or **pet** (type 3) trainer — keys ④–⑦ of +/// the verified `0x4d85c0`: required level (`[+0x14]` byte) → required-skill value (`[+0x1c]` u32) → +/// localized name → localized rank/subtext, all ascending. Pet is not special-cased anywhere in the +/// selection chain at `0x4d8561`; it lands here with class. +fn class_order(a: &TrainerService, b: &TrainerService) -> std::cmp::Ordering { let name = |s: &TrainerService| s.name.clone().unwrap_or_default(); let rank = |s: &TrainerService| s.subtext.clone().unwrap_or_default(); a.level_req .cmp(&b.level_req) - .then_with(|| skillval(a).cmp(&skillval(b))) + .then_with(|| skill_value(a).cmp(&skill_value(b))) .then_with(|| collate(&name(a), &name(b))) .then_with(|| collate(&rank(a), &rank(b))) } -/// Build the display tree from the flat services (decision 0247): group by skill line (dropping the -/// unresolved `skill_line == 0`), sort each group's services by [`service_order`], and sort the groups -/// by localized name (skill-line id breaks a name tie). The service positions index back into the -/// unchanged `services` slice, so every getter still resolves a row to its full service data. -fn build_groups(services: &[TrainerService]) -> Vec { +/// The within-group comparator for a **tradeskill** trainer (type 2) — the whole of `0x4d8760`'s +/// row half: required-skill value **ascending** (`0x4d87d9`/`0x4d8801`), then the localized name. +/// +/// The two keys it pointedly does *not* have are the interesting ones, and both were in benilla's +/// output until 1124: there is **no `[+0x14]` required-level key** (which is what used to sink the +/// profession-learn row — the only row with a level — to the bottom of every profession trainer), +/// and **no `[+0x204]` rank tie-break**. +fn tradeskill_order(a: &TrainerService, b: &TrainerService) -> std::cmp::Ordering { + let name = |s: &TrainerService| s.name.clone().unwrap_or_default(); + skill_value(a) + .cmp(&skill_value(b)) + .then_with(|| collate(&name(a), &name(b))) +} + +/// The within-group comparator for a **mount** trainer (type 1, the client's "talent") — the whole +/// of `0x4d8850`'s row half: the **state byte** `[+0x30]` ascending (`0x4d88f1`, so available → +/// unavailable → used), then the localized name. The one comparator on which the green/red/gray is a +/// sort key rather than only a colour; like the tradeskill one it has no level, skill-value or rank +/// key. +fn talent_order(a: &TrainerService, b: &TrainerService) -> std::cmp::Ordering { + let name = |s: &TrainerService| s.name.clone().unwrap_or_default(); + a.category + .state_key() + .cmp(&b.category.state_key()) + .then_with(|| collate(&name(a), &name(b))) +} + +/// Build the display tree from the flat services (decisions 0247/1124), by the trainer type's own +/// three laws: +/// +/// * **group** on the app-resolved [`TrainerService::group_key`], dropping the unresolved `0` (which +/// a type-2 list never produces — its partition is total); +/// * **order each group's services** with that type's row comparator; +/// * **order the headers** by raw key ascending at type 2 (`0x4d7c30`: one key, no name, no +/// tie-break), else by localized name with the key breaking a name tie (`0x4d7b90`). +/// +/// The service positions index back into the unchanged `services` slice, so every getter still +/// resolves a row to its full service data. +fn build_groups(services: &[TrainerService], trainer_type: u32) -> Vec { let mut map: std::collections::HashMap = std::collections::HashMap::new(); for (i, s) in services.iter().enumerate() { - if s.skill_line == 0 { + if s.group_key == 0 { continue; } - map.entry(s.skill_line) + map.entry(s.group_key) .or_insert_with(|| TrainerGroup { - skill_line: s.skill_line, - name: s.skill_line_name.clone(), + key: s.group_key, + name: s.group_name.clone(), services: Vec::new(), }) .services .push(i); } let mut groups: Vec = map.into_values().collect(); + let order = match trainer_type { + TRAINER_TYPE_TRADESKILL => tradeskill_order, + TRAINER_TYPE_MOUNT => talent_order, + _ => class_order, + }; for g in &mut groups { g.services - .sort_by(|&a, &b| service_order(&services[a], &services[b])); + .sort_by(|&a, &b| order(&services[a], &services[b])); + } + if trainer_type == TRAINER_TYPE_TRADESKILL { + groups.sort_by_key(|g| g.key); + } else { + // `0x4d7b90`: the `-1` (already-known) group first, then localized name. Its second key — + // `hdr[+0x24] == 0` first, between those two — is uncarved and unreachable here: benilla's + // headers are one per distinct skill line, so nothing ties on the name that it would break. + groups.sort_by(|a, b| { + (a.key != TRAINER_GROUP_KNOWN) + .cmp(&(b.key != TRAINER_GROUP_KNOWN)) + .then_with(|| collate(&a.name, &b.name)) + .then_with(|| a.key.cmp(&b.key)) + }); } - groups.sort_by(|a, b| collate(&a.name, &b.name).then(a.skill_line.cmp(&b.skill_line))); groups } -/// The visible rows in display order (decision 0247): each group's header (always shown), then — when -/// the group is not collapsed — its services that pass the state filter. The Lua's 1-based `index` is -/// a position in *this* list. +/// The visible rows in display order (decisions 0247/1124). Per group, in order: the header — **only +/// if at least one of the group's services passes the state filter** — then, when the group is not +/// collapsed, those services. The Lua's 1-based `index` is a position in *this* list. +/// +/// **The filter and the collapse are deliberately asymmetric**, and that asymmetry is structural in +/// the client rather than incidental. The finalizer builds a per-group flag `hdr[+0x1c]` by walking +/// the group's per-state member counts through the **live** state mask `ds:0xb73a1c` (`0x4d8431`, +/// `0x4d8447`), and the hide it drives at `0x4d8535` has **no header-row exemption** — so a group +/// every one of whose rows the dropdown hides disappears entirely, header included, and unchecking +/// every box empties the window (`GetNumTrainerServices() == 0`). The collapse test three +/// instructions later (`0x4d8541`) *does* exempt headers (`0x4d853d`) and reads a different field +/// (`hdr[+0x20]`), so a collapsed group keeps its header — which is what makes it re-expandable. +/// benilla had both cases keeping the header, with a test asserting it; 1124 inverted the filter +/// half. fn rows(model: &Model) -> Vec { let Some(t) = model.trainer.as_ref() else { return Vec::new(); }; let mut out = Vec::new(); for (gi, g) in t.groups.iter().enumerate() { - out.push(Row::Header(gi)); - if !model.trainer_collapsed.contains(&g.skill_line) { - for &si in &g.services { - if model.trainer_filter[t.services[si].category.filter_slot()] { - out.push(Row::Service(si)); - } - } + let mut shown = g + .services + .iter() + .copied() + .filter(|&si| model.trainer_filter[t.services[si].category.filter_slot()]) + .peekable(); + if shown.peek().is_none() { + continue; } + out.push(Row::Header(gi)); + if model.trainer_collapsed.contains(&g.key) { + continue; + } + out.extend(shown.map(Row::Service)); } out } @@ -362,7 +489,7 @@ fn set_collapsed(model: &mut Model, id: usize, collapse: bool) { model .trainer .as_ref() - .map(|t| t.groups.iter().map(|g| g.skill_line).collect()) + .map(|t| t.groups.iter().map(|g| g.key).collect()) .unwrap_or_default() } else { match rows(model).get(id - 1) { @@ -370,7 +497,7 @@ fn set_collapsed(model: &mut Model, id: usize, collapse: bool) { .trainer .as_ref() .and_then(|t| t.groups.get(*gi)) - .map(|g| g.skill_line) + .map(|g| g.key) .into_iter() .collect(), _ => Vec::new(), @@ -435,7 +562,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { match row { Row::Header(gi) => { let g = &t.groups[gi]; - let expanded = !model.trainer_collapsed.contains(&g.skill_line); + let expanded = !model.trainer_collapsed.contains(&g.key); Ok(MultiValue::from_vec(vec![ Value::String(lua.create_string(&g.name)?), Value::Nil, @@ -573,22 +700,29 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { })?, )?; - // IsTradeskillTrainer() → 1/nil for the whole trainer (drives the tradeskill-vs-class layout). + // IsTradeskillTrainer() → 1/nil for the whole trainer (drives the tradeskill-vs-class layout): + // `0x4d8ea0` tests trainerType == 2. g.set( "IsTradeskillTrainer", lua.create_function(|lua, ()| { let model = lua.app_data_ref::().expect("model app_data"); - Ok(era_bool( - model.trainer.as_ref().is_some_and(|t| t.is_tradeskill), - )) + Ok(era_bool(model.trainer.as_ref().is_some_and(|t| { + t.trainer_type == TRAINER_TYPE_TRADESKILL + }))) })?, )?; - // IsTalentTrainer() → always nil (benilla drives no talent trainer; the ref window's talent hacks - // guard on this). + // IsTalentTrainer() → 1/nil: `0x4d8ed0` tests trainerType == 1 — vmangos's MOUNT trainers, which + // the client's own vocabulary calls "talent" (decision 1124). It used to return a hardcoded nil + // on the belief that benilla drives no such trainer; the shipped world has 23 of them. g.set( "IsTalentTrainer", - lua.create_function(|_, ()| Ok(Value::Nil))?, + lua.create_function(|lua, ()| { + let model = lua.app_data_ref::().expect("model app_data"); + Ok(era_bool( + model.trainer.as_ref().is_some_and(|t| t.trainer_type == 1), + )) + })?, )?; // GetTrainerGreetingText() → the trainer's greeting line ("" when no trainer is open). @@ -702,339 +836,5 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { Ok(()) } - #[cfg(test)] -mod tests { - use super::*; - use crate::script::UiScript; - - fn svc( - spell_id: u32, - name: &str, - cat: TrainerServiceCategory, - skill_line: u32, - line_name: &str, - ) -> TrainerService { - TrainerService { - spell_id, - // The fixture's default subject: the wire spell, no hop (the "no learn wrapper" - // fallback); the tooltip tests set the arm they mean explicitly. - tooltip: TrainerTooltip::Spell { - spell_id, - alt_caster: false, - }, - name: Some(name.into()), - subtext: Some("Rank 1".into()), - texture: Some(format!("Interface\\Icons\\Spell_{spell_id}")), - description: format!("Teaches {name}."), - cost: 100, - prof_first_rank: false, - category: cat, - level_req: 10, - skill_req: None, - ability_reqs: vec![], - is_trade_skill: false, - skill_line, - skill_line_name: line_name.into(), - } - } - - /// A two-line warrior trainer. Groups sort by name (Arms < Fury); within Arms, the level keys - /// order Heroic Strike (level 1) before Cleave (level 20). The default-expanded tree is thus: - /// `[H:Arms, Heroic Strike(avail), Cleave(used), H:Fury, Bloodrage(unavail)]` — a 5-row list whose - /// service rows sit at indices 2, 3, 5. - fn trainer() -> TrainerState { - let mut hs = svc( - 78, - "Heroic Strike", - TrainerServiceCategory::Available, - 26, - "Arms", - ); - hs.level_req = 1; - let mut cl = svc(284, "Cleave", TrainerServiceCategory::Used, 26, "Arms"); - cl.level_req = 20; - let br = svc( - 285, - "Bloodrage", - TrainerServiceCategory::Unavailable, - 256, - "Fury", - ); - TrainerState { - greeting: "What would you like to learn?".into(), - is_tradeskill: false, - groups: Vec::new(), - services: vec![hs, cl, br], - } - } - - #[test] - fn tree_interleaves_headers_and_ordered_services() { - let mut s = UiScript::new().unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 0); - assert!(s - .eval::("return GetTrainerServiceInfo(1) == nil") - .unwrap()); - - s.set_trainer(Some(trainer())); - // 2 headers + 3 services = 5 visible rows. - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); - - // Row 1 is the "Arms" header: name, nil subtext, "header", isExpanded=1. - let (hn, hsub, ht, hexp) = s - .eval::<(String, Option, String, Option)>( - "local n,s,t,e = GetTrainerServiceInfo(1) return n,s,t,e", - ) - .unwrap(); - assert_eq!((hn.as_str(), ht.as_str()), ("Arms", "header")); - assert_eq!((hsub, hexp), (None, Some(1))); - - // Rows 2..5: services (name/state) then the Fury header then its service — the sorted tree. - let info = |s: &mut UiScript, i: i64| { - s.eval::<(String, String)>(&format!( - "local n,_,t = GetTrainerServiceInfo({i}) return n,t" - )) - .unwrap() - }; - assert_eq!( - info(&mut s, 2), - ("Heroic Strike".into(), "available".into()) - ); - assert_eq!(info(&mut s, 3), ("Cleave".into(), "used".into())); - assert_eq!(info(&mut s, 4), ("Fury".into(), "header".into())); - assert_eq!(info(&mut s, 5), ("Bloodrage".into(), "unavailable".into())); - } - - #[test] - fn service_getters_read_the_row_at_a_visible_index() { - let mut s = UiScript::new().unwrap(); - let mut t = trainer(); - // Heroic Strike (services[0], the row at index 2) carries a full gate set. - t.services[0].skill_req = Some(TrainerSkillReq { - name: "Blacksmithing".into(), - rank: 100, - met: true, - }); - t.services[0].ability_reqs = vec![TrainerAbilityReq { - name: "Apprentice".into(), - met: false, - }]; - s.set_trainer(Some(t)); - - // Cost/level/desc/reqs all resolve at the SERVICE index 2 (not the header at 1). - assert_eq!( - s.eval::<(i64, i64, i64)>("return GetTrainerServiceCost(2)") - .unwrap(), - (100, 0, 0) - ); - assert_eq!( - s.eval::("return GetTrainerServiceLevelReq(2)") - .unwrap(), - 1 - ); - assert_eq!( - s.eval::("return GetTrainerServiceDescription(2)") - .unwrap(), - "Teaches Heroic Strike." - ); - let (skill, rank, has) = s - .eval::<(String, i64, i64)>("return GetTrainerServiceSkillReq(2)") - .unwrap(); - assert_eq!((skill.as_str(), rank, has), ("Blacksmithing", 100, 1)); - assert!(s - .eval::( - "local n,h = GetTrainerServiceAbilityReq(2,1) return n=='Apprentice' and h==nil" - ) - .unwrap()); - assert!(s - .eval::("local l,p = IsTrainerServiceLearnSpell(2) return l==1 and p==nil") - .unwrap()); - - // A HEADER row (index 1) has no service data — the getters no-op to defaults. - assert_eq!( - s.eval::("return GetTrainerServiceLevelReq(1)") - .unwrap(), - 0 - ); - assert!(s - .eval::("return GetTrainerServiceIcon(1) == nil") - .unwrap()); - assert!(s - .eval::("return GetTrainerServiceSkillReq(1) == nil") - .unwrap()); - } - - #[test] - fn state_filter_hides_services_not_headers() { - let mut s = UiScript::new().unwrap(); - s.set_trainer(Some(trainer())); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); - - // Hide "used": Cleave drops (5 → 4), but BOTH headers stay — a state filter culls services - // only. Row 3 is now the Fury header (Cleave was there). - s.run("SetTrainerServiceTypeFilter('used', 0)").unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 4); - let types: Vec = (1..=4) - .map(|i| { - s.eval::(&format!( - "local _,_,t = GetTrainerServiceInfo({i}) return t" - )) - .unwrap() - }) - .collect(); - assert_eq!(types, ["header", "available", "header", "unavailable"]); - - // Hide ALL states: the two headers remain (each group keeps its header even with no services). - s.run("SetTrainerServiceTypeFilter('available', 0)") - .unwrap(); - s.run("SetTrainerServiceTypeFilter('unavailable', 0)") - .unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 2); - assert!(s - .eval::( - "local _,_,t1 = GetTrainerServiceInfo(1) \ - local _,_,t2 = GetTrainerServiceInfo(2) \ - return t1=='header' and t2=='header'" - ) - .unwrap()); - } - - #[test] - fn collapse_by_header_index_and_collapse_all() { - let mut s = UiScript::new().unwrap(); - s.set_trainer(Some(trainer())); - - // Collapse the Arms group by its header's display index (1): its two services vanish, the - // header stays and now reports isExpanded=nil. 5 → 3 (H:Arms, H:Fury, Bloodrage). - s.run("CollapseTrainerSkillLine(1)").unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 3); - assert!(s - .eval::("local _,_,t,e = GetTrainerServiceInfo(1) return t=='header' and e==nil") - .unwrap()); - assert_eq!( - s.eval::("local n = GetTrainerServiceInfo(2) return n") - .unwrap(), - "Fury", - "Arms' services are folded; Fury's header is now row 2" - ); - - // Expand it back by the same header index. - s.run("ExpandTrainerSkillLine(1)").unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); - - // Collapse-all (id 0): both groups fold → just the two headers. - s.run("CollapseTrainerSkillLine(0)").unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 2); - // Expand-all (id 0): back to the full tree. - s.run("ExpandTrainerSkillLine(0)").unwrap(); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); - } - - #[test] - fn collapse_survives_a_content_update_and_resets_on_close() { - let mut s = UiScript::new().unwrap(); - s.set_trainer(Some(trainer())); - s.run("CollapseTrainerSkillLine(1)").unwrap(); // fold Arms - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 3); - - // A re-list (same skill lines) keeps the fold — a buy re-lists and the tree shouldn't jump open. - s.set_trainer(Some(trainer())); - assert_eq!( - s.eval::("return GetNumTrainerServices()").unwrap(), - 3, - "Arms stays collapsed across a content update" - ); - - // Close → the collapse set resets; a re-open is fully expanded. - s.set_trainer(None); - s.set_trainer(Some(trainer())); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); - } - - #[test] - fn buy_queues_the_selected_services_spell_id_headers_no_op() { - let mut s = UiScript::new().unwrap(); - s.set_trainer(Some(trainer())); - // Row 5 is Bloodrage (spell 285). Buying it queues its spell id, not its index. - s.run("BuyTrainerService(5)").unwrap(); - assert_eq!(s.take_trainer_buys(), vec![285]); - assert!(s.take_trainer_buys().is_empty(), "drained"); - - // Buying a HEADER row (index 1) queues nothing. - s.run("BuyTrainerService(1)").unwrap(); - assert!(s.take_trainer_buys().is_empty(), "a header is not buyable"); - } - - #[test] - fn selection_and_close_intents() { - let mut s = UiScript::new().unwrap(); - s.set_trainer(Some(trainer())); - assert_eq!( - s.eval::("return GetTrainerSelectionIndex()").unwrap(), - 0 - ); - s.run("SelectTrainerService(2)").unwrap(); - assert_eq!( - s.eval::("return GetTrainerSelectionIndex()").unwrap(), - 2 - ); - s.run("SelectTrainerService(9)").unwrap(); // OOB (past the 5 rows) clears - assert_eq!( - s.eval::("return GetTrainerSelectionIndex()").unwrap(), - 0 - ); - - assert!(!s.take_trainer_close()); - s.run("CloseTrainer()").unwrap(); - assert!(s.take_trainer_close()); - assert!(!s.take_trainer_close(), "drained"); - } - - #[test] - fn tradeskill_and_talent_flags() { - let mut s = UiScript::new().unwrap(); - assert!(s - .eval::("return IsTradeskillTrainer() == nil") - .unwrap()); - let mut t = trainer(); - t.is_tradeskill = true; - t.services[0].is_trade_skill = true; // Heroic Strike → the row at index 2 - s.set_trainer(Some(t)); - assert!(s.eval::("return IsTradeskillTrainer() == 1").unwrap()); - assert!(s - .eval::("return IsTrainerServiceTradeSkill(2) == 1") - .unwrap()); - assert!(s.eval::("return IsTalentTrainer() == nil").unwrap()); - } - - #[test] - fn unresolved_skill_line_is_dropped() { - let mut s = UiScript::new().unwrap(); - let mut t = trainer(); - // A service whose skill line didn't resolve (0) is dropped from the tree entirely. - t.services.push(svc( - 999, - "Orphan Spell", - TrainerServiceCategory::Available, - 0, - "", - )); - s.set_trainer(Some(t)); - // Still 5 rows — the orphan contributes neither a header nor a service row. - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); - } - - #[test] - fn clearing_empties_and_resets_selection() { - let mut s = UiScript::new().unwrap(); - s.set_trainer(Some(trainer())); - s.run("SelectTrainerService(2)").unwrap(); - s.set_trainer(None); - assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 0); - assert_eq!( - s.eval::("return GetTrainerSelectionIndex()").unwrap(), - 0 - ); - } -} +mod tests; diff --git a/crates/benilla-ui/src/script/trainer/tests.rs b/crates/benilla-ui/src/script/trainer/tests.rs new file mode 100644 index 00000000..35c3bf3c --- /dev/null +++ b/crates/benilla-ui/src/script/trainer/tests.rs @@ -0,0 +1,620 @@ +//! The trainer tree's tests — the per-`trainerType` ordering laws (decisions 0247/1124) pinned +//! against wow-re's own emulated runs of the real finalizer, plus the filter/collapse/intent +//! surface. Split out of `mod.rs` when the four comparators pushed it past the file budget. + +use super::*; +use crate::script::UiScript; + +fn svc( + spell_id: u32, + name: &str, + cat: TrainerServiceCategory, + group_key: u32, + group_name: &str, +) -> TrainerService { + TrainerService { + spell_id, + // The fixture's default subject: the wire spell, no hop (the "no learn wrapper" + // fallback); the tooltip tests set the arm they mean explicitly. + tooltip: TrainerTooltip::Spell { + spell_id, + alt_caster: false, + }, + name: Some(name.into()), + subtext: Some("Rank 1".into()), + texture: Some(format!("Interface\\Icons\\Spell_{spell_id}")), + description: format!("Teaches {name}."), + cost: 100, + prof_first_rank: false, + category: cat, + level_req: 10, + skill_req: None, + ability_reqs: vec![], + is_trade_skill: false, + group_key, + group_name: group_name.into(), + } +} + +/// A two-line warrior trainer (type 0). Groups sort by name (Arms < Fury); within Arms, the level +/// keys order Heroic Strike (level 1) before Cleave (level 20). The default-expanded tree is thus: +/// `[H:Arms, Heroic Strike(avail), Cleave(used), H:Fury, Bloodrage(unavail)]` — a 5-row list whose +/// service rows sit at indices 2, 3, 5. +fn trainer() -> TrainerState { + let mut hs = svc( + 78, + "Heroic Strike", + TrainerServiceCategory::Available, + 26, + "Arms", + ); + hs.level_req = 1; + let mut cl = svc(284, "Cleave", TrainerServiceCategory::Used, 26, "Arms"); + cl.level_req = 20; + let br = svc( + 285, + "Bloodrage", + TrainerServiceCategory::Unavailable, + 256, + "Fury", + ); + TrainerState { + greeting: "What would you like to learn?".into(), + trainer_type: 0, + groups: Vec::new(), + services: vec![hs, cl, br], + } +} + +/// Every visible row as `(name, serviceType)` — the shape the ported XML actually renders. +fn visible(s: &mut UiScript) -> Vec<(String, String)> { + let n = s.eval::("return GetNumTrainerServices()").unwrap(); + (1..=n) + .map(|i| { + s.eval::<(String, String)>(&format!( + "local n,_,t = GetTrainerServiceInfo({i}) return n,t" + )) + .unwrap() + }) + .collect() +} + +#[test] +fn tree_interleaves_headers_and_ordered_services() { + let mut s = UiScript::new().unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 0); + assert!(s + .eval::("return GetTrainerServiceInfo(1) == nil") + .unwrap()); + + s.set_trainer(Some(trainer())); + // 2 headers + 3 services = 5 visible rows. + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); + + // Row 1 is the "Arms" header: name, nil subtext, "header", isExpanded=1. + let (hn, hsub, ht, hexp) = s + .eval::<(String, Option, String, Option)>( + "local n,s,t,e = GetTrainerServiceInfo(1) return n,s,t,e", + ) + .unwrap(); + assert_eq!((hn.as_str(), ht.as_str()), ("Arms", "header")); + assert_eq!((hsub, hexp), (None, Some(1))); + + // Rows 2..5: services (name/state) then the Fury header then its service — the sorted tree. + let info = |s: &mut UiScript, i: i64| { + s.eval::<(String, String)>(&format!( + "local n,_,t = GetTrainerServiceInfo({i}) return n,t" + )) + .unwrap() + }; + assert_eq!( + info(&mut s, 2), + ("Heroic Strike".into(), "available".into()) + ); + assert_eq!(info(&mut s, 3), ("Cleave".into(), "used".into())); + assert_eq!(info(&mut s, 4), ("Fury".into(), "header".into())); + assert_eq!(info(&mut s, 5), ("Bloodrage".into(), "unavailable".into())); +} + +#[test] +fn service_getters_read_the_row_at_a_visible_index() { + let mut s = UiScript::new().unwrap(); + let mut t = trainer(); + // Heroic Strike (services[0], the row at index 2) carries a full gate set. + t.services[0].skill_req = Some(TrainerSkillReq { + name: "Blacksmithing".into(), + rank: 100, + met: true, + }); + t.services[0].ability_reqs = vec![TrainerAbilityReq { + name: "Apprentice".into(), + met: false, + }]; + s.set_trainer(Some(t)); + + // Cost/level/desc/reqs all resolve at the SERVICE index 2 (not the header at 1). + assert_eq!( + s.eval::<(i64, i64, i64)>("return GetTrainerServiceCost(2)") + .unwrap(), + (100, 0, 0) + ); + assert_eq!( + s.eval::("return GetTrainerServiceLevelReq(2)") + .unwrap(), + 1 + ); + assert_eq!( + s.eval::("return GetTrainerServiceDescription(2)") + .unwrap(), + "Teaches Heroic Strike." + ); + let (skill, rank, has) = s + .eval::<(String, i64, i64)>("return GetTrainerServiceSkillReq(2)") + .unwrap(); + assert_eq!((skill.as_str(), rank, has), ("Blacksmithing", 100, 1)); + assert!(s + .eval::( + "local n,h = GetTrainerServiceAbilityReq(2,1) return n=='Apprentice' and h==nil" + ) + .unwrap()); + assert!(s + .eval::("local l,p = IsTrainerServiceLearnSpell(2) return l==1 and p==nil") + .unwrap()); + + // A HEADER row (index 1) has no service data — the getters no-op to defaults. + assert_eq!( + s.eval::("return GetTrainerServiceLevelReq(1)") + .unwrap(), + 0 + ); + assert!(s + .eval::("return GetTrainerServiceIcon(1) == nil") + .unwrap()); + assert!(s + .eval::("return GetTrainerServiceSkillReq(1) == nil") + .unwrap()); +} + +/// The state filter takes a group's **header with it** once it hides the group's last service — the +/// finalizer's `[+0x1c]` hide has no header-row exemption (`0x4d8528`/`0x4d8535`, decision 1124). +/// This asserted the opposite until 1124: benilla rendered bare headers over empty groups, and an +/// all-boxes-off filter left a window full of headings and nothing else. +#[test] +fn state_filter_takes_a_groups_header_with_its_last_service() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); + + // Hide "used": Cleave drops (5 → 4), and BOTH headers stay — Arms still has Heroic Strike. + s.run("SetTrainerServiceTypeFilter('used', 0)").unwrap(); + assert_eq!( + visible(&mut s) + .into_iter() + .map(|(_, t)| t) + .collect::>(), + ["header", "available", "header", "unavailable"] + ); + + // Now hide "available" too: Arms loses its last row, so the ARMS HEADER GOES WITH IT — only + // Fury's header and its unavailable service remain. + s.run("SetTrainerServiceTypeFilter('available', 0)") + .unwrap(); + assert_eq!( + visible(&mut s), + [ + ("Fury".to_string(), "header".to_string()), + ("Bloodrage".to_string(), "unavailable".to_string()), + ] + ); + + // Every box off renders NOTHING — not a list of bare headings. + s.run("SetTrainerServiceTypeFilter('unavailable', 0)") + .unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 0); + assert!(s + .eval::("return GetTrainerServiceInfo(1) == nil") + .unwrap()); +} + +/// Collapse is the *asymmetric* case, and deliberately so: it hides a group's services but keeps the +/// header — a different field (`hdr[+0x20]`) tested three instructions after the filter's, with a +/// header-row exemption the filter's lacks (`0x4d853d`). Without it a collapsed group could never be +/// re-expanded, having no header left to click. +#[test] +fn collapse_keeps_the_header_the_filter_would_remove() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + s.run("CollapseTrainerSkillLine(1)").unwrap(); // fold Arms — both its services hide + assert_eq!( + visible(&mut s), + [ + ("Arms".to_string(), "header".to_string()), + ("Fury".to_string(), "header".to_string()), + ("Bloodrage".to_string(), "unavailable".to_string()), + ] + ); + // Collapse-all keeps every header; the filter's empty case keeps none. + s.run("CollapseTrainerSkillLine(0)").unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 2); +} + +#[test] +fn collapse_by_header_index_and_collapse_all() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + + // Collapse the Arms group by its header's display index (1): its two services vanish, the + // header stays and now reports isExpanded=nil. 5 → 3 (H:Arms, H:Fury, Bloodrage). + s.run("CollapseTrainerSkillLine(1)").unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 3); + assert!(s + .eval::("local _,_,t,e = GetTrainerServiceInfo(1) return t=='header' and e==nil") + .unwrap()); + assert_eq!( + s.eval::("local n = GetTrainerServiceInfo(2) return n") + .unwrap(), + "Fury", + "Arms' services are folded; Fury's header is now row 2" + ); + + // Expand it back by the same header index. + s.run("ExpandTrainerSkillLine(1)").unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); + + // Collapse-all (id 0): both groups fold → just the two headers. + s.run("CollapseTrainerSkillLine(0)").unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 2); + // Expand-all (id 0): back to the full tree. + s.run("ExpandTrainerSkillLine(0)").unwrap(); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); +} + +#[test] +fn collapse_survives_a_content_update_and_resets_on_close() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + s.run("CollapseTrainerSkillLine(1)").unwrap(); // fold Arms + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 3); + + // A re-list (same skill lines) keeps the fold — a buy re-lists and the tree shouldn't jump open. + s.set_trainer(Some(trainer())); + assert_eq!( + s.eval::("return GetNumTrainerServices()").unwrap(), + 3, + "Arms stays collapsed across a content update" + ); + + // Close → the collapse set resets; a re-open is fully expanded. + s.set_trainer(None); + s.set_trainer(Some(trainer())); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); +} + +#[test] +fn buy_queues_the_selected_services_spell_id_headers_no_op() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + // Row 5 is Bloodrage (spell 285). Buying it queues its spell id, not its index. + s.run("BuyTrainerService(5)").unwrap(); + assert_eq!(s.take_trainer_buys(), vec![285]); + assert!(s.take_trainer_buys().is_empty(), "drained"); + + // Buying a HEADER row (index 1) queues nothing. + s.run("BuyTrainerService(1)").unwrap(); + assert!(s.take_trainer_buys().is_empty(), "a header is not buyable"); +} + +#[test] +fn selection_and_close_intents() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + assert_eq!( + s.eval::("return GetTrainerSelectionIndex()").unwrap(), + 0 + ); + s.run("SelectTrainerService(2)").unwrap(); + assert_eq!( + s.eval::("return GetTrainerSelectionIndex()").unwrap(), + 2 + ); + s.run("SelectTrainerService(9)").unwrap(); // OOB (past the 5 rows) clears + assert_eq!( + s.eval::("return GetTrainerSelectionIndex()").unwrap(), + 0 + ); + + assert!(!s.take_trainer_close()); + s.run("CloseTrainer()").unwrap(); + assert!(s.take_trainer_close()); + assert!(!s.take_trainer_close(), "drained"); +} + +/// The two whole-trainer predicates fork on the wire `trainer_type` — `IsTradeskillTrainer` on +/// `== 2` (`0x4d8ea0`), `IsTalentTrainer` on `== 1` (`0x4d8ed0`) — so at most one is ever true, and +/// at a class trainer neither is. +#[test] +fn tradeskill_and_talent_flags() { + let mut s = UiScript::new().unwrap(); + assert!(s + .eval::("return IsTradeskillTrainer() == nil") + .unwrap()); + + // A class trainer: neither predicate. + s.set_trainer(Some(trainer())); + assert!(s + .eval::("return IsTradeskillTrainer() == nil and IsTalentTrainer() == nil") + .unwrap()); + + // A tradeskill trainer — one "Recipes" group, so its one service is the row at index 2. + let mut recipe = svc( + 2743, + "Copper Chain Pants", + TrainerServiceCategory::Unavailable, + 2, + "Recipes", + ); + recipe.is_trade_skill = true; + s.set_trainer(Some(TrainerState { + greeting: String::new(), + trainer_type: 2, + groups: Vec::new(), + services: vec![recipe], + })); + assert!(s + .eval::("return IsTradeskillTrainer() == 1 and IsTalentTrainer() == nil") + .unwrap()); + assert!(s + .eval::("return IsTrainerServiceTradeSkill(2) == 1") + .unwrap()); + + // A mount trainer — the "talent" predicate, which used to be a hardcoded nil. + s.set_trainer(Some(TrainerState { + greeting: String::new(), + trainer_type: 1, + groups: Vec::new(), + services: vec![svc( + 33, + "Riding", + TrainerServiceCategory::Available, + 762, + "Riding", + )], + })); + assert!(s + .eval::("return IsTalentTrainer() == 1 and IsTradeskillTrainer() == nil") + .unwrap()); +} + +#[test] +fn unresolved_skill_line_is_dropped() { + let mut s = UiScript::new().unwrap(); + let mut t = trainer(); + // A service whose skill line didn't resolve (0) is dropped from the tree entirely. + t.services.push(svc( + 999, + "Orphan Spell", + TrainerServiceCategory::Available, + 0, + "", + )); + s.set_trainer(Some(t)); + // Still 5 rows — the orphan contributes neither a header nor a service row. + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 5); +} + +#[test] +fn clearing_empties_and_resets_selection() { + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(trainer())); + s.run("SelectTrainerService(2)").unwrap(); + s.set_trainer(None); + assert_eq!(s.eval::("return GetNumTrainerServices()").unwrap(), 0); + assert_eq!( + s.eval::("return GetTrainerSelectionIndex()").unwrap(), + 0 + ); +} + +/// **The tradeskill order, pinned against wow-re's emulated run of the real finalizer** (decision +/// 1124). The fixture is creature 957 "Dane Lindgren" — the blacksmithing trainer in the director's +/// report — with his 19 real `npc_trainer` rows: 18 recipes on skill line 164 with ascending +/// `reqSkillValue`, plus the profession-learn service 2020 ("Apprentice Blacksmith", `reqLevel 5`, +/// no skill gate). wow-re fed exactly this set through the builder `0x4d7560` + finalizer `0x4d8410` +/// with `0xb73a08 = 2` and the real `qsort`/collator, and got the 21 rows asserted below. +/// +/// Two things this pins that benilla had wrong, and one it never had: +/// - the learn row is **first among services**, under its own `Development Skills` header — placed +/// by the `SKILL_STEP` group partition, *not* by any level key; +/// - `reqLevel` is **inert** at type 2 (the class comparator's first key, which used to sink 2020 to +/// the bottom of the list); +/// - recipes ascend by `reqSkillValue`, ties broken by name. +#[test] +fn tradeskill_trainer_matches_the_emulated_reference_order() { + // (wire spell, name, reqSkillValue) — the recipes, in wire order (ascending here, but the + // fixture is shuffled below to prove the order is the comparator's and not the input's). + let recipes: &[(u32, &str, u32)] = &[ + (2743, "Copper Chain Pants", 1), + (2754, "Copper Mace", 15), + (2755, "Copper Axe", 20), + (3340, "Copper Chain Boots", 20), + (2756, "Copper Shortsword", 25), + (3341, "Rough Grinding Stone", 25), + (9984, "Copper Claymore", 30), + (8881, "Copper Dagger", 30), + (2744, "Copper Battle Axe", 35), + (3299, "Copper Chain Belt", 35), + (3342, "Runed Copper Gauntlets", 40), + (3343, "Runed Copper Pants", 45), + (2746, "Coarse Sharpening Stone", 65), + (7409, "Coarse Weightstone", 65), + (3118, "Heavy Copper Maul", 65), + (3300, "Runed Copper Belt", 70), + (2747, "Thick War Axe", 70), + (3344, "Coarse Grinding Stone", 75), + ]; + let mut services: Vec = recipes + .iter() + .map(|&(id, name, req)| { + // Group key 2 = TRADESKILL_SERVICE_LEARN: no SKILL_STEP effect on the wire spell. + let mut s = svc(id, name, TrainerServiceCategory::Unavailable, 2, "Recipes"); + s.subtext = None; + s.level_req = 0; + s.skill_req = Some(TrainerSkillReq { + name: "Blacksmithing".into(), + rank: req, + met: false, + }); + s + }) + .collect(); + services.reverse(); // wire order is not display order — and must not be + // Group key 1 = TRADESKILL_SERVICE_STEP: spell 2020 carries Effect 44 SKILL_STEP. + let mut learn = svc( + 2020, + "Apprentice Blacksmith", + TrainerServiceCategory::Available, + 1, + "Development Skills", + ); + learn.subtext = None; + learn.level_req = 5; // the key that used to decide everything, and decides nothing here + learn.prof_first_rank = true; + services.insert(services.len() / 2, learn); + + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(TrainerState { + greeting: "Care to learn how to turn the ore that you find into weapons?".into(), + trainer_type: 2, + groups: Vec::new(), + services, + })); + + let rows = visible(&mut s); + let expected: Vec<(&str, &str)> = vec![ + ("Development Skills", "header"), + ("Apprentice Blacksmith", "available"), + ("Recipes", "header"), + ("Copper Chain Pants", "unavailable"), + ("Copper Mace", "unavailable"), + ("Copper Axe", "unavailable"), + ("Copper Chain Boots", "unavailable"), + ("Copper Shortsword", "unavailable"), + ("Rough Grinding Stone", "unavailable"), + ("Copper Claymore", "unavailable"), + ("Copper Dagger", "unavailable"), + ("Copper Battle Axe", "unavailable"), + ("Copper Chain Belt", "unavailable"), + ("Runed Copper Gauntlets", "unavailable"), + ("Runed Copper Pants", "unavailable"), + ("Coarse Sharpening Stone", "unavailable"), + ("Coarse Weightstone", "unavailable"), + ("Heavy Copper Maul", "unavailable"), + ("Runed Copper Belt", "unavailable"), + ("Thick War Axe", "unavailable"), + ("Coarse Grinding Stone", "unavailable"), + ]; + let got: Vec<(&str, &str)> = rows.iter().map(|(n, t)| (n.as_str(), t.as_str())).collect(); + assert_eq!(got, expected); +} + +/// **The mount (type 1, the client's "talent") order, pinned against wow-re's emulated run** +/// (decision 1124): the already-known services fold into the `-1` "My Talents" group, which the +/// header comparator puts **first**, ahead of the name-ordered skill-line headers; within a group +/// the state byte sorts available → unavailable → used, then the name. +#[test] +fn mount_trainer_folds_known_services_into_my_talents() { + let known = |id: u32, name: &str| { + svc( + id, + name, + TrainerServiceCategory::Used, + TRAINER_GROUP_KNOWN, + "My Talents", + ) + }; + let services = vec![ + svc( + 33, + "Riding", + TrainerServiceCategory::Available, + 762, + "Riding", + ), + known(6648, "Tiger Riding"), + svc( + 824, + "Horse Riding", + TrainerServiceCategory::Unavailable, + 148, + "Horse Riding", + ), + svc( + 8394, + "Ram Riding", + TrainerServiceCategory::Available, + 152, + "Ram Riding", + ), + ]; + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(TrainerState { + greeting: "Ready to ride?".into(), + trainer_type: 1, + groups: Vec::new(), + services, + })); + + let got: Vec = visible(&mut s).into_iter().map(|(n, _)| n).collect(); + assert_eq!( + got, + [ + "My Talents", + "Tiger Riding", + "Horse Riding", + "Horse Riding", + "Ram Riding", + "Ram Riding", + "Riding", + "Riding", + ], + "the -1 group leads; every other header is name-ordered" + ); + // And the type predicates fork on the same dword the sort does. + assert!(s.eval::("return IsTalentTrainer() == 1").unwrap()); + assert!(s + .eval::("return IsTradeskillTrainer() == nil") + .unwrap()); +} + +/// The **state byte is a sort key at type 1 and only there** — the same three services, ordered by +/// state (available → unavailable → used) inside one group, where the class cascade would order +/// them by level and the tradeskill one by skill value. +#[test] +fn talent_order_sorts_on_state_within_a_group() { + let one = |id: u32, name: &str, cat: TrainerServiceCategory| svc(id, name, cat, 762, "Riding"); + let services = vec![ + one(3, "Cee", TrainerServiceCategory::Available), + one(1, "Aye", TrainerServiceCategory::Used), + one(2, "Bee", TrainerServiceCategory::Unavailable), + ]; + let mut s = UiScript::new().unwrap(); + s.set_trainer(Some(TrainerState { + greeting: String::new(), + trainer_type: 1, + groups: Vec::new(), + services: services.clone(), + })); + let got: Vec = visible(&mut s).into_iter().map(|(n, _)| n).collect(); + assert_eq!(got, ["Riding", "Cee", "Bee", "Aye"]); + + // The same three at a CLASS trainer sort by name instead (equal levels, no skill gates) — the + // proof that the comparator really is selected by the type and not shared. + s.set_trainer(Some(TrainerState { + greeting: String::new(), + trainer_type: 0, + groups: Vec::new(), + services, + })); + let got: Vec = visible(&mut s).into_iter().map(|(n, _)| n).collect(); + assert_eq!(got, ["Riding", "Aye", "Bee", "Cee"]); +}