Derived durations, and the measurement that was one tick long - #1
Open
JDS300 wants to merge 13 commits into
Open
Derived durations, and the measurement that was one tick long#1JDS300 wants to merge 13 commits into
JDS300 wants to merge 13 commits into
Conversation
The brief asked to re-anchor DoT timers on the cast line. The log says no: across six DoTs with both fade lines and an exact wiki duration, cast->fade matches nothing and runs long by each spell's own cast time. fade->firstTick matches the wiki exactly. The first tick is the landing. The real defect is a phantom trailing tick in the no-fade measurement path, which is where Immolate's 54s against a wiki 48s came from. Also settled: base durations come from the harvest already on disk rather than a runtime fetch, and rank resolution asks the catalog first, because 121 spells legitimately end in a Roman numeral and Clarity II is not tier-2 Clarity.
Six tasks, TDD throughout: drop the phantom tick, parse ranks, promote the catalog, wire the three-tier lookup into the tracker, mark the estimate, then replay the real log in the real app. Writing it surfaced one thing the spec did not cover: _recentCasts is pruned only when a new cast arrives, so pairing a tick to a cast without a window would quietly become "the last rank I ever saw" - the guess the design rejected. Bounded at 15s, with a test that pins it.
The plan quoted 923 as the baseline for EQBuddy.Tests, but 923 is both suites combined. Measured on branch: 842 Core, 81 Avalonia.
Two defects found scanning the plan against the existing tests before any dispatch: Expiry() fell back to the base tick name when no cast explained an effect, so it would have derived a tier-0 duration for exactly the case the design says must read '--'. The unknown-rank test in the plan would have failed against the plan's own implementation. castName is now separate and nullable, and nothing derives without it. Task 1 breaks four existing tests that encode the phantom tick. Naming them, with their new values and the four fade-path tests that must NOT move, so the implementer corrects the evidence rather than guessing at it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate quoted the 1,589 spells that have a parsed duration_seconds without subtracting the 896 that are Instant/zero and the 340 level-scaled ones the promote script correctly excludes. 693 positive durations collapse to 680 unique names. The Task 3 implementer stopped at the gate exactly as told to, and its number was the right one.
Promotes the eqlwiki spell harvest into an embedded 680-entry duration catalog (SpellDurations.json) and resolves cast names against it. The catalog is the authority on names: an exact hit always beats a rank interpretation, since 121 wiki spells are genuinely named with a trailing roman numeral (Clarity II, Burnout IV) rather than being ranks of something else. Level-scaled and Instant durations are excluded on purpose, so an absent spell means unknown, never a guess.
promote(): the 693 -> 680 name collapse silently picked last-wins on 13 collisions; two (Rabies, Solon's Bravura) are genuinely different spells with different durations, not duplicates. Now logged to stderr, naming both pages and which value survives, so a knowledge refresh can't flip the pick unnoticed. Behavior unchanged - output JSON is byte-identical. SpellDurationCatalogTests: the numeral-vs-rank ordering test had no "Clarity" entry in its fixture, so a reversed (rank-before-exact) implementation would simply miss the rank branch and fall through to the same exact hit - passing for the wrong reason. Added a real "Clarity" base entry to the fixture and a new test against the embedded catalog's real Clarity/Burnout collision, both of which now fail under a reversed Resolve() (1944 vs 2100, 5040 vs 900) and pass under the correct one - verified by temporarily swapping the two branches.
Wire SpellDurationCatalog into DebuffTracker: effects are keyed by base name (tick/fade lines never carry the rank) while the chip displays the ranked name, samples are recorded per-rank, and Expiry falls back to the catalog's derived duration only when a cast supplied the rank. Trust order stays Measured > Derived > Unknown; a measurement is never adjusted toward the catalog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A derived duration is an estimate, and the real spell can outlast it - replaying the user's own log, Shiftless Deeds IV measured 214.0s against a derived 210.0s. Retiring the chip at the estimate also dropped the effect, so the fade found nothing, nothing was recorded, and every later cast re-derived the same value: the spell pinned at the guess for the rest of the session, one second of margin away from happening for real. So retirement is now two separate things. Active() moves a retired non-ticking effect into _awaitingFade instead of dropping it, OnFade measures from whichever store still remembers it, and _awaitingFade is pruned at LandedAt + UnknownCap so it cannot grow without bound. A fresh landing on the same key supersedes what is waiting there. The chip itself is now bounded too. A derived expiry defeated UnknownCap - a mis-attributed Valor would have held the panel for 54 minutes, which is the exact thing that cap was written to stop - so a non-ticking chip retires at min(ExpiresAt + ExpiryLinger, LandedAt + UnknownCap). Ticking DoTs still retire on TickGrace, which was already right. Two smaller things the refresh path was doing wrong. It read _samples under the tick's BASE name while displaying the RANKED one, so a rank-N effect could take its countdown from tier-0 samples and show it as Measured - the pooling this design forbids, arriving through the display rather than through the store. And _recentCasts was pruned at CastToLand (8s) while OnTick read it at CastToTick (15s), so the wider window described a list that could not contain anything that old; it now prunes at the wider of the two. With that fixed the sample-key bug turns out to be unreachable, since a live effect's next tick is always within TickGrace of the cast that set the recast flag - but reading a key you will never write to is worth not doing anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OnTick was the one place a key skipped BaseNameOf: tick.Source went in raw. The three keys agreed only because real tick lines never carry a numeral - for a spell genuinely NAMED with one and missing from the catalog, the tick key and the fade key diverge and fade-ending silently stops working. The _recastPending lookups had the same split, written with the base name and read with the raw one, so recast detection could not fire for such a spell either. Both now go through BaseNameOf; the chip still displays what the log actually said. _active's tuple field was called Spell while every call site puts a base name in it. It is BaseName now - that naming confusion is what made the sample-key bug possible to write. The doc comments claimed 121 catalog spells end in a roman numeral. True of the 1,929-spell harvest; the shipped catalog has 10, and only 4 of those (Burnout II/III/IV, Clarity II) also carry a base entry, which is where the two readings actually collide. The rule and the reasoning were right, so only the counts and the examples changed - "Cannibalize IV" and "Berserker Madness III" do not ship. "680 spells" is 679 keys at runtime: the OrdinalIgnoreCase loader folds the two spellings of Invisibility Versus Undead, both 1620.0. And a test that built a Measured state with no expiry - a combination production never produces - now asks for Unknown, so the file has one genuine Unknown state in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cold-start countdowns for DoTs and debuffs, drawn from the wiki's base duration scaled by spell rank and marked as estimates — plus a fix to the measurement that reported every DoT one tick too long.
The anchoring task changed direction
The brief asked to re-anchor DoT timers on
SpellCastEvent. The log says no. Six DoTs with both fade lines and an exact wiki duration, medians over 138 completed casts:+61 minuteAnchoring on the cast matches nothing — it runs long by each spell's own cast time (Immolate 2.5s, Shiftless Deeds 6.0s), which is why the error was never a constant six seconds. The first tick is the landing, and the fade line arrives at the last tick, not one after it. The real defect was a phantom trailing tick in the no-fade measurement path. Drifting Death is the lone miss and explains itself: its wiki value is the prose
1 minute, rounded from 54.Three tiers of trust
Measured (from the log, authoritative) > Derived (
base × (1 + 0.10 × tier), shown~2:40) > Unknown (--, never a guessed number). A measurement is never corrected toward the wiki: Tepid Deeds keeps its measured 126s against a wiki 150 whose own page contradicts itself.Base durations are shipped, not fetched
Data/SpellDurations.json(680 entries) is promoted from the harvest already in the repo byscripts/harvests/eqlwiki/spells-promote.py. No HTTP mid-fight, works offline.Instantand level-scaled durations (Cripple's6.3 minutes @L53 to 7.0 minutes @L60) are excluded — an absent spell means unknown, never a guess.The catalog is the authority on names
121 harvested spells are genuinely named with a trailing numeral —
Clarity II,Burnout IV— and are not ranks. So an exact catalog hit beats a rank interpretation. Read the other way round,Burnout IVwould resolve to 3600 × 1.4 = 5040s instead of 900s.Validation against the real 690k-line log
Heroic Leap I: 7,987 samples, always--. No catalog entry, no invented number.Drifting Death --,Immolate 0:00,Tepid Deeds ~1:20,Shiftless Deeds IV ~2:18.Bugs found and fixed along the way
_recastPendingstored the ranked cast name and was read with the unranked tick name, so recast detection could never fire for a ranked DoT — the exact failure its own comment documents. Every key now goes throughBaseNameOf._awaitingFade).UnknownCap, holding a chip for 54 minutes.Rabies2880 vs 314). It now warns instead of choosing in silence.Tests
EQBuddy.Tests842 → 882,EQBuddy.Avalonia.Tests81. All green.Known, out of scope
The slice-2 landing/cast pairing mis-attributes:
Stun,Wrath,Valorand others surface as slow chips because the landing line names no spell. Pre-existing, but this feature upgrades two of them from--to a confident wrong countdown. The harvest carries abeneficialflag and a beneficial spell can never explain a "slows down" line — worth its own slice.🤖 Generated with Claude Code