tracecheck: threat_off_tank counts only mobs that are on the holder, with a live tank (ties go to the tank)

README.md

@@ -1459,7 +1459,7 @@ the others look at what only the agent sees (orders, rest gates, walks, the feed
 | `rest_share` | the rest gates took more than this share of the run | 40 % | the run is shorter than 60 s |
 | `stale_party` | a mana user's power did not change for this long while someone else's health or power did (the party data stopped updating); full mana and dead members are ignored | 60 s | no frames with a mana user |
 | `walk_stops` | the lead stood still (at least 1.5 s) in the middle of a walk of the plan more often than this per 100 yd walked; the last 3 s of a walk are arriving | 4 stops / 100 yd | under 100 yd walked |
-| `threat_off_tank` | a party member other than the tank held the top threat of a living mob during a pull for longer than this; the tank is `header.tank`, else the `tank` role, else whoever holds the top threat most often | 4 s | no threat data (group runs have none) |
+| `threat_off_tank` | a mob in combat is on a party member other than the tank — its `tg` is that member, or the member ties the tank's threat — during a pull for longer than this; a dead tank, a mob with no target (polymorphed, fleeing, evading) and a mob out of combat do not count; the tank is `header.tank` (an observed run names its lead), else the `tank` role, else whoever holds the top threat most often; the detail gives the spans per tank-combat-hour | 4 s | no threat data (group runs have none) |
 | `deaths` | any death or wipe | 0 | never |
 | `order_refusals` | more than this share of the orders was refused (`err` replies and `refusal` events) | 20 % | fewer than 3 orders |
 | `feed_stalls` | the longest gap between two feed frames is over this | 10 s | no feed frames (group runs) |

internal/dungeon/observe_test.go

@@ -391,6 +391,26 @@ func TestObserveWipeLatchReArmsBelowThreeDead(t *testing.T) {
 	}
 }
 
+// An observed run is driven by the server's run leader, which is the lead bot: the trace header names it as the tank, so the threat
+// oracle does not have to guess the tank from the frames. A driven run keeps the tank it was given (TestTraceOfARemoteRun).
+func TestObserveTraceNamesTheLeadAsTheTank(t *testing.T) {
+	g := newFakeLeader(t, happyScript()...)
+	dir := t.TempDir()
+	rep, err := runRemote(t, g, observePlan(), observeOpts(ObserveSpec{}), func(c *Config) {
+		c.Trace = tracerun.Config{On: true, Dir: dir, RunID: "r-observe-tank"}
+	})
+	if err != nil || rep.Trace == nil || rep.Trace.Path == "" {
+		t.Fatalf("%v %+v", err, rep.Trace)
+	}
+	f, err := trace.Read(rep.Trace.Path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if f.Header.Lead != "Tanky" || f.Header.Tank != f.Header.Lead {
+		t.Fatalf("header lead %q tank %q, want both Tanky", f.Header.Lead, f.Header.Tank)
+	}
+}
+
 // A party with one bot alive is no wipe: four dead of five count nothing.
 func TestObserveCountsNoWipeWhileABotLives(t *testing.T) {
 	g := newFakeLeader(t, wipeScript()...)

internal/dungeon/runtrace.go

@@ -393,7 +393,13 @@ func (d *RemoteDriver) traceBegin(fromLeg int) {
 	}
 	// The feed has facing and the bot's target but no movement flag or class, and it has threat lists.
 	caps := []string{trace.CapFacing, trace.CapThreat, trace.CapOrders, trace.CapWalks, trace.CapPartyList}
-	hdr := trace.Header{Map: d.plan.Map, Plan: d.plan.Name, FromLeg: fromLeg, Lead: d.opt.Lead, Tank: d.opt.Tank, Options: opts, Caps: caps}
+	// An observed run is driven by the server's run leader, which is the lead bot: the header names it as the tank, or the threat check
+	// would have to guess it from the frames.
+	tank := d.opt.Tank
+	if d.observing() {
+		tank = d.opt.Lead
+	}
+	hdr := trace.Header{Map: d.plan.Map, Plan: d.plan.Name, FromLeg: fromLeg, Lead: d.opt.Lead, Tank: tank, Options: opts, Caps: caps}
 	hdr.Meta = runMetaHeader(d.cfg.Trace.Meta, d.opt.RunMeta)
 	d.tr.Begin(hdr)
 	if !d.tr.On() {

internal/tracecheck/tracecheck.go

@@ -406,11 +406,21 @@ func threatOffTank(f *trace.File, c Config) trace.Check {
 	var worstWho string
 	var ev []int64
 	spans := map[string]bool{}
+	combatFrames := 0
 	for _, fr := range f.Frames {
+		alive, inCombat := tankState(fr, tank)
+		if inCombat {
+			combatFrames++
+		}
 		if !inPull(fr.T) {
 			open2 = map[string]int64{}
 			continue
 		}
+		// A tank that is dead (or not in the frame) holds nothing: its death is not threat lost to a non-tank.
+		if tank != "" && !alive {
+			open2 = map[string]int64{}
+			continue
+		}
 		seen := map[string]bool{}
 		for _, h := range fr.Mobs {
 			if len(h.Threat) == 0 || h.HPPct <= 0 {
@@ -421,6 +431,12 @@ func threatOffTank(f *trace.File, c Config) trace.Check {
 			if tank == "" || !member[holder] || holder == tank {
 				continue
 			}
+			// The mob must be on the holder: a mob that still swings at the tank, or that has no target (polymorphed, fleeing,
+			// evading), is not threat lost to a non-tank. A tie with the tank is the tank's: the core keeps the tie in insertion
+			// order, so the holder is listed first although the mob is still the tank's within the 110 %/130 % rule.
+			if !h.Combat || h.Target != holder || tiedWithTank(h.Threat, tank) {
+				continue
+			}
 			seen[h.G] = true
 			from, ok := open2[h.G]
 			if !ok {
@@ -449,10 +465,46 @@ func threatOffTank(f *trace.File, c Config) trace.Check {
 	if worstWho != "" {
 		d = fmt.Sprintf("%s held the top threat of a mob for %.0f s (tank %s)", worstWho, worst, f.Name(tank))
 	}
+	// The rate makes runs of different lengths comparable: the episodes per hour the tank spends in combat.
+	interval := f.Header.IntervalMS
+	if interval <= 0 {
+		interval = 1000
+	}
+	combatS := float64(combatFrames) * float64(interval) / 1000
+	if combatS > 0 {
+		d += fmt.Sprintf("; %d spans in %.0f s of tank combat, %.0f per tank-combat-hour", len(spans), combatS, float64(len(spans))*3600/combatS)
+	}
 	fail := worst > c.ThreatOffTankS
 	return trace.Check{ID: id, Name: name, Verdict: verdict(fail), Count: countIf(fail, len(spans)), Value: round1(worst), Threshold: c.ThreatOffTankS, Unit: "s", Detail: d, Evidence: ev}
 }
 
+// tankState reads the tank in a frame: whether it is alive and whether it is in combat. A frame that does not list the tank says neither.
+func tankState(fr trace.Frame, tank string) (alive, combat bool) {
+	if tank == "" {
+		return false, false
+	}
+	for _, b := range fr.Bots {
+		if b.G == tank {
+			return b.Alive, b.Alive && b.Combat
+		}
+	}
+	return false, false
+}
+
+// tiedWithTank says whether the tank sits level with the top of a threat list: the values are raw threat as a share of the top, so an
+// equal share is a tie, and a tie between the tank and the holder counts as the tank's.
+func tiedWithTank(thr []trace.Threat, tank string) bool {
+	if tank == "" || len(thr) < 2 {
+		return false
+	}
+	for _, t := range thr[1:] {
+		if t.G == tank {
+			return t.Pct == thr[0].Pct
+		}
+	}
+	return false
+}
+
 // ---- deaths and wipes ----
 
 func deathsWipes(f *trace.File) trace.Check {

internal/tracecheck/tracecheck_test.go

@@ -203,6 +203,33 @@ func mob(g string, hp int, top string) trace.Mob {
 	m := trace.Mob{G: g, HPPct: hp, Combat: true}
 	if top != "" {
 		m.Threat = []trace.Threat{{G: top, Pct: 100}}
+		m.Target = top // the mob is on the member that holds it
+	}
+	return m
+}
+
+// threatTrace is a 40 s traced fight with a pull from 0 s to 30 s: one feed frame a second from Tank, Healer and Mage, with the bots and
+// mobs that tick builds for that second.
+func threatTrace(tick func(s int64) (bots []trace.Bot, mobs []trace.Mob)) *build {
+	b := newTrace(40)
+	b.f.Header.Tank = "Tank"
+	b.ev(0, "pull_start").ev(30_000, "pull_end")
+	for s := int64(0); s <= 40; s++ {
+		bots, mobs := tick(s)
+		fr := b.frame(s*1000, "feed", bots...)
+		fr.Mobs = mobs
+	}
+	return b
+}
+
+// mageHolds is the fixture's off-tank stretch: the mage is top of the mob from 10 s to 20 s.
+func mageHolds(s int64) bool { return s >= 10 && s < 20 }
+
+// threatMob is the fixture's mob: the holder is top with 100, and the tank is second with tankPct when that is above zero.
+func threatMob(holder string, tankPct int) trace.Mob {
+	m := trace.Mob{G: "0xf1", HPPct: 80, Combat: true, Target: holder, Threat: []trace.Threat{{G: holder, Pct: 100}}}
+	if tankPct > 0 {
+		m.Threat = append(m.Threat, trace.Threat{G: gTank, Pct: tankPct})
 	}
 	return m
 }
@@ -275,6 +302,112 @@ func TestThreatOffTheTank(t *testing.T) {
 	expect(t, get(t, Score(none.f, Defaults()), IDThreat), trace.NA)
 }
 
+// A mob that keeps swinging at the tank is not threat lost to the mage on top of its list.
+func TestThreatIgnoresAMobThatIsNotOnTheHolder(t *testing.T) {
+	b := threatTrace(func(s int64) ([]trace.Bot, []trace.Mob) {
+		m := threatMob(gTank, 0)
+		if mageHolds(s) {
+			m = threatMob(gMage, 0)
+		}
+		m.Target = gTank
+		return members(), []trace.Mob{m}
+	})
+	c := get(t, Score(b.f, Defaults()), IDThreat)
+	expect(t, c, trace.Pass)
+	if c.Value != 0 || c.Count != 0 {
+		t.Fatalf("%+v", c)
+	}
+}
+
+// A mob with no target (polymorphed, fleeing, evading) is not on the mage either.
+func TestThreatIgnoresAMobWithNoTarget(t *testing.T) {
+	b := threatTrace(func(s int64) ([]trace.Bot, []trace.Mob) {
+		m := threatMob(gTank, 0)
+		if mageHolds(s) {
+			m = threatMob(gMage, 0)
+			m.Target = ""
+		}
+		return members(), []trace.Mob{m}
+	})
+	expect(t, get(t, Score(b.f, Defaults()), IDThreat), trace.Pass)
+}
+
+// A mob out of combat does not hit anyone.
+func TestThreatIgnoresAMobThatIsNotInCombat(t *testing.T) {
+	b := threatTrace(func(s int64) ([]trace.Bot, []trace.Mob) {
+		m := threatMob(gTank, 0)
+		if mageHolds(s) {
+			m = threatMob(gMage, 0)
+			m.Combat = false
+		}
+		return members(), []trace.Mob{m}
+	})
+	expect(t, get(t, Score(b.f, Defaults()), IDThreat), trace.Pass)
+}
+
+// A dead tank loses no threat: its death is not a span. The same stretch with the tank alive fails, so the frames are counted.
+func TestThreatIgnoresTheFramesOfADeadTank(t *testing.T) {
+	mk := func(dead func(s int64) bool) *build {
+		return threatTrace(func(s int64) ([]trace.Bot, []trace.Mob) {
+			bots := members()
+			if dead(s) {
+				bots[0].Alive = false
+			}
+			m := threatMob(gTank, 0)
+			if mageHolds(s) {
+				m = threatMob(gMage, 0)
+			}
+			return bots, []trace.Mob{m}
+		})
+	}
+	// The tank dies at 10 s and is back at 20 s: the mage held the mob on the way out, not while the tank lay dead.
+	dead := func(s int64) bool { return mageHolds(s) }
+	expect(t, get(t, Score(mk(dead).f, Defaults()), IDThreat), trace.Pass)
+	alive := get(t, Score(mk(func(int64) bool { return false }).f, Defaults()), IDThreat)
+	expect(t, alive, trace.Fail)
+	if alive.Value != 9 || alive.Count != 1 {
+		t.Fatalf("%+v", alive)
+	}
+}
+
+// The values are raw threat as a share of the top, and a tie between the mage and the tank is the tank's: the core keeps the tie in
+// insertion order. One point below the mage is not a tie.
+func TestThreatCountsATieWithTheTankAsTheTanks(t *testing.T) {
+	mk := func(tankPct int) *build {
+		return threatTrace(func(s int64) ([]trace.Bot, []trace.Mob) {
+			m := threatMob(gTank, 0)
+			if mageHolds(s) {
+				m = threatMob(gMage, tankPct)
+			}
+			return members(), []trace.Mob{m}
+		})
+	}
+	expect(t, get(t, Score(mk(100).f, Defaults()), IDThreat), trace.Pass)
+	c := get(t, Score(mk(90).f, Defaults()), IDThreat)
+	expect(t, c, trace.Fail)
+	if c.Value != 9 || c.Count != 1 {
+		t.Fatalf("%+v", c)
+	}
+}
+
+// The detail carries the episodes per hour the tank spends in combat: one 9 s span in 41 s of combat is 88 per hour.
+func TestThreatReportsTheRatePerTankCombatHour(t *testing.T) {
+	b := threatTrace(func(s int64) ([]trace.Bot, []trace.Mob) {
+		bots := members()
+		bots[0].Combat = true
+		m := threatMob(gTank, 0)
+		if mageHolds(s) {
+			m = threatMob(gMage, 0)
+		}
+		return bots, []trace.Mob{m}
+	})
+	c := get(t, Score(b.f, Defaults()), IDThreat)
+	expect(t, c, trace.Fail)
+	if !strings.Contains(c.Detail, "1 spans in 41 s of tank combat, 88 per tank-combat-hour") {
+		t.Fatalf("%+v", c)
+	}
+}
+
 func TestDeaths(t *testing.T) {
 	expect(t, get(t, Score(newTrace(100).f, Defaults()), IDDeaths), trace.Pass)
 	c := get(t, Score(newTrace(100).ev(5000, "death", "name", "Mage").f, Defaults()), IDDeaths)