Sync Skills & State Skills & Misc - #445
Conversation
WalkthroughThis update introduces new enums and constants, refactors skill and buff handling to use strongly typed enums, and enhances quest, skill, and achievement management. It adds new utility and debugging methods, restructures skill casting interfaces and logic, and improves regeneration and state skill handling. Several method signatures and internal logic were updated for clarity and robustness. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant JobCommand
participant QuestManager
participant AchievementManager
Player->>JobCommand: Request job change/awakening
alt Awakening
JobCommand->>QuestManager: DebugCompleteChapter(chapterId)
QuestManager-->>JobCommand: Complete all quests in chapter
JobCommand->>JobCommand: Advance to awakened job
JobCommand->>JobCommand: UnlockMasterSkills()
else Normal job change
JobCommand->>JobCommand: JobAdvance(job)
end
JobCommand-->>Player: Update job, stats, and skills
Player->>AchievementManager: UnlockAllTrophies (debug)
AchievementManager->>AchievementManager: DebugCompleteAllTrophies()
AchievementManager-->>Player: All trophies unlocked
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
Maple2.Server.Game/Manager/QuestManager.cs (1)
593-593: Consider error handling for invalid chapter IDs.The method assumes that
session.QuestMetadata.GetQuestsByChapter(chapterId)will always return a valid collection, but doesn't handle the case where an invalid chapter ID is provided.- IEnumerable<int> questIds = session.QuestMetadata.GetQuestsByChapter(chapterId).Select(q => q.Id); + var questMetadata = session.QuestMetadata.GetQuestsByChapter(chapterId); + if (!questMetadata.Any()) { + logger.Warning("No quests found for chapter ID: {chapterId}", chapterId); + return; + } + IEnumerable<int> questIds = questMetadata.Select(q => q.Id);Maple2.Server.Game/Manager/AchievementManager.cs (1)
270-279: Minor perf/readability tweak – captureDateTime.Nowonce per trophy
DateTime.Nowis evaluated for every single grade.
Grabbing it once per trophy avoids redundant sys-calls and keeps all grades in the same millisecond.- for (int grade = achievement.CurrentGrade; grade <= maxGrade; grade++) { - if (achievement.Grades.ContainsKey(grade)) { - achievement.Grades[grade] = DateTime.Now.ToEpochSeconds(); + long now = DateTime.Now.ToEpochSeconds(); + for (int grade = achievement.CurrentGrade; grade <= maxGrade; grade++) { + if (achievement.Grades.ContainsKey(grade)) { + achievement.Grades[grade] = now; GiveReward(achievement); continue; } - achievement.Grades.Add(grade, DateTime.Now.ToEpochSeconds()); + achievement.Grades.Add(grade, now); GiveReward(achievement); }Maple2.Server.Game/Manager/Config/BuffManager.cs (2)
29-39: Expose concurrency intent in the public type
Compulsionsis initialised with aConcurrentDictionarybut typed as plainIDictionary.
Down-casting loses compile-time guarantees and invites accidental non-thread-safe replacements.- public IDictionary<BuffCompulsionEventType, IDictionary<int, AdditionalEffectMetadataStatus.CompulsionEvent>> Compulsions { get; init; } + public ConcurrentDictionary<BuffCompulsionEventType, IDictionary<int, AdditionalEffectMetadataStatus.CompulsionEvent>> Compulsions { get; init; }Mirror this change in the constructor to avoid the extra generic cast.
(No functional change, but reinforces the contract to future maintainers.)
302-309: Thread safety when summing compulsion rates
nestedCompulsionDic.Valuescan be mutated concurrently by other threads.
Consider snapshotting the collection (e.g.,ToArray()) before summing or locking the dictionary to avoidInvalidOperationException.Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
186-202: Broadcast regen updates so nearby clients stay in sync
RecoverHp/Sp/StaminaonlySession.Send(...)to the owner.
Other players never receive the updated stats, so party frames & damage
calculations drift.Consider broadcasting after a successful regen tick:
case BasicAttribute.Health: RecoverHp((int) regen.Total); + Field.Broadcast(StatsPacket.Update(this, BasicAttribute.Health)); continue;Apply the same pattern for Spirit and Stamina.
Maple2.Server.Game/Model/Field/Actor/Actor.cs (2)
337-339: Consider moving the broadcast out ofCastSkillfor clearer separation of concerns
CastSkillboth creates a record and broadcasts it.
Handlers now need to remember not to broadcast again (bug above). Extracting the broadcast keepsActorfree of networking code and avoids future duplication.If you keep it here, at least document it prominently.
201-206: Potential N² splash-effect loop
foreachtarget ➜foreacheffect ➜AddSkillcan create one skill entity per target × effect.
Large AoE skills with many targets & splash effects will scale poorly.Consider:
• Deduplicating identical skill placements.
• Caching effect positions when multiples overlap.Not urgent, but keep an eye on profiler data.
Maple2.Server.Game/Commands/PlayerCommand.cs (1)
141-155: Verbose switch duplication – maintainability concernThe giant
JobCode⇄Jobmapping appears three times (normal, awakened, base).
A single staticDictionary<JobCode, (Job Base, Job Awakened)>would:• Remove copy-paste errors
• Make future job additions one-liner changesNot blocking, but worth refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Maple2.Server.World/Migrations/20250304061437_InteractCubeFix.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (28)
Maple2.Database/Storage/Metadata/QuestMetadataStorage.cs(1 hunks)Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/SkillMapper.cs(6 hunks)Maple2.Model/Enum/Buff.cs(2 hunks)Maple2.Model/Enum/CompulsionEventType.cs(0 hunks)Maple2.Model/Enum/Skill.cs(2 hunks)Maple2.Model/Metadata/AdditionalEffectMetadata.cs(1 hunks)Maple2.Model/Metadata/Constants.cs(1 hunks)Maple2.Model/Metadata/SkillMetadata.cs(2 hunks)Maple2.Server.Game/Commands/PlayerCommand.cs(3 hunks)Maple2.Server.Game/Manager/AchievementManager.cs(1 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(4 hunks)Maple2.Server.Game/Manager/NpcScriptManager.cs(1 hunks)Maple2.Server.Game/Manager/QuestManager.cs(1 hunks)Maple2.Server.Game/Manager/StatsManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/Actor.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.SkillCastTask.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs(4 hunks)Maple2.Server.Game/Model/Field/Actor/IActor.cs(2 hunks)Maple2.Server.Game/Model/Skill/SkillQueue.cs(2 hunks)Maple2.Server.Game/Model/Skill/SkillRecord.cs(1 hunks)Maple2.Server.Game/Model/Stats.cs(1 hunks)Maple2.Server.Game/PacketHandlers/SkillHandler.cs(9 hunks)Maple2.Server.Game/PacketHandlers/StateSkillHandler.cs(2 hunks)Maple2.Server.Game/Packets/NpcTalkPacket.cs(1 hunks)Maple2.Server.Game/Service/ChannelService.Heartbeat.cs(1 hunks)Maple2.Server.Game/Util/DamageCalculator.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Maple2.Model/Enum/CompulsionEventType.cs
🧰 Additional context used
🧬 Code Graph Analysis (4)
Maple2.Server.Game/Model/Skill/SkillQueue.cs (2)
Maple2.Server.Game/Model/Skill/SkillRecord.cs (2)
SkillRecord(7-70)SkillRecord(39-44)Maple2.Model/ModelExtensions.cs (1)
ActorState(97-246)
Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (8)
Maple2.Server.Game/Session/GameSession.cs (1)
GameEvent(547-547)Maple2.Server.Game/Model/Field/Actor/Actor.cs (3)
Update(298-315)SkillRecord(319-340)SkillCastConsume(349-351)Maple2.Server.Game/Model/Stats.cs (8)
Stat(153-211)Stat(161-161)Stat(163-167)Stats(8-151)Stats(17-33)Stats(35-41)Total(118-128)Add(194-196)Maple2.Server.Game/Model/Skill/SkillQueue.cs (2)
Add(19-27)SkillRecord(29-37)Maple2.Model/Metadata/Constants.cs (1)
Constant(10-957)Maple2.Server.Game/Packets/StatsPacket.cs (1)
StatsPacket(10-101)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
Broadcast(67-67)Maple2.Server.Game/Packets/SkillPacket.cs (1)
SkillPacket(13-113)
Maple2.Server.Game/Manager/StatsManager.cs (2)
Maple2.File.Ingest/Utils/AttributeExtensions.cs (1)
BasicAttribute(6-44)Maple2.Server.Game/Model/Stats.cs (1)
AddRate(190-192)
Maple2.Server.Game/Manager/AchievementManager.cs (3)
Maple2.Database/Model/Achievement.cs (1)
Achievement(11-57)Maple2.Database/Storage/Game/GameStorage.Achievement.cs (4)
Achievement(14-20)Achievement(60-66)GameStorage(12-68)Request(13-67)Maple2.Database/Extensions/DateTimeExtensions.cs (1)
ToEpochSeconds(6-12)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (40)
Maple2.Server.Game/Manager/NpcScriptManager.cs (1)
379-379: Good update to cutscene call signature.The method call has been properly updated to include the
scriptFunction.MoveMapIdparameter, which aligns with the modifiedCutscenemethod signature inNpcTalkPacket.cs.Maple2.Server.Game/Packets/NpcTalkPacket.cs (2)
101-101: Method signature extended to include map ID parameter.The
Cutscenemethod signature has been updated to include the mapId parameter, which enables the cutscene functionality to specify which map to transition to.
105-105: Writing map ID to the cutscene packet.The new mapId parameter is now being written to the packet, allowing the client to receive this information during cutscene transitions.
Maple2.Database/Storage/Metadata/QuestMetadataStorage.cs (1)
64-66: Method implementation looks good!New method
GetQuestsByChapterfollows the same pattern as other filter methods in the class. This is a well-implemented addition that enables filtering quests by chapter ID.Maple2.Server.Game/Model/Skill/SkillRecord.cs (1)
35-35: Good addition for state skill managementThe new
StateNextTickfield with clear documentation will help track timing for state skills, which aligns with the PR objectives to fix sync skill casting and state skills.Maple2.Server.Game/Manager/StatsManager.cs (1)
183-185: Variable name improvement and helpful commentThe variable name change from
ratespecialAttributetorateBasicAttributebetter represents the data being processed. The added comment explains the purpose of ensuring minimum regen intervals, which aligns with the PR objectives to fix state skills.Maple2.Model/Metadata/Constants.cs (1)
105-105: Good constant addition for minimum stat intervalThis new constant
MinStatIntervalTickwith value 100 (0.1 seconds) provides a clear, centralized definition for the minimum regeneration interval, which supports the PR's objective to fix state skills and skill casting.Maple2.Model/Enum/Buff.cs (2)
35-35: Removed ambiguity in BuffCategory enumThe question mark has been removed from the
Slow = 8enum value, indicating that its purpose has been confirmed and is no longer uncertain.
95-100: Added new enum to improve type safetyThe new
BuffCompulsionEventTypeenum replaces the previously deletedCompulsionEventTypeenum, maintaining the same values but with a more descriptive name that better indicates its relationship to the buff system.Using a byte as the underlying type is appropriate for this small enum and helps with memory efficiency. The naming is consistent with C# conventions and the values match the expected usage in the compulsion event system.
Maple2.Server.Game/Util/DamageCalculator.cs (3)
13-13: Updated to use the new BuffCompulsionEventType enumReferences to the previous
CompulsionEventTypeenum have been replaced with the newBuffCompulsionEventTypeenum for better type safety and naming consistency.
18-18: Updated to use the new BuffCompulsionEventType enumReferences to the previous
CompulsionEventTypeenum have been replaced with the newBuffCompulsionEventTypeenum for better type safety and naming consistency.
83-83: Updated to use the new BuffCompulsionEventType enumReferences to the previous
CompulsionEventTypeenum have been replaced with the newBuffCompulsionEventTypeenum for better type safety and naming consistency.Maple2.Model/Metadata/AdditionalEffectMetadata.cs (1)
88-88: Updated CompulsionEvent record to use new enum typeThe
CompulsionEventrecord now uses theBuffCompulsionEventTypeenum instead of the previousCompulsionEventTypeenum, which is consistent with the type replacement across the codebase.Maple2.Server.Game/Model/Skill/SkillQueue.cs (3)
1-3: Added necessary using directive for ActorState enumAdded the required using directive for the Maple2.Model.Enum namespace, which is needed for the ActorState enum used in the new conditions.
11-11: Added StateSkill tracking to improve state skill managementAdded a new public nullable field to explicitly track the most recent skill with a non-None state, making it easier to access and manage state skills.
This change aligns with the PR objective of fixing issues with state skills and improves the handling of state-related skills like fast swimming.
22-24: Added logic to track state skills separatelyEnhanced the Add method to update the StateSkill field whenever a skill with a non-None state is added to the queue. This provides an explicit way to track the active state skill without disrupting the existing circular buffer functionality.
This implementation ensures that state skills (like swimming or other movement-related skills) are properly tracked and can be easily accessed, which helps resolve the issues mentioned in the PR objectives.
Maple2.Server.Game/Model/Stats.cs (2)
169-173: Great defensive programming improvement!Adding
Math.Max(0, ...)ensures that stat values never go below zero, preventing potential issues with negative stats in game mechanics. This is a valuable safeguard that improves game state consistency.
175-178: Good consistency with non-negative enforcementSimilarly, this change ensures the
TotalandCurrentvalues remain non-negative when modified through theAddTotalmethod, maintaining the same protection pattern applied toAddBase.Maple2.Model/Metadata/SkillMetadata.cs (2)
45-45: Improved type safety with enum replacementReplacing the integer field with the strongly-typed
SuperArmorenum improves code clarity and reduces the risk of bugs due to invalid values. This makes the codebase more maintainable and self-documenting.
142-142: Good use of strongly-typed enumSimilar to the previous change, using the
BounceTypeenum instead of an integer improves type safety and makes the code more readable and maintainable. This change aligns with C# best practices.Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs (2)
161-164: Consistent enum usage across codebaseThis change replaces
CompulsionEventTypewithBuffCompulsionEventType, maintaining consistent enum usage throughout the system. This standardization helps prevent bugs from mismatched enum types and improves code maintainability.
167-167: Maintaining logical consistency with new enum typeThe condition check has been updated to use the new enum type while preserving the same logical behavior, ensuring seamless integration with the rest of the codebase.
Maple2.Server.Game/Service/ChannelService.Heartbeat.cs (3)
4-4: Good addition of structured loggingAdding Serilog for structured logging is a great improvement that will help with debugging and monitoring.
10-15: Excellent error handling improvementReplacing thrown exceptions with logged warnings and graceful failure responses is a significant improvement in error handling. This approach is much more robust for a service that needs to handle potentially invalid requests without disrupting other operations.
16-21: Improved service reliabilitySimilar to the previous change, this replaces an exception with proper logging and a controlled failure response. This pattern makes the service more reliable and easier to debug when issues occur.
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.SkillCastTask.cs (1)
92-92: Method call updated to include field tick parameter.The
CastSkillmethod call now includes the current field tick (cast to an integer) and explicitly specifies themotionPointparameter. This aligns with the broader changes to skill casting functionality that require precise timing information.Maple2.Server.Game/Manager/QuestManager.cs (1)
586-630: New debugging utility for completing chapter quests added.This new method provides a clean way to silently complete all quests in a specified chapter for debugging purposes. It correctly handles both existing and non-existing quests, properly manages quest conditions, and updates the database appropriately.
The implementation:
- Retrieves all quests for the specified chapter
- Updates existing quests or creates new completed ones
- Loads the quests to refresh the client state
This complements other debugging utilities in the PR like trophy completion.
Maple2.File.Ingest/Mapper/SkillMapper.cs (5)
20-21: Unused lists added for future implementation.Two new empty lists (
magicPathsandcubeMagicPaths) have been added but are not yet used in the code. These appear to be placeholders for future functionality related to magic path tracking.Are these lists intended to be used in a future commit, or were they meant to be used already in this implementation?
73-73: Improved type safety with BounceType enum.The cast to the newly added
BounceTypeenum provides better type safety and readability compared to using raw integer values.
119-126: Enhanced state parsing using attribute-based lookup.The parsing of the
Stateproperty has been significantly improved by using attribute-based lookup forActorStateenum values. This is a more robust approach that relies onDescriptionAttributemetadata rather than simple string matching, and properly defaults toActorState.Nonewhen the value is empty or unmatched.
138-138: Improved type safety with SuperArmor enum.The cast to the newly added
SuperArmorflags enum provides better type safety and readability compared to using integer values.
153-153: Added mapping for 'circle' region type.Properly mapping the
"circle"string to theSkillRegion.Cylinderenum value ensures that skills defining circular regions will be correctly interpreted.Maple2.Model/Enum/Skill.cs (3)
204-211: New BounceType enum added for bounce behaviors.A new
BounceTypeenum has been introduced to define various bounce behaviors for skills. This supports the explicit typing inSkillMapper.csand improves code readability by replacing raw integers with meaningful enum values.
213-218: New SuperArmor flags enum added for immunity types.A new
SuperArmorflags enum has been introduced to represent different types of immunity (stun and knockback). Using a flags enum is appropriate here as a skill can provide multiple types of immunity simultaneously.
186-190:Details
✅ Verification successful
CompulsionType enum has been revised with new values.
The
CompulsionTypeenum has been updated to replace its previous members with new members (Hit,Critical,Interrupt), withInterruptmarked as unconfirmed. This change affects how compulsion effects are interpreted in skills.Have all references to the old
CompulsionTypevalues been updated throughout the codebase? Run this script to check for potential issues:
🏁 Script executed:
#!/bin/bash # Check for any remaining references to old CompulsionType values echo "Checking for references to old CompulsionType values..." rg -l "CompulsionType\.Evasion|CompulsionType\.Block" --type csLength of output: 186
All old CompulsionType values removed and no lingering references found
A search for
CompulsionType.EvasionandCompulsionType.Blockreturned no matches in the codebase, confirming that all references to the old enum members have been updated.Maple2.Server.Game/PacketHandlers/StateSkillHandler.cs (1)
43-44: Verify tick-unit conversion & possible overflow
FieldTickappears to count server ticks, whereasTimeSpan.TotalMillisecondsreturns real-time milliseconds.
IfFieldTickis not millisecond-based this will skewStateNextTick.
Also, casting the double result tointcan overflow for long animations.Please double-check the units or switch to
long:- cast.StateNextTick = session.Field.FieldTick + (int) TimeSpan.FromSeconds(cast.Motion.MotionProperty.SequenceSpeed).TotalMilliseconds; + long delayMs = (long) TimeSpan.FromSeconds(cast.Motion.MotionProperty.SequenceSpeed).TotalMilliseconds; + cast.StateNextTick = session.Field.FieldTick + delayMs;Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
34-35: Interface update looks goodThe enriched
CastSkillsignature propagates timing and spatial data and aligns
with the concrete implementations. No issues spotted.Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
142-145: Early game-event update placement is sensibleMoving
Session.GameEvent.Update(tickCount)to the top guarantees event logic
runs even if the player dies early in the frame. Nice catch.Maple2.Server.Game/PacketHandlers/SkillHandler.cs (1)
349-353: Good: failure path now notifies the callerThe extra
SkillUseFailedPacket.Fail(record)guarantees clients receive feedback whenSkillCastConsumevetoes the cast.
Nice improvement.Maple2.Server.Game/Commands/PlayerCommand.cs (1)
503-509: Good simplification of trophy unlock logic
DebugCompleteAllTrophiescentralises the heavy work, and the command now provides clear user feedback – nice 👍
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
326-329:⚠️ Potential issueFix rotation assignment logic
There's an issue with the rotation assignment in the
CastSkillmethod. The current implementation always assigns the propertyRotationto itself whenRotation == default, which has no effect.- Rotation = Rotation == default ? Rotation : rotation, + Rotation = rotation == default ? Rotation : rotation, Rotate2Z = rotateZ, ServerTick = castTick,This fix ensures that when a default rotation is provided, the actor's current rotation is used; otherwise, the provided rotation is applied.
🧹 Nitpick comments (2)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
209-211: Consider adding documentation for the empty virtual methodThis new virtual method is meant to be overridden by derived classes, but it's currently lacking documentation to explain its purpose and how it should be used.
+ /// <summary> + /// Handles skill attack point processing. Override this method in derived classes to implement specific behavior. + /// </summary> + /// <param name="record">The skill record containing skill data</param> + /// <param name="attackPoint">The attack point to process</param> public virtual void SkillAttackPoint(SkillRecord record, byte attackPoint) { }Maple2.Server.Game/Commands/PlayerCommand.cs (1)
178-222: Consider reducing code duplication in job mappingThe method correctly implements the awakening functionality, but contains duplicate job code mapping logic.
Consider refactoring the duplicate job mapping code into a helper method:
- private void Awaken(InvocationContext ctx, JobCode jobCode) { - Job awakenedJob = jobCode switch { - JobCode.Newbie => Job.Newbie, - JobCode.Knight => Job.KnightII, - JobCode.Berserker => Job.BerserkerII, - JobCode.Wizard => Job.WizardII, - JobCode.Priest => Job.PriestII, - JobCode.Archer => Job.ArcherII, - JobCode.HeavyGunner => Job.HeavyGunnerII, - JobCode.Thief => Job.ThiefII, - JobCode.Assassin => Job.AssassinII, - JobCode.RuneBlader => Job.RuneBladerII, - JobCode.Striker => Job.StrikerII, - JobCode.SoulBinder => Job.SoulBinderII, - _ => throw new ArgumentException($"Invalid JobCode: {jobCode}"), - }; - Job baseJob = jobCode switch { - JobCode.Newbie => Job.Newbie, - JobCode.Knight => Job.Knight, - JobCode.Berserker => Job.Berserker, - JobCode.Wizard => Job.Wizard, - JobCode.Priest => Job.Priest, - JobCode.Archer => Job.Archer, - JobCode.HeavyGunner => Job.HeavyGunner, - JobCode.Thief => Job.Thief, - JobCode.Assassin => Job.Assassin, - JobCode.RuneBlader => Job.RuneBlader, - JobCode.Striker => Job.Striker, - JobCode.SoulBinder => Job.SoulBinder, - _ => throw new ArgumentException($"Invalid JobCode: {jobCode}"), - }; + private void Awaken(InvocationContext ctx, JobCode jobCode) { + Job awakenedJob = GetAwakenedJob(jobCode); + Job baseJob = GetBaseJob(jobCode); + + // Add these helper methods to the class: + // private Job GetAwakenedJob(JobCode jobCode) { ... } + // private Job GetBaseJob(JobCode jobCode) { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Maple2.Server.Game/Commands/PlayerCommand.cs(3 hunks)Maple2.Server.Game/Manager/AchievementManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/Actor.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Maple2.Server.Game/Manager/AchievementManager.cs
- Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs
- Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (8)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (3)
54-59: Good addition of thread-safe local ID counterThe introduction of a thread-safe local ID counter using
Interlocked.Incrementis a solid implementation choice for generating unique skill casting IDs. This will prevent conflicts when multiple skills are cast simultaneously in a multi-threaded environment.
201-207: Good implementation of splash skill effectsThis enhancement properly handles splash effects by iterating over targets and applying effects at their positions. The implementation aligns with the PR objective of fixing sync skill casting issues.
Note: The code uses collection expression syntax
[target.Position]which requires C# 12.
333-334: Good error logging for invalid motion pointAdding detailed error logging for invalid motion points will help with debugging issues in production. Consider including more context about why the motion point is invalid, such as expected range or requirements.
Maple2.Server.Game/Commands/PlayerCommand.cs (5)
86-86: Fix implemented for experience commandThe code now correctly adds experience points using the
ExpType.expDropparameter, addressing the issue mentioned in the PR objectives.
157-170: Improved job command logic with proper exit code handlingThe refactored logic properly handles both regular job changes and awakening cases, with appropriate exit code assignment in all paths (addressing the previous review feedback about missing exit codes).
224-249: Well-structured job advancement implementationThe method properly handles different job advancement scenarios, including:
- Clearing skill tabs when changing job codes
- Removing awakening skills when downgrading
- Refreshing buffs and stats
- Broadcasting the job change
This implementation helps fulfill the PR objective of fixing job command functionality.
251-262: Master skills unlocking logic implemented correctlyThis method successfully implements the automatic completion of the master skill quest line, as mentioned in the PR objectives.
502-509: Trophy command simplified and fixedThe implementation now correctly uses
DebugCompleteAllTrophies()followed by reloading achievements, which should fix the issue mentioned in the PR objectives about trophy unlocking not working properly.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor