Feat: Additional Buff Handling - #416
Conversation
|
""" WalkthroughThis update introduces significant enhancements and refactoring to the skill, buff, and ride systems across the codebase. The buff management system now supports multiple buffs per ID with caster differentiation, improved stacking, cooldown enforcement, and batch removal. The ride system is modularized with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant Session
participant BuffManager
participant RideManager
participant FieldManager
participant StatsManager
Player->>Session: Requests to mount ride
Session->>RideManager: Mount(item or effect)
RideManager->>FieldManager: Validate ride metadata
RideManager->>RideManager: Create Ride instance
RideManager->>FieldManager: Broadcast ride start
RideManager->>StatsManager: Set battle mount stats (if applicable)
RideManager-->>Session: Ride mounted
Player->>Session: Applies skill/buff
Session->>BuffManager: AddBuff(effect, caster, ...)
BuffManager->>BuffManager: Check for existing buff (by ID/caster)
BuffManager->>BuffManager: Stack or add new buff, enforce cooldown
BuffManager->>StatsManager: Update stats if needed
BuffManager-->>Session: Buff applied
Player->>Session: Dismount ride
Session->>RideManager: Dismount(type, forced)
RideManager->>FieldManager: Broadcast ride stop
RideManager->>BuffManager: Remove battle mount buffs (if needed)
RideManager->>StatsManager: Refresh stats
RideManager-->>Session: Ride dismounted
Assessment against linked issues
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (27)
Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs (1)
45-45: Consider using null conditional operator for more concise code.While the null check with null-forgiving operator works, a more idiomatic approach would be to use the null conditional operator.
- RideId: data.RideeProperty == null! ? 0 : data.RideeProperty.rideeID, + RideId: data.RideeProperty?.rideeID ?? 0,Maple2.File.Ingest/Mapper/TableMapper.cs (2)
350-350: Updated empty array creation syntax in ParseConditionalChanged from
Array.Empty<InteractObjectMetadataEffect.ConditionEffect>()to a more concise empty array literal[]to use modern C# syntax.
359-359: Updated empty array creation syntax in ParseInvokeChanged from
Array.Empty<InteractObjectMetadataEffect.InvokeEffect>()to a more concise empty array literal[]to use modern C# syntax.Maple2.Server.Game/PacketHandlers/FallDamageHandler.cs (1)
16-18: Consider reconciling hard-coded threshold vs.BASE_FALL_DISTANCE.
The code subtracts1000ffrom the distance but still has aBASE_FALL_DISTANCEconstant defined. If the fallback threshold differs from1000f, review whether it's accurate to keep that constant around or if it should be updated/removed.Maple2.Server.Game/Packets/FieldPacket.cs (1)
153-155: Consistent buff enumeration for NPCs.
Applying the same buff enumeration pattern for NPCs ensures uniformity. Similar to the player logic, confirm that hitting a large buff count won't overflow theshort.Maple2.Model/Metadata/AdditionalEffectMetadata.cs (1)
137-139: Clarify use-case forOffsetCountinAdditionalEffectMetadataModifyOverlapCount.If negative offset values are used or expected, ensure that appropriate validation or documentation exists. Address any potential edge cases (e.g., large or invalid offsets) to avoid unexpected behavior.
Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
25-25: Consider extracting the additional parameters into an overload or parameter object.A single method signature with many defaulted parameters can become cumbersome and error-prone. Splitting them into a specialized method or struct can improve clarity and maintainability.
Maple2.Server.Game/Model/Field/Actor/Actor.cs (3)
76-80: Add error handling or fallback logic for the new parameters.Expanding
ApplyEffectwith extra parameters is useful but consider rejecting invalid values (e.g., negativeskillId, unexpectedEventConditionType) to prevent inconsistent buff application.
88-88: ConstructingDamageRecordTargetfor each hit might be costly.If multiple calculations occur within a short timeframe, consider reusing or pooling
DamageRecordTargetobjects. This could improve performance in high-load scenarios.
246-248: ConfirmOnDeathevent does not conflict with final buff or resource updates.Triggering
Buffs.TriggerEventupon death is sensible. If additional cleanup steps (e.g., removing ride buffs or shutting down attached triggers) are needed, ensure they happen before final disposal or state transitions.Maple2.Model/Game/Ride/RideOnAction.cs (2)
28-28: Consider validating theItemobject.
Although this class is straightforward, it might be safer to add a null check (or similar validation) to prevent potential null references in downstream logic.
61-61: Use of record-like syntax is concise and clear.
DefiningRideOnActionObjectwith a simple inline constructor and no additional body is a neat approach; just ensure there is no future need for expanded logic or overrides that might warrant a more verbose structure.Maple2.Server.Game/Util/SkillUtils.cs (1)
143-143: Refactoring supports new event checks.
The private overload forBeginConditionTarget.Checkalso gains the event parameters, allowing for thorough filtering. Make sure all referencing code is updated to feed these parameters accurately.Maple2.Server.Game/Manager/RideManager.cs (2)
69-87: Dismount logic covers forced vs. unforced cases.
Removing buffs for battle mount and refreshing stats is correct. Consider concurrency protection if other code might trigger dismount simultaneously.
89-124: Clean and extensible switch forRideOffType.
ReturningRideOffActionDead,RideOffActionTaxi, etc. is straightforward. IfRideOffType.Reactis truly obsolete, removing its commented-out code can reduce clutter.Maple2.Server.Game/PacketHandlers/RideHandler.cs (2)
100-119: Manual resetting of the ride reference.
While nullifyingsession.Ride.Ridedirectly is valid, consider invoking a shared helper (e.g.,DismountorChangeRide) to keep the code consistent and easier to maintain.- session.Ride.Ride = null; + // Possibly refactor into a dedicated method for ride transitions + session.Ride.Dismount(RideOffType.Change, forced: false);
145-164: Leaving ride logic is correct but can be more modular.
Clearing the passenger seat and settingsession.Ride.Rideto null works, but a dedicated method for removing a passenger might simplify updates if the ride logic evolves.Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
549-572: Fall damage formula is robust.
The exponential decay on HP ratio and the cap at 25% of current HP help avoid extreme cases. Consider validating that distance is non-negative to prevent unexpected results.+ if (distance <= 0) { + return; + } double distanceScalingFactor = 0.04813; ...Maple2.Server.Game/Manager/Config/BuffManager.cs (9)
95-97: Recommend logging for failed stack attempts.
Whenexisting.Stack(...)returns false, consider adding a log to help debug why the stack was refused.if (!existing.Stack(startTick, durationMs: durationMs)) { + logger.Debug("Stack refused for buff {Id} on owner {Owner}", existing.Id, existing.Owner.ObjectId); return; }
116-117: Optional fallback action on add failure.
IfTryAdd(buff)returns false, consider whether to revert partial changes or handle it gracefully beyond just logging an error.
185-186: Efficient enumeration methods.
Returning an empty list[]is concise, though it’s a new C# 9+ feature. ConsiderArray.Empty<Buff>()if older C# versions need support.
188-200: Consider multi-buff checks in HasBuff.
GetBuff(effectId)only returns a single buff. If multiple same-ID buffs exist, you may miss a valid match.
319-328: Mount buff logic.
Ifplayer.Session.Ride.Mount(...)fails, removing the buff is correct. Just confirm you intend to remove the buff instead of only disabling it.
343-350: Event-based skill application.
Looping through all enabled buffs to handleTriggerEventis good. Consider short-circuiting if certain events cannot stack.
375-380: Duration modification logic.
Adjusting end times mid-flight is fine. Consider logging for debugging extended or shortened durations.
383-383: Updating each buff every tick.
Ensure performance remains optimal if many buffs are active. Possibly condense or schedule updates less frequently.
446-462: Removing item buffs in bulk.
Again, watch out for the[]list initialization. Otherwise, the approach is consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
Maple2.Database/Context/MetadataContext.cs(1 hunks)Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs(2 hunks)Maple2.File.Ingest/Mapper/FunctionCubeMapper.cs(0 hunks)Maple2.File.Ingest/Mapper/RideMapper.cs(2 hunks)Maple2.File.Ingest/Mapper/TableMapper.cs(2 hunks)Maple2.File.Ingest/MapperExtensions.cs(3 hunks)Maple2.Model/Enum/Buff.cs(2 hunks)Maple2.Model/Enum/InvokeEffectType.cs(0 hunks)Maple2.Model/Enum/RideType.cs(1 hunks)Maple2.Model/Enum/Skill.cs(2 hunks)Maple2.Model/Game/Ride/RideOnAction.cs(3 hunks)Maple2.Model/Metadata/AdditionalEffectMetadata.cs(3 hunks)Maple2.Model/Metadata/BeginCondition.cs(1 hunks)Maple2.Model/Metadata/Constants.cs(1 hunks)Maple2.Model/Metadata/RideMetadata.cs(1 hunks)Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Commands/KillCommand.cs(1 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(12 hunks)Maple2.Server.Game/Manager/Config/ConfigManager.cs(1 hunks)Maple2.Server.Game/Manager/Config/SkillManager.cs(1 hunks)Maple2.Server.Game/Manager/ExperienceManager.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(2 hunks)Maple2.Server.Game/Manager/Items/EquipManager.cs(1 hunks)Maple2.Server.Game/Manager/RideManager.cs(1 hunks)Maple2.Server.Game/Manager/StatsManager.cs(4 hunks)Maple2.Server.Game/Model/Field/Actor/Actor.cs(5 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPet.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs(7 hunks)Maple2.Server.Game/Model/Field/Actor/IActor.cs(2 hunks)Maple2.Server.Game/Model/Field/Buff.cs(5 hunks)Maple2.Server.Game/Model/Skill/DamageRecord.cs(1 hunks)Maple2.Server.Game/Model/Stats.cs(2 hunks)Maple2.Server.Game/PacketHandlers/FallDamageHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/InsigniaHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/InteractObjectHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/RideHandler.cs(4 hunks)Maple2.Server.Game/PacketHandlers/SkillHandler.cs(1 hunks)Maple2.Server.Game/Packets/FieldPacket.cs(5 hunks)Maple2.Server.Game/Session/GameSession.State.cs(1 hunks)Maple2.Server.Game/Session/GameSession.cs(4 hunks)Maple2.Server.Game/Trigger/TriggerContext.Field.cs(1 hunks)Maple2.Server.Game/Trigger/TriggerContext.Npc.cs(2 hunks)Maple2.Server.Game/Trigger/TriggerContext.Player.cs(2 hunks)Maple2.Server.Game/Util/DamageCalculator.cs(2 hunks)Maple2.Server.Game/Util/SkillUtils.cs(3 hunks)
💤 Files with no reviewable changes (2)
- Maple2.File.Ingest/Mapper/FunctionCubeMapper.cs
- Maple2.Model/Enum/InvokeEffectType.cs
🧰 Additional context used
🧬 Code Graph Analysis (8)
Maple2.Server.Game/Model/Field/Actor/FieldPet.cs (1)
Maple2.Server.Game/Model/Skill/DamageRecord.cs (2)
DamageRecordTarget(43-60)DamageRecordTarget(52-55)
Maple2.Model/Metadata/BeginCondition.cs (1)
Maple2.File.Ingest/MapperExtensions.cs (1)
BeginConditionTarget(356-445)
Maple2.Server.Game/Session/GameSession.State.cs (1)
Maple2.Model/Game/Ride/Ride.cs (2)
Ride(5-17)Ride(11-16)
Maple2.Server.Game/Manager/ExperienceManager.cs (1)
Maple2.Server.Game/Manager/Config/ConfigManager.cs (1)
UpdateDeathPenalty(321-344)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (3)
Maple2.Database/Storage/Metadata/RideMetadataStorage.cs (1)
RideMetadataStorage(7-32)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
FallDamage(549-572)Maple2.Model/Metadata/Constants.cs (1)
Constant(10-945)
Maple2.Server.Game/Manager/Items/EquipManager.cs (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
RemoveItemBuffs(445-462)
Maple2.Server.Game/Commands/KillCommand.cs (1)
Maple2.Server.Game/Model/Skill/DamageRecord.cs (2)
DamageRecordTarget(43-60)DamageRecordTarget(52-55)
Maple2.Server.Game/Packets/FieldPacket.cs (4)
Maple2.Model/Game/Ride/Ride.cs (2)
Ride(5-17)Ride(11-16)Maple2.Model/Game/Ride/RideOnAction.cs (3)
RideOnAction(8-26)RideOnAction(13-13)RideOnAction(15-19)Maple2.Server.Game/Manager/Config/BuffManager.cs (4)
List(185-185)List(186-186)List(560-564)Buff(159-170)Maple2.Server.Game/Model/Field/Buff.cs (2)
Buff(13-338)Buff(41-56)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (119)
Maple2.Model/Enum/RideType.cs (1)
4-7:Details
❓ Verification inconclusive
Significant enum refactoring - verify all usages are updated
The
RideOnTypeenum has been completely redefined with new semantic meanings:
- Added
None = 0as the default value (previouslyDefault = 0)- Renamed
UseItem = 1toDefault = 1- Renamed
AdditionalEffect = 2toBattle = 2- Renamed
HideAndSeek = 3toObject = 3This change better reflects the ride's purpose rather than its trigger mechanism, which aligns with the new buff handling system for rides.
🏁 Script executed:
#!/bin/bash # Search for any instances where the old enum values might still be referenced echo "Searching for references to old enum values..." rg --type cs "RideOnType\.(UseItem|AdditionalEffect|HideAndSeek)" -A 3 -B 3Length of output: 180
Enum Refactoring Verification – Manual Confirmation Needed
Based on the automated search, there were no references found to the old enum values (
UseItem,AdditionalEffect, orHideAndSeek). This suggests that the refactoring inMaple2.Model/Enum/RideType.cshas updated all related usages as intended. However, because the search output was minimal (i.e. no output was produced), please perform a manual verification to ensure that no residual references exist elsewhere in the codebase.
- Updated enum definition (Maple2.Model/Enum/RideType.cs, Lines 4-7):
None = 0Default = 1(previouslyUseItem = 1)Battle = 2(previouslyAdditionalEffect = 2)Object = 3(previouslyHideAndSeek = 3)If the manual check confirms that no old references are present, this refactoring can be considered complete.
Maple2.Server.Game/Manager/Config/ConfigManager.cs (1)
185-185: Addition of default parameter adds flexibility for cooldown managementMaking the
levelparameter optional with a default value of 1 improves API usability by not requiring explicit level specification for simple cooldown scenarios, which aligns with the enhanced cooldown system.Maple2.Model/Metadata/Constants.cs (1)
940-943: New constants for regeneration timing and fall damageAdded constants to standardize regeneration wait ticks for HP, SP, and EP (all set to 1000ms), along with a consistent fall distance parameter for damage calculations. These constants support the regeneration mechanics fixes and fall damage formula adjustments mentioned in the PR objectives.
Maple2.File.Ingest/MapperExtensions.cs (3)
246-249: Changed skillOwner handling to match updated entity rolesThe assignment of
SkillEntity.Ownernow aligns with the updated enum values, ensuring proper target and owner relationships for skills. This change supports the revised buff architecture that distinguishes between different caster types.
355-355: Non-nullable EventCondition ensures consistent condition handlingThe
DefaultBeginConditionTargetnow properly initializes theEventproperty with a validEventConditioninstance, avoiding null reference issues and ensuring that event conditions are consistently processed throughout the codebase.
416-422: Simplified event parsing ensures valid conditionsModified the
ParseEventmethod to always return a properly constructedEventConditionwith appropriate default values. This improves robustness by eliminating potential null references and providing consistent event condition handling for the new buff system.Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs (2)
44-44: Good implementation of cooldown conversion.The implementation correctly parses the cooldown string to float, handles parsing failures with a default of 0, and properly converts seconds to milliseconds.
61-66: Clean implementation for ModifyOverlapCount array creation.The implementation correctly zips the effect codes and offset counts into an array of
AdditionalEffectMetadataModifyOverlapCountobjects. The use of LINQ and array initializer syntax is appropriately modern.However, similar to the RideId property, consider using the null conditional operator:
- ModifyOverlapCount: data.ModifyOverlapCountProperty == null! ? [] : data.ModifyOverlapCountProperty.effectCodes + ModifyOverlapCount: data.ModifyOverlapCountProperty?.effectCodesMaple2.Server.Game/Model/Field/Actor/FieldPet.cs (1)
69-69: Good refactoring to use DamageRecordTarget constructor.This change correctly adopts the updated pattern for creating DamageRecordTarget instances, making the code more consistent with the new design where ObjectId is derived from the target actor rather than being explicitly set.
Maple2.Database/Context/MetadataContext.cs (1)
76-76: Appropriate database configuration for new property.The addition of JSON conversion for the ModifyOverlapCount property is consistent with how other properties are configured and ensures proper database storage.
Maple2.Server.Game/Commands/KillCommand.cs (1)
156-159: Good refactoring to use DamageRecordTarget constructor.This change correctly adopts the updated pattern for creating DamageRecordTarget instances, similar to the change in FieldPet.cs. It aligns with the new design where ObjectId is derived from the target actor, while still setting the Position and Direction properties directly.
Maple2.Server.Game/Manager/Items/EquipManager.cs (1)
308-308: Good implementation of buff cleanup on item unequipAdding this call to
RemoveItemBuffsensures that any buffs associated with the unequipped item and its socketed gemstones are properly removed when the item is unequipped. This prevents orphaned buffs from remaining active after their source items are removed.Maple2.Server.Game/PacketHandlers/InsigniaHandler.cs (1)
29-29: Updated buff removal with caster ID parameterThe code now correctly specifies both the buff ID and the caster's object ID when removing an insignia buff. This change aligns with the updated buff management system that now supports multiple buffs of the same ID from different casters.
Maple2.Model/Metadata/RideMetadata.cs (1)
14-14: Improved type safety with enum replacementReplacing the integer type with the strongly typed
RideOnTypeenum enhances code safety and readability. This change ensures consistent and type-safe handling of ride types across the codebase and reduces the risk of errors from invalid numeric values.Maple2.File.Ingest/Mapper/RideMapper.cs (2)
4-4: Added necessary enum importAdded import for the
Maple2.Model.Enumnamespace to access theRideOnTypeenum.
27-27: Implemented safer enum parsing with fallbackThe new code properly parses string values into the
RideOnTypeenum with a fallback toRideOnType.Default. This safer approach replaces the previous direct integer cast and handles potential parsing failures gracefully.Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs (1)
522-524: Updated buff removal to include caster IDThe buff removal now includes both the buff ID and the caster's ObjectId, aligning with the improved buff management system that supports multiple buffs with the same ID but different casters.
Maple2.Model/Metadata/BeginCondition.cs (1)
39-39: Event property made non-nullable for BeginConditionTargetThe
Eventproperty inBeginConditionTargetwas changed from nullable to non-nullable, ensuring every target has a defined event condition. This aligns with the mapper changes where defaultEventConditionis now initialized withEventConditionType.Activateinstead of being null.Maple2.Server.Game/Manager/Config/SkillManager.cs (2)
54-57: Updated condition check to target SkillEntity.OwnerThe passive skill effect condition target check was changed from
SkillEntity.TargettoSkillEntity.Owner, fixing the target alignment for passive buff application.
59-59: Enhanced ApplyEffect call with additional context parametersThe
ApplyEffectmethod now includes field tick, event type, and skill ID parameters, supporting the enhanced buff system that can apply effects conditionally based on event context.Maple2.Server.Game/Session/GameSession.State.cs (1)
55-57: Updated ride check to use RideManagerThe check was updated from
Ride == nulltoRide.Ride == nullto reflect the architectural change where ride handling was refactored into a dedicatedRideManagerclass, improving modularity and encapsulation of ride-related functionality.Maple2.Model/Enum/Skill.cs (2)
134-138: Improved code documentation with valuable grouping informationAdding these categorized groups of EventConditionType values provides helpful context for developers working with these enums, making it clear which condition types belong to specific functional categories.
82-83:Details
✅ Verification successful
Swapped enum values for Target and Owner in SkillEntity
The integer values for
TargetandOwnerhave been swapped, which aligns with corresponding logic updates in other parts of the codebase. This change ensures proper entity targeting in the buff system.Let's verify that this enum value change is consistent with usage across the codebase:
🏁 Script executed:
#!/bin/bash # Check for references to SkillEntity.Target and SkillEntity.Owner rg "SkillEntity\.Target" -A 2 -B 2 rg "SkillEntity\.Owner" -A 2 -B 2Length of output: 6670
Updated Verification: Enum Value Swap is Consistent Across the Codebase
The changes in
Maple2.Model/Enum/Skill.cs(swapping toTarget = 2andOwner = 1) have been verified against references in several modules (e.g., inBuff.cs,Actor.cs,FieldSkill.cs,FieldManager.State.cs, and others). The enum usages remain consistent—comparisons are done by constant name—and the logic updates (such as proper entity targeting in the buff system) align with the intended design.
- The grep results confirm that both
SkillEntity.TargetandSkillEntity.Ownerare used appropriately across multiple files.- The handling in the buff system, actor targeting, and skill management reflects the swap, ensuring consistency.
No further modifications are required.
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
50-50: Added RideMetadataStorage property for ride buff integrationThis property enables the FieldManager to access ride metadata, supporting the new ride buff system. This change integrates with the new RideManager implementation to properly handle ride buffs.
297-297: Added fall damage when players fall off the mapImplementing fall damage when players move outside map boundaries improves gameplay consistency by ensuring fall damage is applied in all appropriate scenarios.
The implementation correctly uses the
FallBoundingAddedDistanceconstant to apply a standardized damage formula from the providedFieldPlayer.FallDamage()method.Maple2.Server.Game/Util/DamageCalculator.cs (2)
134-134: Updated buff iteration to use EnumerateBuffs() methodModified the iteration over target's buffs to use the new
EnumerateBuffs()method, which supports the enhanced buff management system that allows multiple buffs per ID and caster.
143-143: Added target's ObjectId parameter to buff removalUpdated the buff removal to include the target's ObjectId parameter, allowing for proper caster-specific buff management when removing buffs.
Maple2.Server.Game/PacketHandlers/SkillHandler.cs (1)
264-270:Details
✅ Verification successful
LGTM - Switched enum case handling to align with updated enum values
The swap of switch cases for
SkillEntity.OwnerandSkillEntity.Targetproperly aligns with the updated enum values whereTargetandOwnerinteger values were exchanged.It's important to ensure that all other places in the codebase that use these enum values have been properly updated for consistency. The changes here correspond to similar changes in
FieldManager.State.cs, but let's verify all occurrences:
🏁 Script executed:
#!/bin/bash # Search for all usages of SkillEntity.Owner and SkillEntity.Target to ensure consistent handling rg "SkillEntity\.(Owner|Target)" --type csharp -A 3 -B 3Length of output: 7601
LGTM – Enum case handling consistency confirmed
The changes in
SkillHandler.csswapping the cases forSkillEntity.OwnerandSkillEntity.Targethave been cross-verified against the codebase (e.g., inFieldManager.State.cs,Buff.cs, andFieldSkill.cs). These updates align correctly with the revised enum integer values. One minor note: please verify that the behavior inActor.cs(where bothSkillEntity.OwnerandSkillEntity.Targetcurrently map totarget) is intentional.Maple2.Server.Game/Manager/ExperienceManager.cs (1)
187-187: LGTM - Reset death penalty on level upResetting the death penalty when a player levels up provides a gameplay benefit and fresh start with each level. This is a good quality-of-life improvement.
Maple2.Server.Game/Trigger/TriggerContext.Field.cs (1)
391-391:Details
✅ Verification successful
LGTM - Updated buff removal to support caster differentiation
The change to pass both the buff ID and the field actor's object ID when removing buffs correctly aligns with the enhanced buff management system that now supports multiple buffs per ID with caster differentiation.
Let's verify that all other buff removal calls have been consistently updated:
Also applies to: 395-395
🏁 Script executed:
#!/bin/bash # Find all instances of buff removal to ensure they pass the caster ID rg "Buffs\.Remove\(" --type csharpLength of output: 1875
LGTM – Verified Consistency of Buff Removal Updates
All instances of buff removals across the codebase now correctly pass the caster's object ID. The search results confirmed that files like
Maple2.Server.Game/Commands/BuffCommand.cs,Maple2.Server.Game/Util/DamageCalculator.cs, and others (including both lines 391 and 395 inMaple2.Server.Game/Trigger/TriggerContext.Field.cs) use the updated signature properly. No further changes are needed.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
479-485: LGTM - Updated target filtering to align with swapped enum valuesThe switch cases in
GetTargetshave been properly updated to align with the swapped enum values. NowSkillEntity.Targetfilters players andSkillEntity.Ownerfilters mobs, which is consistent with the changes inSkillHandler.cs.Maple2.Server.Game/Trigger/TriggerContext.Player.cs (2)
46-48: Good defensive programming!Adding this boundary check prevents potential
IndexOutOfRangeExceptionwhen the requested count exceeds available players in the box.
220-220: Good refactoring to use the updated buff APIThe change to use
player.Buffs.HasBuff()aligns with the improved buff management system that now supports multiple buffs per ID with caster differentiation.Maple2.Server.Game/Model/Skill/DamageRecord.cs (2)
44-45: Good object-oriented design improvementReplacing the direct
ObjectIdproperty with a computed property that delegates to theTargetobject follows the principle of encapsulation and ensures the ID is always in sync with the actual actor.
52-54: Constructor update maintains object consistencyThe constructor now properly initializes the
Targetfield, ensuring that theObjectIdproperty will always return a valid value from the actor.Maple2.Server.Game/Manager/StatsManager.cs (4)
38-40: Good addition for battle mount stats supportThis new method supports the PR objective of adding ride buffs for battle mounts, allowing mount-specific stats to be applied directly.
115-118: Performance optimization for mounted playersSkipping stat refresh when a player is mounted on a battle or object ride type prevents unnecessary calculations and potential stat conflicts between character and mount stats.
175-175: Improved buff enumeration consistencyUsing
player.Buffs.EnumerateBuffs()instead of direct dictionary access ensures consistent handling of the updated buff system that supports multiple buffs per ID with caster differentiation.
192-192: Consistent buff enumeration approachSimilar to the change in
AddBuffs, usingEnumerateBuffs()ensures proper handling of the updated buff storage structure throughout the stat conversion process.Maple2.Server.Game/Session/GameSession.cs (4)
66-66: Required dependency for ride system integrationAdding the required
RideMetadataStorageproperty is necessary for the new ride system to access ride-related metadata.
105-105: New RideManager property supports ride buff functionalityThis change supports the PR objective of implementing ride buffs for battle mounts and enables centralized management of ride-related functionality.
179-179: Properly initializing RideManager during server entryInitializing the
RideManagerduring server entry ensures ride-related functionality is available from the moment the player enters the game.
354-355: Important buff lifecycle management improvementCalling
Buffs.LeaveField()before removing the player from the field ensures proper buff cleanup during field transitions, preventing potential buff-related issues in the new field.Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (2)
93-93: Use of caster ID in buff removal aligns with new multi-buff logic.This modification correctly provides the NPC’s
ObjectIdwhen removing the buff, ensuring consistency with the refactored buff system that tracks the caster/source ID.
187-194: Confirm whether you intend to check any or all NPCs for the buff.The loop immediately returns once it encounters the first matching NPC, potentially skipping others with the same spawn ID. If you need to confirm the buff’s presence in all NPCs (or at least any NPC), consider refining the iteration logic.
Maple2.Server.Game/Model/Stats.cs (1)
17-25: Enhanced parameter naming for clarity.Renaming the parameter to
statsDictionarymakes the constructor’s intent more explicit. The existing logic to skipPhysicalAtkandMagicalAtkappears consistent with how you set these later based on job code.Maple2.Server.Game/Commands/BuffCommand.cs (6)
58-58: Include caster ID in buff removal.Providing
session.Player.ObjectIdfor the remove operation ensures proper removal of buffs from the correct caster. Good job aligning with the multi-buff removal logic.
62-66: Verify stacking logic across all enumerated buffs.By enumerating and stacking each discovered buff ID instance, you apply multiple stacks at once. Confirm that this reflects the desired behavior (e.g., stacking them all vs. a single buff instance).
81-81: Consistent caster-based buff removal.Again, specifying the caster’s
ObjectIdis consistent with recently introduced buff removal patterns. No issues spotted here.
86-90: Confirm multi-stacking behavior for targeted player.Same as above: stacking is applied to every matching buff instance. Ensure this aggregating approach is intentional.
94-94: Caster ID parameter for self buffs.Including your own
ObjectIdis correct when removing buffs you originally cast on yourself.
99-103: Check aggregated stacking usage.As with the other segments, confirm whether stacking all existing buffs meets your intended buff design or if you’d prefer a single stack approach.
Maple2.Server.Game/PacketHandlers/FallDamageHandler.cs (1)
21-21: Good delegation to the Player model.
Invokingsession.Player.FallDamage(distance)centralizes fall damage logic and improves maintainability by consolidating computations in one place.Maple2.Server.Game/Packets/FieldPacket.cs (4)
7-7: New import is consistent with buff metadata usage.
This import forMaple2.Model.Metadatafollows the updated references to Buff and effect data. No issues here.
41-43: Ensuresession.Ride.Rideis safely initialized.
Checkingsession.Ride.Ride != nullis correct, but double-check that no race conditions can causeRideto be null unexpectedly. Otherwise, this logic cleanly aligns with the newRideManagerdesign.
87-89: Enumerating buffs is a cleaner approach.
UsingEnumerateBuffs()avoids manual dictionary traversal, simplifying code and adhering to the new buff manager workflow. Ensure that the total count fits in ashortif there might be a large number of buffs.
168-172: Confirm usage of the hard-coded "Eff_Test_Regen" string.
While writing a fixed effect string here may be intended, consider sourcing the effect name from NPC metadata if you plan to handle different effect types in the future.Maple2.Server.Game/PacketHandlers/InteractObjectHandler.cs (2)
3-3: Import for metadata usage.
This additionalusingstatement is necessary for accessingInteractObjectMetadataEffectreferences. No concerns here.
116-118: Condition update logic looks consistent.
ApplyingConditionType.interact_objectupon a positiveModifyCodeis logical, but verify it aligns with the intended design for distinguishing object interactions.Maple2.Model/Metadata/AdditionalEffectMetadata.cs (2)
40-41: Check whether negative values are permissible forCooldownTimeandRideId.If negative values have no meaning here, consider enforcing validation or using unsigned types to prevent accidental misuse.
19-19:Details
✅ Verification successful
Validate null handling for
ModifyOverlapCount.Consider ensuring that
ModifyOverlapCountis never null at runtime, or handle the case where metadata ingestion or downstream logic might produce a null reference. Using a nullable array property often invites potential null-safety issues.Please verify any deserialization or ingestion logic that might result in
ModifyOverlapCountbeing null before consumption. If needed, confirm with a short script searching references to this property and ensuring proper handling of null cases.
🏁 Script executed:
#!/bin/bash rg -A 3 "ModifyOverlapCount"Length of output: 2503
Null Handling Verified for ModifyOverlapCount
The review comment raised a valid concern about potential null-safety issues. After reviewing the usage:
Ingestion Mapping: In
Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs, the code assignsModifyOverlapCountto an empty array when the source property is null:ModifyOverlapCount: data.ModifyOverlapCountProperty == null! ? [] : data.ModifyOverlapCountProperty.effectCodesThis ensures that the property is never null during deserialization.
Runtime Usage: In
Maple2.Server.Game/Manager/Config/BuffManager.cs, the code directly accesses the array (e.g., using.Lengthand iterating over it), which confirms the expectation thatModifyOverlapCountis always initialized, thereby avoiding null reference issues.Given that the ingestion logic reliably transforms a potential null into an empty collection, no further changes are required in the metadata definition or downstream usage.
Maple2.Model/Enum/Buff.cs (2)
77-93: Confirm numeric consistency inInvokeEffectTypevalues.There are gaps (e.g., skipping from 3 to 5, then jumping to 23). If these IDs align with legacy data or external references, please ensure all references match the correct numeric mapping.
58-62:Details
❓ Verification inconclusive
Validate code references to renamed values.
ResetEndTickandReplaceare more descriptive than the old naming. Verify all occurrences of the old enum names are updated accordingly to prevent runtime mismatches.
🏁 Script executed:
#!/bin/bash rg -A 2 "BuffResetCondition"Length of output: 1887
Action Required: Confirm Enum Renaming Consistency & Verify "Reset2" Usage
- The grep results confirm that all references to the enum in the codebase now use the updated names—
ResetEndTick,PersistEndTick, andReplace.- In files such as
Maple2.Server.Game/Model/Field/Buff.csandMaple2.Server.Game/Manager/Config/BuffManager.cs, the new descriptive names are consistently applied.- Note: The
Reset2value (with the comment “behaves the same as Reset ??”) remains present. Please confirm that its usage is intentional and that any potential ambiguity regarding its behavior versus a legacyResethas been properly addressed.Maple2.Server.Game/Model/Field/Actor/Actor.cs (2)
109-109: Verify if fallback creation ofDamageRecordTargetmerges or overwrites existing damage.When
DamageDealers.TryGetValuefails, a new record is created. Ensure that existing partial damage is not lost if a concurrency or ordering issue occurs.
117-132: Ensure correct event triggers for complementary damage types.
OnOwnerAttackHitandOnAttackMissare triggered conditionally. Validate coverage for all damage types (e.g., block, reflect). If other event conditions are needed, add triggers or confirm their absence is intentional.Maple2.Model/Game/Ride/RideOnAction.cs (3)
33-33: Ensure constructor parameters align with usage.
The constructor takes anItemobject without any internal checks. If the calling code ensures non-null items, this is fine; otherwise, consider a guard clause to provide clearer errors when a null item is passed.
45-45: Renaming class toRideOnActionBattleis consistent with the enum refactor.
This change aligns with the newRideOnType.Battle. Good job keeping the naming consistent throughout the codebase.
49-49: Constructor maintains consistent approach.
The new parameters (skillIdandskillLevel) fit well for a battle-oriented ride action, mirroring the usage ofRideOnType.Battle.Maple2.Server.Game/Util/SkillUtils.cs (5)
71-71: Extended parameters for event-based checks.
AddingeventType,eventSkillId, andeventBuffIdprovides more nuanced condition handling. Ensure all call sites pass appropriate values to avoid unexpected logic branches.
132-137: Battle mount checks look correct.
The introduction ofOnlyOnBattleMountandAllowOnBattleMountconditions is consistent with the newRideOnType.Battle. The null-safe access (?.) helps avoid runtime issues ifRideis unset.
140-140: Unified condition chaining is maintainable.
Passing the new parameters throughcondition.Caster.Check(...),condition.Owner.Check(...), andcondition.Target.Check(...)ensures each participant is validated with consistent logic. This is a good design choice.
149-173: Buff enumeration logic is robust.
Iterating over buffs to check level, ownership, and stack count is well-structured. The loop already breaks early for performance gains. This approach fits the multi-buff scenario introduced in the PR.
222-232: Event-based buff and skill ID checks.
These conditions further refine the target check by event context. Good use of.Contains(...)to filter out irrelevant buffs or skills. Verify that no partial-match scenario is needed.Maple2.Server.Game/Manager/RideManager.cs (3)
19-23: Properties align with new ride system.
Ride? RideandRideType => ...are succinct ways to centralize ride information. TheRideOnType.Nonedefault is useful; just ensure that logic elsewhere handles the “none” state gracefully.
27-29: Constructor injection ofGameSessionis a clean design.
Holding a reference tosessionwithinRideManagerensures that ride operations have direct access to relevant session context. This fosters a clear separation of concerns.
48-66: Battle mount integration looks consistent.
This method properly checks if a ride is already active, fetches metadata, and broadcasts the mount. Nice usage ofStats.SetBattleMountStatsto apply relevant stats for battle mounts.Maple2.Server.Game/PacketHandlers/RideHandler.cs (3)
56-59: Check for ride eligibility looks solid.
This conditional correctly ensures that a player cannot initiate a ride if the field disallows rides or if the player is already on a ride.
69-85: Default ride check and item validation are well-handled.
The logic cleanly verifies ride type, item expiration, and matching ride IDs before callingsession.Ride.Mount(item). This prevents mismatches and ensures consistency.
87-96: Graceful handling of Stop command for rides.
The checks for a valid field and non-null ride, followed by a call toDismount, look correct. The forced parameter is passed properly, too.Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (5)
163-198: Regeneration loop appears well-structured.
The approach of removing attributes from regen tracking once they are full and resetting timers is clear. Ensure that concurrency or parallel modifications of stats won't cause unexpected states.
421-425: HP consumption triggers regeneration wait properly.
The added logic to start HP regeneration if the player is alive is consistent with the overall regen pattern.
465-470: SP regeneration wait is skipped.
By design, you are not adding a post-consumption wait for Spirit. Please confirm this is intended, as it differs from HP and Stamina.
483-483: Stamina recovery check is consistent.
The condition to only recover if the current stamina is below total is straightforward.
501-506: Stamina consumption logic matches other resources.
Similar to HP logic, the code properly sets up regeneration if not flagged as noRegen.Maple2.Server.Game/Model/Field/Buff.cs (10)
71-80: Buff stacking with reset conditions.
The handling of differentBuffResetConditionvalues is correct. EnsuringPersistEndTickdoesn’t overwrite existing durations is a good approach.
82-85: Event trigger upon reaching max stacks works fine.
InvokingOwner.Buffs.TriggerEvent(...)when stacks hit maximum is well-aligned with buff-based event logic.
89-92: Stack modification with field broadcast.
This appropriately ensures that other players see the updated buff stacks.
117-118: Removing buff on expiration.
The call toOwner.Buffs.Remove(Id, Caster.ObjectId)on expiry is consistent with the new multi-buff structure.
125-130: Skipping proc checks if not ready or not permitted.
The logic to return early iftickCount < NextProcTickor!canProcprevents unnecessary computations.
169-184: Skill application condition might be inverted.
The code skips effect application wheneffect.Condition.Condition.Check(...)returns true. Confirm that continuing upon a true check is intended; otherwise, it might bypass expected effects.
262-263: Correct target selection for DoT buff application.
Switching todotBuff.Targetfor deciding which actor to place the buff on nicely aligns with multi-target buff logic.
276-291: Refined cancellation logic with buff enumeration.
Filtering buffs by ID and categories ensures correct removal. The checks on same-caster are properly respected.
295-312: Dynamic buff duration modifications.
AdjustingEndTickand broadcasting updates is well-structured. The re-enabling ofcanProcifNextProcTick < EndTickis a nice touch.
316-319: Explicit end time updates.
UpdateEndTimeoffers a clean way to adjustEndTickand notify the field.Maple2.Server.Game/Manager/Config/BuffManager.cs (25)
26-26: Confirm concurrency approach for multi-buff storage.
Storing multiple buffs under the same key in aConcurrentDictionary<int, List<Buff>>is correct for multi-caster support. Just ensure that concurrent modifications (e.g., during enumeration) are handled gracefully.
30-30: No immediate concerns for CooldownTimes structure.
Persisting cooldowns in aConcurrentDictionary<int, long>appears fine. Make sure to handle concurrency thoroughly when reading/writing cooldown entries.
50-53: Potential concurrency caution for ResetActor enumeration.
EnumeratingBuffswhile other threads might modify them could skip or omit items. Confirm if external locking or single-threaded invocation is guaranteed.
61-61: EnterField invocation.
CallingEnterField()inLoadFieldBuffs()is consistent with typical field initialization. No issues noted.
73-75: Validate cooldown check logic.
This early return enforces a cooldown ifstartTick < cooldownTick. Ensure server tick synchronization to avoid unintended buff blocking.
86-88: Cooldown assignment and existing buff retrieval look good.
No concerns here. AssigningCooldownTimes[id]before checking for an existing buff is logically consistent.
89-92: Buff replacement logic is valid.
Removing the existing buff first and then re-adding aligns with a clean “Replace” reset condition.
107-110: Verify group-based buff removal intent.
This code removes all buffs in the same group, potentially including buffs from different casters. Confirm that mass removal is desired.
127-127: Proactive stack modification call.
InvokingModifyBuffStackCount()afterSetUpdates()ensures you capture any overlap adjustments. Looks good.
146-148: Guard against repeated exp awarding.
Repeatedly adding static EXP if buffs are re-applied could be exploited unless it’s explicitly intended.
156-156: Initiating mount logic after buff creation.
InvokingSetMount(buff)post-add is sensible, but be mindful of concurrency side effects if the mount operation fails.
159-170: Check whether only returning the first matching buff is intended.
GetBuff()returns the first buff found for a given ID/caster. If multiple buffs from the same caster can exist simultaneously, this might be a limitation.
203-203: Event-type check is straightforward.
Enumerating all buffs and checkingProperty.EventTypeis acceptable. No issues.
307-317: Overlap stack modification.
Modifying stacks for all buffs sharing the providedIdcan be powerful. Ensure it’s not over-modifying (especially if multiple distinct caster buffs exist).
338-338: Resetting skill cooldown.
No concerns. Just confirm you still manage global cooldown or concurrency if multiple buffs reset the same skill.
355-361: Same caster check for cancel logic.
This is correct if you only want to remove buffs matching the same caster. No further concerns.
366-369: Category-based removal.
Removes all buffs of certain categories. Confirm that removing them from every caster is intentional.
372-373: Batch removal approach.
Remove(buffsToRemove.ToArray())is safer than removing in-loop. Good pattern to avoid concurrent modifications.
389-402: LeaveField batch removal.
Removes entrance buffs plus those flaggedRemoveOnLeaveField. This is consistent with exit logic.
404-421: EnterField logic with Pvp constraints.
Buff removal on entering PvP is standard. Good job using a batch approach.
481-485: Bulk remove wrapper.
Method overloading to remove multiple(id, casterId)pairs simplifies external calls. Looks good.
541-548: Death event buff check.
Removing non-KeepOnDeathbuffs is standard. Everything else is re-enabled if needed. No further issues.
551-551: CheckImmunity iteration.
Returnsfalseon the first detected immune conflict. No concerns.
561-563: Filtering out RemoveOnLogout buffs.
Saves only persistent buffs to the cache. Implementation looks good.
567-567: UpdateEnabled usage.
Iterating all buffs to refresh their enabled state is straightforward and aligns with post-death or zone transitions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Maple2.Server.Game/Manager/Config/BuffManager.cs (3)
192-192:⚠️ Potential issueInvalid C# syntax with collection expressions.
The collection expression syntax
[buff]is not valid in older C# versions.Change to:
-return Buffs.TryAdd(buff.Id, [buff]); +return Buffs.TryAdd(buff.Id, new List<Buff> { buff });
365-365:⚠️ Potential issueC# compatibility issue with collection expressions.
The collection initialization syntax
[]is not compatible with older C# versions.Change to:
-List<(int buffId, int casterId)> buffsToRemove = []; +List<(int buffId, int casterId)> buffsToRemove = new();
510-510:⚠️ Potential issueCritical bug in Resistances subtraction.
Using
Math.Min(0, Resistances[attribute] - value)clamps the result to a maximum of 0, potentially forcing negative values when it should prevent them.Change to:
-Resistances[attribute] = Math.Min(0, Resistances[attribute] - value); +Resistances[attribute] = Math.Max(0, Resistances[attribute] - value);
🧹 Nitpick comments (1)
Maple2.Server.Game/Model/Field/Buff.cs (1)
130-130: Minor spacing issue in condition.There's a missing space after the
!canProc ||condition.- if (!canProc ||tickCount < NextProcTick) { + if (!canProc || tickCount < NextProcTick) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)Maple2.Server.Game/Model/Field/Buff.cs(6 hunks)Maple2.Server.Game/Packets/FieldPacket.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.Server.Game/Commands/BuffCommand.cs
- Maple2.Server.Game/Packets/FieldPacket.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
Maple2.Server.Game/Manager/Config/BuffManager.cs (7)
26-26: Validated change for multiple buff support per ID.The change from a dictionary of buffs to a dictionary of lists of buffs is well-implemented and enables the core functionality of this PR - supporting multiple buffs per ID with different casters.
90-107: Well-implemented reset condition handling.The implementation of different buff reset conditions is robust and accounts for all cases:
ResetEndTick: Resets the end timePersistEndTick: Keeps the existing end timeReplace: Removes the existing buff and adds a new oneThis enhances the reliability of buff duration management across the system.
355-362: Good implementation of event-driven buff system.The
TriggerEventmethod enables buffs to apply their effects based on game events, which is a significant enhancement that supports the new ride buffs and other event-based mechanics mentioned in the PR description.
317-329: Well-structured stack count modification.The implementation correctly maintains buff stack counts across related buffs, providing a solid foundation for handling complex buff stacking cases.
331-340: Good integration with ride system.The
SetMountmethod properly integrates buff management with the ride system, aligning with the PR's goal of implementing ride buffs for battle mounts.
493-497: Effective batch removal implementation.The implementation of batch removal for multiple buffs is efficient and prevents collection modification issues during iteration.
543-549: Well-structured internal removal with error logging.The
RemoveInternalmethod is a good practice that centralizes the buff removal logic and adds proper error logging for debugging purposes.Maple2.Server.Game/Model/Field/Buff.cs (5)
41-50: Improved constructor with explicit end tick.The updated constructor now accepts an explicit end tick value rather than calculating it internally, providing more flexibility and supporting the various reset conditions implemented in BuffManager.
78-97: Enhanced stacking implementation with event triggering.The stack method has been significantly improved to:
- Prevent negative stack counts
- Handle stack adjustment amounts
- Trigger events when max stacks are reached
- Return a boolean to indicate whether the stack count was changed
This provides a more robust foundation for buff stacking mechanics.
170-195: Well-implemented event-based skill application.The new
ApplySkillsmethod properly handles conditional skill application based on the event type, target type, and other parameters. This is a key element for the event-driven buff system and aligns well with the PR objectives.
178-186: Corrected target entity handling.The implementation properly addresses the target entity references (Owner, Caster, Target) and applies effects to the appropriate entities. This ensures that buff effects are applied to the correct actors in the game.
317-320: Good implementation of duration modification.The
UpdateEndTimemethod provides a clean way to adjust buff durations and broadcasts the changes to clients. This supports the PR's objectives of implementing more dynamic buff handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Maple2.Server.Game/Manager/Config/BuffManager.cs (2)
363-364: Empty list initialization.
List<(int buffId, int casterId)> buffsToRemove = [];is invalid in older C# versions.
182-193:⚠️ Potential issueImplemented safe buff addition.
The
TryAddmethod ensures buffs aren't duplicated from the same caster and handles both the case of existing and new buff collections.There's a syntax issue in line 192:
-return Buffs.TryAdd(buff.Id, [buff]); +return Buffs.TryAdd(buff.Id, new List<Buff> { buff });The collection expression syntax
[buff]is only available in C# 12 and might not be compatible with older environments. Usenew List<Buff> { buff }instead.
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
317-327: Implemented stack count modification for related buffs.This method efficiently updates stack counts for buffs identified in the metadata and broadcasts the changes to clients.
Consider using LINQ to make this more concise:
-IEnumerable<Buff> buffs = EnumerateBuffs(modifyOverlapCount.Id).Where(b => b.Stack(modifyOverlapCount.OffsetCount)); -foreach (Buff buffResult in buffs) { - Actor.Field.Broadcast(BuffPacket.Update(buffResult)); -} +foreach (Buff buffResult in EnumerateBuffs(modifyOverlapCount.Id).Where(b => b.Stack(modifyOverlapCount.OffsetCount))) { + Actor.Field.Broadcast(BuffPacket.Update(buffResult)); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Maple2.Model/Enum/Skill.cs(2 hunks)Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)Maple2.Server.Game/Manager/ExperienceManager.cs(2 hunks)Maple2.Server.Game/Manager/RideManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(6 hunks)Maple2.Server.Game/Model/Stats.cs(2 hunks)Maple2.Server.Game/PacketHandlers/RideHandler.cs(4 hunks)Maple2.Server.Game/Packets/FieldPacket.cs(5 hunks)Maple2.Server.Game/Util/SkillUtils.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- Maple2.Server.Game/Manager/ExperienceManager.cs
- Maple2.Server.Game/Model/Stats.cs
- Maple2.Model/Enum/Skill.cs
- Maple2.Server.Game/Packets/FieldPacket.cs
- Maple2.Server.Game/Commands/BuffCommand.cs
- Maple2.Server.Game/Manager/RideManager.cs
- Maple2.Server.Game/PacketHandlers/RideHandler.cs
- Maple2.Server.Game/Model/Field/Buff.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (7)
Maple2.Server.Game/Model/Field/Buff.cs (11)
Buff(13-334)Buff(41-59)ResetActor(61-68)UpdateEndTime(70-76)UpdateEndTime(312-315)Stack(78-97)Update(106-135)Disable(149-152)ApplySkills(170-195)ModifyDuration(294-310)UpdateEnabled(137-147)Maple2.Server.Game/Manager/StatsManager.cs (2)
ResetActor(146-148)Refresh(110-144)Maple2.Server.Game/Packets/BuffPacket.cs (1)
BuffPacket(10-54)Maple2.Server.Game/Model/Field/Actor/Actor.cs (6)
Update(205-222)Actor(24-249)Actor(56-67)IActor(196-203)Reflect(137-154)OnDeath(246-248)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (4)
Update(140-201)FieldPlayer(15-573)FieldPlayer(94-107)OnDeath(319-329)Maple2.Server.Game/Manager/RideManager.cs (3)
Mount(31-50)Mount(52-71)Dismount(73-91)Maple2.Server.Game/Manager/Config/ConfigManager.cs (1)
SetSkillCooldown(185-192)
🔇 Additional comments (26)
Maple2.Server.Game/Util/SkillUtils.cs (6)
71-71: Extended method signature to support event-based condition checking.The
Checkmethod now includes optional parameters for event type, skill ID, and buff ID, enhancing the condition system to filter based on event context. This change supports more granular control over when skills and buffs can be triggered.
132-137: Good addition of mount-related condition checks.These new conditions allow skills to be properly restricted based on whether a player is on a battle mount, enhancing gameplay balance by controlling which abilities can be used while mounted.
140-140: Updated call chain to pass event parameters.This line ensures event context is properly propagated to the nested condition checks, maintaining consistency throughout the condition checking system.
143-171: Improved buff checking to support multiple buffs per ID.The buff checking logic has been enhanced to iterate through all buffs with the same ID, properly checking level, ownership, and stack count requirements. This refactoring aligns with the buff manager changes to support multiple buff instances from different casters.
158-168: Comprehensive stack comparison implementation.The comparison logic now properly handles all CompareType cases when evaluating buff stacks, making the condition system more flexible for different gameplay requirements.
221-230: Added event condition verification.These new checks validate that the event type, buff ID, and skill ID match the expected values in the condition, enabling more precise control over when effects apply based on specific triggering events.
Maple2.Server.Game/Manager/Config/BuffManager.cs (20)
26-26: Restructured buff storage to support multiple buffs per ID.Changed from storing a single buff to storing a list of buffs for each ID. This architectural change allows the game to properly handle multiple instances of the same buff from different casters.
30-30: Added cooldown tracking for buffs.The new
CooldownTimesdictionary tracks when buffs can be reapplied, preventing rapid reapplication and better controlling buff frequency. The TODO comment suggests this might be moved to a cache later.
50-54: Updated ResetActor to handle nested buff structure.The method now correctly iterates through all buff instances for each ID, ensuring all references are properly updated when an actor changes.
73-75: Added buff cooldown enforcement.This check prevents buffs from being reapplied before their cooldown expires, preventing potential exploits and ensuring balanced gameplay.
86-106: Implemented buff reset condition handling.The code now properly handles different reset behaviors when reapplying buffs:
- ResetEndTick: Resets the duration
- PersistEndTick: Maintains the original end time
- Replace: Removes the existing buff before adding the new one
This provides more control over buff behavior and supports different gameplay mechanics.
117-121: Improved group buff handling.The code now properly handles removal of all buffs in the same group when adding a new buff of that group, ensuring that mutually exclusive buffs don't coexist.
126-129: Added error logging for failed buff additions.This logging will help identify and diagnose issues with buff application in production environments.
169-180: Implemented targeted buff retrieval.The new
GetBuffmethod retrieves a specific buff by ID and optionally by caster ID, supporting the individual buff tracking needed for the new buff system.
195-196: Added convenient buff enumeration methods.These methods simplify accessing all buffs or buffs with a specific ID, hiding the complexity of the nested collection structure.
329-338: Added ride buff support.The new
SetMountmethod handles mounting logic for buffs that grant rides, including error handling and automatic removal if mounting fails.
353-360: Implemented event-based buff triggers.The new
TriggerEventmethod allows buffs to apply skill effects based on specific events, supporting the event condition system added toSkillUtils.
371-372: Updated buff removal to track caster ID.The code now tracks both buff ID and caster ID when removing buffs, allowing removal of specific buff instances from specific casters.
382-383: Batch buff removal.Using a single call to
Removewith an array of buff IDs and caster IDs improves performance by batching removals.
385-389: Enhanced duration modification for multiple buffs.The method now correctly handles duration changes for all instances of a buff with the same ID.
401-411: Improved field exit buff cleanup.The code now properly collects and batch-removes buffs that should be removed when leaving a field.
419-430: Enhanced PvP zone entry handling.The code now collects and batch-removes buffs that should be removed when entering a PvP zone.
455-471: Improved item buff removal.The code now properly collects and batch-removes buffs when unequipping items with buffs.
490-527: Comprehensive buff removal implementation.The method now properly handles removal of specific buff instances by caster ID, updating resistances, and refreshing stats when needed.
507-507: Fixed resistance subtraction.Using
Math.Max(0, Resistances[attribute] - value)ensures resistance values don't go negative.
540-546: Added internal buff removal helper.The new helper method cleanly encapsulates the logic to remove a buff from the nested collection structure, with proper error logging.
Revert previous commit
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Maple2.Server.Game/Manager/Config/BuffManager.cs (4)
195-196: New enumeration methods for the nested buff structure.These methods provide a convenient way to flatten the nested buff structure for iteration and filtering. The second method specifically retrieves all buffs with a given ID. Note that the empty collection initializer
[]on line 196 also uses C# 12 syntax.Following the previous reviewer's comment, consider using an IEnumerable return type instead of creating a new List:
-public List<Buff> EnumerateBuffs() => Buffs.Values.SelectMany(list => list).ToList(); -public List<Buff> EnumerateBuffs(int buffId) => Buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : []; +public IEnumerable<Buff> EnumerateBuffs() => Buffs.Values.SelectMany(list => list); +public IEnumerable<Buff> EnumerateBuffs(int buffId) => Buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : Enumerable.Empty<Buff>();This avoids unnecessary list allocations when enumeration is all that's needed.
510-510: Fixed resistance calculation.This uses
Math.Max(0, Resistances[attribute] - value)to prevent resistances from going below 0, which is the correct approach.
365-365:⚠️ Potential issueEmpty list initialization with C# 12 syntax.
The code uses the C# 12 collection initializer syntax
[]which is not compatible with older C# versions.-List<(int buffId, int casterId)> buffsToRemove = []; +List<(int buffId, int casterId)> buffsToRemove = new();
182-193:⚠️ Potential issueIncorrect syntax in TryAdd method.
There's a syntax error in the TryAdd method. The collection initializer syntax
[buff](line 192) is only valid in C# 12 and newer. For broader compatibility, you should usenew List<Buff> { buff }instead.-return Buffs.TryAdd(buff.Id, [buff]); +return Buffs.TryAdd(buff.Id, new List<Buff> { buff });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Maple2.Model/Enum/Skill.cs(2 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)Maple2.Server.Game/Manager/ExperienceManager.cs(2 hunks)Maple2.Server.Game/Manager/RideManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(6 hunks)Maple2.Server.Game/Model/Stats.cs(2 hunks)Maple2.Server.Game/PacketHandlers/RideHandler.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Maple2.Server.Game/Manager/ExperienceManager.cs
- Maple2.Model/Enum/Skill.cs
- Maple2.Server.Game/Model/Stats.cs
- Maple2.Server.Game/Manager/RideManager.cs
- Maple2.Server.Game/PacketHandlers/RideHandler.cs
- Maple2.Server.Game/Model/Field/Buff.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (21)
Maple2.Server.Game/Manager/Config/BuffManager.cs (21)
26-26: Significant architecture change to support multiple buffs per ID.The
Buffsdictionary structure has been changed from mapping a buff ID to a single buff, to mapping a buff ID to a list of buffs. This allows multiple buff instances with the same ID but from different casters to exist simultaneously, which is a significant improvement for buff management.
30-30: Cooldown tracking implementation.A new
CooldownTimesdictionary has been added to prevent premature reapplication of buffs. This is a good practice to ensure buffs respect their cooldown periods.
50-54: Updated ResetActor to handle nested buff lists.The ResetActor method has been correctly updated to accommodate the new nested structure of the Buffs dictionary. It now properly iterates through all buffs in each inner list.
73-75: Respect buff cooldowns.Good implementation of cooldown checking before applying a buff. This prevents buffs from being reapplied before their cooldown period has elapsed.
86-106: Improved buff duration handling with reset conditions.This switch statement properly handles different buff reset conditions:
- ResetEndTick: Uses the new duration
- PersistEndTick: Maintains the existing end time
- Replace: Removes the existing buff first
This more granular control over buff durations is a significant improvement.
117-118: Group buff handling refactored for multiple buffs per ID.The code now properly retrieves all buffs in the same group using the new
EnumerateBuffs()method, ensuring that group-exclusive buffs work correctly even with the new multi-buff structure.
124-128: New buff creation and addition with error handling.The code now uses the new
TryAddmethod and includes appropriate error logging when a buff cannot be added. This improves robustness and makes troubleshooting easier.
169-180: New GetBuff method with caster filtering.This new method retrieves a buff by ID with optional filtering by caster ID, which is essential for the new multi-buff architecture. The method properly handles the case where casterObjectId is provided or not.
317-329: New ModifyBuffStackCount method for buff stack manipulation.This new method handles modifying stack counts of related buffs when a buff is added. It properly uses the new EnumerateBuffs method to get all buffs with a specific ID, then updates their stack counts accordingly.
331-340: New SetMount method for ride handling.This method checks if the buff grants a ride and attempts to mount the player. If mounting fails, it removes the buff and logs an error. This is a good defensive programming practice.
355-362: New TriggerEvent method for event-based skill application.This method handles triggering skills based on specific events, properly iterating through all buffs using EnumerateBuffs. It's a clean implementation that ensures events are processed correctly for all active buffs.
367-374: Updated CancelBuffs to handle multiple buffs per ID.This loop now properly iterates through all buffs with a specific ID using EnumerateBuffs, applying caster checking as needed before adding them to the removal list.
387-392: New ModifyDuration method with multi-buff support.This method correctly modifies the duration of all buffs with a specific ID, using the EnumerateBuffs method to get all matching buffs.
400-414: LeaveField method updated for batch buff removal.The method now collects all buffs to be removed in a list first, then removes them in batch. This avoids modifying the collection during iteration, which is a good practice.
493-497: New Remove method for batch removal.This method allows removing multiple buffs at once by ID and caster ID, which is useful for operations like leaving a field or unequipping an item that might remove multiple buffs.
499-542: Updated Remove method with caster ID filtering.This significantly updated Remove method now properly handles removing buffs by both ID and caster ID. It collects buffs to remove, updates resistances, marks for stats refresh, and broadcasts removal to the field. The code is well-structured and handles all the necessary cleanup.
543-549: New RemoveInternal helper method with error logging.This helper method handles the actual removal of a buff from the internal collection, with proper error logging if removal fails. This makes debugging easier when buff removal issues occur.
553-560: Updated OnDeath method for multi-buff handling.The method now correctly iterates through all buffs using EnumerateBuffs and removes those that shouldn't persist after death.
563-570: Updated CheckImmunity method for multi-buff handling.This method now iterates through all buffs to check if any provide immunity to the new buff, which is consistent with the multi-buff architecture.
573-576: Updated GetSaveCacheBuffs method for multi-buff handling.The method now correctly uses EnumerateBuffs to get all buffs that should be saved, filtering out those that should be removed on logout.
579-581: Updated UpdateEnabled method for multi-buff handling.This method now correctly updates the enabled status of all buffs using EnumerateBuffs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
Maple2.Server.Game/Manager/Config/BuffManager.cs (9)
195-196: Consider returning IEnumerable for better performance.Creating a new list with
ToList()in frequently called methods can impact performance. Consider returningIEnumerable<Buff>for better efficiency.-public List<Buff> EnumerateBuffs() => Buffs.Values.SelectMany(list => list).ToList(); -public List<Buff> EnumerateBuffs(int buffId) => Buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : new(); +public IEnumerable<Buff> EnumerateBuffs() => Buffs.Values.SelectMany(list => list); +public IEnumerable<Buff> EnumerateBuffs(int buffId) => Buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : Enumerable.Empty<Buff>();
516-516: Correctly fixed the resistance subtraction.The code now uses
Math.Max(0, Resistances[attribute] - value)which properly prevents resistances from becoming negative.
192-192:⚠️ Potential issueFix C# collection expression syntax for older versions.
The collection expression syntax
[buff]is not supported in older C# versions and will cause compilation errors.-return Buffs.TryAdd(buff.Id, [buff]); +return Buffs.TryAdd(buff.Id, new List<Buff> { buff });
196-196:⚠️ Potential issueFix C# collection expression syntax for older versions.
The empty collection expression syntax
[]is not supported in older C# versions and will cause compilation errors.-public List<Buff> EnumerateBuffs(int buffId) => Buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : []; +public List<Buff> EnumerateBuffs(int buffId) => Buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : new();
371-371:⚠️ Potential issueFix C# collection expression syntax for older versions.
The empty collection expression syntax
[]is not supported in older C# versions and will cause compilation errors.-List<(int buffId, int casterId)> buffsToRemove = []; +List<(int buffId, int casterId)> buffsToRemove = new();
508-508:⚠️ Potential issueFix C# collection expression syntax for older versions.
The empty collection expression syntax
[]is not supported in older C# versions and will cause compilation errors.-List<Buff> buffsToRemove = []; +List<Buff> buffsToRemove = new();
428-428:⚠️ Potential issueFix C# collection expression syntax for older versions.
The empty collection expression syntax
[]is not supported in older C# versions and will cause compilation errors.-List<(int id, int casterId)> buffsToRemove = []; +List<(int id, int casterId)> buffsToRemove = new();
410-410:⚠️ Potential issueFix C# collection expression syntax for older versions.
The empty collection expression syntax
[]is not supported in older C# versions and will cause compilation errors.-List<(int id, int casterId)> buffsToRemove = []; +List<(int id, int casterId)> buffsToRemove = new();
464-464:⚠️ Potential issueFix C# collection expression syntax for older versions.
The empty collection expression syntax
[]is not supported in older C# versions and will cause compilation errors.-List<(int id, int casterId)> buffsToRemove = []; +List<(int id, int casterId)> buffsToRemove = new();
🧹 Nitpick comments (2)
Maple2.Server.Game/Manager/Config/BuffManager.cs (2)
337-346: Add additional error handling for failed mounting.When mounting fails, the buff is removed but there's no further error handling or recovery mechanism. Consider adding more robust error handling or fallback options.
private void SetMount(Buff buff) { if (buff.Metadata.Property.RideId == 0 || Actor is not FieldPlayer player) { return; } if (!player.Session.Ride.Mount(buff.Metadata)) { logger.Error("Failed to mount {Id} on {Object}", buff.Id, Actor.ObjectId); + // Consider notifying the player about the mounting failure + if (player.Session.Connected) { + player.Session.Send(UserPacket.MountFailed(buff.Metadata.Property.RideId)); + } Remove(buff.Id, Actor.ObjectId); } }
549-556: Improve error handling in RemoveInternal.The method logs an error if buff removal fails but doesn't provide a way to handle or recover from this situation. Consider adding more comprehensive error handling.
void RemoveInternal(Buff buffToRemove) { if (Buffs.TryGetValue(buffToRemove.Id, out List<Buff>? buffList)) { if (!buffList.Remove(buffToRemove)) { logger.Error("Failed to remove buff {Id} from {Object}", buffToRemove.Id, Actor.ObjectId); + // Consider additional error handling steps here, such as: + // 1. Force removal by recreating the list without the problematic buff + // 2. Track failed removals for later cleanup } + // Clean up empty lists to prevent memory leaks + if (buffList.Count == 0) { + Buffs.TryRemove(buffToRemove.Id, out _); + } } + else { + logger.Warning("Attempted to remove non-existent buff {Id} from {Object}", buffToRemove.Id, Actor.ObjectId); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
Maple2.Server.Game/Manager/Config/BuffManager.cs (5)
26-26: Good implementation of multi-buff architecture.The change from
ConcurrentDictionary<int, Buff>toConcurrentDictionary<int, List<Buff>>enables multiple buffs with the same ID from different casters, a significant architecture improvement.
73-75: Good addition of cooldown enforcement.Adding cooldown checks prevents premature buff reapplication, improving game balance and preventing abuse.
90-107: Good implementation of diverse buff reset conditions.The switch statement properly handles the different reset conditions with distinct behavior for each case, providing flexibility in buff management.
499-503: Well-designed removal system for multiple buffs.The method to remove multiple buffs with different caster IDs is concise and properly handles the removal by delegating to the instance-specific removal method.
361-367: Good implementation of buff event triggering.The
TriggerEventmethod properly iterates through all buffs and dispatches the event to enabled buffs, providing a clean mechanism for event-based buff effects.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Maple2.Server.Game/Manager/Config/BuffManager.cs (4)
464-479:⚠️ Potential issueItem buff removal with batch processing.
This properly collects and removes buffs from items in a batch operation.
The empty collection initialization syntax
[]is not compatible with older C# versions:-List<(int id, int casterId)> buffsToRemove = []; +List<(int id, int casterId)> buffsToRemove = new();
407-420:⚠️ Potential issueEnhanced LeaveField implementation with null checking.
The method now properly checks if the field is null before attempting to process buffs, and uses batch removal.
The empty collection initialization syntax
[]is not compatible with older C# versions:-List<(int id, int casterId)> buffsToRemove = []; +List<(int id, int casterId)> buffsToRemove = new();
182-193:⚠️ Potential issueAdded safe buff addition to prevent duplicates.
The
TryAddmethod properly prevents duplicate buffs from the same caster ID.The collection initialization syntax
[buff]is not compatible with older C# versions, as noted in past review comments:-return Buffs.TryAdd(buff.Id, [buff]); +return Buffs.TryAdd(buff.Id, new List<Buff> { buff });
371-371:⚠️ Potential issueCollection initialization syntax issue.
The empty collection initialization syntax
[]is not compatible with older C# versions:-List<(int buffId, int casterId)> buffsToRemove = []; +List<(int buffId, int casterId)> buffsToRemove = new();
🧹 Nitpick comments (6)
Maple2.Server.Game/Manager/Config/BuffManager.cs (6)
30-30: Implement caching for CooldownTimes as noted in TODO.The cooldown tracking system is a good addition, but the TODO comment indicates it needs caching to improve performance.
Consider implementing an expiration mechanism or background cleanup for outdated cooldown entries:
-public ConcurrentDictionary<int, long> CooldownTimes { get; } = new(); // TODO: Cache this +public ConcurrentDictionary<int, long> CooldownTimes { get; } = new(); + +// Add a method to periodically clean up expired cooldowns +public void CleanupExpiredCooldowns(long currentTick) { + List<int> expiredCooldowns = CooldownTimes.Where(kvp => kvp.Value < currentTick) + .Select(kvp => kvp.Key) + .ToList(); + foreach (int id in expiredCooldowns) { + CooldownTimes.TryRemove(id, out _); + } +}
87-107: Added comprehensive buff reset condition handling.The implementation correctly handles different reset conditions for buff durations, improving flexibility for game design.
Clean up the commented-out code that's redundant with previously set values:
switch (additionalEffect.Property.ResetCondition) { case BuffResetCondition.ResetEndTick: case BuffResetCondition.Reset2: // This isn't correct, but it seems to behave VERY close to ResetEndTick - // endTick = startTick + durationMs; break; case BuffResetCondition.PersistEndTick: if (existing != null) { endTick = existing.EndTick; } break; case BuffResetCondition.Replace: if (existing != null) { Remove(existing.Id, existing.Caster.ObjectId); existing = null; } - //endTick = startTick + durationMs; break; }
116-122: Improved buff group removal logic.The code correctly disables and removes all buffs in the same group before adding a new one.
Clarify the purpose of calling
Disable()by removing or explaining the uncertainty:if (additionalEffect.Property.Group > 0) { List<Buff> buffs = EnumerateBuffs().Where(b => b.Metadata.Property.Group == additionalEffect.Property.Group).ToList(); foreach (Buff existingBuff in buffs) { - existingBuff.Disable(); // Disable? + existingBuff.Disable(); // Disable buff effects before removal owner.Field.Broadcast(BuffPacket.Remove(existingBuff)); } }
169-180: Buff retrieval by ID and caster ID added.The
GetBuffmethod enables selective retrieval of buffs, supporting the multi-buff architecture.Simplify the method to reduce code duplication:
private Buff? GetBuff(int buffId, int casterObjectId = 0) { - if (casterObjectId > 0) { - if (Buffs.TryGetValue(buffId, out List<Buff>? buffs)) { - return buffs.FirstOrDefault(buff => buff.Caster.ObjectId == casterObjectId); - } - } else { - if (Buffs.TryGetValue(buffId, out List<Buff>? buffs)) { - return buffs.FirstOrDefault(); - } - } - return null; + if (Buffs.TryGetValue(buffId, out List<Buff>? buffs)) { + return casterObjectId > 0 + ? buffs.FirstOrDefault(buff => buff.Caster.ObjectId == casterObjectId) + : buffs.FirstOrDefault(); + } + return null; }
428-439: PvP zone buff handling with potential duplicate removals.The code currently adds buffs to the removal list twice if they meet both conditions. While the
Removemethod should handle duplicates, it's cleaner to avoid adding duplicates in the first place:List<(int id, int casterId)> buffsToRemove = new(); foreach (Buff buff in EnumerateBuffs()) { - if (buff.Metadata.Property.RemoveOnPvpZone) { + if (buff.Metadata.Property.RemoveOnPvpZone || !buff.Metadata.Property.KeepOnEnterPvpZone) { buffsToRemove.Add((buff.Id, Actor.ObjectId)); - } - - if (!buff.Metadata.Property.KeepOnEnterPvpZone) { - buffsToRemove.Add((buff.Id, Actor.ObjectId)); } }Also fix the collection initialization:
-List<(int id, int casterId)> buffsToRemove = []; +List<(int id, int casterId)> buffsToRemove = new();
505-547: Enhanced buff removal with resistance and stat handling.The method now properly handles the removal of multiple buffs with the same ID but different casters.
- Return a meaningful boolean indicating whether any buffs were actually removed:
public bool Remove(int id, int casterId) { //TODO: Check if buff is removable/should be removed bool refreshStats = false; - List<Buff> buffsToRemove = []; + List<Buff> buffsToRemove = new(); // ... existing code ... foreach (Buff buff in buffsToRemove) { RemoveInternal(buff); Actor.Field.Broadcast(BuffPacket.Remove(buff)); // ... existing code ... } if (refreshStats) { Actor.Stats.Refresh(); } - return true; + return buffsToRemove.Count > 0;
- Fix the collection initialization syntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (11)
Maple2.Server.Game/Manager/Config/BuffManager.cs (11)
26-27: Correct data structure modification for multiple buff support.The change from
ConcurrentDictionary<int, Buff>toConcurrentDictionary<int, List<Buff>>properly enables multiple buffs with the same ID from different casters, aligning with the PR objectives.
73-75: Proper cooldown enforcement added.The check prevents applying a buff that's still on cooldown, increasing system robustness.
125-129: New buff creation and addition with proper error logging.The addition of error logging makes troubleshooting failed buff additions much easier.
166-166: Ride mounting support for battle buffs.This call to
SetMountproperly implements the PR requirement for ride buffs, specifically for battle mounts.
198-216: Updated HasBuff method for multi-buff support.The method properly checks all buffs with the given ID against the specified level and stack criteria.
323-335: Buff stack count modification functionality added.This method properly implements the ability to modify stack counts for related buffs.
337-346: Added mount functionality with error handling.This method properly sets up mounts from buffs and handles failures by removing the buff and logging errors.
361-368: New event triggering system for buff skills.This method enables buffs to trigger skills based on events, supporting the PR objective for enhanced buff handling.
390-390: Batch removal of buffs.This approach efficiently processes multiple buffs for removal in a batch operation.
499-503: Added batch buff removal method.This method efficiently processes multiple buffs for removal in a batch operation.
549-556: Improved internal buff removal with error logging.The
RemoveInternalmethod properly logs failures when removing buffs from the collection.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
372-372: Collection initializer syntax compatibility issue.The collection initializer syntax
[]is only available in C# 12 and may not be compatible with older versions of C#.-List<(int buffId, int casterId)> buffsToRemove = []; +List<(int buffId, int casterId)> buffsToRemove = new();
🧹 Nitpick comments (3)
Maple2.Server.Game/Manager/Config/BuffManager.cs (3)
26-26: Collection initializer syntax compatibility issue.The collection initializer syntax
[]is only available in C# 12 and may not be compatible with older versions of C#.-private readonly ConcurrentDictionary<int, List<Buff>> buffs = []; +private readonly ConcurrentDictionary<int, List<Buff>> buffs = new();
96-113: Comprehensive buff reset condition handling.The detailed switch statement for handling different buff reset conditions is good, but there are two commented-out code lines and an unclear comment about
Reset2behavior.switch (additionalEffect.Property.ResetCondition) { case BuffResetCondition.ResetEndTick: - case BuffResetCondition.Reset2: // This isn't correct, but it seems to behave VERY close to ResetEndTick - // endTick = startTick + durationMs; + case BuffResetCondition.Reset2: // Behaves similarly to ResetEndTick break; case BuffResetCondition.PersistEndTick: if (existing != null) { endTick = existing.EndTick; } break; case BuffResetCondition.Replace: if (existing != null) { Remove(existing.Id, existing.Caster.ObjectId); existing = null; } - //endTick = startTick + durationMs; break; }
199-200: Efficient buff enumeration methods.These methods provide a clean API for accessing buffs. However, there's a compatibility issue with the empty collection initializer.
public List<Buff> EnumerateBuffs() => buffs.Values.SelectMany(list => list).ToList(); -public List<Buff> EnumerateBuffs(int buffId) => this.buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : []; +public List<Buff> EnumerateBuffs(int buffId) => this.buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : new List<Buff>();Consider using IEnumerable instead of List for better efficiency if the results don't need to be materialized immediately:
-public List<Buff> EnumerateBuffs() => buffs.Values.SelectMany(list => list).ToList(); +public IEnumerable<Buff> EnumerateBuffs() => buffs.Values.SelectMany(list => list);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Commands/PlayerCommand.cs(1 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)Maple2.Server.Game/Model/Field/Buff.cs(6 hunks)Maple2.Server.Game/PacketHandlers/JobHandler.cs(1 hunks)Maple2.Server.Game/Util/NpcTalkUtil.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Maple2.Server.Game/Util/NpcTalkUtil.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.Server.Game/Commands/BuffCommand.cs
- Maple2.Server.Game/Model/Field/Buff.cs
🧰 Additional context used
🧬 Code Graph Analysis (2)
Maple2.Server.Game/Commands/PlayerCommand.cs (2)
Maple2.Model/Game/User/Player.cs (2)
Player(8-22)Player(17-21)Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
Clear(57-61)
Maple2.Server.Game/PacketHandlers/JobHandler.cs (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
Clear(57-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (15)
Maple2.Server.Game/PacketHandlers/JobHandler.cs (1)
93-93: Improved buff clearing during job advancement.The change from accessing the internal buff collection directly to using the
Clear()method improves encapsulation and aligns with the refactored buff management system that now supports multiple buffs per ID from different casters.Maple2.Server.Game/Commands/PlayerCommand.cs (1)
175-175: Improved buff clearing during job change command.The change from accessing the internal buff collection directly to using the
Clear()method improves encapsulation and aligns with the refactored buff management system that now supports multiple buffs per ID from different casters.Maple2.Server.Game/Manager/Config/BuffManager.cs (13)
30-30: New cooldown tracking added.Good addition of cooldown tracking for buffs. Consider removing the TODO comment once the caching strategy is determined.
57-61: Well-implemented buff clearing method.Good implementation of the
Clear()method that properly removes all buffs by their ID and caster's object ID. This method is now used by both job advancement code paths.
79-81: Effective cooldown enforcement.Good addition of cooldown checks before adding new buffs.
130-132: Good error logging for buff addition failures.Proper error logging when buff addition fails.
173-184: Well-implemented buff retrieval with caster filtering.The new
GetBuffmethod provides a clean way to retrieve buffs with optional caster filtering, aligning with the multi-buff per ID architecture.
186-197: Well-implemented buff addition with duplicate prevention.The
TryAddmethod correctly prevents duplicate buffs from the same caster and logs errors when addition fails.However, the collection initializer syntax
[buff]on line 196 is only available in C# 12 and may not be compatible with older versions.-return this.buffs.TryAdd(buff.Id, [buff]); +return this.buffs.TryAdd(buff.Id, new List<Buff> { buff });
202-220: Fixed and improved HasBuff method.The
HasBuffmethod now correctly checks all buffs with the same ID, using proper logical conditions for level and stack checks.
327-336: Well-designed stack modification method.Good implementation of modifying stack counts for related buffs. The method correctly uses the
EnumerateBuffsmethod and broadcasts updates when stacks are modified.
338-347: Robust mount handling with error recovery.The
SetMountmethod includes proper error handling by removing the buff if mounting fails, which prevents inconsistent states.
362-368: Enhanced event triggering for multiple buffs.The
TriggerEventmethod correctly iterates through all buffs using the new enumeration method and applies skills for each active buff.
389-390: Efficient batch removal of buffs.Good implementation of batch removal of buffs by passing multiple buff IDs and caster IDs.
495-499: Well-designed buff removal method.The method correctly handles multiple buff removals by iterating through the provided ID and caster pairs.
501-552: Comprehensive buff removal with proper cleanup.The buff removal implementation correctly:
- Handles caster-specific buffs
- Updates resistances
- Refreshes stats when needed
- Cleans up related resources (Reflect, Invokes, Compulsions)
- Handles ride-specific buffs
- Logs errors if removal fails
This provides a robust and thorough approach to buff removal.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
372-373: Empty list initialization compatibility issue.The empty collection initialization syntax
[]is not compatible with older C# versions.-List<(int buffId, int casterId)> buffsToRemove = []; +List<(int buffId, int casterId)> buffsToRemove = new();
🧹 Nitpick comments (4)
Maple2.Server.Game/Manager/Config/BuffManager.cs (4)
26-26: Collection initialization syntax compatibility issue.The empty collection initialization syntax
[]is not compatible with older C# versions. This could cause compilation issues if targeting an older runtime.-private readonly ConcurrentDictionary<int, List<Buff>> buffs = []; +private readonly ConcurrentDictionary<int, List<Buff>> buffs = new();
93-113: Enhanced buff reset condition handling.The switch statement properly handles different reset conditions for buffs. However, there are commented out lines and a note about
BuffResetCondition.Reset2behavior that could use clarification.- case BuffResetCondition.ResetEndTick: - case BuffResetCondition.Reset2: // This isn't correct, but it seems to behave VERY close to ResetEndTick - // endTick = startTick + durationMs; + case BuffResetCondition.ResetEndTick: + case BuffResetCondition.Reset2: // Uses default behavior similar to ResetEndTick + // Default calculation: endTick = startTick + durationMs (already set above) break;Also, consider removing this commented line as it's redundant:
- //endTick = startTick + durationMs;
186-197: New TryAdd method prevents duplicate buffs from the same caster.This method correctly handles both adding to an existing list and creating a new list. However, it also uses the C# 12 collection initialization syntax which might not be compatible with older C# versions.
- return this.buffs.TryAdd(buff.Id, [buff]); + return this.buffs.TryAdd(buff.Id, new List<Buff> { buff });
199-200: New EnumerateBuffs methods for buff collection access.These methods provide a clean way to access buffs, but have two issues:
- The collection initialization syntax is not compatible with older C# versions
- Returning IEnumerable instead of materializing a List immediately could be more efficient
-public List<Buff> EnumerateBuffs() => buffs.Values.SelectMany(list => list).ToList(); -public List<Buff> EnumerateBuffs(int buffId) => this.buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : []; +public IEnumerable<Buff> EnumerateBuffs() => buffs.Values.SelectMany(list => list); +public List<Buff> EnumerateBuffs(int buffId) => this.buffs.TryGetValue(buffId, out List<Buff>? buffs) ? buffs : new List<Buff>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (9)
Maple2.Server.Game/Model/Field/Buff.cs (8)
Buff(13-338)Buff(41-59)UpdateEndTime(70-76)UpdateEndTime(316-319)Stack(78-97)Update(106-135)ApplySkills(170-195)ModifyDuration(294-314)Maple2.Server.Game/Model/Skill/InvokeRecord.cs (2)
InvokeRecord(5-16)InvokeRecord(12-15)Maple2.Tools/Extensions/EnumerableExtensions.cs (1)
RemoveAll(99-104)Maple2.Server.Game/Packets/BuffPacket.cs (1)
BuffPacket(10-54)Maple2.Server.Game/Model/Field/Actor/Actor.cs (4)
Update(205-222)Actor(24-249)Actor(56-67)OnDeath(246-248)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (4)
Update(140-201)FieldPlayer(15-573)FieldPlayer(94-107)OnDeath(319-329)Maple2.Server.Game/Manager/ExperienceManager.cs (1)
AddStaticExp(153-161)Maple2.Server.Game/Manager/RideManager.cs (3)
Mount(31-50)Mount(52-71)Dismount(73-91)Maple2.Server.Game/Manager/Config/ConfigManager.cs (1)
SetSkillCooldown(185-192)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
Maple2.Server.Game/Manager/Config/BuffManager.cs (9)
57-60: Improved Clear method using EnumerateBuffs.This implementation now properly uses the new collection structure, ensuring buffs are correctly removed with their caster IDs.
79-81: Added cooldown check for buff application.This is a good enhancement that prevents rapid reapplication of buffs while they're still on cooldown. This helps enforce proper buff timing and prevents potential exploits.
173-184: New GetBuff method with caster filtering.Good implementation that supports the multi-buff architecture. The method efficiently filters buffs by caster ID when needed.
202-220: Updated HasBuff implementation handles multiple buffs per ID.The method now properly checks all buffs with the given ID, supporting the multi-buff architecture. The loop implementation is correct, continuing if a buff doesn't meet the criteria and returning true as soon as a matching buff is found.
327-336: New ModifyBuffStackCount method for stack manipulation.This method efficiently updates stack counts of related buffs. Good use of LINQ with Where to filter buffs that were successfully stacked before broadcasting the update.
338-347: New SetMount method for handling ride buffs.This implementation properly connects buff effects to the ride system, with good error handling that removes the buff if mounting fails. This prevents getting into an inconsistent state where a buff is applied but the mount operation failed.
362-368: New TriggerEvent method for applying conditional buff effects.This is a key enhancement that enables buffs to trigger effects based on events. Works well with the Event Condition Type system mentioned in the PR objectives.
495-499: New Remove method for batch buff removal.This is an excellent addition that enables efficient batch removal of buffs. The implementation correctly iterates through the array of tuples and calls the individual Remove method for each.
575-577: Improved GetSaveCacheBuffs using EnumerateBuffs.This method now correctly uses the EnumerateBuffs method, making it compatible with the multi-buff architecture.
Fixes #331
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores