Add support for fisher_grade condition type and update mastery handling - #561
Conversation
…ling Previously when the fishing mastery had been updated the session had been updated with `ConditionType.mastery_grade` and the mastery type. However for fishing this needs to be `ConditionType.fisher_grade` with the fishing level. Furthermore the Condition checks for code and target now support the `fisher_grade` type. Refs: MS2Community#33
WalkthroughAdds special-case handling for Fishing mastery in MasteryManager’s indexer to update a new fisher_grade condition, and updates ConditionUtil to evaluate fisher_grade in both code and target checks alongside existing fish-related cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant MasteryManager
participant ConditionUtil
Player->>MasteryManager: Gain mastery (type, startLevel, currentLevel, startValue)
alt type == Fishing and (startLevel < currentLevel or startValue == 0)
Note right of MasteryManager: New path
MasteryManager->>ConditionUtil: Update Condition (fisher_grade, codeLong=currentLevel)
else
MasteryManager->>ConditionUtil: Update Condition (mastery_grade, codeLong=(int)type)
end
Note over ConditionUtil: fisher_grade evaluated via existing Range/Integers logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 (4)
Maple2.Server.Game/Util/ConditionUtil.cs (3)
193-200: Range comparisons appear inverted (likely always-false edge cases)The checks use
Min >= value && Max <= value. Typical range checks should beMin <= value && value <= Max. This predates the PR but is a correctness bug hazard.Apply:
- if (target.Range != null && target.Range.Value.Min >= session.Player.Value.Character.MapId && - target.Range.Value.Max <= session.Player.Value.Character.MapId) { + if (target.Range != null && + session.Player.Value.Character.MapId >= target.Range.Value.Min && + session.Player.Value.Character.MapId <= target.Range.Value.Max) { return true; }If you prefer consistency, consider using the existing
InRangehelper instead:- if (target.Range != null && target.Range.Value.Min >= session.Player.Value.Character.MapId && - target.Range.Value.Max <= session.Player.Value.Character.MapId) { + if (target.Range != null && + InRange(target.Range.Value, session.Player.Value.Character.MapId)) { return true; }Happy to open a follow-up PR that normalizes all range checks to
InRange(...).
206-213: Same inverted range comparison for fish/fish_big/fall_damage blockThe comparison should be
Min <= longValue && longValue <= Max.- if (target.Range != null && target.Range.Value.Min >= longValue && - target.Range.Value.Max <= longValue) { + if (target.Range != null && + longValue >= target.Range.Value.Min && + longValue <= target.Range.Value.Max) { return true; }
245-253: Same issue in movement/music/chat block: inverted range comparisonAlign with correct semantics or use
InRange.- if (target.Range != null && target.Range.Value.Min >= longValue && - target.Range.Value.Max <= longValue) { + if (target.Range != null && + longValue >= target.Range.Value.Min && + longValue <= target.Range.Value.Max) { return true; }Alternatively:
- if (target.Range != null && target.Range.Value.Min >= longValue && - target.Range.Value.Max <= longValue) { + if (target.Range != null && InRange(target.Range.Value, (int) longValue)) { return true; }Maple2.Server.Game/Manager/MasteryManager.cs (1)
90-95: Consider also emitting fisher_grade on downgrade for symmetryToday the downgrade path emits
set_mastery_grade(type-based) for all masteries. If achievements/quests will listen specifically tofisher_grade, they won’t see downgrades. If that’s intended, ignore; if not, mirror the upgrade path for Fishing.Proposed minimal change (keep existing
set_mastery_gradefor backward compatibility):if (startLevel > currentLevel) { session.ConditionUpdate(ConditionType.set_mastery_grade, codeLong: (int) type); - if (type == MasteryType.Music) { + if (type == MasteryType.Fishing) { + session.ConditionUpdate(ConditionType.fisher_grade, codeLong: currentLevel); + } else if (type == MasteryType.Music) { session.ConditionUpdate(ConditionType.music_play_grade); } }Please confirm intended event semantics for fishing downgrades (should
fisher_gradefire, or onlyset_mastery_grade?).
📜 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 (2)
Maple2.Server.Game/Manager/MasteryManager.cs(2 hunks)Maple2.Server.Game/Util/ConditionUtil.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Maple2.Server.Game/Manager/MasteryManager.cs (1)
Maple2.Server.Game/Session/GameSession.cs (1)
ConditionUpdate(551-554)
🔇 Additional comments (3)
Maple2.Server.Game/Util/ConditionUtil.cs (2)
269-269: Target check for fisher_grade intentionally unrestrictedIncluding
ConditionType.fisher_gradein the always-true Target group mirrors other mastery-related conditions (e.g.,mastery_grade). Target is ignored; grading is driven by Code only. This is consistent with the intended semantics.
62-62: fisher_grade is now correctly routed viacodeLong—no further changes needed
- In MasteryManager.cs (line 85), the only emission of
ConditionType.fisher_gradeuses thecodeLong: currentLevelparameter, and there are no other call sites for this condition type.- In ConditionUtil.cs,
ConditionType.fisher_gradeis included in both “integer/code” switch‐case groups (around lines 60–64 and 268–271), alongside othercodeLong-driven conditions.- There are no remaining references treating fishing under
mastery_grade, so there’s no mismatch or stale logic.All verifications pass—approving these changes as is.
Maple2.Server.Game/Manager/MasteryManager.cs (1)
84-88: Fishing now emits fisher_grade with currentLevel — matches PR intentOn grade-up (or initial unlock), Fishing emits
ConditionType.fisher_gradewithcodeLong: currentLevel, while other masteries keep usingmastery_gradewithcodeLong: (int)type. This exactly implements the requirement to key fishing progression off level instead of type.
Overview
Previously when the fishing mastery had been updated the session had been updated with
ConditionType.mastery_gradeand the mastery type. However for fishing this needs to beConditionType.fisher_gradewith the fishing level. Furthermore the Condition checks for code and target now support thefisher_gradetype.Refs: #33
Video
2025-08-25_22-42-47.mp4
Summary by CodeRabbit