Conversation
WalkthroughThis pull request introduces several new game features and updates. A new Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant SC as SurveyCommand
participant FM as FieldManager
participant PAP as HomeActionPacket
P->>SC: Issue survey command (open/add/start/end)
SC->>FM: Update or create HomeSurvey state
FM->>PAP: Construct survey packet (question/options/status)
PAP->>P: Broadcast survey update to field
sequenceDiagram
participant P as Player
participant BC as BallCommand
participant PAP as HomeActionPacket
P->>BC: Issue ball command with size parameter
BC->>BC: Validate home map ownership and check for existing ball
alt Existing Ball Present
BC->>PAP: Build removal packet for existing BallGuideObject
end
BC->>BC: Create new BallGuideObject with specified size
BC->>PAP: Build addition packet for new ball
PAP->>P: Broadcast new ball addition to the field
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 8
🔭 Outside diff range comments (1)
Maple2.Server.Game/Commands/CommandRouter.cs (1)
41-41:⚠️ Potential issueFix inconsistent case handling in RegisterCommands method.
The constructor converts aliases to lowercase but RegisterCommands doesn't, leading to inconsistent behavior.
- dictionaryBuilder.Add(alias, command); + dictionaryBuilder.Add(alias.ToLower(), command);
🧹 Nitpick comments (14)
Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs (3)
27-27: Address the TODO comment about party chat and dungeon validation.The TODO comment indicates that additional validation is needed for party chat and dungeon scenarios.
Would you like me to help implement the party chat and dungeon validation logic?
33-35: Consider providing feedback when roll is disabled.The command silently fails when not in home and
EnableRollEverywhereis false. Consider sending a message to inform the user why the command didn't work.if (!Constant.EnableRollEverywhere) { + session.Send(NoticePacket.Notice(NoticePacket.Flags.Message, + new InterfaceText(StringCode.s_err_cannot_use_here))); return; }
22-39: Consider extracting magic numbers into constants.The random number range (1-100) is hardcoded. Consider extracting these values into named constants for better maintainability.
+ private const int MIN_ROLL = 1; + private const int MAX_ROLL = 100; + private void Handle(InvocationContext context) { Character character = session.Player.Value.Character; bool isHome = character.MapId is Constant.DefaultHomeMapId; - int rng = Random.Shared.Next(1, 100); + int rng = Random.Shared.Next(MIN_ROLL, MAX_ROLL + 1);Maple2.Server.Game/Commands/HomeCommands/AlarmCommand.cs (1)
13-21: Consider enhancing command usability and documentation.The command setup could be improved in the following ways:
- Consider using a more readable command name like "host-alarm" or "hostAlarm"
- Add validation constraints to the message argument (e.g., max length)
- Enhance the command description to include usage examples and permission requirements
- public AlarmCommand(GameSession session) : base("hostalarm", "Send a message to all players in the map") { + public AlarmCommand(GameSession session) : base("host-alarm", + "Broadcasts a message to all players in the current home map. Only usable by the home owner.") { this.session = session; IsHidden = Constant.HideHomeCommands; - var message = new Argument<string[]>("message", "Message to send to all players in the map"); + var message = new Argument<string[]>("message", + "Message to broadcast (max 100 characters when joined)") { + Arity = ArgumentArity.OneOrMore + };Maple2.Server.Game/Commands/HomeCommands/GravityCommand.cs (1)
12-24: Consider adding argument validation and XML documentation.The command structure is well-organized, but could benefit from:
- Adding argument validation to ensure gravity is within acceptable ranges at construction time
- Including XML documentation for the class and public members
+/// <summary> +/// Command to modify the gravity of a player's home map. +/// </summary> public class GravityCommand : Command { private readonly GameSession session; + /// <summary> + /// Initializes a new instance of the GravityCommand. + /// </summary> + /// <param name="session">The game session for the player.</param> public GravityCommand(GameSession session) : base("hostgravity", "Change the gravity of the map") { this.session = session; IsHidden = Constant.HideHomeCommands; - var gravity = new Argument<float>("gravity", "Gravity value to set"); + var gravity = new Argument<float>("gravity", "Gravity value to set (0-10)") + { + Arity = ArgumentArity.ExactlyOne + }; + gravity.AddValidator(result => + { + if (result.GetValueForArgument<float>(gravity) < 0 || + result.GetValueForArgument<float>(gravity) > 10) + { + result.ErrorMessage = "Gravity must be between 0 and 10"; + } + });Maple2.Model/Metadata/Constants.cs (1)
114-115: Add XML documentation for the new constants.These constants control important gameplay features and should be documented to explain:
- When and where rolling is allowed
- Which commands are affected by the hide setting
- The rationale behind the default values
Add XML documentation above the constants:
+ /// <summary> + /// Controls whether rolling is allowed outside of designated areas. + /// When false, rolling is restricted to specific locations. + /// </summary> public const bool EnableRollEverywhere = false; + /// <summary> + /// Controls the visibility of home-related commands in the UI. + /// When true, home commands are hidden from the command list. + /// </summary> public const bool HideHomeCommands = true;Maple2.Server.Game/Packets/HomeActionPacket.cs (1)
153-167: Roll method uses multiple magic numbers.
Consider adding inline comments or constants to explain their meaning (e.g.,pWriter.WriteByte(1); pWriter.WriteInt(1);). This improves maintainability.Maple2.Model/Game/User/HomeSurvey.cs (1)
4-17: Public fields are straightforward, but consider encapsulation.
You might use properties or private fields with public getters/setters to maintain better control over modifications, especially for critical data likeIdandPublic.Maple2.Server.Game/Commands/HomeCommands/BallCommand.cs (2)
19-19: Document size parameter limits and behavior.The size parameter's behavior and limits should be documented in the argument description.
- var ballSize = new Argument<float>("size", () => 1, "Size of the ball"); + var ballSize = new Argument<float>("size", () => 1, "Size of the ball (0-10, scales to 60-330)");
45-48: Extract magic numbers into named constants.The size calculation uses magic numbers which should be extracted into named constants for better maintainability.
+ private const float MIN_BALL_SIZE = 60; + private const float BASE_BALL_SIZE = 30; + private const float MAX_BALL_SIZE = 330; + - size = Math.Min(30 + size * 30, 330); + size = Math.Min(BASE_BALL_SIZE + size * BASE_BALL_SIZE, MAX_BALL_SIZE); if (size < 0) { - size = 60; + size = MIN_BALL_SIZE; }Maple2.Server.Game/Commands/CommandRouter.cs (1)
63-65: Optimize string case conversion.Multiple ToLower() calls could be optimized to reduce memory allocations.
- string commandName = args[0].ToLower(); - if (aliasLookup.TryGetValue(commandName, out Command? command)) { - args[0] = commandName; // Ensure the command name is in the correct case so it's filtered by Invoke and doesn't show in the argument list. + if (aliasLookup.TryGetValue(args[0].ToLower(), out Command? command)) { + args[0] = command.Name; // Use the command's canonical nameMaple2.Server.Game/Commands/HomeCommands/SurveyCommand.cs (2)
42-43: Define command actions as constants or enum.Command strings should be defined as constants or an enum to prevent typos and improve maintainability.
+ private enum SurveyAction { + Open, + Secret, + Add, + Start, + End + } + - string firstOption = options.First().ToLower(); - switch (firstOption) { + if (!Enum.TryParse<SurveyAction>(options.First(), true, out var action)) { + session.Send(HomeActionPacket.SurveyMessage()); + return; + } + switch (action) {
26-40: Extract common validation logic.The validation logic for map ID and plot ownership is repeated in other commands. Consider extracting it to a shared helper method.
+ private bool ValidateHomeOwnership(out Character? character, out Plot? plot) { + character = session.Player?.Value?.Character; + if (character == null || character.MapId is not Constant.DefaultHomeMapId) { + return false; + } + + plot = session.Housing.GetFieldPlot(); + return plot != null && plot.OwnerId == character.AccountId; + } + private void Handle(InvocationContext context, string[] options) { - Character character = session.Player.Value.Character; - if (character.MapId is not Constant.DefaultHomeMapId) { - return; - } - - Plot? plot = session.Housing.GetFieldPlot(); - if (plot == null || plot.OwnerId != character.AccountId) { + if (!ValidateHomeOwnership(out _, out _)) { return; }Maple2.Server.Game/PacketHandlers/HomeActionHandler.cs (1)
45-45: Document packet structure.Add documentation describing the packet structure for survey responses.
+ /// <summary> + /// Handles survey response packets with the following structure: + /// - byte: Unknown + /// - long: Character ID + /// - long: Survey ID + /// - byte: Response Index + /// </summary> private static void HandleSurvey(GameSession session, IByteReader packet) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Maple2.Server.World/Migrations/20250214234115_CubeInteract.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (12)
Maple2.Model/Game/Cube/GuideObject.cs(1 hunks)Maple2.Model/Game/User/HomeSurvey.cs(1 hunks)Maple2.Model/Metadata/Constants.cs(1 hunks)Maple2.Server.Game/Commands/CommandRouter.cs(2 hunks)Maple2.Server.Game/Commands/HomeCommands/AlarmCommand.cs(1 hunks)Maple2.Server.Game/Commands/HomeCommands/BallCommand.cs(1 hunks)Maple2.Server.Game/Commands/HomeCommands/GravityCommand.cs(1 hunks)Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs(1 hunks)Maple2.Server.Game/Commands/HomeCommands/SurveyCommand.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager.State.cs(2 hunks)Maple2.Server.Game/PacketHandlers/HomeActionHandler.cs(3 hunks)Maple2.Server.Game/Packets/HomeActionPacket.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (29)
Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs (1)
12-20: LGTM! Well-structured command initialization.The class is well-organized with proper dependency injection, clear command description, and configurable visibility.
Maple2.Server.Game/Commands/HomeCommands/AlarmCommand.cs (1)
1-10: LGTM! Well-organized imports and class structure.The imports are comprehensive and the class is properly organized within the HomeCommands namespace.
Maple2.Server.Game/Commands/HomeCommands/GravityCommand.cs (1)
1-46: Overall implementation looks good with room for improvement.The command implementation is well-structured and functional. The suggested improvements around constants, documentation, and user feedback will enhance maintainability and user experience, but the core functionality is solid.
Maple2.Model/Metadata/Constants.cs (1)
114-115:Details
❓ Verification inconclusive
Verify the security implications of these settings.
The default values appear to be restrictive (
EnableRollEverywhere = false,HideHomeCommands = true), which is good from a security perspective. However, we should verify:
- That these settings cannot be bypassed client-side
- That server-side validation is in place
Let's check for server-side validation:
🏁 Script executed:
#!/bin/bash # Search for server-side validation of these constants rg -A 5 "EnableRollEverywhere|HideHomeCommands" --type csLength of output: 208
I'm not seeing any references due to the file-type filtering issue. Let's try searching for these constants across the entire repository to see if there's any server-side validation or related usage of these settings. Please execute the following script:
Once we have the output, we can confirm whether the server-side validations are in place (and that these settings cannot be bypassed from the client).
🏁 Script executed:
#!/bin/bash # Re-run the search without using a file-type filter to capture all references to the constants rg -n "EnableRollEverywhere|HideHomeCommands" .Length of output: 49
Security Configuration Verification: Manual Review Required
Automated searches for references to these constants (i.e.,
EnableRollEverywhereandHideHomeCommands) yielded no evidence of explicit server-side validation. This absence of automated findings suggests that we should manually verify the following:
- Client-Side Bypass: Confirm that these settings are not inadvertently overridden or bypassed via client-side operations.
- Server-Side Enforcement: Ensure that every usage of these constants in the server-side logic is adequately validated and enforced, preventing any security loopholes.
Please review the code manually where these constants are used or referenced (likely in compiled/inlined logic), and verify that the security measures relying on these settings are robust and not solely dependent on compile-time values.
Maple2.Server.Game/Packets/HomeActionPacket.cs (15)
2-2: No issues with adding System.Numerics.
This import is necessary for the Vector3 usage and appears correctly placed.
4-4: No issues with adding Maple2.Model.Enum.
Likely used for enum definitions in this file.
9-9: No issues with adding Maple2.Server.Game.Model.
Consistent with existing conventions for referencing classes in the server game model.
29-29: SurveyCommand enum extension looks correct.
AddingEnd = 6is consistent with the range of existing commands.
55-63: Validate or gracefully handle a null/empty alarm message.
Currently, there's no null-check for the "message" parameter. This might be acceptable if higher-level code ensures non-null values, but consider adding a safeguard to avoid potential null reference issues.
65-71: No immediate issues with SurveyMessage.
This packet writes minimal data, suggesting it's a placeholder or simple broadcast.
73-81: Consider null-check for the survey parameter.
Ifsurveycan be null at runtime, this could cause aNullReferenceException. Ensure it's never null or add a guard.
83-93: Handle potential empty Options collection.
Callingsurvey.Options.Keys.Last()can throw if the dictionary is empty. Consider checking for at least one option before writing the last key.
95-109: SurveyStart logic appears valid.
It accurately writes the owner ID, question, and options. Be mindful of null or empty data, but the approach seems coherent.
111-118: SurveyAnswer contains minimal data.
Only the respondent's name is written. Confirm that’s sufficient for your business logic (e.g., not including which option they chose).
120-151: SurveyEnd approach is flexible and handles both public and private modes.
It properly writes participant names only ifsurvey.Publicis true. This seems correct for privacy.
169-179: AddBall logic looks consistent.
The code writes the guide object's identifiers and position, then delegates writing extended data toguideObject.Value.WriteTo(pWriter).
181-188: RemoveBall method is straightforward.
No concerns, as it properly references the object ID.
190-201: UpdateBall logic is consistent.
It updates position and two velocity vectors. Verified that writing multiple vectors is correct for your intended usage.
203-213: HitBall method is well-structured.
Ensures the ball’s position and new velocity are included. No issues detected.Maple2.Model/Game/User/HomeSurvey.cs (6)
1-2: Namespace definition looks fine.
No issues with the placement or naming.
3-3: Class definition is clear and well-scoped.
TheHomeSurveyclass is an appropriate container for survey data.
19-30: Constructor correctly initializes members.
Trims the question string and sets default states. Ensure an empty[]list usage in C# 9+ is consistently recognized asnew List<string>().
32-37: Start method updates survey ownership and populates participants.
Logic is sound. Confirm that switchingStartedto true only after everything (like options) is set is intended.
39-43: End method setsEndedand resets state.
Clearing the question string ensures it won’t be reused unintentionally. This is consistent with finalizing a survey.
45-52: AddOption returns a boolean for success.
Good approach for controlling duplicate options. Validate that empty or whitespace-only options are handled as desired.Maple2.Model/Game/Cube/GuideObject.cs (3)
57-59: Class definition and Type property.
BallGuideObjectinherits fromIGuideObjectand setsType => Construction;Confirm that “Construction” is the intended type or if a separate enum member for “Ball” is needed.
60-64: Constructor and private field usage.
Storing a single floatsizeis concise. Ensure that negative or zero sizes are handled if relevant to gameplay.
66-68: WriteTo method writes the size only.
If more properties get added later, ensure they’re appended consistently for network serialization.Maple2.Server.Game/Manager/Field/FieldManager.State.cs (1)
45-46: LGTM! Well-designed property declaration.The property follows good encapsulation practices with a private setter and proper nullability handling.
Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs
Outdated
Show resolved
Hide resolved
| pWriter.WriteByte(1); | ||
| pWriter.WriteInt(1); |
There was a problem hiding this comment.
do we know what these are?
There was a problem hiding this comment.
Byte looks like its a branch
- 0 structure becomes Int and String
- 1 strucutre becomes int int(string code) int (string amount)
now the that int i can't tell what it is, but with 0 doesnt work
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Maple2.Server.Game/Packets/HomeActionPacket.cs (2)
120-151: Consider extracting public survey logic.The method mixes public and private survey handling logic. Consider extracting the public survey specific logic into a separate method for better maintainability.
public static ByteWriter SurveyEnd(HomeSurvey survey) { var pWriter = Packet.Of(SendOp.HomeAction); pWriter.Write<HomeActionCommand>(HomeActionCommand.Survey); pWriter.Write<SurveyCommand>(SurveyCommand.End); pWriter.WriteLong(survey.Id); pWriter.WriteBool(survey.Public); pWriter.WriteUnicodeString(survey.Question); pWriter.WriteByte((byte) survey.Options.Count); - foreach (KeyValuePair<string, List<string>> option in survey.Options) { - pWriter.WriteUnicodeString(option.Key); - pWriter.WriteByte((byte) option.Value.Count); - - if (!survey.Public) { - continue; - } - - foreach (string name in option.Value) { - pWriter.WriteUnicodeString(name); - } - } - - pWriter.WriteByte((byte) survey.AvailableCharacters.Count); - if (!survey.Public) { - return pWriter; - } - - foreach (string name in survey.AvailableCharacters) { - pWriter.WriteUnicodeString(name); - } + WriteOptions(pWriter, survey); + WriteAvailableCharacters(pWriter, survey); + + return pWriter; + } + + private static void WriteOptions(ByteWriter pWriter, HomeSurvey survey) { + foreach (var option in survey.Options) { + pWriter.WriteUnicodeString(option.Key); + pWriter.WriteByte((byte) option.Value.Count); + + if (survey.Public) { + foreach (string name in option.Value) { + pWriter.WriteUnicodeString(name); + } + } + } + } + + private static void WriteAvailableCharacters(ByteWriter pWriter, HomeSurvey survey) { + pWriter.WriteByte((byte) survey.AvailableCharacters.Count); + if (survey.Public) { + foreach (string name in survey.AvailableCharacters) { + pWriter.WriteUnicodeString(name); + } + } + }
153-167: Consider adding constants for magic numbers.The method contains several magic numbers that could benefit from explicit constants:
- The byte values at lines 159-160 (as discussed in past comments)
- The number 2 at line 162 representing the number of strings
+ private const byte ROLL_BRANCH = 1; + private const int ROLL_CODE = 1; + private const int ROLL_STRING_COUNT = 2; public static ByteWriter Roll(Character character, int number) { var pWriter = Packet.Of(SendOp.HomeAction); pWriter.Write<HomeActionCommand>(HomeActionCommand.Roll); pWriter.WriteLong(character.AccountId); pWriter.WriteUnicodeString(character.Name); pWriter.WriteByte(); - pWriter.WriteByte(1); - pWriter.WriteInt(1); + pWriter.WriteByte(ROLL_BRANCH); + pWriter.WriteInt(ROLL_CODE); pWriter.Write(StringCode.s_ugcmap_fun_roll); - pWriter.WriteInt(2); // number of strings + pWriter.WriteInt(ROLL_STRING_COUNT); pWriter.WriteUnicodeString(character.Name); pWriter.WriteUnicodeString(number.ToString()); return pWriter; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Packets/HomeActionPacket.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (3)
Maple2.Server.Game/Packets/HomeActionPacket.cs (3)
2-9: LGTM! Well-structured command categorization.The enums provide a clear and organized categorization of commands, with sequential values and descriptive names.
Also applies to: 15-37
103-106: Consider adding collection size validation.The byte casting
(byte)survey.Options.Countcould overflow if the collection grows beyond 255 items.Add validation to ensure the collection size remains within byte limits:
+ if (survey.Options.Count > byte.MaxValue) { + throw new ArgumentException($"Survey options count {survey.Options.Count} exceeds maximum allowed {byte.MaxValue}"); + } pWriter.WriteByte((byte) survey.Options.Count);Also applies to: 127-139
169-213: LGTM! Well-implemented ball physics handling.The ball-related methods are well-structured, following a consistent pattern and properly handling the ball's state, position, and velocity vectors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Field/FieldManager.State.cs (1)
46-52: Optimize NPC enumeration for better performance.The
EnumerateNpcsmethod creates a new list by concatenating two collections, which could be inefficient for large collections. Consider returning an iterator instead.Apply this diff to improve performance:
#region Helpers -public ICollection<FieldNpc> EnumerateNpcs() => Npcs.Values.Concat(Mobs.Values).ToList(); +public IEnumerable<FieldNpc> EnumerateNpcs() => Npcs.Values.Concat(Mobs.Values); public IReadOnlyDictionary<int, FieldPlayer> GetPlayers() { return Players; } public ICollection<FieldPortal> GetPortals() => fieldPortals.Values; #endregion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Maple2.Server.Game/Manager/Field/FieldManager.Home.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager.State.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager.cs(0 hunks)
💤 Files with no reviewable changes (1)
- Maple2.Server.Game/Manager/Field/FieldManager.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (1)
Maple2.Server.Game/Manager/Field/FieldManager.Home.cs (1)
5-17: Add validation and event broadcasting.The survey management implementation needs improvements:
- Add null validation in
SetHomeSurvey- Broadcast state changes to clients
- Add XML documentation
- Consider adding state validation
Apply this diff to improve the implementation:
public partial class FieldManager { public readonly long OwnerId; public HomeSurvey? HomeSurvey { get; private set; } + /// <summary> + /// Sets the home survey for this field. + /// </summary> + /// <param name="survey">The survey to set. Must not be null.</param> + /// <exception cref="ArgumentNullException">Thrown when survey is null.</exception> + /// <exception cref="InvalidOperationException">Thrown when a survey already exists.</exception> public void SetHomeSurvey(HomeSurvey survey) { + ArgumentNullException.ThrowIfNull(survey); + + if (HomeSurvey != null) { + throw new InvalidOperationException("A survey already exists in this field."); + } + HomeSurvey = survey; + Broadcast(HomeActionPacket.SurveyStart(survey)); } + /// <summary> + /// Removes the current home survey from this field. + /// </summary> public void RemoveHomeSurvey() { + if (HomeSurvey == null) { + return; + } + + var survey = HomeSurvey; HomeSurvey = null; + Broadcast(HomeActionPacket.SurveyEnd(survey)); } }
2 participants
Add home commands:
Summary by CodeRabbit
New Features
Improvements