Maximum HP and MP stop being a login-time snapshot - #2321
Merged
Conversation
Max HP was computed once, in SelectPacketHandler, from class and level, and
nothing ever recomputed it. Three things followed, none of which raise anything:
* the Hp and Mp fields of the worn pieces counted for nothing - the parser
has always read Item.Hp and nobody looked at it;
* BCard type 33 "Maximum HP/MP", which sits on 706 items and is the second
most widespread effect in the files, had no handler at all;
* on level-up the full heal topped the player up to the *previous* level's
maximum, and the new level's health only appeared after a relog.
VitalityService answers "what is this character's maximum right now" and is
called at the five points where the answer can change: login (after the
inventory exists to look at), level-up (before the full heal, or the heal tops
up to the old number), equip, unequip, and a buff arriving or expiring.
BCard.dat, type 33, is what settles the order:
11: Maximum HP is increased by %s. 31: Increases maximum HP by %s%%.
21: Maximum MP is increased by %s. 41: Increases maximum MP by %s%%.
51: Maximum HP and MP are increased.
For 31 and 41 the file adds "(Only used by buffs.)", so the percentages apply
to the total that already includes the equipment, not to the base alone: a
twenty percent buff is twenty percent of what is worn.
Two things worth a reviewer's attention:
* HitQueue.TryApplyHit is now async and awaits the buff application. It was
fire-and-forget; the recomputation that follows has to see the effect
already applied. The worker already serialised per target, so this orders
work that was happening anyway.
* Taking off a piece that gave HP lowers the maximum, so current HP is
clamped under it - otherwise the client draws the bar past its own edge and
the percentage in `su` passes a hundred.
The tests equip a piece whose only contribution is HP, and a piece whose only
contribution is a type 33 card, and read MaxHp back. Removing either
contribution fails four of them.
Depends on IEquipmentStatsService (NosCoreIO#2293).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (10)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe change adds ChangesVitality recalculation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change recalculates maximum HP and MP when equipment or buffs change, with targeted tests and a warning-free build reported; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant WearHandler
participant VitalityService
participant EquipmentStatsService
participant PlayerBundle
WearHandler->>VitalityService: RefreshAndNotifyAsync(character)
VitalityService->>EquipmentStatsService: Resolve equipment stats
EquipmentStatsService-->>VitalityService: Equipment HP/MP values
VitalityService->>PlayerBundle: Update maxima and current HP/MP
VitalityService->>PlayerBundle: Send updated stat packet
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
erwan-joly
reviewed
Aug 27, 2026
| request.Skill.BCards, request.Origin) | ||
| .ConfigureAwait(false); | ||
|
|
||
| // Awaited and not fire-and-forget any more: the maximum HP below is read from |
Collaborator
There was a problem hiding this comment.
again too much comments
Contributor
Author
There was a problem hiding this comment.
Done in 103a9e7: 89 lines of my comments removed across the PR, and the 9 ConfigureAwait(false) I had added are gone too. What is left is one line per non-obvious spot, nothing more.
Two of your existing comments were touched by my first commit; I put one back word for word (// blow, not race it.) and removed the other, because the code now awaits and the sentence said fire-and-forget was fine — a wrong comment is worse than none.
Build clean, 1004 tests green.
erwan-joly
reviewed
Aug 27, 2026
| await buffService | ||
| .ApplySkillBuffAsync(target, request.Skill.SkillVnum, request.Skill.Duration, | ||
| request.Skill.BCards, request.Origin) | ||
| .ConfigureAwait(false); |
Collaborator
There was a problem hiding this comment.
we don't need the configureawait
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 28, 2026
erwan-joly
pushed a commit
that referenced
this pull request
Aug 28, 2026
…tionalTypes (#2333) #2325 deleted `AdditionalTypes.cs` and replaced the (type, subtype) pair with a single `BCardEffect` key. #2321 merged right after it, carrying a `VitalityService` written against the old API, and the two are individually green and broken together: `error CS0246: AdditionalTypes could not be found`, fourteen times, on every build since. Ported: `card.Effect()` instead of casting `card.SubType` to a per-type enum, and the flattened members - `BCardEffect.QuestAdditionalHpPercent`, `BCardEffect.MaxHpmpMaximumHpIncreased` and the rest. The two switches become one because the key already carries the type, with the same early filter in front so `ScaleByLevel` is still only called for the two types that matter. The test moved the same way and no longer names a subtype by hand: `BCardEffect.MaxHpmpMaximumHpIncreased.Type()` / `.SubType()`. Behaviour is unchanged - every case maps one to one. Tested: build 0 errors 0 warnings; `dotnet test --filter TestCategory!=OPTIONAL-TEST` green, 1013 passed. Not played: the NosCore servers are not run here. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
denislauri1999
added a commit
to denislauri1999/NosCore
that referenced
this pull request
Aug 28, 2026
Same reason given on NosCoreIO#2321: they are not useful in this codebase. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
erwan-joly
pushed a commit
to denislauri1999/NosCore
that referenced
this pull request
Aug 29, 2026
Same reason given on NosCoreIO#2321: they are not useful in this codebase. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
erwan-joly
pushed a commit
that referenced
this pull request
Aug 29, 2026
* feat(combat): the cards a skill inflicts, which nothing read
Type 25 is the most widespread effect in the game - 1344 skills declare one -
and NosCore read none of them. A skill does not carry the effect: it carries a
BCard saying "with N% chance, apply Card number M", and M is a real entry of
Card.dat with its own duration and its own BCards.
11: Has a %s%% probability of causing [%s].
12: There is a %s%% chance that %s will be removed.
FirstData is the percentage and SecondData is the card id. The names settle it:
Star Attack declares 60% of card 7 "Blackout", Hit of Rage 2% of card 4 "Anger",
Blood Oath 100% of card 17 "Blood Oath". 1340 of the 1341 ids exist in the file.
Reading it the other way round is easy and quiet. With 2759 cards spread over
ids 0 to 4440, FirstData is *also* a valid card id 1278 times out of 1341, so
checking whether the number exists gives the wrong answer with a crushing
majority. What separates them is the shape of the columns: 32 distinct values on
one side, 780 on the other. An id does not repeat 717 times; a probability does.
WHO RECEIVES THE CARD is not in the BCard, and the files cannot say. Battle Cry
declares "100% of card Battle Cry" and is a self-buff; Suppress declares its
card with the same structure and is a debuff on the enemy. The difference is in
the skill's TARGET section, which the file writes as bare numbers with no
sentence explaining them.
So this hangs off a blow that has landed, where the question does not arise: the
entity that took the damage is the one the card goes on. Skills that damage
nobody never reach the path, so self-buffs are outside it by construction rather
than by omission - 704 of the 1341 declarations, the ones with TargetType 0.
BuffService.ApplyAsync already did the right thing, `bf` packet included, and
had no callers. CardCatalog is the way back from a card id to the Card, needed
because NosCore.Data keeps the navigation collections internal.
HitQueue.TryApplyHit becomes async: the effect a blow carries has to follow it,
not race it.
Swapping the two fields fails three of the tests.
* test(combat): the wiring itself, not only the service behind it
The two HitQueue tests passed an anonymous IInflictedCardService mock, so
removing the call from HitQueue, or inverting its condition, broke nothing. The
service had seven tests and the step that reaches it had none - which is the
same shape of hole the PR is about.
Named the mock in both. A landed hit now asserts InflictAsync ran once, with the
target and the skill's BCards; a killing hit asserts it never ran, because
poisoning a corpse costs a packet and a buff icon on something about to stop
existing.
Removing the call fails one test, inverting the condition fails two.
* test(combat): that the hit waits for the card, not just that it asks for it
A Verify passes whether the call is awaited or fired and forgotten - Moq answers
with a completed Task either way - so the previous test did not defend the one
property TryApplyHit was made async for. And fire-and-forget is exactly what
this used to be.
The mock now returns a task that stays pending, and the hit must not report
itself finished while it does.
The wait can only fail in the safe direction: if the call is awaited the hit can
never complete, so the delay always wins. A loaded machine can make this pass
when it should not, never the reverse.
Putting the `_ =` back fails it.
* test(combat): wait for the call instead of trusting a delay, and open the gate in a finally
Two holes in the test added last commit, both pointed out in review.
It relied on a delay alone, so it also passed on a machine slow enough that the
worker never reached the call at all - a pass for the wrong reason, which is the
kind this PR is about. The mock now signals on entry and the test waits for that
signal first; only then does it assert the hit is still unfinished.
And the gate is released in a finally. A failed assertion used to leave the
queue's worker parked on a task nobody would ever complete, for the rest of the
run.
Putting the `_ =` back still fails it.
* review: the inflicted cards move into IBuffService, and the comments go
Two things asked for on the PR.
`InflictedCardService` is gone: its loop is `BuffService.InflictCardsAsync`, next
to `ApplySkillBuffAsync`, which is the other skill-originated entry point. That
drops an interface, a class and a constructor parameter on HitQueue, and the two
calls it made are now local ones.
The tests changed shape because of it. They used to verify calls on a mocked
IBuffService, which is not available once the code IS IBuffService, so they now
assert the real thing through GetActiveBuffs and HasBuff, on a MonsterComponentBundle
carrying just the two components the buff path reads. Red measured first, by
swapping FirstData and SecondData in the loop: 3 failed of 7.
And every comment is gone, here and in the tests. What they said - that FirstData
is the percentage and SecondData the card id, that 717 of the 1341 declarations
say 100 so the comparison has to be "less than", and that one declared id is not
in Card.dat - is in the PR description.
Build clean, 999 tests green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* review: the last comment on the hit queue
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* review: drop the ConfigureAwait(false) calls
Same reason given on #2321: they are not useful in this codebase.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <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
2 participants
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.
Maximum HP and MP were computed once, at login, from class and level. Two things counted for nothing:
Item.Hp, and nothing consumed it;A thousand-HP armour gave zero, and nothing reported it: the bar the player saw was the right one for their level, so the defect was invisible.
VitalityServicerecomputes the maximum from class and level, plus equipment, plus active effects, and notifies. It runs where the inputs change: on wear/unwear, and after a skill's buff has been applied.The order is read from the files, not chosen.
BCard.dattype 33 subtypes 31 and 41 (Increases maximum HP/MP by %s%%) add "(Only used by buffs.)", which settles it: the percentages apply to the total that already includes the equipment, not to the base alone. Applying them first would give lower numbers, wrong in a way nothing reports.One await changed in
HitQueue: the buff application is now awaited before the recomputation, so a type 33 buff that has just landed is visible to it. The worker already serialises per target, so this orders work that was already happening rather than adding any.Tested:
NosCore.GameObject.Tests455/455 (13 new, covering the flat and percentage subtypes and the order between them),NosCore.PacketHandlers.Tests410/410, solution builds with zero warnings. Not played — I do not start the servers.Summary by CodeRabbit
New Features
Bug Fixes
Tests