Field Spawn Groups - #357
Conversation
WalkthroughThis pull request enhances the game’s systems by integrating new spawn types and interactive elements. The changes update metadata storage by adding dictionaries and lazy-loaded combine spawn tables, and they extend mapper logic to process additional parameters and new spawn cases. New enums and record types are introduced for proper categorization and polymorphic behavior, while FieldManager gains methods for spawning NPCs, interactable objects, and toggling spawn states. Packet handlers and trigger contexts are also revised to support defense guard actions, UI visibility controls, and refined broadcast and logging mechanisms. Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant IU as ItemUseHandler
participant NS as NpcMetadataService
participant FM as FieldManager
participant SCH as Scheduler
P->>IU: Use DefenseGuard item
IU->>IU: Validate parameters
IU->>NS: Retrieve NPC metadata (npcId)
NS-->>IU: Return NPC metadata
IU->>FM: Calculate spawn position & request NPC spawn
FM->>FM: Spawn NPC and broadcast addition
FM->>SCH: Schedule NPC removal (lifeSpanSeconds)
SCH-->>FM: Removal confirmed
IU->>P: Consume item
sequenceDiagram
participant FM as FieldManager
participant IO as InteractObject
participant BC as Broadcast System
FM->>FM: Call SpawnInteractObject(metadata)
FM->>IO: Create interact object instance
FM->>FM: Add interact object to fieldChests
FM->>BC: Broadcast interact object addition
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: 11
🧹 Nitpick comments (22)
Maple2.Server.Game/PacketHandlers/UserChatHandler.cs (1)
1-2: Imports added without corresponding implementation changes.Only import statements have been added to the file without any changes to the implementation code. The new imports suggest preparation for future functionality that will integrate chat with:
- Vector mathematics (
System.Numerics)- Game model classes (
Maple2.Server.Game.Model)- Scripting triggers (
Maple2.Server.Game.Scripting.Trigger)Consider adding a code comment explaining the purpose of these imports if they're being added proactively for future development. If they're not currently used, it would be helpful to understand their intended purpose.
Also applies to: 13-13, 15-15
Maple2.Model/Enum/CombineSpawnGroupType.cs (2)
1-2: Confirm Using Directives
The file importsSystem.ComponentModel. Please verify if this using directive is required in this file; if it’s not used now, it may be removed in a future cleanup for better maintainability.
3-10: Enum Naming Conventions
The enumeration members are defined in lowercase (e.g.,none,npc,interactObject), which deviates from standard C# naming conventions that prefer PascalCase (e.g.,None,Npc,InteractObject). Adopting the conventional casing would improve code readability and consistency across the codebase.Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs (1)
488-512: Defense Guard implementation is well structured.The implementation correctly:
- Parses parameters from the item metadata
- Calculates spawn position relative to player
- Spawns the NPC and sets up its lifecycle
- Consumes the item upon successful spawning
Some minor suggestions:
- The TODO comment on line 503 should be addressed or clarified
- Consider adding validation for the NPC spawning position (e.g., checking for collisions or invalid terrain)
- Vector3 position = session.Player.Transform.Position + session.Player.Transform.FrontAxis * distance; - // TODO: Do we check Z? + Vector3 position = session.Player.Transform.Position + session.Player.Transform.FrontAxis * distance; + // Ensure the NPC is spawned on valid terrain + if (session.Field.TryGetValidPosition(position, out Vector3 validPosition)) { + position = validPosition; + }Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (3)
8-21: Consider adding XML documentation for public types and members.The class structure looks good, but adding XML documentation would improve code maintainability and make it easier for other developers to understand the purpose of this class and its properties.
Also, I noticed that the
activefield is set in the constructor but not used elsewhere in this file. Ensure this is intentional or consider how it will be used in future implementations.
38-40: Consider adding logging when NPC spawn fails.When
Field.SpawnNpcreturns null, the code simply continues to the next iteration. Consider adding a log entry similar to the one on line 32 to help with debugging when NPC spawning fails.if (npc == null) { + Log.Logger.Warning("Failed to spawn Npc {NpcId} at spawn point {SpawnId} in map {MapId}", spawn.NpcId, SpawnId, Field.MapId); continue; }
43-45: Consider error handling for broadcast operations.While the
Field.Broadcastmethods might handle exceptions internally, it's good practice to ensure these operations won't cause issues if they fail. Consider wrapping these in a try-catch block to prevent any potential exceptions from stopping the spawn process.Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs (1)
25-49: Implication of fallback behavior.The
Init()method adds entries to eithernpcsorinteractObjectsbased on the group type. If the group type is invalid, an error log is written. Consider throwing or handling that scenario more robustly if an invalid type should halt further processing in some contexts.Maple2.Model/Metadata/ServerTable/CombineSpawnTable.cs (3)
10-15: Good use of record for spawn group metadata.The record provides an immutable data structure with clear property names. Consider adding XML documentation comments to explain the purpose of each property, especially
ResetTickwhich might not be self-explanatory.
17-21: Properly defined NPC metadata record.The record captures the essential properties needed for NPC spawning. Consider adding documentation for the
Weightproperty to clarify its meaning and how it affects spawn behavior.
23-34: Comprehensive interact object metadata.This record contains all necessary properties for interactive objects, but some property names could be more self-explanatory:
- What does
Reactablerepresent? Is it a path to a reactable asset or a boolean flag?- What does
KeepAnimatecontrol in the spawn behavior?Consider adding XML documentation to clarify these properties.
Maple2.Server.Game/Packets/TriggerPacket.cs (1)
111-120: Enhanced SidePopupTalk method with additional parameters.The method now supports different talk types and includes a
usmparameter. The default parameter values help maintain backward compatibility. The packet writing order has been adjusted to match the client's expectations.For future reference, consider adding a comment explaining what the
usmparameter represents, as its purpose is not immediately clear from the name.Maple2.Server.Game/Trigger/TriggerContext.Field.cs (1)
459-465: Remove or utilize unused parameters.
The parameterdescis not used, which might indicate dead code or an incomplete feature. Consider either removing it or implementing its usage to avoid confusion.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
316-334: Handle concurrent iteration for improved stability.
Enumerating theNpcs,Mobs, andPetsdictionaries while they are potentially modified in parallel may produce partial or inconsistent results. To avoid concurrency issues, consider capturing a snapshot or employing additional synchronization if strict consistency is required.
372-376: Validate concurrent enumeration in multi-dictionary queries.
Similar to theGetActorsBySpawnIdmethod, constructing theIEnumerablefrom multiple dictionaries may yield inconsistent data if these collections change at runtime. For stricter consistency, consider copying to a local list inside a locked section or employing thread-safe iteration patterns.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
374-380: Use consistent naming for clarity.
The local variablefieldSpawnGroupis confusingly named when retrieving a spawn point NPC. Renaming it tospawnPointNpcor similar could improve readability.
382-406: Align naming to match object usage.
Objects created inSpawnInteractObjectare stored infieldChests, which can be misleading if they are not exclusively chests. You might consider a different collection name (e.g.,fieldInteractibles) or clarifying comments.Maple2.Server.Game/Trigger/TriggerContext.Interface.cs (2)
162-166: Fix the incorrect log message labelThe debug log message uses "[SideNpcTalkBottom]" when it should be "[SideNpcTalk]" to match the actual method name.
- DebugLog("[SideNpcTalkBottom] npcId:{NpcId}, illust:{Illustration}, duration:{Duration}, script:{Script}, voice:{Voice}", + DebugLog("[SideNpcTalk] npcId:{NpcId}, illust:{Illustration}, duration:{Duration}, script:{Script}, voice:{Voice}", npcId, illust, duration, script, voice);
178-181: Fix incorrect log message labelThe debug log message uses "[SideNpcMovie]" when it should be "[SideNpcCutin]" to match the actual method name.
- DebugLog("[SideNpcMovie] illust:{Illustration}, duration:{Duration}", illust, duration); + DebugLog("[SideNpcCutin] illust:{Illustration}, duration:{Duration}", illust, duration);Maple2.Server.Game/Packets/SurvivalEventPacket.cs (2)
9-12: Add documentation and consider removing empty enumThe
SurvivalEventPacketclass lacks documentation explaining its purpose and usage. Additionally, theCommandenum is currently empty, which suggests it might be intended for future use but doesn't serve any purpose now.Consider either:
- Adding enum values if they're already known
- Removing the empty enum until needed
- Adding a TODO comment explaining planned values
13-19: Improve method naming and documentationThe method name
Testsuggests this might be temporary or experimental code. Consider renaming it to better reflect its purpose (e.g.,SendSurvivalContentsWidgetorBroadcastSurvivalEvent).Also, the comment on line 15 "bool to enable?" is unclear. If this is a boolean flag to enable something, it should be properly documented what exactly is being enabled.
- public static ByteWriter Test(SurvivalContentsWidget widget) { + /// <summary> + /// Creates a packet for broadcasting survival content widget information to clients. + /// </summary> + /// <param name="widget">The survival contents widget containing storm and safe zone data</param> + /// <returns>A ByteWriter containing the packet data</returns> + public static ByteWriter BroadcastSurvivalWidget(SurvivalContentsWidget widget) { var pWriter = Packet.Of(SendOp.SurvivalEvent); - pWriter.WriteByte(0); // bool lto enable? + pWriter.WriteByte(0); // 0 = Enable survival content display pWriter.WriteClass<SurvivalContentsWidget>(widget); return pWriter; }Maple2.Server.Game/Model/Field/Widget/SurvivalContentsWidget.cs (1)
36-43: Document hardcoded values and add property accessorsThe
WriteTomethod contains several hardcoded values with minimal comments. These should be properly documented or extracted as constants with meaningful names.Also, consider adding public properties for
StormCenterandSafeZoneCenterto allow setting these values from outside the class.+// Constants for survival content configuration +private const short STORM_DAMAGE = 500; +private const short STORM_RADIUS = 3000; +private const short STORM_INTERVAL = 1000; +private const short SAFE_ZONE_RADIUS = 1000; +// Properties to allow setting center positions +public Vector3 StormCenterPosition { + get => StormCenter; + set => StormCenter = value; +} + +public Vector3 SafeZoneCenterPosition { + get => SafeZoneCenter; + set => SafeZoneCenter = value; +} public void WriteTo(IByteWriter writer) { - writer.WriteShort(500); - writer.WriteShort(3000); // radius - writer.WriteShort(1000); + writer.WriteShort(STORM_DAMAGE); // Damage per tick inside storm + writer.WriteShort(STORM_RADIUS); // Storm radius + writer.WriteShort(STORM_INTERVAL); // Time interval between damage ticks writer.Write<Vector3>(StormCenter); - writer.WriteShort(1000); // radius of center + writer.WriteShort(SAFE_ZONE_RADIUS); // Safe zone radius writer.Write<Vector3>(SafeZoneCenter); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
Maple2.Database/Storage/Metadata/MapEntityStorage.cs(3 hunks)Maple2.Database/Storage/Metadata/ServerTableMetadataStorage.cs(3 hunks)Maple2.File.Ingest/Mapper/ItemMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/MapEntityMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/ServerTableMapper.cs(2 hunks)Maple2.Model/Enum/CombineSpawnGroupType.cs(1 hunks)Maple2.Model/Enum/Interact.cs(1 hunks)Maple2.Model/Game/IFieldProperty.cs(1 hunks)Maple2.Model/Metadata/ItemMetadata.cs(1 hunks)Maple2.Model/Metadata/MapEntity/MapEntity.cs(1 hunks)Maple2.Model/Metadata/MapEntity/Ms2RegionBoxSpawn.cs(1 hunks)Maple2.Model/Metadata/MapEntityMetadata.cs(1 hunks)Maple2.Model/Metadata/ServerTable/CombineSpawnTable.cs(1 hunks)Maple2.Model/Metadata/ServerTableMetadata.cs(1 hunks)Maple2.Server.DebugGame/Maple2.Server.DebugGame.csproj(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(9 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(3 hunks)Maple2.Server.Game/Model/Field/Entity/FieldInteract.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs(1 hunks)Maple2.Server.Game/Model/Field/Widget/SurvivalContentsWidget.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ItemPickupHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs(3 hunks)Maple2.Server.Game/PacketHandlers/UserChatHandler.cs(2 hunks)Maple2.Server.Game/Packets/SurvivalEventPacket.cs(1 hunks)Maple2.Server.Game/Packets/TriggerPacket.cs(3 hunks)Maple2.Server.Game/Scripting/Trigger/TriggerEnums.cs(1 hunks)Maple2.Server.Game/Trigger/TriggerContext.Field.cs(2 hunks)Maple2.Server.Game/Trigger/TriggerContext.Interface.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (40)
Maple2.Server.Game/PacketHandlers/ItemPickupHandler.cs (1)
56-62: Good implementation of pickup effect handling.This new block correctly handles items with pickup effects by applying their buffs to the player without adding them to inventory. The approach ensures that items with these effects are processed correctly and consistently.
Maple2.Model/Metadata/ItemMetadata.cs (1)
86-89: Record property addition looks good.The new
PickUpEffectboolean property in theItemMetadataAdditionalEffectrecord is a clean addition that maintains the model's structure while enabling the new functionality.Maple2.File.Ingest/Mapper/ItemMapper.cs (1)
218-220: Properly updated to accommodate the model change.The mapper now correctly includes the
dropEffectvalue when creatingItemMetadataAdditionalEffectinstances, ensuring data consistency between the parsed file data and the model objects.Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs (2)
1-2: Added required namespace for Vector3.The addition of the System.Numerics namespace is appropriate for the Vector3 usage in the new method.
104-106: New case properly integrated.The implementation of the DefenseGuard case in the switch statement correctly calls the new handler method.
Maple2.Model/Enum/Interact.cs (1)
68-72:Details
❓ Verification inconclusive
Appropriate enum extensions for spawn group functionality.
The new enum values (
Notice,InstallNPC,Portal,SpawnPoint,PVP) appropriately support the PR objectives of implementing spawn groups for Mushking Royale and Kritias Invasion features. The additions are correctly placed at the end of the enum without explicit values, maintaining backward compatibility.If there are any switch statements that check all cases of
InteractCubeControlType, they might need updating to handle these new values. Consider running:
🏁 Script executed:
#!/bin/bash # Search for switch statements on InteractCubeControlType rg "switch.*InteractCubeControlType" -A 10 -B 2Length of output: 49
Action required: Confirm switch statement handling for new enum values
The new enum values in
Maple2.Model/Enum/Interact.cs(lines 68–72) correctly extend support for spawn group functionality while preserving backward compatibility. The enum values are appended without explicit assignments, so they function as intended.However, our automated search for switch statements referencing
InteractCubeControlTypedid not yield any results. This result is inconclusive, and there’s a possibility that switch cases elsewhere in the codebase might not yet account for the new enum values.
- Review location:
Maple2.Model/Enum/Interact.cs(lines 68–72)- Action: Please manually verify if there are any switch statements on
InteractCubeControlTypethat need updates to gracefully handle the new enum values.Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (1)
36-42: Setting SpawnPointId to 0 seems counterintuitive.On line 41,
npc.SpawnPointId = 0is set despite the NPC being spawned from a spawn point. This might be intentional, but it seems contradictory. Consider if this should instead reference the current spawn point's ID or document why it's explicitly set to 0.Maple2.Server.DebugGame/Maple2.Server.DebugGame.csproj (1)
27-27: Package upgrade looks goodThe SixLabors.ImageSharp package has been updated to version 3.1.7, which is appropriate. This package is used for image processing in the debug game.
Maple2.Model/Metadata/MapEntity/Ms2RegionBoxSpawn.cs (1)
5-11: New record type for box region spawns looks well-structuredThis new record type
Ms2RegionBoxSpawnprovides a clear data structure for box-shaped region spawns with appropriate spatial properties (position, rotation, scale) and identifying properties (Id, SpawnTypeId).The inheritance from
MapBlockis appropriate as this is a type of map entity block.Maple2.Model/Metadata/MapEntity/MapEntity.cs (1)
60-60: JsonDerivedType attribute correctly added for Ms2RegionBoxSpawnThe JsonDerivedType attribute has been properly added for the new Ms2RegionBoxSpawn type with a unique discriminator value. This ensures proper JSON serialization/deserialization capabilities for this new map entity type.
Maple2.File.Ingest/Mapper/MapEntityMapper.cs (1)
122-126: Looks good!This new case for
IMS2RegionBoxSpawnintegrates seamlessly into the existing switch statement and properly populates theBlockproperty withMs2RegionBoxSpawn. The usage of(int) boxSpawn.SpawnTypeIDappears appropriate, aligning with the typical pattern throughout the mapper.Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs (5)
9-17: Validate potential concurrency and data structures usage.This class declaration and its private fields look well structured, with separate
WeightedSet<SpawnNpcMetadata>andWeightedSet<SpawnInteractObjectMetadata>for NPCs and interactable objects. However, confirm that the field manager does not call these methods concurrently. If concurrency is possible, ensure thread-safety for the shared collections (spawnIds,npcs, etc.).Would you like a script to check for concurrency usages or potential multi-threaded calls to this class?
18-23: Constructor logic is clear.Initializing
resetTickand callingInit()right after setting up the weighted sets is a straightforward approach, ensuring the spawn data is populated early. No issues spotted here.
51-59: ToggleActive flipping spawn states.The
ToggleActivemethod correctly clears or re-initializes spawns based on the bool toggle. Very straightforward logic.
61-74: Initialization method is consistent.
InitializeSpawns()checks ifTotalCount> 0 and dispatches to eitherSpawnNpcs()orSpawnInteractObjects()depending on the group type. This is consistent with the approach inInit().
120-132: Re-initializing spawns upon reset tick.Overriding
Updateto reset the spawns works well for timed re-initialization. EnsureToggleActive(false)cleans up states properly when a field is unloaded or no longer active, to avoid unbounded growth ofspawnIds.Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
1645-1705: Implementation is coherent and well-organized.The new
ParseCombineSpawn()method consistently partitions spawn data into dictionaries for spawn groups, NPCs, and interactable objects. Returning aCombineSpawnTablewith these structures is a clean approach. This looks properly aligned with how other parse methods inServerTableMapperhandle grouped data.Maple2.Model/Metadata/ServerTable/CombineSpawnTable.cs (1)
6-8: Well-structured dictionary usage with clear key definitions.The dictionaries are appropriately defined as read-only with nested structure, and the comments clearly document the meaning of each key (MapId, groupId, combineId). This enables type-safe access while maintaining good organization of the spawn metadata.
Maple2.Server.Game/Packets/TriggerPacket.cs (3)
8-8: Appropriate namespace import.Adding the Trigger namespace import enables the use of the SideNpcTalkType enum.
26-26: Good renaming of enum value.Replacing the generic
Unknown19with the descriptiveSetVisibleUiimproves code readability and self-documentation.
125-132: Well-implemented UI visibility control.This new method allows toggling UI elements visibility through a simple interface. The implementation writes the command and parameters in the correct order.
Maple2.Model/Game/IFieldProperty.cs (2)
77-81: Improved FieldPropertySightRange with better property access.The changes enhance flexibility by:
- Changing
inittosetaccessors for runtime modification- Consolidating fade values into a single array
- Setting sensible defaults
This is a good refactoring that simplifies the code while maintaining functionality.
86-88: Efficient fade values writing using array iteration.Using a loop to write fade values from the array is cleaner than writing each fade value individually. This approach is more maintainable and accommodates potential future changes to the number of fade values.
Maple2.Model/Metadata/MapEntityMetadata.cs (1)
19-19:Details
✅ Verification successful
Good addition of BoxRegionSpawns property.
The new property follows the same pattern as existing properties, using a required read-only dictionary for type safety and immutability. This addition logically extends the metadata to support box-shaped spawn regions alongside the existing region spawns.
Consider ensuring that the corresponding mapper and storage implementations initialize this dictionary correctly, even if empty, to prevent null reference exceptions.
🏁 Script executed:
#!/bin/bash # Check if BoxRegionSpawns is properly initialized in the mapper code # Look for initialization pattern similar to other region spawns echo "Searching for MapEntityStorage implementation..." rg -A 5 "boxRegionSpawns.*=.*new" --type csLength of output: 833
Code Initialization Verified – No Changes Required
The new
BoxRegionSpawnsproperty is well implemented. The mapper and storage code (seeMaple2.Database/Storage/Metadata/MapEntityStorage.cs) correctly initializes the dictionary, ensuring it won’t be null. The property follows the established pattern for type safety and immutability.Maple2.Model/Metadata/ServerTableMetadata.cs (1)
46-46: Well-implemented JSON polymorphism support for CombineSpawnTableThe addition of
CombineSpawnTableas a derived type follows the established pattern in the codebase for polymorphic JSON serialization/deserialization. This change properly integrates the new table type into the existing framework.Maple2.Server.Game/Scripting/Trigger/TriggerEnums.cs (1)
16-16: Good implementation of NPC dialog presentation typesThis new enum provides well-defined types for different NPC dialog presentation styles, supporting the UI trigger fixes mentioned in the PR description. The byte-based enum pattern is consistent with other enumerations in this file.
Maple2.Database/Storage/Metadata/ServerTableMetadataStorage.cs (1)
28-28: Clean integration of CombineSpawnTableThe implementation follows the established pattern of lazy-loaded metadata tables. The field declaration, property accessor, and initialization in the constructor are consistent with the existing code structure, making this a seamless integration.
Also applies to: 48-48, 69-69
Maple2.Database/Storage/Metadata/MapEntityStorage.cs (1)
41-42: Proper implementation of boxRegionSpawnsThe addition of
Ms2RegionBoxSpawnhandling is well-integrated into the existing entity processing system:
- The dictionary declaration follows the same pattern as other entity collections
- The switch case correctly processes and stores the new entity type
- The dictionary is properly included in the returned MapEntityMetadata
This implementation provides the foundation needed for the spawn groups feature mentioned in the PR objectives.
Also applies to: 72-74, 134-134
Maple2.Server.Game/Trigger/TriggerContext.Field.cs (2)
84-92: Check for potential out-of-bounds access and clarify usage ofUnknown.
The loop writing tosightRange.Fades[i]may risk an out-of-bounds error ifrangeexceedsFades.Length. Consider validating thatrangedoes not exceed the array length. Additionally,Unknown = enabled;lacks clarity, which could lead to confusion. Using a more descriptive property name or documentation would be helpful.
444-456: Iterate safely and consider logging toggled groups.
These lines properly handle missing group metadata. However, you might consider also logging successful toggles for better traceability when diagnosing spawn issues. Other than that, the flow looks good.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
369-370: Combined dictionary check appears correct.
Merging the checks forfieldInteracts,fieldAdBalloons, andfieldChestsinto a single return statement is concise and understandable. No immediate concerns.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (5)
6-6: Added imports look correct.
The newusingdirectives forMaple2.Model.CommonandMaple2.Tools.VectorMathappear legitimate.Also applies to: 18-18
36-36: NewfieldChestsdictionary introduced.
Creating this concurrent dictionary is aligned with the other concurrency patterns. No concerns here.
40-41: Additional dictionaries for NPCs and groups.
Using concurrent dictionaries forfieldSpawnPointNpcsandfieldSpawnGroupsprovides consistent parallel access. Implementation aligns with existing concurrency usage.
126-128: Overload to spawn NPC fromSpawnPointNPClooks fine.
This overload delegates to the existingSpawnNpcmethod, minimizing duplicated logic.
354-363: Confirm potential ID collisions.
AddSpawnPointNpcdirectly indexesfieldSpawnPointNpcsbymetadata.Idwithout checking if a conflicting entry exists. You might consider verifying the ID's uniqueness or gracefully handling duplicates.Maple2.Server.Game/Trigger/TriggerContext.Interface.cs (4)
137-140: Nice implementation of UI visibility broadcast!The changes correctly implement broadcasting UI visibility settings to clients and appropriately downgrade logging from error to debug level. This change aligns with the PR objectives of addressing UI triggers.
168-171: Well-implemented NPC talk broadcast functionalityGood job implementing the broadcast functionality for bottom NPC talks with the appropriate talk type and downgrading the log level from error to debug.
173-176: Proper implementation of movie side popup broadcastThe change appropriately implements broadcasting for movie popups and uses the correct debug log level.
183-185: Appropriate log level change for widget actionsThe change from error to debug level logging is appropriate since widget actions are normal operations rather than error conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs (2)
90-97:⚠️ Potential issuePrevent potential infinite loop in NPC spawning.
When
WeightedSetkeeps returning duplicates already inspawnIds, the loop can run indefinitely. This mirrors a previously noted concern.Consider imposing a retry limit, for instance:
-do { +int retryCount = 0; do { SpawnNpcMetadata npc = npcs.Get(); if (spawnIds.Contains(npc.SpawnId)) { continue; } Field.ToggleNpcSpawnPoint(npc.SpawnId); spawnIds.Add(npc.SpawnId); -} while (totalCount > spawnIds.Count); +} while (totalCount > spawnIds.Count && retryCount++ < 50);
113-120:⚠️ Potential issueSimilar infinite loop risk for interact objects.
This loop is virtually the same as the NPC loop;
WeightedSetduplicates can cause indefinite repetition. Introduce a safety mechanism, similar to the NPC loop fix.-do { +int retryCount = 0; do { SpawnInteractObjectMetadata interactObject = interactObjects.Get(); if (spawnIds.Contains(interactObject.RegionSpawnId)) { continue; } Field.SpawnInteractObject(interactObject); spawnIds.Add(interactObject.RegionSpawnId); -} while (totalCount > spawnIds.Count); +} while (totalCount > spawnIds.Count && retryCount++ < 50);
🧹 Nitpick comments (5)
Maple2.Server.Game/Packets/SurvivalEventPacket.cs (3)
9-17: The class looks well-structured but could benefit from documentation.The implementation follows the packet builder pattern used elsewhere in the codebase. Consider adding XML documentation to explain the purpose of this class and its role in handling survival events (Mushking Royale and Kritias Invasion).
+ /// <summary> + /// Handles packet operations for survival events such as Mushking Royale and Kritias Invasion. + /// </summary> public static class SurvivalEventPacket { + /// <summary> + /// Creates a packet to update the survival contents widget with current state. + /// </summary> + /// <param name="widget">The widget containing survival content state to be updated</param> + /// <returns>A ByteWriter containing the serialized packet data</returns> public static ByteWriter Update(SurvivalContentsWidget widget) {
12-12: Fix the unclear comment.The comment "bool lto enable?" seems to have a typo or is unclear. Consider clarifying the purpose of this byte value.
- pWriter.WriteByte(0); // bool lto enable? + pWriter.WriteByte(0); // Boolean flag to enable the survival event
13-13: Consider adding null check for widget parameter.If the
widgetparameter is null, theWriteClassmethod might throw an exception. Consider adding a null check or using null-conditional operators if appropriate.- pWriter.WriteClass<SurvivalContentsWidget>(widget); + pWriter.WriteClass<SurvivalContentsWidget>(widget ?? throw new ArgumentNullException(nameof(widget)));Also ensure you add the corresponding
using System;directive at the top of the file.Maple2.Server.Game/Model/Field/Widget/SurvivalContentsWidget.cs (1)
50-57: Use constants for magic numbersThe WriteTo method contains several magic numbers that would benefit from being defined as constants with meaningful names to improve code readability and maintainability.
+ // Constants for survival content parameters + private const short DEFAULT_STORM_DECAY_RATE = 500; + private const short DEFAULT_STORM_RADIUS = 3000; + private const short DEFAULT_STORM_DAMAGE = 1000; + private const short DEFAULT_SAFE_ZONE_RADIUS = 1000; + public void WriteTo(IByteWriter writer) { - writer.WriteShort(500); - writer.WriteShort(3000); // radius - writer.WriteShort(1000); + writer.WriteShort(DEFAULT_STORM_DECAY_RATE); + writer.WriteShort(DEFAULT_STORM_RADIUS); + writer.WriteShort(DEFAULT_STORM_DAMAGE); writer.Write<Vector3>(StormCenter); - writer.WriteShort(1000); // radius of center + writer.WriteShort(DEFAULT_SAFE_ZONE_RADIUS); writer.Write<Vector3>(SafeZoneCenter); }Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs (1)
77-121: Refactor repetitive spawning logic.The methods
SpawnNpcs()andSpawnInteractObjects()share similar structures (removal of invalid IDs, do-while logic, etc.). Consolidate into a single method with parameters to reduce duplication and enhance maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(10 hunks)Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs(1 hunks)Maple2.Server.Game/Model/Field/Widget/SurvivalContentsWidget.cs(1 hunks)Maple2.Server.Game/Packets/SurvivalEventPacket.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
Maple2.Server.Game/Model/Field/Widget/SurvivalContentsWidget.cs (3)
18-19: Proper initialization of Vector3 fieldsGood job initializing the Vector3 fields to prevent serializing uninitialized values. This addresses a previously identified issue.
22-36: Well-structured action handlingThe switch-based approach for function dispatch is more efficient and type-safe than reflection-based method invocation. This is a good implementation that properly handles unknown functions with appropriate logging.
39-48: Document empty method implementationsThe empty methods still lack documentation explaining their intended behavior or implementation plan.
private void StormData(string step) { - + // TODO: Implement storm data processing when step changes + // This should update StormCenter based on the current step } private void EnterStep(string step) { + // TODO: Implement actions when entering a new step + // This should handle any state changes needed when a player enters a step } private void ExitStep(string step) { - + // TODO: Implement actions when exiting a step + // This should handle any cleanup or state changes when a player exits a step }Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (13)
36-36: New field for storing chest interactions.This field adds storage for chest-type interactions, which is a necessary extension to keep field objects organized by their category.
40-41: Added storage for spawn point NPCs and spawn groups.These concurrent dictionaries provide thread-safe storage for tracking spawn points and groups, which appears to be part of the field spawn groups feature implementation.
114-114: Modified SpawnPointId assignment logic.Changed to use
0as a default value when owner is null, which appears to be more appropriate than the previous implementation that likely used a global ID.
126-128: Made SpawnNpc method public.The method's access modifier change from private to public enables external classes to create NPCs with SpawnPointNPC data, supporting the new spawn functionality.
354-363: New method to add and initialize spawn point NPCs.This method properly follows the established pattern of other "Add" methods in the class and correctly initializes the spawn on creation.
365-372: Use atomic operations or locks to avoid race conditions.If multiple threads call
ToggleCombineSpawnwith the sameGroupIdat roughly the same time, the code could create multipleFieldSpawnGroupinstances. Consider usingGetOrAddor locks for a clean concurrency solution.
374-380: Added method to toggle NPC spawn points.The method correctly checks if the spawn point exists before attempting to trigger it, following a defensive programming approach.
382-407: New method to spawn interact objects.This method handles the creation of interactive objects based on metadata and adds them to the field. The implementation correctly:
- Validates input data before proceeding
- Creates appropriate objects with necessary properties
- Broadcasts the addition to all relevant clients
509-516: Create default properties based on requested type.Currently, any missing field property is replaced with
FieldPropertySightRangeeven if a different property was requested. This can lead to incorrect property usage. Consider properly instantiating the requested property type or throwing a descriptive error if unavailable.
657-676: Consider removing objects fromfieldChestsas well.Your method removes the interact object only from
fieldAdBalloonsorfieldInteracts, ignoring the case in which an object resides infieldChests. This discrepancy may lead to stale data. Update the removal logic to handle all dictionaries consistently.
678-693: Improved NPC removal logic.The updated implementation:
- First checks if the NPC exists before scheduling removal
- Added a safety check to prevent double-removal
- Maintains the existing delayed removal functionality
This is a good improvement that prevents potential errors and race conditions.
737-739: Added chest objects loading for new players.This change ensures that when new players enter the field, they correctly receive all chest interact objects, maintaining consistent state across all clients.
805-807: Added display of relevant timers.This addition ensures that timers marked as displayable are properly shown to players, fixing the issue mentioned in the PR description where timers would only display when relevant.
2 participants
Summary by CodeRabbit
New Features
Enhancements