Skip to content

feat(bcard): name the seven negated halves the client describes - #2372

Closed
denislauri1999 wants to merge 0 commit into
NosCoreIO:masterfrom
denislauri1999:pr/bcard-seven-negated-names
Closed

feat(bcard): name the seven negated halves the client describes#2372
denislauri1999 wants to merge 0 commit into
NosCoreIO:masterfrom
denislauri1999:pr/bcard-seven-negated-names

Conversation

@denislauri1999

@denislauri1999 denislauri1999 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

BCard.dat declares 24 (type, subtype) pairs that BCardEffect does not name. Seven of them
can be read; this adds those seven and leaves the other seventeen alone.

Why these seven and not the rest. A pair's text lives in LISTn-1 / LISTn-2, where the
second number is the high or the low half. For these seven the HIGH half has a client text and
the LOW half has none - so the low half is the negation of a sentence that is written down, not
a mechanic that has to be guessed. For the other seventeen the high half is mute too, and a name
there would be invented.

Why Negated and not Increase/Decrease. Both conventions are already in the enum, and
which one applies is decided by the text: where the client says "increased / reduced" the names
say Increase/Decrease (176 pairs), and where there is one sentence for both halves the low one
is X + Negated (186 of 194, 96%). All seven high halves describe a CAPABILITY - "Allows you
to use Full Moon skills", "Reveals hidden enemies within %s spaces" - and a capability is not
reduced, it is negated. The one pair in this shape that already has a name, 88/22
Type88DecreaseDamageVsHiden, is Increase/Decrease because its high half names a magnitude
("%s additional damage") - the same rule, applied to the one case that has one.

documentation/dat/BCard.dat.md is updated in the same commit, so
EveryEffectIsInTheVocabulary and EveryNamedRowMatchesItsEffect stay in step.

What was tested, and how. dotnet test test/NosCore.Parser.Tests --filter "FullyQualifiedName~BCardVocabularyTests&TestCategory!=OPTIONAL-TEST" - the two mandatory
vocabulary tests pass. The opt-in EveryDeclaredEffectIsNamed, which the CI filter excludes,
goes from 24 unnamed pairs to 17.

What was NOT tested. Nothing was played. These are enum members and a documentation table:
no code reads them yet, and this PR deliberately does not wire any behaviour to them. Whether
any of the seven should also DO something is a separate question.

Summary by CodeRabbit

  • New Features

    • Added support for previously unnamed negated buff effects, including enemy focus, miniland return, hidden-enemy reveal, skill reset, skill booster, full moon, and lotus power effects.
  • Documentation

    • Updated effect documentation with names for all newly supported entries.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a13cce4e-2f6e-4f50-aad0-27da185e90d8

📥 Commits

Reviewing files that changed from the base of the PR and between 671fa07 and 421e33a.

📒 Files selected for processing (2)
  • documentation/dat/BCard.dat.md
  • src/NosCore.Data/Enumerations/Buff/BCardEffect.cs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


Walkthrough

The change adds seven negated BCardEffect enum members. It updates the corresponding subtype declarations in BCard.dat.md.

Changes

BCardEffect negations

Layer / File(s) Summary
Add negated effect members
src/NosCore.Data/Enumerations/Buff/BCardEffect.cs
Adds negated enum members for focus-enemy attention, Type 88 effects, and Type 91 effects.
Map documented subtypes
documentation/dat/BCard.dat.md
Assigns the new members to subtypes 63/12, 88/12, 88/42, 88/52, 91/22, 91/32, and 91/42.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 421e3

This change only names seven existing card effects and updates their documentation; it does not alter runtime behavior or accepted values. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: erwan-joly

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: naming seven previously unnamed negated BCard effects.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

2 participants