Adjust Field Instancing & Vibrate Cubes - #431
Conversation
WalkthroughThis update introduces several changes across the codebase. It adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant FieldManager
participant FieldAccelerationStructure
Client->>Server: Send Vibrate Request (with skill UID, entity ID)
Server->>FieldManager: Retrieve SkillRecord by UID
Server->>FieldManager: Construct DamageRecord from SkillRecord
Server->>FieldAccelerationStructure: GetVibrateEntity(entityId)
alt BreakDefense logic
FieldAccelerationStructure-->>Server: Return FieldVibrateEntity
Server->>Server: Compare BreakDefense with BrokenOffence
Note right of Server: (TODO: Track broken state/respawn)
end
Server->>Client: Broadcast VibratePacket with DamageRecord
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (6)
Maple2.File.Ingest/Mapper/MapDataMapper.cs (1)
138-147:Details
❓ Verification inconclusive
Potential naming inconsistency between
brokenDefenceandBreakDefense.
IMS2VibrateexposesbrokenDefence(British spelling), but the downstream field record is namedBreakDefense(US spelling).
If both spellings intentionally map the same concept that is fine; otherwise a typo in either the interface or the record could silently propagate incorrect data.
🏁 Script executed:
#!/usr/bin/env bash # Verify property spellings across the codebase. rg -n 'brokenDefence' rg -n 'BreakDefense'Length of output: 1035
Check British vs US spelling in brokenDefence → BreakDefense mapping
I seevibrate.brokenDefence(British spelling) is mapped to the US-spelledBreakDefense. If the upstream contract intentionally uses British spelling and you’re standardizing to US in your entity, that’s fine; otherwise consider aligning the spelling to avoid confusion.
- File: Maple2.File.Ingest/Mapper/MapDataMapper.cs: line 144
Maple2.Server.Game/Model/Field/Actor/FieldActor.cs (1)
16-42: Avoid reallocatingTransform& generate onePointPrisminstance per actor.
base(field, objectId, mapMetadata, npcMetadata)most likely already initialisesTransform. Re-assigning a brand-newTransform()drops any data the base ctor prepared (e.g., position passed through parameters), and may break reference equality relied on elsewhere.
Shape => new PointPrism(Position)allocates a fresh object on every call. IfShapeis accessed from hot code (e.g., collision queries), consider caching the prism and updating its centre whenPositionmutates.- Transform = new Transform(); + // Re-use the Transform created by the base Actor constructor + // (if the base does not create one, keep the assignment as is).Optionally memoise the prism:
private readonly PointPrism prism = new(default); public override IPrism Shape { get { prism.Position = Position; return prism; } }Maple2.Server.Game/PacketHandlers/VibrateHandler.cs (2)
24-30:motionPoint/attackPointcurrently read but ignoredIf these fields are trusted inputs (e.g., combo-step or damage-table lookup), silently discarding them opens the door for desync or soft-cheat scenarios.
Either (a) validate and persist them (see previous diff) or (b) remove the reads to avoid dead code.
52-56:TODOleft behind – break-state tracking still missingThe comparison against
BreakDefenseis in place but no state change is recorded. Until implemented, cubes with low break-defense can never enter a “broken” state, despite meeting the condition.
Please implement or create a follow-up task to avoid logical dead ends.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (1)
387-392: Non-instance maps choose the first field without health checkThe same disposed-field issue exists here. Re-use the safety filter:
- FieldManager? firstField = mapFields.Values.FirstOrDefault(); + FieldManager? firstField = mapFields.Values + .FirstOrDefault(f => !f.Disposed);Optionally, sort by
Players.Countto balance capacity.Maple2.Model/Game/Field/FieldAccelerationStructure.cs (1)
277-279: Linear search inGetVibrateEntitymay not scale
FirstOrDefaultis O(N); a busy map can host thousands of vibrate entities.
Storing aDictionary<string, FieldVibrateEntity>keyed byId.Id(populated together withvibrateEntities) would reduce look-ups to O(1) and avoid frame spikes when skills hit multiple cubes in quick succession.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Maple2.File.Ingest/Mapper/ItemMapper.cs(0 hunks)Maple2.File.Ingest/Mapper/MapDataMapper.cs(1 hunks)Maple2.Model/Enum/Skill.cs(1 hunks)Maple2.Model/Game/Field/FieldAccelerationStructure.cs(5 hunks)Maple2.Model/Metadata/FieldEntity/FieldEntity.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs(3 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/FieldActor.cs(2 hunks)Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs(2 hunks)Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/VibrateHandler.cs(2 hunks)Maple2.Server.Game/Packets/VibratePacket.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Maple2.File.Ingest/Mapper/ItemMapper.cs
🧰 Additional context used
🧬 Code Graph Analysis (5)
Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs (2)
Maple2.Database/Storage/Game/GameStorage.User.cs (1)
Player(196-264)Maple2.Model/Game/User/Player.cs (2)
Player(8-22)Player(17-21)
Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (1)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
VibrateObjects(574-592)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
FieldManager(33-661)FieldManager(85-110)Maple2.Server.Game/GameServer.cs (1)
FieldManager(101-103)
Maple2.Model/Game/Field/FieldAccelerationStructure.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(70-73)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (4)
Maple2.Server.Game/Model/Field/Actor/FieldActor.cs (2)
FieldActor(16-45)FieldActor(33-42)Maple2.Server.Game/Manager/Config/BuffManager.cs (4)
NextLocalId(24-24)List(218-218)List(219-219)List(601-605)Maple2.Server.Game/Model/Skill/DamageRecord.cs (2)
DamageRecord(8-42)DamageRecord(27-41)Maple2.Model/Game/Field/FieldAccelerationStructure.cs (8)
List(158-164)List(170-176)List(217-223)List(229-235)List(241-247)List(257-263)List(269-275)FieldVibrateEntity(277-279)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: format
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
Maple2.Model/Enum/Skill.cs (1)
92-101:Details
✅ Verification successful
Double-check external references after renaming
Player3→RegionBuff.The numeric value (6) is unchanged, so packets/DB rows remain binary-compatible, but any code that still references the old
ApplyTargetType.Player3identifier will now fail to compile at best or, worse, be reflected as magic-number6literals at runtime.
Please run a quick search to confirm that no dangling symbol or hard-coded6slipped through.
🏁 Script executed:
#!/usr/bin/env bash # Find any usage of the removed enum member or suspicious magic number. rg -n --fixed-strings 'Player3' || true rg -n 'ApplyTargetType\s*\.\s*6' || trueLength of output: 142
🏁 Script executed:
#!/usr/bin/env bash # Display context around the remaining Player3 entry sed -n '180,240p' Maple2.Model/Enum/Skill.csLength of output: 571
No dangling ApplyTargetType.Player3 references remain
I’ve searched for bothPlayer3and any magic-number6usages againstApplyTargetTypeand found none. The rename toRegionBuffis safe and requires no further changes.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
497-500: ConfirmRegionBufftarget selection logic.The new case always returns all players irrespective of the caster type.
If an NPC casts a region-buff that is meant only for allies (or the caster’s faction), this may apply unintended buffs to hostile PCs.
Please review game-design intent and, if necessary, branch oncasteras done forFriendly/Hostile.Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs (1)
306-306: Mark player as in battle when lifting objects.Setting the
InBattleproperty to true when a player lifts an object is a good addition. This ensures that the battle state is properly tracked when players interact with liftable objects, which aligns with the PR objective for vibrate cubes that respond to combat actions.Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (1)
46-47: Added properties for vibration entity breaking mechanics.The addition of
BreakDefenseandBreakTickproperties to theFieldVibrateEntityrecord supports the PR objective of making vibrate cubes only respond to skills with sufficient breaking power. These properties provide the necessary data model to determine when an entity should vibrate based on skill break offense values.Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (1)
176-176: Updated vibration logic to use damage record.Changed from using
record(SkillRecord) todamage(DamageRecord) when callingVibrateObjects, which correctly aligns with the updated signature in FieldManager. This change supports the new requirement that vibrate cubes only respond to skills with BrokenOffense > 0.Maple2.Server.Game/Packets/VibratePacket.cs (1)
15-25: Updated Attack method to use DamageRecord.The signature and implementation of the
Attackmethod have been properly updated to useDamageRecordinstead ofSkillRecord. All the property accesses have been correctly updated to maintain functionality while supporting the new vibration mechanics based on break offense values.Maple2.Model/Game/Field/FieldAccelerationStructure.cs (1)
571-579: New fields default to zero – verify serialization versioning
BreakDefenseandBreakTickdefault to0. When older saved maps (without these integers) are loaded,ReadEntitywill still attempt to read the two ints, causing stream misalignment.Ensure the file/packet format version is bumped or gated by a feature flag so older assets can still load safely.
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
93-93: Constructor parameter update is properly aligned with the FieldActor refactoring.The constructor now passes a unique local ID generated by
NextLocalId()to theFieldActor, which aligns with the changes made to theFieldActorclass (now inheriting fromActor<MapMetadata>). This ensures each field actor has a properly initialized object ID.
119-119: FieldTick initialization is now explicit and correctly placed.Setting
FieldTickto the current environment tick count immediately after the field is initialized ensures consistent timing behavior from the start. This is good practice as it establishes a clear baseline for all time-dependent operations in the field.
2 participants
channelScaletype, allow for others to also join first available field (This allows for event maps not to be solo instances)Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores