Fix: gear score calculation and item_gear_score condition - #560
Conversation
WalkthroughIntroduces gear-score aggregation in StatsManager using Lua.CalcItemLevel tuple results with item-type adjustments, defers condition updates until post-aggregation, and excludes shields/spellbooks from gear score. AchievementManager’s RankUp now receives ConditionType and handles item_gear_score using max-based counters. Lua tests are split into item-specific cases reflecting new calculations. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant StatsManager
participant Lua
participant AchievementManager
Player->>StatsManager: AddEquips(equippedItems)
StatsManager->>StatsManager: Reset total GearScore
loop For each item
StatsManager->>StatsManager: Skip shields/spellbooks
StatsManager->>Lua: CalcItemLevel(item)
Lua-->>StatsManager: (gearScore, enchantScore)
StatsManager->>StatsManager: total = gearScore + enchantScore
StatsManager->>StatsManager: If ThrowingStar/Dagger, total /= 2
StatsManager->>StatsManager: Accumulate total into GearScore
end
StatsManager->>AchievementManager: Update(item_gear_score, GearScore)
AchievementManager->>AchievementManager: RankUp(conditionType, achievement, count)
alt Rank-up occurred
AchievementManager-->>Player: Rank-up notification/update
else No rank-up
AchievementManager-->>Player: Progress update only
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Maple2.Server.Game/Manager/AchievementManager.cs (2)
82-85: Avoid sending redundant updates when no rank-up or counter change occursWith the new item_gear_score handling, Update() will call RankUp() on every gear-score refresh. If the score didn't increase (Math.Max keeps it the same), RankUp returns false and this path will send an AchievementPacket.Update(achievement) every time. That can spam the client with unchanged payloads.
Consider only sending the Update packet when something actually changed (counter or grade). One way is to compare the counter before and after RankUp for item_gear_score and skip the packet if unchanged.
If you want to keep changes minimal, you can gate the send for item_gear_score based on whether the counter increased.
Example adjustment inside this block:
- if (!RankUp(conditionType, achievement, count)) { - session.Send(AchievementPacket.Update(achievement)); - } + long before = achievement.Counter; + bool ranked = RankUp(conditionType, achievement, count); + bool progressed = achievement.Counter > before; + if (!ranked && (!progressed || conditionType != ConditionType.item_gear_score)) { + // For item_gear_score, suppress no-op updates; still allow other conditions to behave as before. + session.Send(AchievementPacket.Update(achievement)); + }
91-101: Gear-score uses max-based aggregation — good; consider guarding invalid inputsThe new RankUp signature and the Math.Max path for item_gear_score correctly prevent regressions when gear score drops. This aligns with StatsManager’s post-aggregation update.
Minor: If there’s any chance external callers pass negative counts, you may want to clamp count to >= 0 for safety before using it in Math.Max / +=.
- if (conditionType is ConditionType.item_gear_score) { + if (conditionType is ConditionType.item_gear_score) { achievement.Counter = Math.Max(achievement.Counter, count); // Using Math.Max to ensure gear score doesn't decrease. } else { - achievement.Counter += count; + achievement.Counter += Math.Max(0, count); }Maple2.Server.Game/Manager/StatsManager.cs (2)
189-197: Tuple-based GS + enchant + halving logic looks right; confirm rarity argument semantics
- Deconstructing CalcItemLevel’s (gearScore, enchantScore) and then halving the total for Throwing Star/Dagger matches the test approach, which halves the total and then derives base as needed.
- Please confirm that item.Rarity maps 1:1 to the Lua “grade” parameter across all item families; if a mismatch exists for any niche item types, expectations can drift.
Optional: If you ever need exact parity with the tests’ rounding strategy, you can mirror their “halve total and enchant, derive base” approach, but for aggregate GS the current “halve total” is sufficient.
200-203: Post-aggregation updates are in the right place; consider change detection to reduce churnUpdating dungeon limits and emitting the item_gear_score condition once after aggregation is correct. To avoid unnecessary ConditionUpdate calls when GS hasn’t changed, consider caching the last reported GearScore (e.g., on the player/session) and only emitting when it differs.
Maple2.Server.Tests/Lua/LuaTests.cs (2)
167-183: Codex (spellbook) base component test is fine; consider adding a StatsManager-level test for exclusionThis test checks the Lua function only. Given the new rule to exclude spellbooks from GS aggregation, consider adding an integration-style test that verifies the StatsManager does not add codex/spellbook GS into the player’s total.
I can sketch a lightweight test harness around StatsManager.AddEquips to assert GS excludes a codex if desired.
15-208: Reduce duplication in test halving scaffoldingThe halving scaffolding is repeated across multiple tests. Consider extracting a small helper to compute (base, enchant, total) with star/dagger handling, or using TestCaseSource to feed different items/enchant levels through the same routine, keeping the assertions succinct.
Example helper:
private static (int baseGS, int enchantGS, int totalGS) CalcWithHalving(Maple2.Lua.Lua m2, int gearScore, int rarity, ItemType itemType, int enchantLevel, int limitBreakLevel) { var (baseGS, enchGS) = m2.CalcItemLevel(gearScore, rarity, itemType.Type, enchantLevel, limitBreakLevel); int total = baseGS + enchGS; if (itemType.IsThrowingStar || itemType.IsDagger) { int halvedTotal = total / 2; int halvedEnchant = enchGS / 2; int halvedBase = halvedTotal - halvedEnchant; return (halvedBase, halvedEnchant, halvedTotal); } return (baseGS, enchGS, total); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Maple2.Server.Game/Manager/AchievementManager.cs(2 hunks)Maple2.Server.Game/Manager/StatsManager.cs(1 hunks)Maple2.Server.Tests/Lua/LuaTests.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
Maple2.Server.Game/Manager/StatsManager.cs (4)
Maple2.Server.Game/LuaFunctions/Lua.cs (2)
Lua(10-3172)CalcItemLevel(75-459)Maple2.Model/Game/User/Character.cs (1)
Character(8-58)Maple2.Server.Game/Manager/DungeonManager.cs (1)
UpdateDungeonEnterLimit(92-118)Maple2.Server.Game/Session/GameSession.cs (1)
ConditionUpdate(551-554)
Maple2.Server.Game/Manager/AchievementManager.cs (2)
Maple2.Database/Model/Achievement.cs (1)
Achievement(10-56)Maple2.Database/Storage/Game/GameStorage.Achievement.cs (2)
Achievement(11-17)Achievement(57-63)
Maple2.Server.Tests/Lua/LuaTests.cs (2)
Maple2.Model/Game/Item/ItemType.cs (1)
ItemType(4-4)Maple2.Server.Game/LuaFunctions/Lua.cs (2)
CalcItemLevel(75-459)Lua(10-3172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
Maple2.Server.Game/Manager/StatsManager.cs (1)
184-187: Correctly excludes off-hand categories from gear scoreSkipping shields and spellbooks from gear score matches the stated rules and prevents them from inflating GS.
Maple2.Server.Tests/Lua/LuaTests.cs (5)
16-51: Halving logic for throwing stars/daggers matches intended rounding behaviorThe test halves total GS and enchant separately, deriving base as total - enchant to avoid rounding bias. This is consistent and yields deterministic expectations for odd values.
53-89: Enchanted star case: good coverage of rounding and tuple semanticsAsserting both components and the final halved total validates the tuple return and rounding behavior for odd enchant values. Nice.
91-127: Knife test mirrors star/dagger path — ensure ItemType.IsDagger covers this IDTest intent is solid. Double-check that ItemType(13160314) indeed sets IsDagger = true; otherwise the halving branch will be skipped and assertions will fail in locales/databases where the mapping differs.
129-165: Low-tier star case validates non-enchanted halvingGood to have a low-gear-score item to validate the halving branch without enchant contributions.
185-208: Scepter test provides a strong non-halving, enchanted baselineThis anchors expectations for a standard weapon with both base and enchant contributions and no halving. Looks good.
Summary by CodeRabbit
Bug Fixes
Tests