Handling for music_play_instrument_mastery condition type - #563
Conversation
WalkthroughImplements delta-based mastery updates with per-type handlers, adds music-instrument mastery condition checks, adjusts achievement reward-grade progression when rewards are missing, and replaces the single MasteryExp admin command with nested addexp/setexp commands. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor ServerProcess
participant MasteryManager
participant ConditionUtil
participant AchievementManager
participant DB
ServerProcess->>MasteryManager: set(masterType, startLevel,startValue,currentLevel,value,meta)
MasteryManager->>MasteryManager: compute deltaLevel, deltaExp
alt deltaLevel > 0
MasteryManager->>DB: update level counters (music_play_grade / fisher_grade / mastery_grade)
else deltaLevel < 0
MasteryManager->>DB: set_mastery_grade (decrease)
end
alt deltaExp > 0
MasteryManager->>DB: update exp counters (music_play_instrument_mastery for Music)
end
MasteryManager->>ConditionUtil: evaluate music_play_instrument_mastery (if relevant)
ConditionUtil-->>MasteryManager: condition result
MasteryManager->>AchievementManager: notify potential achievement/trophy
AchievementManager->>DB: check grade reward
alt reward exists
AchievementManager->>ServerProcess: grant reward
else no reward & hasMoreGrades
AchievementManager->>DB: clamp RewardGrade ≤ currentGrade and return
else last grade
AchievementManager->>DB: advance RewardGrade and return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Maple2.Server.Game/Manager/AchievementManager.cs (1)
136-140: Condition type mismatch: hero_achieve vs hero_achieve_gradeRankUp sends ConditionType.hero_achieve, but ConditionUtil handles hero_achieve_grade. This likely prevents those conditions from ever matching.
- session.ConditionUpdate(ConditionType.hero_achieve, codeLong: achievement.Id, targetLong: achievement.CurrentGrade); + session.ConditionUpdate(ConditionType.hero_achieve_grade, codeLong: achievement.Id, targetLong: achievement.CurrentGrade);If hero_achieve is expected elsewhere, alternatively add handling for hero_achieve in ConditionUtil.
🧹 Nitpick comments (5)
Maple2.Server.Game/Util/ConditionUtil.cs (1)
239-254: Target check for instrument mastery should not be unconditional trueRight now, CheckTarget returns true for music_play_instrument_mastery regardless of target parameters, which could accept unintended updates if metadata ever supplies target filters. Align it with the nearby music-play conditions that honor target.Integers/Range.
Proposed change: move music_play_instrument_mastery into the block that validates target.Range/target.Integers and remove it from the unconditional-true block.
@@ - case ConditionType.skill: - case ConditionType.music_play_instrument_time: - case ConditionType.music_play_score: - case ConditionType.music_play_ensemble_in: + case ConditionType.skill: + case ConditionType.music_play_instrument_time: + case ConditionType.music_play_score: + case ConditionType.music_play_ensemble_in: + case ConditionType.music_play_instrument_mastery: if (target.Range != null && target.Range.Value.Min >= longValue && target.Range.Value.Max <= longValue) { return true; } if (target.Integers != null && target.Integers.Contains((int) longValue)) { return true; } break; @@ - case ConditionType.music_play_grade: - case ConditionType.music_play_ensemble: - case ConditionType.music_play_instrument_mastery: + case ConditionType.music_play_grade: + case ConditionType.music_play_ensemble: return true;Also applies to: 275-276
Maple2.Server.Game/Manager/AchievementManager.cs (1)
169-177: Use a more precise “has more grades” checkComparing count to CurrentGrade assumes contiguous grade keys starting at 1. Safer to compare against the max grade key.
- bool hasMoreGrades = achievement.Metadata.Grades.Count > achievement.CurrentGrade; + int maxGradeKey = achievement.Metadata.Grades.Keys.Max(); + bool hasMoreGrades = achievement.CurrentGrade < maxGradeKey;Maple2.Server.Game/Commands/PlayerCommand.cs (1)
37-45: Update help text and validate positive EXPThe command now adds EXP; reflect that in descriptions and guard against non-positive input to avoid no-op/confusion.
- public MasteryExpCommand(GameSession session) : base("exp", "Set player mastery experience.") { + public MasteryExpCommand(GameSession session) : base("exp", "Add player mastery experience.") { @@ - var exp = new Argument<int>("exp", "Experience points to add."); + var exp = new Argument<int>("exp", "Experience points to add (positive).");Add minimal validation near the handler:
- private void Handle(InvocationContext ctx, MasteryType masteryType, int exp) { + private void Handle(InvocationContext ctx, MasteryType masteryType, int exp) { try { + if (exp <= 0) { + ctx.Console.Error.WriteLine("exp must be a positive integer."); + ctx.ExitCode = 1; + return; + } session.Mastery[masteryType] = session.Mastery[masteryType] + exp;Maple2.Server.Game/Manager/MasteryManager.cs (2)
108-120: Propagate multi-level jumps to mastery_gradeDefault branch increments mastery_grade by 1 even if multiple levels were gained. Use counter: deltaLevel for accuracy.
- default: - session.ConditionUpdate(ConditionType.mastery_grade, codeLong: (int) type); + default: + session.ConditionUpdate(ConditionType.mastery_grade, counter: deltaLevel, codeLong: (int) type); return;
138-145: Skip zero/negative EXP deltas and avoid category 0 emissionsPrevent no-op traffic and avoid sending category 0 (unknown instrument), which could misclassify progress.
- private void HandleMasteryExpIncrease(MasteryType type, int deltaExp) { + private void HandleMasteryExpIncrease(MasteryType type, int deltaExp) { + if (deltaExp <= 0) return; switch (type) { case MasteryType.Music: - session.ConditionUpdate(ConditionType.music_play_instrument_mastery, counter: deltaExp, codeLong: session.Instrument?.Value.Category ?? 0); + var category = session.Instrument?.Value.Category ?? 0; + if (category > 0) { + session.ConditionUpdate(ConditionType.music_play_instrument_mastery, counter: deltaExp, codeLong: category); + } return; default: return; } }
📜 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 (4)
Maple2.Server.Game/Commands/PlayerCommand.cs(1 hunks)Maple2.Server.Game/Manager/AchievementManager.cs(1 hunks)Maple2.Server.Game/Manager/MasteryManager.cs(2 hunks)Maple2.Server.Game/Util/ConditionUtil.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Maple2.Server.Game/Commands/PlayerCommand.cs (1)
Maple2.Model/Game/User/Mastery.cs (1)
Mastery(7-35)
Maple2.Server.Game/Manager/MasteryManager.cs (1)
Maple2.Server.Game/Session/GameSession.cs (1)
ConditionUpdate(551-554)
⏰ 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 (3)
Maple2.Server.Game/Util/ConditionUtil.cs (1)
113-131: Good addition: code-side filtering for instrument masteryAdding music_play_instrument_mastery to the code-based range/integer checks is correct and aligns with how fisher_grade et al. are handled.
Maple2.Server.Game/Manager/AchievementManager.cs (1)
169-177: Fix prevents RewardGrade from leapfrogging CurrentGrade when no reward existsThe hasMoreGrades guard and capped increment resolve the UI-grade skip. Looks good.
Maple2.Server.Game/Commands/PlayerCommand.cs (1)
50-51: Correct: switch to additive mastery EXPChanging to additive semantics matches the command’s intent and avoids overwriting progress.
|
|
||
| private void Handle(InvocationContext ctx, MasteryType masteryType, int exp) { | ||
| try { | ||
| session.Mastery[masteryType] = exp; |
There was a problem hiding this comment.
if its a set command we shouldn't add.
There was a problem hiding this comment.
Will change the description of the subcommand. Did forget to do that as the exp positional argument description already describes it as add.
Furthermore adding would be more inline with the other commands e.g. /home as it has exp and setexp subcommands.
If you'd like I can also introduce a setexp subcommand for the mastery command as well.
There was a problem hiding this comment.
i think would be better to introduce a subcommand like setexp/addexp
| if (deltaLevel > 0) { | ||
| HandleMasteryLevelIncrease(type, currentLevel, deltaLevel); | ||
| } | ||
| if (startLevel > currentLevel) { | ||
| session.ConditionUpdate(ConditionType.set_mastery_grade, codeLong: (int) type); | ||
| if (type == MasteryType.Music) { | ||
| session.ConditionUpdate(ConditionType.music_play_grade); | ||
| } | ||
|
|
||
| if (deltaLevel < 0) { | ||
| HandleMasteryLevelDecrease(type); |
There was a problem hiding this comment.
agreed with tade that I don't think we need to make the HandleMasteryLevelIncrease/Decrease be their own functions with only 1 use.
small nit but this should be an else if statement
There was a problem hiding this comment.
Yeah might have been a bit of premature optimization from my part. Will change that to be a singular function instead once I am back home on Saturday/Sunday :)
Handling of mastery updates has been changed to properly update the `music_play_instrument_mastery`. This now enables the following Trophies to work properly: - https://handbook.tadeucci.dev/trophies/23100142 - https://handbook.tadeucci.dev/trophies/23100143 - https://handbook.tadeucci.dev/trophies/23100144 - https://handbook.tadeucci.dev/trophies/23100168 - https://handbook.tadeucci.dev/trophies/23100169 - https://handbook.tadeucci.dev/trophies/23100170 - https://handbook.tadeucci.dev/trophies/23100178 - https://handbook.tadeucci.dev/trophies/23100179 - https://handbook.tadeucci.dev/trophies/23100180 - https://handbook.tadeucci.dev/trophies/23100197 - https://handbook.tadeucci.dev/trophies/23100198 - https://handbook.tadeucci.dev/trophies/23100199 - https://handbook.tadeucci.dev/trophies/23100224 - https://handbook.tadeucci.dev/trophies/23100225 - https://handbook.tadeucci.dev/trophies/23100226 - https://handbook.tadeucci.dev/trophies/23100232 - https://handbook.tadeucci.dev/trophies/23100233 - https://handbook.tadeucci.dev/trophies/23100234 - https://handbook.tadeucci.dev/trophies/23100235 - https://handbook.tadeucci.dev/trophies/23100236 - https://handbook.tadeucci.dev/trophies/23100237 - https://handbook.tadeucci.dev/trophies/23100254 - https://handbook.tadeucci.dev/trophies/23100255 - https://handbook.tadeucci.dev/trophies/23100256 - https://handbook.tadeucci.dev/trophies/23100277 - https://handbook.tadeucci.dev/trophies/23100278 - https://handbook.tadeucci.dev/trophies/23100310 - https://handbook.tadeucci.dev/trophies/23100311 - https://handbook.tadeucci.dev/trophies/23100353 - https://handbook.tadeucci.dev/trophies/23100354 - https://handbook.tadeucci.dev/trophies/23100355 - https://handbook.tadeucci.dev/trophies/23100410 - https://handbook.tadeucci.dev/trophies/23100411 - https://handbook.tadeucci.dev/trophies/23100412 - https://handbook.tadeucci.dev/trophies/23100413 Furthermore the `/player mastery exp` command has been updated to now add exp rather then set the mastery value to the specified exp value as this causes issues for trophy tracking and is now more inline with the actual description. Additionally the `RewardGrade` for Trophies has been miscalculated if they had no reward but had more additional grades that needed completing. Previously the `RewardGrade` had been increased past the actual `Grade` of the Trophy to mark it as completed. This was only correct for trophies that did not have additional `Grades` like e.g. music performance trophies causing the UI to skip to the next `Grade` for display. Refs: MS2Community#484
1cc95c1 to
77a6879
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Maple2.Server.Game/Commands/PlayerCommand.cs (1)
101-106: NRE risk when requested level is below the first reward threshold.FirstOrDefault(...) can return default KVP; accessing .Value.Value will throw.
- exp = masteryRewardMetadata.OrderByDescending(mastery => mastery.Key).FirstOrDefault(mastery => level >= mastery.Key).Value.Value; + var kvp = masteryRewardMetadata + .OrderByDescending(m => m.Key) + .FirstOrDefault(m => level >= m.Key); + exp = kvp.Equals(default(KeyValuePair<int, MasteryRewardTable.Entry>)) ? 0 : kvp.Value.Value;Also consider validating “level” against table min/max and emitting a friendly error when out of range.
Maple2.Server.Game/Manager/MasteryManager.cs (1)
41-81: Indexer forbids decreases; negative delta paths are dead code.Math.Clamp(value, current, max) prevents lowering mastery, so deltaLevel < 0 never occurs. If decreases should be possible (admin fixes, rollbacks), expose an absolute setter and compute deltas against the requested value before clamping.
- set { + set { + // Consider: if a privileged "absolute" path is required, delegate here to a SetAbsolute(...) that clamps [0..max].Also update callers (setexp/level) to use the absolute path when intended.
♻️ Duplicate comments (2)
Maple2.Server.Game/Commands/PlayerCommand.cs (1)
30-33: Splitting exp into addexp and setexp is the right call.Matches prior review direction and avoids ambiguity.
Maple2.Server.Game/Manager/MasteryManager.cs (1)
84-90: Delta level overcounts when startValue == 0 (false level-up).Remove the +1; this generates spurious level-ups (e.g., 0→0). Also skip processing when no actual change.
- int deltaLevel = currentLevel - startLevel + (startValue == 0 ? 1 : 0); - int deltaExp = value - startValue; + int deltaLevel = currentLevel - startLevel; + int deltaExp = value - startValue; + if (deltaLevel == 0 && deltaExp <= 0) { + return; + }
🧹 Nitpick comments (4)
Maple2.Server.Game/Commands/PlayerCommand.cs (2)
29-33: Rename command description to reflect multiple subcommands.“Set player mastery.” is misleading now that we have addexp and setexp. Suggest “Manage player mastery.”
- public MasteryCommand(GameSession session) : base("mastery", "Set player mastery.") { + public MasteryCommand(GameSession session) : base("mastery", "Manage player mastery.") {
35-58: Minor duplication in addexp/setexp handlers.Both blocks share argument wiring and try/catch patterns. Optional: factor a small helper to reduce duplication.
Also applies to: 60-73
Maple2.Server.Game/Manager/MasteryManager.cs (2)
130-137: Avoid no-op condition updates.Guard HandleMasteryExpIncrease with deltaExp > 0 to reduce noise.
- private void HandleMasteryExpIncrease(MasteryType type, int deltaExp) { + private void HandleMasteryExpIncrease(MasteryType type, int deltaExp) { + if (deltaExp <= 0) return;
94-101: Comment nit: fix grammar.“The type of mastery whos level...” -> “whose level...”
📜 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 (4)
Maple2.Server.Game/Commands/PlayerCommand.cs(2 hunks)Maple2.Server.Game/Manager/AchievementManager.cs(1 hunks)Maple2.Server.Game/Manager/MasteryManager.cs(2 hunks)Maple2.Server.Game/Util/ConditionUtil.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/Manager/AchievementManager.cs
🧰 Additional context used
🧬 Code graph analysis (2)
Maple2.Server.Game/Commands/PlayerCommand.cs (2)
Maple2.Server.Game/Session/GameSession.cs (3)
GameSession(36-852)GameSession(114-124)GameSession(742-742)Maple2.Model/Game/User/Mastery.cs (1)
Mastery(7-35)
Maple2.Server.Game/Manager/MasteryManager.cs (1)
Maple2.Server.Game/Session/GameSession.cs (1)
ConditionUpdate(551-554)
⏰ 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 (4)
Maple2.Server.Game/Util/ConditionUtil.cs (2)
113-132: Correctly routes music_play_instrument_mastery through numeric code checks.Adding to the numeric code path aligns with using codeLong = instrument category. Looks good.
275-330: Verify achievement metadata for music_play_instrument_mastery
No JSON/CSV/XML metadata entries for this condition were found—manually confirm that any achievement definitions formusic_play_instrument_masteryuse the integer Code (category) and Value (threshold) fields for gating, and do not rely on Target.Maple2.Server.Game/Manager/MasteryManager.cs (2)
111-121: Music/fishing/other grade updates look correct.Mappings to fisher_grade, music_play_grade, and mastery_grade are consistent with ConditionUtil dispatch.
132-135: Verify music_play_instrument_mastery achievements handle codeLong = 0
Confirm no achievement definitions formusic_play_instrument_masteryfilter on non-zeroCodes, sincesession.Instrumentmay be null (category 0) and admin awards would otherwise skip progress.
| private void Handle(InvocationContext ctx, MasteryType masteryType, int exp) { | ||
| try { | ||
| session.Mastery[masteryType] = session.Mastery[masteryType] + exp; | ||
| ctx.ExitCode = 0; | ||
| } catch (SystemException ex) { | ||
| ctx.Console.Error.WriteLine(ex.Message); | ||
| ctx.ExitCode = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
setexp cannot decrease mastery due to clamping in MasteryManager indexer.
Both addexp and setexp route through session.Mastery[...] which clamps to [current..max]; setexp cannot lower EXP/level, contrary to “Set” semantics. Provide an absolute setter or an override path for admin commands.
- session.Mastery[masteryType] = exp;
+ // Introduce an absolute setter on MasteryManager that clamps [0..max] and allows decreases:
+ // session.Mastery.SetAbsolute(masteryType, exp);If you prefer to keep the indexer strict, inject a bool allowDecrease into the indexer (default false) and use it only from admin commands.
Also applies to: 74-82
🤖 Prompt for AI Agents
In Maple2.Server.Game/Commands/PlayerCommand.cs around lines 49-57 (and also
apply same change to 74-82): setexp currently uses session.Mastery[...] which
clamps increases only and so cannot lower mastery; change the admin command to
call an absolute setter on the MasteryManager (or add an optional bool
allowDecrease=false parameter to the MasteryManager indexer) and pass true from
admin commands so the value is applied without clamping; update the admin paths
to use the new setter/override and keep the default clamping behavior for normal
gameplay.
Overview
Handling of mastery updates has been changed to properly update the
music_play_instrument_mastery.This now enables the following Trophies to work properly:
Furthermore the
/player mastery expcommand has been updated to now add exp rather then set the mastery value to the specified exp value as this causes issues for trophy tracking and is now more inline with the actual description.Additionally the
RewardGradefor Trophies has been miscalculated if they had no reward but had more additional grades that needed completing. Previously theRewardGradehad been increased past the actualGradeof the Trophy to mark it as completed. This was only correct for trophies that did not have additionalGradeslike e.g. music performance trophies causing the UI to skip to the nextGrade(VII) for display althoughVwas only completed andVIis still in progress.Closes: #484
Refs: #33
Video
2025-08-27_10-57-11.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Refactor