Multiplied attack and defence, which nothing folded in - #2332
Merged
erwan-joly merged 1 commit intoAug 28, 2026
Merged
Conversation
BCard types 34 and 35 - "attack power is multiplied by %s" and "defence is multiplied by %s" - were declared in BCardEffect and absent from the fold, so a card promising a fivefold defence changed nothing. They are the only two families that state a FACTOR rather than a percentage, and their values are small whole numbers: read as percentages the strongest of them would be worth five per cent. The decreasing half says "divided by", so it halves rather than subtracting. "All" and the per-kind subtype add into one factor, the way the flat halves of types 3 and 9 already do. 35/51-52 stays out: it says "damage is reduced by %s%% per debuff stack, up to %s%%", which is a rate and a ceiling rather than a factor, and there is no count of stacks to feed it. 34/41-42 and 34/51-52 stay out too - CombatStats has no magic attack field for the first pair to land in, and the second pair scales with missing HP. 10 tests. Manual checks added to the combat section of the test plan: these are meant to be obvious rather than subtle, so a fivefold defence should be visible in one exchange. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughChangesMultiplicative Battle Stats
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR enables the existing attack and defence multiplier effects in combat calculations and adds focused automated coverage. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CardEffects
participant CardTotals
participant BattleStatsProvider
CardEffects->>CardTotals: Accumulate attack and defence factors
CardTotals->>BattleStatsProvider: Provide summed factor totals
BattleStatsProvider->>BattleStatsProvider: Apply factors to computed stats
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 28, 2026
| - [ ] An "all elements" effect applies to each of fire, water, light and dark rather than | ||
| being ignored | ||
|
|
||
| ### Multiplied attack and defence — BCard types 34 and 35 |
Collaborator
There was a problem hiding this comment.
i missed this file likely this should be in conversation on git not in a md file
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.
BCard types 34 and 35 — "attack power is multiplied by %s" and "defence is multiplied by %s" — are declared in
BCardEffectand were absent from the fold inBattleStatsProvider. A card promising a fivefold defence changed nothing, and nothing raised.They are the only two families that state a factor rather than a percentage. Their values are small whole numbers, so reading one as a percentage would turn the strongest card of the family into five per cent; and the decreasing half says "divided by", so it halves rather than subtracting two.
"All" and the per-kind subtype add into one factor, the way the flat halves of types 3 and 9 already do.
Left out, deliberately
35/51-52— "damage is reduced by %s%% per debuff stack, up to %s%%": a rate and a ceiling, not a factor, and there is no count of stacks to feed it.34/41-42—CombatStatshas no magic attack field for them to land in.34/51-52— these scale with missing HP, which is a different shape again.What was tested
MultiplyingStatTests: the "all" subtype reaching all three defences, each per-kind subtype reaching only its own, the attack factor moving both ends of the roll, the halving, the identity at 1, and "all" plus a kind adding into one factor.NosCore.GameObject.Testsis 452 passed, 0 failed, anddotnet build NosCore.slnis 0 warnings, 0 errors.documentation/manual-test-plan.md; they are deliberately coarse, because a fivefold defence should be visible in a single exchange rather than needing measurement.Summary by CodeRabbit
New Features
Documentation