Let the outlaw (player killer) state actually wear off again

[nolt]

Summary

The remaining time of a hero or outlaw state was never counted down. A player killer therefore
stayed an outlaw forever — in practice a permanent 2nd stage outlaw that only /pkclear could
remove — no matter how long the player stayed online or how many monsters they hunted. This fixes
the countdown and restores the classic timings around it.

The symptom

1. Kill another player three times (no self-defense, no duel, no rival guild), so the character
   becomes a 2nd stage outlaw.
2. Stay online for hours, hunt any amount of monsters.
3. The character never returns to the 1st stage, let alone to normal. Its name stays red/black,
   the drop and experience penalties stay, and Character.StateRemainingSeconds in the database
   keeps growing instead of shrinking.

What did work: escalating into the states, the IUpdateCharacterHeroStatePlugIn notification to
the observers, and /pkclear, which is why this stayed unnoticed for so long. Our servers are
populated with offline/bot players which occasionally kill each other, and every single one of
them accumulated a permanent outlaw state.

Root cause

Two defects in Player.RegenerateHeroStateAsync() (src/GameLogic/Player.cs), which is called
from RegenerateAsync() on the recovery timer:

• The elapsed time was calculated as this._lastRegenerate.Subtract(DateTime.UtcNow). The field
  holds the previous tick, so the result is negative — despite the variable being named
  secondsSinceLastRegenerate. Subtracting it made StateRemainingSeconds grow by exactly the
  elapsed time, so the countdown stood still (in net terms) forever.
• The whole method body was guarded by if (currentCharacter?.StateRemainingSeconds > 0). Killing
  monsters reduces the remaining time by the monster's level
  (AttackableNpcBase.OnDeathAsync()), which can push it to zero or below between two ticks.
  From that moment on, the guard was false and the state change was never reached again.

Together they meant that the state could only ever escalate. git blame dates both to
db15dea508 (2021-11-13); nothing else in the code base lowers Character.State except
/pkclear and the GM command /pk.

The fix

RegenerateHeroStateAsync() is rewritten with early returns:

• The elapsed time is now DateTime.UtcNow.Subtract(this._lastRegenerate). It is subtracted in
  whole seconds, and the remaining fraction is kept for the next tick. Rounding every tick made the countdown speed depend on GameConfiguration.RecoveryInterval
  (at 500 ms it never counted down at all).
• A remaining time which ran below zero steps the state one step towards normal and carries the
  whole surplus over into the next step, so a hunting spree can drop more than one step at a time.
• HeroState.New (a freshly created character, which has no running timer) is now excluded
  explicitly. Previously the > 0 guard did that by accident, and without the guard such a
  character would have been promoted towards HeroState.Hero.
• When a character enters the world, its remaining time is limited to what its state can
  legitimately have: one state duration, and for a 2nd stage outlaw one duration per kill after the
  one which reached that stage. Characters of servers which ran with the broken countdown would
  otherwise carry a remaining time which grew by all the time they spent online.
• Returning to HeroState.Normal also resets PlayerKillCount, which otherwise kept charging the
  /pkclear price for kills the player had already atoned for.

The timings follow the classic (pre-Season 9) behavior again, where they were wrong before:

• An outlaw state lasts three hours instead of one (PlayerKillerStateDuration).
• Stepping up to the next outlaw state restarts that clock instead of adding to it; only kills
  which can no longer escalate the state (i.e. as a 2nd stage outlaw) stack on top of the
  remaining time. Three kills in a row therefore take 3 h + 3 h + 3 h to wear off, and every
  further kill adds another three hours.
• A Math.Max makes sure a kill can never shorten an already longer remaining time.
• Hero states keep their one hour (HeroStateDuration).

Reducing the remaining time by the level of a killed monster is left as it is, and deliberately
stays available on every map and in every outlaw state: that matches the classic rule, where the
penalty was shortened by the monster's level in seconds anywhere, long before later seasons
restricted it to Vulcanus. The only change in AttackableNpcBase is a clarifying comment.

Since the timer is only advanced while the player is in the game — _lastRegenerate is reset when
entering the world — offline time still does not count towards wearing the state off.

Testing

• dotnet build src/GameLogic/MUnique.OpenMU.GameLogic.csproj -p:ci=true in the
  mcr.microsoft.com/dotnet/sdk:10.0 container: 0 errors, and no analyzer warning in either of
  the two touched files.
• Not verified at runtime: this has not been run on a game server yet, so the state transitions
  were reviewed by reading the code only.
• No unit tests added. There is currently no test coverage for the hero state at all; the only
  related test is PKClearChatCommandPlugInTest.

Possible follow-ups (not in this change)

• The three hours and one hour could be made configurable (e.g. in GameConfiguration) instead of
  being constants in Player.
• WarpAction does not check the hero state at all, so an outlaw can still use the warp list. The
  classic rule forbids warping as a 2nd stage outlaw (the original client protocol even has a
  MAPMOVE_FAILED_MURDERER result), and later seasons charge 50x the zen instead.

[sven-n]

Review

Verified the diff against a clone of master. The core fix is correct.

_lastRegenerate is assigned in the finally of RegenerateAsync (Player.cs:942), so it genuinely holds the previous tick — the old _lastRegenerate.Subtract(DateTime.UtcNow) was negative and -= made the timer grow. Both the sign flip and dropping the > 0 guard are right, and the guard really was the second half of the bug: AttackableNpcBase.OnDeathAsync can push StateRemainingSeconds to ≤ 0 between two ticks, permanently wedging the old code.

Escalation in AfterKilledPlayerAsync checks out too: a hero jumps straight to PlayerKillWarning, the Math.Max prevents a kill from shortening a longer timer, and PK2 kills stack. Three kills → 9 h total, as described. The AttackableNpcBase change is comment-only and accurate.

Three inline comments below (one behavioural, one latent/pre-existing, one description nit), plus one general point:

Existing inflated StateRemainingSeconds values aren't normalized

Nothing clamps the values the old bug inflated. On the servers described in the PR — where "every single one" of the offline/bot players accumulated a permanent outlaw state — those characters now carry countdowns that may represent days of online time per state step. The fix will look like it didn't work on exactly the servers that motivated it.

A clamp on character load (e.g. Math.Min(remaining, stateDuration)) or a one-off migration would avoid that. Happy to treat it as a follow-up if you'd rather keep this PR focused.

Verdict

Sound fix for a real bug. I'd merge it after the first inline comment; the rest are defensible as follow-ups.

---

Generated by Claude Code

[sven-n]

Small correction to the PR description, which says HeroState.New "is now excluded explicitly".

New is 0 and Normal is 3, so this else branch would move a New character towards Hero, away from Normal. What actually prevents that is the State < HeroState.Normal && StateRemainingSeconds <= 0 early return at line 1424 — i.e. the same kind of incidental protection the old > 0 guard provided.

No behavioural bug: a freshly created character has no running timer, so it always hits that return. But an explicit State == HeroState.New check (or choosing --/++ from a direct comparison) would make the intent match the description.

---

Generated by Claude Code

[nolt]

Made explicit in e6a9dfd, and the PR description is updated.

[nolt]

Thanks for the thorough review! Pushed two commits:

e6a9dfd — review fixes

• Surplus carry-over (Player.cs:1456): applied your suggestion, the Math.Max(…, 0) is gone. A value below zero is picked up by the next tick, which steps down again. This stays safe for hero states: only outlaw states get their time reduced by killed monsters, so a hero state never ends up below zero and can't get stuck.
• HeroState.New: it's now excluded explicitly in the first guard, and the incidental "hero state without a running timer" check is removed, so the code matches the description.
• Inflated StateRemainingSeconds: included here instead of a follow-up, since those servers are the reason for this PR. The remaining time is limited when the character enters the world. I didn't use a plain Math.Min(remaining, stateDuration) though, because that would cut legitimately stacked time of a 2nd stage outlaw (every kill adds three hours). The limit is one state duration, and for a 2nd stage outlaw 3 h × (PlayerKillCount − 2), which is the most the kills could have added. It logs an information entry when it has to limit a value. One side effect: a state set by the GM command /pk with a count higher than the limit gets limited at the next login.

c6248a6 — countdown independent of the recovery interval

• Separate commit, as it's pre-existing behavior, so it's easy to review or drop on its own. Only whole seconds are subtracted now, and the fraction is kept in a field on Player for the next tick (memory only, reset when entering the world and while there's no state to count down). I kept the countdown instead of an absolute expiry DateTime, because the state should only wear off while the player is online, and it doesn't need a schema change.