Skip to content

fix(dps): stop dropping attacker rows mid-fight; count melee only by default - #62

Merged
prokopto-dev merged 4 commits into
masterfrom
ao/nparseplus-6/dps-row-retention
Aug 10, 2026
Merged

fix(dps): stop dropping attacker rows mid-fight; count melee only by default#62
prokopto-dev merged 4 commits into
masterfrom
ao/nparseplus-6/dps-row-retention

Conversation

@prokopto-dev

Copy link
Copy Markdown
Owner

The DPS meter was reported as "not seeming functional". It was running, but
three things made it read as broken, and two of them were real defects.

1. Rows vanished mid-fight (fix)

EQTool's DPSWindowViewModel.ShouldRemove aged out each EntittyDPS on its
own last hit, and the port carried that over. So anyone who contributed
early and stopped — a stun on the pull, one backstab, a wizard who nuked and
sat — disappeared from the target's group 40s later while the mob was still
up. On anything longer than a trash pull the meter was actively
under-reporting who was on the target.

Staleness now belongs to Fight, not FightEntity: the group is keyed on
the most recent hit from any attacker, so one player still swinging keeps
every other row on screen, and the group retires as a unit. Nothing is pruned
out from under a live fight.

Also drops an abs() from the retention comparison — a last-damage time in
the future (a log line stamped ahead of the wall clock) was being read as
40s stale.

2. Spell damage was credited to you (feat)

<target> was hit by non-melee for N points names no attacker, so
DamageParser has nothing to credit but You. Every proc and nuke in range
landed in your row — including other players' — and a lone nuke opened a
fight group on a mob you never swung at.

FightTracker.melee_only defaults on and drops non-melee before it can
open a fight. The filter lives in the tracker, not the parser:
DamageParser stays the record of what the log said, so triggers and plugins
subscribed to DamageEvent still see spell damage either way.

core/damagetypes.py is the shared vocabulary — its own module because
parsers/damage.py builds its four regexes from the verb list while
core.dps filters by it, and importing the parser to reach a frozenset costs
~100 ms (it loads the master NPC list) on a module the UI imports.
is_melee() reads unknown types as not melee on purpose, so a damage
line the parser learns later can't silently join a melee-only meter.

3. Nothing was adjustable — new Settings > DPS Meter page

Control Moves
Melee damage only the filter above
Attacker dropoff fight-level retention; 0 = never (zone/camp/clear only)
DPS averaging window the span the dps column divides by
Session stat minimum fight the >20s gate on the Best/Now/Last footer

All live on Apply via Backend.apply_dps_settingsFightTracker.configure
— no restart. The tunables are plain attributes rather than constructor-only
because the app builds one tracker per launch and it outlives every settings
window.

That last knob explains a long-standing confusion: most P99 trash dies in
under 20s, so the Best/Now/Last footer sat at 0 / 0 / — all session by
design. It is now lowerable.

Two subtleties, both commented in the code

  • Narrowing the averaging window zeroes best_window_damage — a
    best-in-6s is not comparable to a best-in-12s, and the max-merge would
    otherwise keep the stale larger number forever.
  • Damage already counted is never re-filtered (the hit list does not retain
    damage types), so toggling melee-only takes effect on the next hit rather
    than retroactively.

Behavior changes reviewers should weigh

  • Melee-only is a changed default. Existing users' DPS numbers will drop
    where spell damage was previously (mis)counted. Deliberate — the old number
    was wrong — but it is a visible change.
  • FightEntity.is_stale and Fight.prune_stale are removed; the
    replacement is Fight.is_stale(now, retention_s).
  • parsers/damage.py regexes are now generated from a verb list. Pinned
    character-identical by
    test_the_verb_alternations_are_unchanged_by_the_extraction.
  • New settings.dps block. Unknown keys are ignored, so old settings.json
    files load unchanged and pick up the defaults.

Tests

2068 passed (was 2045), ruff clean. New: tests/core/dps/test_tunables.py
(melee filtering, retention incl. 0 = never, window arithmetic + best-window
invalidation, session gate, live reconfigure) and
tests/ui/test_settings_dps_page.py (page present, round-trips, fires the
callback, reaches a real tracker end-to-end, doesn't widen the window's
minimum).

Also verified by driving the real app offscreen through the log file: an
opener's row survived 3 minutes of someone else's fight and the group retired
as a unit; the melee-only default dropped a 900 nuke and suppressed a
lone-nuke group; a real settings Apply moved the live tracker and the dps
column recomputed 8 → 25 as the window narrowed.

Known gap, not addressed here

DoT ticks (<mob> has taken N damage from your <spell>) are not parsed at
all
— zero hits across src/ and the fixture corpus — so damage-over-time
is absent from the meter regardless of these settings. Consistent now that
melee-only is the default, but still a real gap for necros and druids. Happy
to open an issue.

🤖 Generated with Claude Code

prokopto-dev and others added 2 commits August 10, 2026 07:48
…fought

EQTool's DPSWindowViewModel.ShouldRemove aged out each EntittyDPS on its own
last hit, and the port carried that over. The effect on a long fight is that
anyone who contributed early and then stopped — a stun on the pull, one
backstab, a wizard who nuked and sat — disappeared from the target's group
40s later while the mob was still up, so the meter under-reported who was
actually on the target.

Staleness now belongs to the Fight, not the FightEntity: the group is keyed
on the most recent hit from ANY attacker, so one player still swinging keeps
every other row on screen, and the group retires as a unit once the target
stops taking damage. Nothing that has landed on a target is pruned out from
under it.

Also drops the abs() from the retention comparison: a last-damage time in
the future (a log line stamped ahead of the wall clock) is no longer read as
40s stale.

The >20s gate on the session Best/Now/Last footer is untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The meter counted "<target> was hit by non-melee" as yours. That line names
no attacker, so the parser has nothing to credit but You — every proc and
nuke in range, including other players', landed in your row, and a lone nuke
opened a fight group on a mob you never swung at. A melee meter is what the
window is for, so melee_only defaults on.

The filter lives in the tracker, not the parser: DamageParser stays the
record of what the log said, so triggers and plugins subscribed to
DamageEvent still see spell damage either way. core/damagetypes.py holds the
shared vocabulary — its own module because parsers/damage.py builds its four
regexes from the verb list while core.dps filters by it, and importing the
parser to reach a frozenset costs ~100ms (it loads the master NPC list) on a
module the UI imports. is_melee() reads unknown types as NOT melee, so a
damage line the parser learns later cannot silently join a melee-only meter.

Adds Settings > DPS Meter over four tunables, all live via
Backend.apply_dps_settings -> FightTracker.configure (the tracker outlives
every settings window):

  - Melee damage only
  - Attacker dropoff    the fight-level retention, 0 = never
  - DPS averaging window  the trailing span the dps column divides by
  - Session stat minimum fight  the >20s gate on the Best/Now/Last footer,
                                which is why it reads 0 all session on trash

The averaging window is the only one needing more than an assignment: it is
carried per entity and re-stamped by tick(), which is what reaches fights
already running, and a change zeroes best_window_damage because a best-in-6s
is not comparable to a best-in-12s and the max-merge would otherwise keep
the stale larger number. Damage already counted is not re-filtered — the hit
list does not keep damage types.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@prokopto-dev prokopto-dev left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested

The fight-level retention fix and damage-verb extraction are well covered, and local validation passed (QT_QPA_PLATFORM=offscreen uv run pytest: 2068 passed; uv run ruff check .: clean). Two live-reconfiguration paths can leave displayed or accumulated DPS inconsistent, so this is not ready to merge yet.

Comment thread src/nparseplus/core/dps.py Outdated
Comment thread src/nparseplus/core/dps.py
prokopto-dev and others added 2 commits August 10, 2026 08:16
A time bomb, already armed on master and unrelated to this branch's DPS
work — it just happened to be the first CI run after the fuse burned.

The fake planner minted claim links with `expires = T0 + 24h`, where T0 is
the corpus's fixed 2026-08-09 12:00. But a claim's expiry is the one value
here compared against the REAL wall clock — InventoryUploadHandler
._current_claim reads datetime.now() — so once actual time passed
2026-08-10 12:00 local, every claim the fake handed out was born expired and
six tests failed: has_claim() False, claim_summary() empty, and staged==2
where a POST-then-PUT was expected.

That is why it went red in CI (12:06 UTC) while still passing four timezones
west, and why master's last run at 02:49 UTC was green.

Anchors the fixture's expiry to datetime.now() instead, matching the two
tests that already build past expiries that way. T0 stays correct for
everything else in the file: dump timestamps are log-clock values, only ever
compared to each other.

Verified by running the suite under TZ=UTC, which reproduces the six failures
before the change and passes after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… change

Two live-reconfiguration paths could show numbers no measurement produced.

A slain row is frozen, and that has to include its DIVISOR. tick() calls
update_trailing() for dead entities too, and the window was adopted BEFORE
the death guard returned — so the stored trailing_damage got divided by the
new denominator and a settled row's dps jumped the instant anyone touched
the setting (50 -> 150 on a 12s -> 4s change). The window is now adopted
after the guard, so a slain row keeps the one it froze under; Fight.add_damage
re-stamps live entities only.

The Best/Now footer aggregates are max-merged, so nothing can be recomputed
from them — the readings are gone, and pruned fights with them. Once the
measuring rules move, the retained maxima describe an experiment no longer
being run: a best-dps over 12s is not comparable to one over 4s (the same
reason best_window_damage is already invalidated), a best taken while spell
damage counted is unreachable under melee-only, and a best from a 6s fight
should not survive raising the minimum fight length past it. configure() now
clears them when a measurement rule actually changes.

Deliberately NOT triggering that reset: fight_retention_s, which decides how
long a row is displayed and never what a reading measured; a no-op Apply,
since the settings window fires on every Apply whether or not anything moved;
and last_session, which the user moved aside with end_session() and is a
record rather than a live measurement. All three are pinned by tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@prokopto-dev

Copy link
Copy Markdown
Owner Author

Both findings were real and are fixed in c3e96e7. Thanks — the first one I could reproduce as a visible number change, which made it easy to pin.

1. Frozen rows keeping their divisor (dps.py) — fixed

Reproduced before fixing:

slain row dps @12s window:        50
slain row dps after window->4s:  150

The window was adopted before the death guard returned, so the frozen trailing_damage got divided by the new denominator. A settled fight's dps jumped the instant anyone touched the setting.

The window is now adopted after the guard, so a slain row keeps the window it froze under — the freeze forbids recomputing the numerator, so the denominator has to be frozen with it. Fight.add_damage re-stamps live entities only (elif entity.death_time is None), which also removed a redundant update_trailing call on the hot path.

Pinned by test_a_slain_row_keeps_the_window_it_died_under and, so the fix doesn't overshoot into freezing live fights, test_a_live_row_still_follows_the_new_window.

2. Session aggregates measured under old rules — fixed by reset

Recompute isn't available: best/current_session are max-merged, so the readings they were built from are gone, and pruned fights with them. configure() now clears them when a measurement rule actually changes.

The rule I used is "did this change what a reading means", which is the same argument already applied to best_window_damage: a best-dps over 12s isn't comparable to one over 4s, a best taken while spell damage counted is unreachable under melee-only, and a best from a 6s fight shouldn't survive raising the minimum fight length past it.

Three things deliberately do not trigger the reset, each with a test:

  • fight_retention_s — decides how long a row is displayed, never the value of any reading, so nudging the dropoff timer must not cost the user their session stats (test_changing_the_dropoff_timer_keeps_the_footer).
  • A no-op Apply — the settings window fires on_dps_changed on every Apply whether or not anything moved, so comparing before/after is load-bearing; without it, opening Settings and clicking Apply would silently wipe the session (test_applying_the_same_values_keeps_the_footer).
  • last_session — the user moved it aside with end_session(), making it a record rather than a live measurement (test_a_completed_session_record_survives_a_rule_change).

Also on this push (unrelated to your findings)

d22d630 fixes the red CI, which was not caused by this branch. The p99planner fixture minted claims with expires = T0 + 24h off the corpus's fixed T0 = 2026-08-09 12:00, but claim expiry is compared against the real wall clock in _current_claim. Once actual time passed 2026-08-10 12:00 local, every claim was born expired. That's why it went red at 12:06 UTC while still passing four timezones west, and why master's 02:49 UTC run was green — the same fuse is armed on master. Anchored to datetime.now(), matching the two tests that already build past expiries that way; verified by reproducing the six failures under TZ=UTC and confirming they pass after.

Full suite 2074 passed, ruff clean, and green under TZ=UTC and TZ=Pacific/Kiritimati (UTC+14) as well as local.

@prokopto-dev

Copy link
Copy Markdown
Owner Author

CI is green — the macOS failure was a pre-existing flake, not this branch

Recording the diagnosis so nobody has to redo it.

The macOS job died with exit 139 (SIGSEGV), not a test failure. Re-running the same commit with no changes went green (1m36s), and Ubuntu/Windows passed on that commit both times.

Mechanism. The fault traceback's top frame is Garbage-collecting, inside tests/ui/test_pluginmanager.py's host fixture. Immediately before the crash the progress line reads ...........F — test #53, test_update_all_updates_every_same_source_plugin, had failed.

That test drives a threaded install:

with qtbot.waitSignal(page._install_finished, timeout=5000):
    page._update_all_button.click()
qtbot.waitUntil(lambda: not page._batch_active, timeout=5000)

ui/pluginmanager.py starts bare daemon threads at lines 451, 598 and 963 (plugin-update-check, plugin-install, registry-fetch) that emit signals back into the page, and nothing joins them. On a loaded runner the 5s wait times out → the test fails → the plugin-install thread is still live, holding the page. The next test's fixture builds a backend, GC collects the previous test's Qt objects out from under that thread, and the process segfaults.

This is the same class as #61 (9dde204), which fixed one source of it — a leaked 12s update-check QTimer starting live HTTPS fetches — and whose message called the shot exactly:

Sockets crossing another test's teardown is what segfaulted; whether it lands depends on suite timing, which is why this only started biting as the suite grew.

The network half is already closed by the session-scoped no_live_registry_fetches guard in tests/conftest.py. This is the other half: the worker threads themselves. This branch adds 29 tests, which shifted suite timing enough to land it — it didn't introduce it.

I confirmed locally that nothing leaks while all 55 tests pass (a probe hook joining those thread names found nothing), which fits: the thread only survives when a test times out.

Not fixing it here — it is unrelated to the DPS work and this PR already carries one unrelated CI fix (d22d630). The targeted fix is test-side, in the shape of #61: join any live plugin-install / plugin-update-check / registry-fetch thread at teardown before the next test starts. Happy to open an issue or a follow-up PR — say which you'd prefer.

@prokopto-dev
prokopto-dev merged commit 35c41cb into master Aug 10, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant