Cache Buffs and Skill Cooldowns - #409
Conversation
WalkthroughThe pull request removes legacy handling of skill cooldowns from character configuration and updates database storage methods accordingly. It expands metadata for additional effects and skills by adding properties related to in-game time and cooldown management. Buff and effect application methods now require a unified tick parameter for improved timing precision. New RPC endpoints and supporting classes are added for persisting player configuration (buffs and cooldowns), and dependency injection is updated to register these new components. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorldService
participant PlayerConfigLookUp
Client->>WorldService: PlayerConfig(Request)
alt Get Request
WorldService->>PlayerConfigLookUp: Retrieve(characterId)
PlayerConfigLookUp-->>WorldService: (List<BuffInfo>, List<SkillCooldownInfo>)
WorldService-->>Client: PlayerConfigResponse (data)
else Save Request
WorldService->>PlayerConfigLookUp: Save(saveBuffs, skillCooldownInfos, characterId)
WorldService-->>Client: PlayerConfigResponse (acknowledgment)
end
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
Maple2.Database/Storage/Game/GameStorage.User.cs (1)
359-377:⚠️ Potential issueInconsistency: SkillCooldowns parameter still present in SaveCharacterConfig method
While SkillCooldowns has been removed from the LoadCharacterConfig return tuple, it is still present as a parameter in the SaveCharacterConfig method (line 368). This creates an inconsistency that could lead to runtime issues.
This parameter should be removed from the method signature to align with the changes made to LoadCharacterConfig and the broader restructuring of skill cooldown management across the codebase. Here's the suggested fix:
public bool SaveCharacterConfig( long characterId, IList<KeyBind> keyBinds, IList<QuickSlot[]> hotBars, IEnumerable<SkillMacro> skillMacros, IEnumerable<Wardrobe> wardrobes, IList<int> favoriteStickers, IList<long> favoriteDesigners, IDictionary<LapenshardSlot, int> lapenshards, - IList<SkillCooldown> skillCooldowns, long deathTick, int deathCount, int explorationProgress, StatAttributes.PointAllocation allocation, StatAttributes.PointSources statSources, SkillPoint skillPoint, IDictionary<int, int> gatheringCounts, IDictionary<int, int> guideRecords, SkillBook skillBook) {Don't forget to update all call sites to this method as well.
🧹 Nitpick comments (10)
Maple2.Model/Game/User/SkillCooldown.cs (2)
8-11: New properties should be properly documented and initialized.The new
Level,GroupId, andChargesproperties have been added to support the updated skill cooldown mechanism, but they lack documentation explaining their purpose. Additionally:
GroupIdhas aninitaccessor but isn't initialized in the constructorChargesis mutable but not initialized in the constructorThis could lead to inconsistent object state if not handled properly by callers.
public class SkillCooldown : IByteSerializable { public readonly int SkillId; public readonly short Level; public int GroupId { get; init; } public long EndTick; public int Charges; + + // XML documentation for class and properties would be helpful + // <summary> + // Tracks cooldown information for a skill. + // </summary>
18-23: Verify all properties are correctly serialized.The
WriteTomethod has been updated to writeGroupIdinstead of the removedOriginSkillId, and to writeCharges. However, theLevelproperty is not included in serialization.Also, casting
EndTickfromlongtointcould potentially lose precision if the value exceeds the range of anint.public void WriteTo(IByteWriter writer) { writer.WriteInt(SkillId); + writer.WriteShort(Level); writer.WriteInt(GroupId); - writer.WriteInt((int) EndTick); + writer.WriteLong(EndTick); writer.WriteInt(Charges); }Please verify if there are other components that depend on the existing serialization format before making these changes.
Maple2.Model/Enum/PlayerObjectFlag.cs (1)
16-16: Ensure future flag additions remain within the capacity of the underlying byte type.
Currently, combining these flags yields a value of up to 127 (0x7F). One more bit (0x80) remains unused, but adding more than one new flag could exceed this limit. Consider increasing the underlying enum type toushortorintif further expansion is anticipated.Maple2.Server.Core/proto/world/world.proto (3)
57-58: Revise the comment to capture broader functionality, not just buffs.
The comment “// Buff” no longer fully reflects that this RPC manages both buffs and skill cooldowns. Consider updating it to clarify the scope of player configuration.- // Buff + // Player configuration (buffs, skill cooldowns, etc.)
526-541: Rename theoneof buffto reflect config operations more accurately.
Though functionally correct, naming the oneof field “buff” might be confusing because it also handles skill cooldowns. Consider a more descriptive name, such as “config_op” or “player_config_op.”oneof buff { - Save save = 2; - Get get = 3; + Save save_op = 2; + Get get_op = 3; }
556-563: Consider consistent numeric types for skill cooldown fields.
Similar to BuffInfo, ensure that fields such asms_remaining,stop_time, andchargesdo not need to handle negative values. Switching to unsigned types or adding range checks can prevent potential bugs.Maple2.Server.Game/Session/GameSession.cs (1)
760-795: Well-implemented cache persistence methodThe SaveCacheConfig method effectively:
- Retrieves current buffs and cooldowns
- Calculates remaining durations based on field tick
- Adds server timestamp for synchronization
- Handles errors appropriately with logging
One minor suggestion would be to add a brief comment explaining the purpose of the stopTime value, though its use is fairly clear from context.
long stopTime = DateTime.Now.ToEpochSeconds(); +// stopTime records when these buffs/cooldowns were saved for proper restoration timing long fieldTick = Field.FieldTick;Maple2.Server.World/Service/WorldService.PlayerConfig.cs (2)
18-42: Player configuration retrieval implementation.The method retrieves player buffs and skill cooldowns from the lookup service and transforms them into the appropriate response format. All relevant properties are properly mapped from the internal representation to the response objects.
However, there's no error handling if the lookup service fails or returns invalid data.
Consider adding try/catch blocks to handle potential exceptions from the lookup service:
private PlayerConfigResponse Get(PlayerConfigRequest.Types.Get get, long requesterId) { + try { (List<BuffInfo> buffs, List<SkillCooldownInfo> skillCooldowns) = playerConfigLookUp.Retrieve(requesterId); return new PlayerConfigResponse { Buffs = { buffs.Select(b => new BuffInfo { Id = b.Id, Stacks = b.Stacks, Enabled = b.Enabled, Level = b.Level, MsRemaining = b.MsRemaining, StopTime = b.StopTime, }), }, SkillCooldowns = { skillCooldowns.Select(c => new SkillCooldownInfo { SkillId = c.SkillId, SkillLevel = c.SkillLevel, GroupId = c.GroupId, MsRemaining = c.MsRemaining, StopTime = c.StopTime, Charges = c.Charges, }), }, }; + } catch (Exception ex) { + Logger.Error("Failed to retrieve player configuration for {RequesterId}: {Exception}", requesterId, ex); + return new PlayerConfigResponse(); + } }
44-47: Player configuration storage implementation.The method saves player buffs and skill cooldowns to the lookup service. The implementation is simple and direct, which is good for maintainability.
As with the Get method, consider adding error handling for robustness.
private PlayerConfigResponse Save(PlayerConfigRequest.Types.Save save, long requesterId) { + try { playerConfigLookUp.Save(save.Buffs.ToList(), save.SkillCooldowns.ToList(), requesterId); return new PlayerConfigResponse(); + } catch (Exception ex) { + Logger.Error("Failed to save player configuration for {RequesterId}: {Exception}", requesterId, ex); + return new PlayerConfigResponse(); + } }Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
185-185: Potential overload confusion
There are now twoApplyEffectmethods: one takingbool notifyField, the other takinglong startTick. Consider unifying or renaming them to avoid confusion and ensure consistent usage throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Maple2.Server.World/Migrations/20250404235104_RemoveSkillCooldown.Designer.csis excluded by!Maple2.Server.World/Migrations/*Maple2.Server.World/Migrations/20250404235104_RemoveSkillCooldown.csis excluded by!Maple2.Server.World/Migrations/*Maple2.Server.World/Migrations/Ms2ContextModelSnapshot.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (26)
Maple2.Database/Model/CharacterConfig.cs(0 hunks)Maple2.Database/Storage/Game/GameStorage.User.cs(1 hunks)Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/SkillMapper.cs(1 hunks)Maple2.Model/Enum/PlayerObjectFlag.cs(1 hunks)Maple2.Model/Game/User/SkillCooldown.cs(1 hunks)Maple2.Model/Metadata/AdditionalEffectMetadata.cs(1 hunks)Maple2.Model/Metadata/SkillMetadata.cs(1 hunks)Maple2.Server.Core/proto/world/world.proto(2 hunks)Maple2.Server.Game/Commands/BuffCommand.cs(3 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(8 hunks)Maple2.Server.Game/Manager/Config/ConfigManager.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/Actor.cs(4 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/IActor.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(9 hunks)Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs(1 hunks)Maple2.Server.Game/PacketHandlers/InsigniaHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ItemPickupHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/SkillHandler.cs(1 hunks)Maple2.Server.Game/Session/GameSession.cs(3 hunks)Maple2.Server.Game/Trigger/TriggerContext.Field.cs(1 hunks)Maple2.Server.World/Containers/PlayerConfigLookUp.cs(1 hunks)Maple2.Server.World/Program.cs(1 hunks)Maple2.Server.World/Service/WorldService.PlayerConfig.cs(1 hunks)Maple2.Server.World/Service/WorldService.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Maple2.Database/Model/CharacterConfig.cs
🧰 Additional context used
🧬 Code Definitions (11)
Maple2.Server.World/Program.cs (1)
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (2)
PlayerConfigLookUp(9-128)PlayerConfigLookUp(15-19)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs (2)
Maple2.Server.Game/Trigger/TriggerContext.Field.cs (1)
AddBuff(367-385)Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
AddBuff(64-141)
Maple2.Server.World/Service/WorldService.cs (2)
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (2)
PlayerConfigLookUp(9-128)PlayerConfigLookUp(15-19)Maple2.Server.World/Service/WorldService.PlayerConfig.cs (1)
WorldService(6-48)
Maple2.Server.Game/Session/GameSession.cs (4)
Maple2.Server.World/Service/WorldService.PlayerConfig.cs (2)
PlayerConfigResponse(18-42)PlayerConfigResponse(44-47)Maple2.Server.Game/Manager/Config/BuffManager.cs (2)
SetCacheBuffs(368-389)List(436-438)Maple2.Server.Game/Manager/Config/ConfigManager.cs (3)
SetCacheSkillCooldowns(161-168)IList(202-211)IList(293-295)Maple2.Server.World/Containers/PlayerConfigLookUp.cs (3)
List(55-60)List(62-93)List(95-126)
Maple2.Server.Game/PacketHandlers/SkillHandler.cs (5)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Broadcast(600-609)Maple2.Server.Game/Packets/SkillPacket.cs (1)
SkillPacket(13-113)Maple2.Server.Game/Model/Field/Actor/Actor.cs (2)
ApplyEffect(80-86)ApplyEffect(185-189)Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
ApplyEffect(24-24)Maple2.Server.Game/Manager/Config/ConfigManager.cs (1)
SaveSkillCooldown(169-180)
Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (2)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (2)
ApplyEffect(80-86)ApplyEffect(185-189)Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
ApplyEffect(24-24)
Maple2.Server.Game/Model/Field/Buff.cs (3)
Maple2.Server.Game/Packets/BuffPacket.cs (1)
BuffPacket(10-54)Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
ApplyEffect(24-24)Maple2.Server.Game/Packets/StatsPacket.cs (1)
StatsPacket(10-101)
Maple2.Server.World/Service/WorldService.PlayerConfig.cs (2)
Maple2.Server.World/Service/WorldService.cs (2)
WorldService(8-47)WorldService(22-40)Maple2.Server.World/Containers/PlayerConfigLookUp.cs (4)
Save(21-53)List(55-60)List(62-93)List(95-126)
Maple2.Server.Game/Manager/Config/ConfigManager.cs (3)
Maple2.Model/Game/User/SkillCooldown.cs (2)
SkillCooldown(6-24)SkillCooldown(13-16)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs (1)
SkillMetadata(204-231)Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
AddBuff(64-141)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (3)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
AddBuff(64-141)Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
ApplyEffect(24-24)Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (1)
ApplyEffect(208-229)
Maple2.Server.Game/Manager/Config/BuffManager.cs (3)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (4)
IActor(191-198)Actor(24-236)Actor(51-61)Actor(63-71)Maple2.Server.Game/Model/Field/Buff.cs (3)
Stack(58-66)Buff(13-290)Buff(41-56)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (2)
FieldPlayer(13-401)FieldPlayer(45-58)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (66)
Maple2.Server.Core/proto/world/world.proto (2)
542-545: Structure is appropriate for storing buffs and cooldowns.
The two repeated fields allow returning multiple items in one response. This design is clear and maintainable.
547-554:Details
❓ Verification inconclusive
Confirm BuffInfo integer fields cannot be negative.
Fields likems_remainingandstop_timeare stored as signed integers. If negative values are not anticipated in your domain, you may consider usinguint32or ensuring validations.
🏁 Script executed:
#!/bin/bash # Searching for usage of BuffInfo to confirm negative values usage rg -A 5 'BuffInfo'Length of output: 8131
Verify Assumptions on BuffInfo Field Ranges
The
BuffInfomessage inMaple2.Server.Core/proto/world/world.protodefines fields such asms_remaining(int32) andstop_time(int64) as signed integers. Our review of their usage in multiple locations (e.g., inPlayerConfigLookUp.csandBuffManager.cs) did not reveal any explicit validations against negative values. Please confirm that these fields should indeed never hold negative values. If negatives are not expected per your domain logic, consider either:
- Changing the field types to unsigned integers (e.g.,
uint32/uint64), or- Implementing runtime validations wherever these values are consumed.
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (3)
55-60: Method naming clarity is good; consider caching current time in the calling scope if used repeatedly.
You read the current time at the start ofRetrieve. This is efficient as you pass the same time into both retrieval methods. This approach is recommended.
95-126:Details
❓ Verification inconclusive
Ensure consistent skill removal logic matches intended design for in-game vs. real-world time.
Right now, you remove the skill cooldown if(msPassed > skillCooldownInfo.MsRemaining), which is the real-time second difference. Double-check that the logic forUseInGameTimestill reevaluates “ms_remaining” in milliseconds.
🏁 Script executed:
#!/bin/bash # Searching for references to RetrieveSkillCooldowns usage to confirm logic alignment rg -A 10 "RetrieveSkillCooldowns"Length of output: 2297
Skill Cooldown Logic: Verify Consistent Time Unit Handling
The current implementation uses two separate checks to determine if a skill cooldown should be removed. When
UseInGameTimeis true, the elapsed time is scaled by 1000 before comparing toMsRemaining, yet a subsequent check compares the plainmsPassed(real-time difference) againstMsRemaining. Please verify that this dual-check strategy is intentional and thatMsRemainingis consistently treated as milliseconds in all cases. Specifically, consider whether:
- The logic within the
if (skillMetadata.State.UseInGameTime)block fully accounts for the intended handling of in-game time.- The subsequent check (
if (msPassed > skillCooldownInfo.MsRemaining)) might conflict with or inadvertently override the in-game time adjustment.Review and adjust as necessary to ensure that cooldown removal behaves consistently with the design expectations for both in-game and real-world timing.
62-93:Details
❓ Verification inconclusive
Validate correctness of time-based removal logic for buffs.
- If
UseInGameTimeis false, the condition compares(msPassed > buffInfo.MsRemaining)in real seconds, which might cause partial off-by-one edge cases due to integer usage.- Consider clearer naming, e.g.
realTimePassedvsmsPassed.Would you like to run a test or demonstration scenario to confirm buff expiry boundaries?
🏁 Script executed:
#!/bin/bash # A script to find test usage of RetrieveBuffs to examine edge cases rg -A 10 "RetrieveBuffs"Length of output: 1652
Revise and Validate Buff Expiry Logic & Naming
- The logic in
RetrieveBuffsstill relies on comparing the elapsed time (msPassed) tobuffInfo.MsRemaining, which for non in-game buffs may risk off-by-one issues due to unit assumptions.- For buffs using in-game time, the multiplication by 1000 (via
msSurpassed) needs to be double-checked to ensure consistent units and proper deduction frombuffInfo.MsRemaining.- The variable name
msPassedcan be improved (e.g., torealTimePassed) to clarify that it represents a real-time measurement rather than an in-game time offset.- The current usage of
RetrieveBuffsin the codebase is noted, but the provided output does not reveal dedicated test cases for these edge conditions. Please add tests or demonstration scenarios to specifically target buff expiry boundaries.Maple2.Server.World/Program.cs (1)
72-73: Well-structured dependency registration for the new PlayerConfigLookUp service.The registration follows the established pattern for lookup services in the application, correctly using the SingleInstance scope to ensure a shared instance throughout the application lifetime.
Maple2.Model/Metadata/AdditionalEffectMetadata.cs (1)
31-31: Good addition of UseInGameTime property for enhanced buff timing control.This new property allows distinguishing between effects that should use in-game time versus real-world time for duration calculations, which is a key part of the broader improvements to buff and cooldown management in this PR.
Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs (1)
36-36: Correctly mapped the new UseInGameTime property.This mapping ensures the new property is properly populated from the source data, maintaining consistency with the model changes in AdditionalEffectMetadata.cs.
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs (1)
528-528: Updated AddBuff call to use the field tick timing system.The change correctly adds the field tick parameter to align with the updated buff system that now uses more precise timing controls. This modification is part of the broader improvements to buff and cooldown management in this PR, providing more accurate timing for game effects.
Maple2.File.Ingest/Mapper/SkillMapper.cs (1)
124-128: Adding crucial skill cooldown propertiesThe new properties added to
SkillMetadataStateenhance the cooldown management system:
UseInGameTime: Controls whether the skill's cooldown uses in-game or real-world timeIgnoreReduceCooldown: Flags skills that shouldn't have their cooldown reduced by effectsCooldownGroupId: Enables group-based cooldown sharing between different skillsRechargeMaxCount: Supports skills with charge systemsThese additions directly support the PR's objective of improving buff and skill cooldown caching.
Maple2.Server.World/Service/WorldService.cs (4)
19-19: LGTM: Adding PlayerConfigLookUp fieldThis field will provide access to the buff and cooldown persistence system.
26-26: LGTM: Updated constructor parameterAdding PlayerConfigLookUp to the constructor parameters enables dependency injection.
39-39: LGTM: Field initializationProperly initializes the playerConfigLookUp field from the constructor parameter.
26-39:Details
❓ Verification inconclusive
Verify complete WorldService integration
While the field initialization looks good, make sure the new dependency is properly registered in the DI container.
🏁 Script executed:
#!/bin/bash # Check if PlayerConfigLookUp is registered in the DI container # Look for registration in Program.cs, Startup.cs or similar files rg -A 3 "services\.(?:Add|Register).*PlayerConfigLookUp" --type csLength of output: 68
DI Registration Verification: Confirm PlayerConfigLookUp is Registered in the DI Container
The current automated search for registrations of
PlayerConfigLookUpreturned no results. This might be due to case-sensitivity or differences in the registration syntax. Please ensure that this dependency is properly registered in your dependency injection setup (e.g., inProgram.csorStartup.cs).To aid in further investigation, please run the following case-insensitive search:
Once verified manually or via the updated search results, confirm that the DI registration for PlayerConfigLookUp exists; if it doesn't, update the registration accordingly.
🏁 Script executed:
#!/bin/bash # Re-check for DI registration of PlayerConfigLookUp in a case-insensitive manner rg -i -A 3 "services\.(add|register).*playerconfiglookup" --type csLength of output: 69
Action Required: Verify DI Registration for PlayerConfigLookUp
Our automated searches did not find any DI registration (e.g., in Program.cs or Startup.cs) for the PlayerConfigLookUp dependency. This may indicate that PlayerConfigLookUp is not registered in the DI container—an oversight that could lead to runtime errors.
- Next Steps:
- Manually verify whether the PlayerConfigLookUp dependency is registered in your DI configuration.
- If it is missing, please update your DI setup to register PlayerConfigLookUp appropriately.
Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
24-24:Details
❓ Verification inconclusive
Interface change requiring implementation updates
Adding the
startTickparameter toApplyEffectimproves timing precision for buff/effect application. This interface change affects all implementing classes.
🏁 Script executed:
#!/bin/bash # Check if all implementing classes have been updated to match the new interface signature # Find classes implementing the IActor interface echo "Classes implementing IActor:" rg -l "class\s+\w+\s+:\s+(?:\w+,\s+)*IActor(?:<[^>]+>)?(?:,\s+\w+)*" --type cs # Look for ApplyEffect implementations that might need to be updated echo -e "\nApplyEffect implementations:" rg "public\s+(?:override\s+)?void\s+ApplyEffect\s*\(" --type csLength of output: 337
Action Required: Verify Implementation Update for ApplyEffect in FieldActor.cs
Our verification indicates that only the file
Maple2.Server.Game/Model/Field/Actor/FieldActor.csimplements theIActorinterface. However, we did not detect any explicit override ofApplyEffectwith the new signature (i.e. including thestartTickparameter), which the interface change now requires.
- Confirm whether
FieldActor.csis intentionally relying on the interface’s default implementation.- If a custom behavior is intended, please update the class to include an override that matches the updated signature.
Maple2.Server.Game/Commands/BuffCommand.cs (1)
59-60: Updated to use field-synchronized ticks for stackingThe changes to the
Stackmethod calls now includeplayer.Field.FieldTickto ensure buffs properly track time with respect to the field's timing system. This is critical for consistency with the new buff caching system.Also applies to: 79-80, 89-90
Maple2.Server.Game/PacketHandlers/InsigniaHandler.cs (1)
65-65: Field tick parameter added to AddBuff methodThe buff application now includes the
session.Field.FieldTickparameter, which is consistent with the new requirements for timing precision in buff applications across the system.Maple2.Server.Game/Trigger/TriggerContext.Field.cs (3)
375-375: Good addition of field tick for timing precisionCapturing the field tick at the beginning of the method ensures consistent timing reference for all buffs applied in this method.
378-378: Updated AddBuff call with timing informationThe player buff application has been updated to include the field tick parameter, maintaining consistency with the new buff system requirements.
382-382: Updated monster buff applicationThe monster buff application also includes the field tick parameter, ensuring consistent handling of timing for all actor types.
Maple2.Server.Game/PacketHandlers/ItemPickupHandler.cs (1)
59-59: Field tick parameter added to pickup buff applicationThe AddBuff call now correctly includes the session's field tick for proper timing tracking, consistent with other buff applications in the system.
Maple2.Server.Game/Session/GameSession.cs (2)
250-261: New buff and cooldown loading from cacheThis new code properly loads cached buffs and skill cooldowns from the world service when a player enters the server, enhancing persistence of player state across sessions.
Error handling with the try-catch block is a good practice here since this is a non-critical function that shouldn't prevent login if it fails.
740-740: Added cache config saving during disposalGood addition of the call to SaveCacheConfig() during the session disposal process ensures player buffs and cooldowns are properly persisted.
Maple2.Model/Metadata/SkillMetadata.cs (1)
42-48: Expanded metadata for enhanced cooldown management.The SkillMetadataState record has been expanded with four new properties that enhance cooldown management capabilities:
UseInGameTime: Controls whether cooldowns use in-game time or real timeCooldownGroupId: Helps manage skill cooldowns in groupsIgnoreReduceCooldown: Prevents certain skills from being affected by cooldown reduction effectsRechargeMaxCount: Supports skills with charge-based systemsThese additions align well with the PR objective of expanding metadata for cooldown management and enhancing timing precision.
Maple2.Server.Game/PacketHandlers/SkillHandler.cs (3)
163-166: Unified timing approach for skill effects.The code now captures the current field tick at the beginning of the skill handling and passes it to
ApplyEffect(), ensuring that all effects from a skill share the same start time. This is an excellent improvement for effect timing consistency.
168-168: Consistent cooldown saving with timing information.Updated the
SaveSkillCooldownmethod call to include the start tick, aligning with the code's approach of using consistent timing throughout the skill application process.
157-159:Details
❓ Verification inconclusive
Improved field broadcast and motion retrieval.
The broadcast call was simplified by removing the null conditional operator, and the motion property retrieval now uses the
.First()method instead of direct indexing.While the simplification looks good, using
.First()method could potentially throw an exception if the collection is empty.
🏁 Script executed:
#!/bin/bash # Check if there are any SkillMetadata entries with empty Motions array ast-grep --pattern 'if (metadata.Data.Motions.Length == 0)'Length of output: 61
Attention: Verify Non-Empty Motions Collection
The simplified broadcast and motion retrieval improve readability, but using
.First()without a guarantee thatmetadata.Data.Motionsis non-empty can throw an exception. Our initial search for an explicit empty-check (e.g.,if (metadata.Data.Motions.Length == 0)) did not yield any results. Please manually verify that the motions collection is always populated before this call—or consider adding an appropriate guard to prevent potential runtime exceptions.
- File:
Maple2.Server.Game/PacketHandlers/SkillHandler.cs- Lines: 157-159
Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (2)
218-219: Updated target effect application with tick parameter.The
ApplyEffectmethod call for targets now includes the field tick parameter, ensuring consistent timing for all effects applied by the skill.
222-222: Updated caster effect application with tick parameter.The
ApplyEffectmethod call for the caster now includes the field tick parameter, maintaining timing consistency between target and caster effects.Maple2.Server.World/Service/WorldService.PlayerConfig.cs (1)
7-16: New RPC endpoint for player configuration management.This implementation adds a new gRPC service method to handle player configuration requests, with support for both retrieving and saving buff and cooldown data. The method properly handles different request types and delegates to appropriate handlers.
Maple2.Server.Game/Model/Field/Actor/Actor.cs (5)
84-84: Looks consistent with new buff pattern
UsingField.FieldTickas thestartTickparameter ensures correct timing for buff application.
140-140: Reflect logic updated with startTick
Applying a new reflect buff at the current field tick appears accurate and maintains consistent timing for the reflection effect.
168-168: Capturing current field tick
StoringField.FieldTickinstartTickprovides a uniform reference time for subsequent effect applications, improving timing clarity.
174-174: Invoking ApplyEffect with startTick
Using the newly introducedstartTickparameter for effect application ensures the effect’s timing aligns with the field’s tick count.
187-187: Adding buffs with explicit startTick
PassingstartTickdirectly toAddBuffclarifies the exact timing of buff application. The overall change integrates well with the revised buff system.Maple2.Server.Game/Model/Field/Buff.cs (16)
14-14: Switched from a private field to property
ReferencingOwner.Fieldensures this always reflects the correct field context for the buff’s owner.
41-55: Constructor updated to handle timing
The introduction oflong startTickandint durationMsclarifies buff timing. UsingStack(startTick, durationMs)and computingNextProcTickbased onstartTickis consistent with the new time-based approach.
58-63: Stack method now includes startTick
ResettingStartTickand recalculatingEndTickeach time helps correctly extend or refresh a buff’s lifetime.
90-90: Time-based removal check
Removing the buff whentickCount > EndTickis a clear, direct logic for expiring time-limited buffs.
111-111: Field broadcast of buff update
Notifying all relevant clients when a buff’s enabled state changes preserves visual and functional consistency.
138-138: Applying effect to the Owner
CallingOwner.ApplyEffectusingField.FieldTickensures the effect’s timing corresponds accurately to in-game updates.
141-141: Applying effect to the Caster
Allows buffs or effects (e.g., reflect or self-buffs) to be applied back to the caster, likewise synchronized with the field’s current tick.
149-149: Adding a skill via Field
Caster.Field.AddSkillat the correct tick places the skill effect in the game world at the precise timing.
179-179: Broadcasting stats update on heal
Ensures the field’s participants receive correct health information when a buff triggers HP recovery.
181-181: Heal packet broadcast
Sending a heal packet clarifies the healing effect’s source and timing for visual or UI feedback.
210-210: Stats update after DOT damage
Broadcasting attribute changes to reflect damage-over-time ensures clients stay synced with the actor’s evolving state.
211-211: DOT damage packet
Informing the entire field of the DOT attack preserves accurate status for both visual effects and log calculations.
214-214: Caster’s health restoration
When a DOT or reflect effect recovers the caster’s health, broadcasting updated HP is essential for correct UI and logic.
225-225: Applying DOT buff to the target
InvokingAddBuffwithField.FieldTickmaintains the correct start time for the buff effect.
227-227: Applying DOT buff to the caster
Similarly handles buffs that affect the caster, consistent with the new time-based approach.
269-269: Broadcast after duration modification
Notifying the field when the buff duration changes helps clients reflect updated timers accurately.Maple2.Server.Game/Manager/Config/BuffManager.cs (9)
2-2: Additional using statements
No concerns—these imports appear to support the Session and Service references used elsewhere.Also applies to: 9-9, 11-11
64-71: AddBuff signature expanded
Introducinglong startTickandint durationMsclarifies when the buff starts and how long it persists. The fallback toadditionalEffect.Property.DurationTickis logical.
85-85: Stack refresh
Usingexisting.Stack(startTick)reinitializes or extends the buff’s lifetime in line with the new timing concept.
103-105: Creating a new Buff
PassingstartTickanddurationMsinto theBuffconstructor matches the updated design for time-based buff tracking.
313-313: Entrance buffs on field load
CallingAddBuffwithActor.Field.FieldTickensures that buffs tied to entering a map start at the correct in-game time.
331-332: Shadow World Buffs
Applying region-specific buffs with the field’s tick aligns with the unified buff timing model.
338-338: Item/gem-based buffs
Both item and gem buffers utilizeActor.Field.FieldTick, keeping buff timing consistent with other systems.Also applies to: 345-345
369-389: SetCacheBuffs method
Reintroducing cached buffs usingcurrentTickandinfo.MsRemainingseamlessly restores buffs. Logging invalid buffs and skipping expired ones is prudent.
436-438: Persistent buff retrieval
Filtering out buffs that remove on logout ensures only valid buffs persist across sessions. This clearly separates temporary buffs from those that last beyond a session.Maple2.Server.Game/Manager/Config/ConfigManager.cs (5)
9-9: Additional using directives
No issues found.Also applies to: 12-12
161-168: SetCacheSkillCooldowns
Loading cached skill cooldowns usingcurrentTick + cooldown.MsRemainingaccurately reenacts the cooldown state.
169-178: SaveSkillCooldown method
IncludingstartTickrecalculatesEndTickfor precision. AddingMath.Clamponcooldown.Chargesis a neat improvement to handle multi-charge skills.
182-183: Revised SetSkillCooldown
Switching toshort leveland setting a defaultendTickensures compatibility with the new cooldown system.
238-238: Applying Premium Club Buff
Using the field’s current tick for premium buffs aligns with the established approach for timed effects.Maple2.Database/Storage/Game/GameStorage.User.cs (1)
312-314: Approved: SkillCooldowns properly removed from LoadCharacterConfig return typeThe removal of SkillCooldowns from the tuple return type aligns with the broader changes to remove legacy handling of skill cooldowns from character configuration.
Also applies to: 317-317
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (1)
17-18:⚠️ Potential issueReplace the invalid dictionary initializer.
This syntax will not compile for C#ConcurrentDictionary. You must explicitly instantiate the dictionaries, as previously mentioned in a past review.- buffs = []; - skillCooldowns = []; + buffs = new ConcurrentDictionary<long, ConcurrentDictionary<int, BuffInfo>>(); + skillCooldowns = new ConcurrentDictionary<long, ConcurrentDictionary<int, SkillCooldownInfo>>();
🧹 Nitpick comments (1)
Maple2.Model/Game/User/SkillCooldown.cs (1)
11-11: Consider makingChargesproperty consistent with other properties.Unlike other properties in this class which are readonly or init-only,
Chargesis defined as a mutable field. Consider making it consistent with the class design pattern unless mutability is specifically required for gameplay mechanics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Maple2.Model/Game/User/SkillCooldown.cs(1 hunks)Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(3 hunks)Maple2.Server.World/Containers/PlayerConfigLookUp.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.Server.Game/Commands/BuffCommand.cs
- Maple2.Server.Game/Session/GameSession.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
Maple2.Model/Game/User/SkillCooldown.cs (3)
8-9: LGTM: New properties are well-defined.The addition of the read-only
Levelproperty and init-onlyGroupIdproperty are well-structured for their intended use. The read-only nature ofLevelensures immutability, while the init-only nature ofGroupIdallows it to be set only during object initialization.
13-18: Constructor should initialize all required properties.The constructor has been updated to accept and set the new
Levelparameter, but it doesn't allow for setting a non-default value forGroupIdwhich is an init-only property.- public SkillCooldown(int skillId, short level) { + public SkillCooldown(int skillId, short level, int groupId = 0) { SkillId = skillId; Level = level; - GroupId = 0; + GroupId = groupId; Charges = 0; }
20-25: LGTM: WriteTo method properly updated.The
WriteTomethod has been properly updated to write the newGroupIdandChargesproperties to the byte writer, maintaining the serialization contract.Maple2.Server.World/Containers/PlayerConfigLookUp.cs (1)
21-53: Revisit concurrency assumptions for the Save method.
This code still uses aConcurrentDictionarybut relies on capturing a local reference (list) and iterating through the input lists. This was flagged before; ensure no race conditions arise when multiple threads attempt parallel saves for the same character.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (3)
64-64:⚠️ Potential issueUse a valid empty list instead of
[]to avoid compilation errors.
Returning[]is not valid for aList<BuffInfo>in C#.- return []; + return new List<BuffInfo>();
92-92:⚠️ Potential issueReturn a valid list in place of
[].
Similar to buffs, returning[]for aList<SkillCooldownInfo>is not valid C# syntax.- return []; + return new List<SkillCooldownInfo>();
74-79:⚠️ Potential issueFix the time unit mismatch for buff expiration checks.
currentTimeandStopTimeare in seconds, whileMsRemainingis in milliseconds. Multiplying byTimeSpan.FromMilliseconds(1).TotalMilliseconds(which is 1) does not convert seconds to milliseconds, resulting in incorrect buff durations.- long msSurpassed = (long) ((currentTime - buffInfo.StopTime) * TimeSpan.FromMilliseconds(1).TotalMilliseconds); + long msSurpassed = (long) ((currentTime - buffInfo.StopTime) * 1000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.World/Containers/PlayerConfigLookUp.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: format
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (4)
17-18: Replace the incorrect array initializers for concurrent dictionaries.
This syntax ([]) is invalid for object instantiation in C#. You must create new concurrent dictionaries explicitly.- buffs = []; - skillCooldowns = []; + buffs = new ConcurrentDictionary<long, ConcurrentDictionary<int, BuffInfo>>(); + skillCooldowns = new ConcurrentDictionary<long, ConcurrentDictionary<int, SkillCooldownInfo>>();
21-53: Handle concurrency carefully when saving new entries.
This mirrors a past concern about potential race conditions. While individual concurrent dictionary operations are thread-safe, ensure that other dependent reads/writes (e.g., capturing a local referencelistand iterating it in parallel) do not introduce logical inconsistencies if multiple threads callSavefor the same character at the same time.
68-85: Avoid enumerating and removing items within the same concurrent dictionary loop.
As previously noted,foreach (... in list)pluslist.Remove(...)can lead to unexpected behavior in highly concurrent scenarios. AlthoughConcurrentDictionaryis designed to handle concurrent operations more gracefully than standard dictionaries, consider capturing a snapshot or employing locking if multiple threads might modify it at once.
95-116: Avoid removing dictionary entries while enumerating inRetrieveSkillCooldowns.
As with the buffs dictionary, enumerating.Valueswhile making modifications can yield inconsistent snapshots, particularly under multithreaded access.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
368-388: Refactor SetCacheBuffs method to reduce code duplication.The
SetCacheBuffsmethod has similar code paths in both theifandelsebranches, with the only difference being the duration check in theelsebranch. This could be refactored to reduce duplication.public void SetCacheBuffs(IList<BuffInfo> buffs, long currentTick) { if (Actor is not FieldPlayer player) { return; } foreach (BuffInfo info in buffs) { if (!player.Field.SkillMetadata.TryGetEffect(info.Id, (short) info.Level, out AdditionalEffectMetadata? additionalEffect)) { logger.Error("Invalid buff: {SkillId},{Level}", info.Id, info.Level); continue; } - if (additionalEffect.Property.UseInGameTime) { - AddBuff(Actor, Actor, info.Id, (short) info.Level, currentTick, info.MsRemaining); - } else { - if (info.MsRemaining <= 0) { - continue; - } - AddBuff(Actor, Actor, info.Id, (short) info.Level, currentTick, info.MsRemaining); - } + // Skip buffs with no remaining duration unless they use in-game time + if (!additionalEffect.Property.UseInGameTime && info.MsRemaining <= 0) { + continue; + } + + AddBuff(Actor, Actor, info.Id, (short) info.Level, currentTick, info.MsRemaining); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (4)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (4)
IActor(191-198)Actor(24-236)Actor(51-61)Actor(63-71)Maple2.Server.Game/Model/Field/Buff.cs (3)
Stack(58-66)Buff(13-290)Buff(41-56)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
NextLocalId(247-247)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (2)
FieldPlayer(13-401)FieldPlayer(45-58)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
Maple2.Server.Game/Manager/Config/BuffManager.cs (7)
2-2: Appropriate imports for the new buff caching functionality.The addition of
Maple2.Database.Extensions,Maple2.Server.Game.Session, andMaple2.Server.World.Serviceimports support the new buff caching features being implemented.Also applies to: 9-11
64-64: Good improvement to the AddBuff method signature.Changing from
durationSectodurationMsprovides more precise duration control, and adding thestartTickparameter allows for explicit definition of when the buff starts, which is crucial for accurate timing calculations.
70-71: Duration handling updated for millisecond precision.The code now checks for a negative
durationMsinstead ofdurationSecand sets it toadditionalEffect.Property.DurationTick, which aligns with the new millisecond-based timing system.
85-85: Stack method calls now include startTick parameter.The
Stackmethod calls have been properly updated to include thestartTickparameter, ensuring that buff durations are correctly calculated when stacking occurs.Also applies to: 105-105
103-103: Buff constructor updated for new timing system.The
Buffconstructor call now includesstartTickanddurationMsparameters, correctly aligning with the changes to theBuffclass definition and ensuring consistent timing behavior.
315-315: AddBuff calls consistently updated throughout the class.All calls to
AddBuffacross various methods now include theActor.Field.FieldTickas thestartTickparameter, ensuring consistency in how buffs are added throughout the system.Also applies to: 331-332, 338-338, 345-345
435-437: Well-implemented GetSaveCacheBuffs method.This method efficiently retrieves buffs that should be saved when the player logs out (those without the
RemoveOnLogoutproperty), which is a clean implementation that aligns with the buff caching functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
268-270: 💡 Verification agent🧩 Analysis chain
Uncomment code for skill cooldown reset.
There's a TODO comment indicating to uncomment code when the skill cooldown PR is merged. Since this PR is about caching buffs and skill cooldowns, this commented code should be uncommented and updated.
Please verify if the skill cooldown PR has been merged and uncomment this code if applicable:
🏁 Script executed:
#!/bin/bash # Check if there are references to SetSkillCooldown in the codebase rg "SetSkillCooldown" -A 3 -B 3Length of output: 1266
Action: Uncomment and update the skill cooldown reset call
The skill cooldown PR has been merged—as evidenced by the presence of the
SetSkillCooldownmethod inConfigManager.cs. However, its signature now requires two parameters (int skillId, short level) with an optional third (int endTick = 0). Therefore, simply uncommenting the line inBuffManager.cswon’t compile.Please update the code as follows:
Uncomment the call in
BuffManager.cs(Lines 268-270).Update the call to supply an appropriate skill level. For example, if no specific level is available from the buff metadata, you might use a default value (e.g., 1):
// TODO: Uncomment this when skill cooldown PR is merged - //player.Session.Config.SetSkillCooldown(skillId); + player.Session.Config.SetSkillCooldown(skillId, 1);Verify if an explicit level (or additional parameter like
endTick) should be passed based on game logic.
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
368-383: New method for loading cached buffs.The
SetCacheBuffsmethod is a key addition that enables loading persisted buffs from the database. It checks each buff for validity, handles in-game time buffs, and applies their remaining duration properly.Consider adding error handling for the case where
info.Levelmight be invalid for the buff ID, as right now it's just logging an error but could potentially lead to unexpected behavior if many buffs fail to load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (5)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (4)
IActor(191-198)Actor(24-236)Actor(51-61)Actor(63-71)Maple2.Server.Game/Model/Field/Buff.cs (3)
Stack(58-66)Buff(13-290)Buff(41-56)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
NextLocalId(247-247)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (2)
FieldPlayer(13-401)FieldPlayer(45-58)Maple2.Server.World/Containers/PlayerConfigLookUp.cs (3)
List(55-60)List(62-88)List(90-116)
⏰ 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)
2-2: New imports support buff caching functionality.These new imports are needed for the new buff caching functionality - database extensions for persistence, game session access, and world service integration.
Also applies to: 9-9, 11-11
64-64: Method signature update enhances timing precision.The
AddBuffmethod signature has been changed to usestartTickanddurationMsparameters instead of seconds. This improves timing precision for buff management and aligns with the caching requirements.
70-71: Duration handling updated to use milliseconds.The code has been updated to handle duration in milliseconds rather than seconds, which aligns with the parameter changes and provides more precise timing control.
85-85: Stack method now incorporates timing information.The
Stackmethod call now includes thestartTickparameter, ensuring that buff stacking properly maintains timing information.
103-103: Buff constructor updated for timing precision.The
Buffconstructor now receivesstartTickanddurationMsparameters, which aligns with the changes to theBuffclass shown in the relevant snippets.
105-105: Stack timing consistency maintained.Consistent with earlier changes, the
Stackmethod is now called with thestartTickparameter to ensure proper timing information is maintained when adding buffs to the dictionary.
315-315: Map buff application updated to use field timing.AddBuff calls have been updated to use the field's current tick for timing consistency, ensuring that map-related buffs are properly integrated with the new timing system.
Also applies to: 331-332
338-338: Item buff application updated to use field timing.AddBuff calls for item-related buffs now use the field's current tick, maintaining consistency with the new timing system.
Also applies to: 345-345
430-432: New method for determining which buffs to cache.The
GetSaveCacheBuffsmethod filters buffs that should persist across sessions based on theRemoveOnLogoutproperty. This complements theSetCacheBuffsmethod and completes the buff persistence functionality.
github-project-automation
Bot
moved this from In progress
to Completed
in Maple2 Development
2 participants
Summary by CodeRabbit
New Features
Enhancements