Uh oh!
There was an error while loading. Please reload this page.
Add LongExtensions.Truncate32 and refactor tick handling - #614
Conversation
WalkthroughAdds a 32-bit truncation helper (Truncate32) and applies it across tick serialization and packet writes, exposes a 32-bit FieldTickInt on IField/FieldManager, filters expired buffs/cooldowns when saving cache, and renames a BuffManager parameter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant GameSession
participant Field as FieldManager
participant BuffManager
participant Persistence as PlayerConfigStorage
Player->>GameSession: Request SaveCacheConfig
GameSession->>Field: Read FieldTick (64-bit)
Field-->>GameSession: FieldTickInt (FieldTick.Truncate32())
GameSession->>GameSession: Filter Buffs/Cooldowns (EndTick - FieldTick > 0)
GameSession->>Persistence: Save filtered BuffInfo & SkillCooldownInfo (MsRemaining via Truncate32)
Persistence-->>GameSession: Ack
GameSession-->>Player: Done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)Maple2.Server.Game/Session/GameSession.cs (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Maple2.Tools/Extensions/LongExtensions.cs (1)
10-12: LGTM! Clean truncation logic.The implementation correctly truncates 64-bit values to 32-bit by masking the lower 32 bits and casting to int, which properly handles wraparound (values > int.MaxValue become negative, matching Environment.TickCount behavior).
Optional performance optimization:
Consider marking this method with
[MethodImpl(MethodImplOptions.AggressiveInlining)]to ensure it's inlined at call sites, as it's a simple operation called frequently in serialization paths:+using System.Runtime.CompilerServices;+ namespace Maple2.Tools.Extensions; public static class LongExtensions { /// <summary> /// Truncates a long value to a 32-bit int, handling overflow properly. /// This is useful for converting Environment.TickCount64 to match Environment.TickCount behavior. /// </summary> /// <param name="value">The long value to truncate</param> /// <returns>The truncated int value</returns> + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int Truncate32(this long value) { return (int) (0xFFFFFFFF & value); } }Maple2.Server.Game/Packets/BreakablePacket.cs (1)
26-29: Correct 32‑bit tick truncation; minor consistency refactorUsing Truncate32() here is correct and safer than direct casts. For consistency and tiny perf, also cache Environment.TickCount64 in the single‑update overload.
Apply this diff to the single-update method:
- public static ByteWriter Update(FieldBreakable breakable) {+ public static ByteWriter Update(FieldBreakable breakable) { var pWriter = Packet.Of(SendOp.Breakable); pWriter.Write<Command>(Command.Update); pWriter.WriteString(breakable.EntityId); pWriter.Write<BreakableState>(breakable.State); pWriter.WriteBool(breakable.Visible); - if (breakable.BaseTick > 0) {- pWriter.WriteInt((Environment.TickCount64 - breakable.BaseTick).Truncate32());+ long currentTick = Environment.TickCount64;+ if (breakable.BaseTick > 0) {+ pWriter.WriteInt((currentTick - breakable.BaseTick).Truncate32()); pWriter.WriteInt(breakable.BaseTick.Truncate32()); } else { pWriter.WriteInt(); pWriter.WriteInt(); }Additionally, to guarantee behavior even inside checked contexts, consider making the extension use an unchecked cast:
publicstaticintTruncate32(thislongvalue)=>unchecked((int)value);Based on learnings
Also applies to: 45-47
Maple2.Server.Game/Session/GameSession.cs (1)
813-839: Clamp MsRemaining to int range to avoid overflowCurrent cast may overflow if remaining ms > int.MaxValue. Clamp before casting.
Apply this diff:
- .Where(buff => buff.EndTick - fieldTick > 0)+ .Where(buff => buff.EndTick - fieldTick > 0) .Select(buff => new BuffInfo { Id = buff.Id, Level = buff.Level, - MsRemaining = (int) (buff.EndTick - fieldTick),+ MsRemaining = (int) Math.Min(buff.EndTick - fieldTick, int.MaxValue), Stacks = buff.Stacks, Enabled = buff.Enabled, StopTime = stopTime, }),- .Where(cooldown => cooldown.EndTick - fieldTick > 0)+ .Where(cooldown => cooldown.EndTick - fieldTick > 0) .Select(cooldown => new SkillCooldownInfo { SkillId = cooldown.SkillId, SkillLevel = cooldown.SkillLevel, GroupId = cooldown.GroupId, - MsRemaining = (int) (cooldown.EndTick - fieldTick),+ MsRemaining = (int) Math.Min(cooldown.EndTick - fieldTick, int.MaxValue), StopTime = stopTime, Charges = cooldown.Charges, }),Optional: also clamp DeathInfo.MsRemaining to non-negative to avoid sending negative values when already expired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Maple2.Model/Game/User/SkillCooldown.cs(2 hunks)Maple2.Server.Game/Manager/BuffManager.cs(1 hunks)Maple2.Server.Game/Manager/DungeonManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs(1 hunks)Maple2.Server.Game/Manager/FishingManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(2 hunks)Maple2.Server.Game/Packets/BreakablePacket.cs(3 hunks)Maple2.Server.Game/Packets/InstrumentPacket.cs(1 hunks)Maple2.Server.Game/Packets/RegionSkillPacket.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(1 hunks)Maple2.Server.Tests/Tools/LongExtensionsTests.cs(1 hunks)Maple2.Tools/Extensions/LongExtensions.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
Maple2.Model/Metadata/Constants.cs (1)
Constant(10-963)
Maple2.Model/Game/User/SkillCooldown.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Manager/BuffManager.cs (1)
Maple2.Database/Storage/Metadata/SkillMetadataStorage.cs (1)
TryGetEffect(49-70)
Maple2.Server.Game/Manager/DungeonManager.cs (1)
Maple2.Model/Game/Dungeon/DungeonRoomRecord.cs (2)
DungeonRoomRecord(7-17)DungeonRoomRecord(14-16)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Packets/InstrumentPacket.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Manager/FishingManager.cs (1)
Maple2.Server.Game/Packets/FishingPacket.cs (1)
FishingPacket(12-156)
Maple2.Server.Game/Session/GameSession.cs (2)
Maple2.Server.World/Service/WorldService.PlayerConfig.cs (2)
PlayerConfigResponse(18-43)PlayerConfigResponse(45-48)Maple2.Server.World/Containers/PlayerConfigLookUp.cs (1)
Save(23-62)
Maple2.Server.Game/Packets/BreakablePacket.cs (2)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(69-72)Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Tests/Tools/LongExtensionsTests.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Packets/RegionSkillPacket.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Model/Field/Buff.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (11)
Maple2.Server.Game/Manager/BuffManager.cs (1)
501-516: Approve stack restoration in SetCacheBuffs Passing info.Stacks to AddBuff restores buff stacks correctly; BuffInfo.Stacks is initialized in PlayerConfigResponse.Maple2.Model/Game/User/SkillCooldown.cs (1)
24-24: LGTM! Consistent use of Truncate32 for serialization.The change from direct cast to
Truncate32()makes the 32-bit truncation intent explicit and aligns with the refactoring pattern.Maple2.Server.Game/Manager/DungeonManager.cs (1)
324-326: LGTM! Improved timing consistency.The change from
Environment.TickCounttoLobby.FieldTickcentralizes timing through the field manager, ensuring consistent field-relative timing for dungeon room records.Maple2.Server.Game/Packets/InstrumentPacket.cs (1)
65-65: LGTM! Consistent packet serialization.The change to
Truncate32()makes the 32-bit truncation explicit for packet serialization, aligning with the refactoring pattern.Maple2.Server.Game/Packets/RegionSkillPacket.cs (1)
21-21: LGTM! Consistent packet serialization.The change to
Truncate32()forNextTickserialization aligns with the refactoring pattern and makes truncation intent explicit.Maple2.Server.Game/Model/Field/Buff.cs (1)
322-323: LGTM! Consistent buff serialization.The changes to use
Truncate32()for bothStartTickandEndTickalign with the refactoring pattern and make the 32-bit truncation explicit for byte serialization.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
187-188: LGTM! Consistent use of FieldTickInt for portal timing.The change to use
FieldTickIntdirectly for quest portal timing is cleaner than castingFieldTickand aligns with the 32-bit tick representation pattern.
628-628: LGTM! Consistent portal timing.Using
FieldTickIntfor the bonus map portal'sEndTickaligns with the other portal timing changes and the 32-bit tick pattern.Maple2.Server.Game/Manager/FishingManager.cs (1)
257-257: LGTM! Cleaner fishing timer calculation.Using
session.Field.FieldTickIntdirectly is cleaner than castingFieldTickand aligns with the refactoring to use the 32-bit tick property.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
264-270: Good addition: 32‑bit tick accessor mirrors TickCount semanticsThe Truncate32-based property is appropriate for client-facing 32-bit tick usage and avoids unsafe casts. Nice doc and placement.
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
59-59: IField implementers updated FieldManager definesFieldTickInt; no other IField implementers exist.
Uh oh!
There was an error while loading. Please reload this page.
fdb7f26 to
c0dca8fCompareThere was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Maple2.Server.Tests/Tools/LongExtensionsTests.cs (1)
1-2: Restore missing NUnit using and replace C# 12 collection expression
[Test],[TestCase], andAssertwon’t resolve withoutusing NUnit.Framework;, so the test project doesn’t compile. Thelong[] testValues = [...]literal also requires C# 12; the repo still targets earlier language versions, so that syntax fails to build. Please add the NUnit using and fall back to a classic array initializer so the tests compile again.using Maple2.Tools.Extensions; using System; +using NUnit.Framework;- long[] testValues = [+ long[] testValues = new long[] { 0L, 1L, -1L, int.MaxValue, int.MinValue, (long) int.MaxValue + 1, (long) int.MinValue - 1, 0xFFFFFFFF, 0x100000000L, 0x123456789ABCDEF0L, long.MaxValue, long.MinValue, - ];+ };Also applies to: 121-135
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs (1)
4-4: Verify the import is intentional.The added import appears unused in this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Maple2.Model/Game/User/SkillCooldown.cs(2 hunks)Maple2.Server.Game/Manager/BuffManager.cs(1 hunks)Maple2.Server.Game/Manager/DungeonManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs(1 hunks)Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs(1 hunks)Maple2.Server.Game/Manager/FishingManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(2 hunks)Maple2.Server.Game/Packets/BreakablePacket.cs(3 hunks)Maple2.Server.Game/Packets/InstrumentPacket.cs(1 hunks)Maple2.Server.Game/Packets/RegionSkillPacket.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(1 hunks)Maple2.Server.Tests/Tools/LongExtensionsTests.cs(1 hunks)Maple2.Tools/Extensions/LongExtensions.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- Maple2.Server.Game/Manager/BuffManager.cs
- Maple2.Server.Game/Manager/FishingManager.cs
- Maple2.Server.Game/Manager/DungeonManager.cs
- Maple2.Server.Game/Session/GameSession.cs
- Maple2.Server.Game/Manager/Field/FieldManager/IField.cs
- Maple2.Server.Game/Packets/BreakablePacket.cs
- Maple2.Tools/Extensions/LongExtensions.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs
🧰 Additional context used
🧬 Code graph analysis (5)
Maple2.Model/Game/User/SkillCooldown.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Packets/InstrumentPacket.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Model/Field/Buff.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Packets/RegionSkillPacket.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Tests/Tools/LongExtensionsTests.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
Maple2.Model/Game/User/SkillCooldown.cs (2)
3-3: LGTM: Import required for extension method.The import is necessary for using the
Truncate32()extension method and is correctly placed.
24-24: LGTM: Improved clarity with explicit truncation.The change from
(int) EndTicktoEndTick.Truncate32()is functionally equivalent but significantly improves code clarity by making the truncation intent explicit. This aligns with the broader refactoring pattern applied across the codebase for tick handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Maple2.Server.Tests/Tools/LongExtensionsTests.cs (2)
1-3: Fix missing NUnit import to compile testsAdd NUnit using so [Test]/Assert resolve.
Apply:
using Maple2.Tools.Extensions; using System; +using NUnit.Framework;
121-136: Replace C# 12 collection expression with classic array initializerEnsures compatibility with older language versions.
Apply:
- long[] testValues = [+ long[] testValues = new long[] { 0L, 1L, -1L, int.MaxValue, int.MinValue, (long) int.MaxValue + 1, (long) int.MinValue - 1, - 0xFFFFFFFF,+ 0xFFFFFFFF, 0x100000000L, 0x123456789ABCDEF0L, long.MaxValue, long.MinValue, - ];+ };
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/DungeonManager.cs (1)
352-352: Avoid potential overflow: cast after dividingDivide on long first, then cast to int.
Apply:
- UserRecord.TotalSeconds = (int) (Lobby.DungeonRoomRecord.EndTick - Lobby.DungeonRoomRecord.StartTick) / 1000;+ UserRecord.TotalSeconds = (int) ((Lobby.DungeonRoomRecord.EndTick - Lobby.DungeonRoomRecord.StartTick) / 1000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Maple2.Model/Game/User/SkillCooldown.cs(2 hunks)Maple2.Server.Game/Manager/BuffManager.cs(1 hunks)Maple2.Server.Game/Manager/DungeonManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs(1 hunks)Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs(1 hunks)Maple2.Server.Game/Manager/FishingManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(2 hunks)Maple2.Server.Game/Packets/BreakablePacket.cs(3 hunks)Maple2.Server.Game/Packets/InstrumentPacket.cs(1 hunks)Maple2.Server.Game/Packets/RegionSkillPacket.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(1 hunks)Maple2.Server.Tests/Tools/LongExtensionsTests.cs(1 hunks)Maple2.Tools/Extensions/LongExtensions.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs
🚧 Files skipped from review as they are similar to previous changes (7)
- Maple2.Server.Game/Manager/FishingManager.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs
- Maple2.Server.Game/Session/GameSession.cs
- Maple2.Model/Game/User/SkillCooldown.cs
- Maple2.Tools/Extensions/LongExtensions.cs
- Maple2.Server.Game/Packets/RegionSkillPacket.cs
- Maple2.Server.Game/Manager/Field/FieldManager/IField.cs
🧰 Additional context used
🧬 Code graph analysis (7)
Maple2.Server.Game/Packets/BreakablePacket.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Model/Field/Buff.cs (2)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(69-72)Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Packets/InstrumentPacket.cs (2)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(69-72)Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
Maple2.Model/Metadata/Constants.cs (1)
Constant(10-963)Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Manager/DungeonManager.cs (1)
Maple2.Model/Game/Dungeon/DungeonRoomRecord.cs (2)
DungeonRoomRecord(7-17)DungeonRoomRecord(14-16)
Maple2.Server.Tests/Tools/LongExtensionsTests.cs (1)
Maple2.Tools/Extensions/LongExtensions.cs (1)
Truncate32(10-12)
Maple2.Server.Game/Manager/BuffManager.cs (1)
Maple2.Database/Storage/Metadata/SkillMetadataStorage.cs (1)
TryGetEffect(49-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
187-188: LGTM: Safe 32-bit truncation for quest portal ticks.The changes correctly use
Truncate32()for EndTick andFieldTickIntfor StartTick. The explicit cast tolongbefore adding milliseconds prevents double-precision arithmetic, andTruncate32()safely masks the lower 32 bits without sign extension issues.
628-628: LGTM: Consistent tick truncation pattern.The EndTick calculation correctly applies the same safe truncation pattern used for quest portals, ensuring consistent tick handling across portal types.
Maple2.Server.Game/Model/Field/Buff.cs (1)
322-324: Consistent 32-bit tick serializationUsing Truncate32() here is correct and consistent with packet expectations.
Maple2.Server.Game/Packets/InstrumentPacket.cs (1)
65-65: LGTM: Use Truncate32 for StartTickCorrectly avoids checked-cast overflow and preserves lower 32 bits.
Maple2.Server.Game/Packets/BreakablePacket.cs (1)
27-29: LGTM: Safe 32-bit truncation for tick diff and base tickTruncate32() matches TickCount wrap semantics and avoids overflow on casts.
Also applies to: 45-47
Maple2.Server.Game/Manager/DungeonManager.cs (1)
324-326: EndTick assignment already uses FieldTick—time bases matchMaple2.Server.Game/Manager/BuffManager.cs (1)
501-516: LGTM! Good refactor and bug fix.The changes improve code clarity and correctness:
Parameter rename (
buffs→cacheBuffs): Avoids shadowing the class-levelbuffsfield, making the code easier to read and maintain.Stack preservation (line 513): Now passes
info.StackstoAddBuff, correctly preserving stack counts when loading cached buffs. Previously, stacks would default to 0, losing this information.The validation logic and filtering (lines 507-512) appropriately handle invalid buffs and skip expired buffs that don't use in-game time.
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests