Weddings Pt. 2 - #279
Conversation
WalkthroughThe pull request introduces several enhancements related to wedding functionalities within the Maple2 application. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 48
🧹 Outside diff range and nitpick comments (32)
Maple2.Server.Game/Packets/WeddingBillboardPacket.cs (1)
14-24: LGTM with a minor suggestion: Consider adding null check.The
Loadmethod effectively constructs the packet for wedding billboard data. The use of generics and the overall structure are appropriate. However, consider adding a null check for thehallsparameter to prevent potential null reference exceptions.Consider adding a null check at the beginning of the method:
public static ByteWriter Load(IList<WeddingHall> halls) { + if (halls == null) + throw new ArgumentNullException(nameof(halls)); + var pWriter = Packet.Of(SendOp.WeddingBillboard); // ... rest of the method }Maple2.Server.Game/Service/ChannelService.Marriage.cs (1)
7-10: LGTM: Proper method signature and error handling.The method signature is correct for a gRPC service method, and the initial error handling for a non-existent session is appropriate. The use of pattern matching with
out GameSession? sessionis a good practice.Consider using string interpolation for better readability in the error message:
- throw new RpcException(new Status(StatusCode.NotFound, $"Unable to find: {request.ReceiverId}")); + throw new RpcException(new Status(StatusCode.NotFound, $"Unable to find session for receiver ID: {request.ReceiverId}"));Maple2.Server.Core/proto/common.proto (2)
246-256: LGTM! Consider adding comments for clarity.The
MarriageRequestmessage structure is well-designed and consistent with other request messages in the file. The use ofoneoffor different marriage-related actions provides flexibility and extensibility.Consider adding brief comments to explain the purpose of
idandreceiver_idfields for better clarity. For example:// ID of the character initiating the request int64 id = 1; // ID of the character's spouse int64 receiver_id = 2;
258-260: LGTM! Consider enhancing the response structure.The
MarriageResponsemessage structure is consistent with other response messages in the file. However, to provide more informative responses, consider the following enhancements:
- Add a boolean
successfield to explicitly indicate the operation's success.- Include a
string messagefield for detailed error messages or success confirmations.Here's a suggested improvement:
message MarriageResponse { bool success = 1; int32 error_code = 2; string message = 3; }This structure would allow for more detailed and flexible responses to marriage-related operations.
Maple2.Database/Model/Mail.cs (2)
77-77: LGTM: Conversion operator updated to include WeddingInvite.The implicit conversion operator has been correctly updated to include the new
WeddingInviteproperty when converting fromMaple2.Model.Game.Mail. This ensures bidirectional preservation of wedding invitation information.For consistency, consider moving the
WeddingInviteassignment to be grouped with other string properties likeTitleandContent. This would improve readability and maintain a logical grouping of similar properties.
21-21: Summary: WeddingInvite property successfully integrated.The addition of the
WeddingInviteproperty and its integration into the conversion operators has been implemented correctly. These changes support the PR objectives for implementing wedding-related functionalities while maintaining consistency with the existing code structure.Consider the following to further improve the implementation:
- Ensure that any methods or services that interact with the
WeddingInviteproperty appropriately.- Update any relevant documentation or comments to reflect the addition of this new property and its purpose in the wedding reservation system.
- If not already planned, consider adding unit tests to verify the behavior of the
WeddingInviteproperty, especially focusing on the conversion operators.Also applies to: 45-45, 77-77
Maple2.Model/Metadata/TableMetadata.cs (1)
80-80: LGTM! Consider documenting the wildcard usage.The addition of
WeddingTablewith the type discriminator "wedding*" aligns well with the PR objectives of implementing wedding-related features. The use of a wildcard provides flexibility for future wedding-related tables without requiring further modifications to this file.Consider adding a comment explaining the rationale behind using the wildcard discriminator "wedding*". This will help future developers understand the intended usage and potential implications.
Example:
// Using "wedding*" as a wildcard discriminator to accommodate various wedding-related tables [JsonDerivedType(typeof(WeddingTable), typeDiscriminator: "wedding*")]Maple2.Model/Game/Mail.cs (1)
75-77: Add null check forhallparameter.The
SetWeddingInvitemethod is a good addition for encapsulating the logic of setting wedding invitations. However, to improve robustness, consider adding a null check for thehallparameter to prevent potentialNullReferenceExceptions.Here's a suggested improvement:
public void SetWeddingInvite(WeddingHall hall) { if (hall == null) { throw new ArgumentNullException(nameof(hall)); } WeddingInvite = $"""<ms2><wedding groom="{hall.ReserverName}" bride="{hall.PartnerName}" date="{hall.CeremonyTime}" package="{hall.PackageId}" hall="{hall.PackageHallId}"/></ms2>"""; }Maple2.Model/Game/Sync/StateSync.cs (1)
53-54: Consistent renaming improves code organizationThe changes to the region name and variable are consistent with the earlier enum renaming, improving overall code clarity and organization.
Consider renaming the region to
#region Animationfor even better consistency with the flag name.Maple2.Server.Core/proto/channel/channel.proto (1)
37-38: LGTM! Consider adding a comment for consistency.The addition of the
MarriageRPC method is appropriate and aligns with the PR objectives to implement wedding-related functionalities. The method signature follows the established pattern in theChannelservice.For consistency with other RPC methods in this service, consider adding a brief comment describing the purpose of this method. For example:
+ // Handle marriage-related requests. rpc Marriage(maple2.MarriageRequest) returns (maple2.MarriageResponse);Maple2.Model/Error/WeddingError.cs (2)
7-20: LGTM: Enum declaration and initial entries.The
WeddingErrorenum is correctly declared as a public short type. The use of Description attributes for error messages is a good practice for providing clear, user-friendly messages.Consider adding XML documentation comments for the enum and its members to improve code documentation and IDE support.
27-94: LGTM: Comprehensive wedding error messages.This section of the enum provides a wide range of clear and descriptive error messages for various wedding-related scenarios. The use of placeholders (e.g., {0}) in some messages allows for dynamic content insertion, which is good for flexibility.
For consistency, consider using placeholders in all messages where dynamic content might be needed. For example, in the message for
s_wedding_result_err_lack_meratal, you might want to include the amount of merets required:[Description("Insufficient merets. {0} merets required.")] s_wedding_result_err_lack_meratal = 37,This would allow for more informative error messages across all scenarios.
Maple2.Server.Core/proto/world/world.proto (1)
49-50: LGTM! Consider adding documentation and import.The new
MarriageRPC method is a good addition to support the wedding hall reservation functionality. However, to improve clarity and maintainability:
- Consider adding a comment to document the purpose of this method, similar to other methods in the service.
- Ensure that the
maple2package is imported at the beginning of the file if it's not already present.Example:
// Handle marriage-related requests such as creating, changing, or canceling wedding hall reservations. rpc Marriage(maple2.MarriageRequest) returns (maple2.MarriageResponse);Maple2.Server.Game/Util/ConditionUtil.cs (2)
139-142: LGTM! Consider grouping wedding-related conditions for better readability.The addition of wedding-related conditions is consistent with the existing pattern and aligns with the PR objectives. Good job!
To improve code readability, consider grouping all wedding-related conditions together. You could add a comment above them to clearly indicate the wedding-related section:
+ // Wedding-related conditions case ConditionType.wedding_propose: case ConditionType.wedding_propose_decline: case ConditionType.wedding_propose_declined: case ConditionType.wedding_hall_cancel:This grouping will make it easier to maintain and expand wedding-related functionality in the future.
260-263: LGTM! Consider refactoring to reduce duplication withCheckCode.The addition of wedding-related conditions is consistent with the changes in the
CheckCodemethod and aligns with the PR objectives. Well done!To reduce code duplication between
CheckCodeandCheckTargetmethods, consider creating a shared method for conditions that always return true. This could improve maintainability and reduce the risk of inconsistencies. Here's a suggestion:private static bool IsAlwaysTrueCondition(ConditionType conditionType) { return conditionType switch { ConditionType.wedding_propose or ConditionType.wedding_propose_decline or ConditionType.wedding_propose_declined or ConditionType.wedding_hall_cancel or // ... other always true conditions ... => true, _ => false }; }Then, in both
CheckCodeandCheckTargetmethods, you can use:if (IsAlwaysTrueCondition(conditionType)) { return true; }This approach would centralize the logic for these conditions, making it easier to maintain and extend in the future.
Maple2.Server.Game/Manager/Items/InventoryManager.cs (2)
541-551: Great addition of tag-based item searchThe new
Find(ItemTag itemTag)method is a valuable addition to theInventoryManagerclass. It provides a convenient way to search for items across all inventory tabs based on their tags. The implementation is consistent with existing patterns, including thread-safety and expiration checks.Consider a minor optimization:
To potentially improve performance, you could break the inner loop early if the item is found:
public IEnumerable<Item> Find(ItemTag itemTag) { lock (session.Item) { foreach ((InventoryType type, ItemCollection items) in tabs) { foreach (Item item in items) { if (item.IsExpired()) continue; if (item.Metadata.Property.Tag != itemTag) continue; yield return item; + break; // Exit the inner loop after finding a matching item } } } }This change would return only the first non-expired item with the matching tag from each tab. If multiple items with the same tag in a single tab are needed, then the current implementation should be kept.
534-551: Overall improvement to inventory managementThe changes in this file significantly enhance the
InventoryManagerclass. The addition of expiration checks in the existingFindmethod and the new tag-based search functionality improve the robustness and flexibility of the inventory system. These enhancements align well with the PR objectives, potentially facilitating easier management of wedding-related items and reservations.Consider adding unit tests to verify the behavior of these new and modified methods, especially focusing on edge cases like expired items and various item tag scenarios.
Maple2.Model/Metadata/Constants.cs (1)
Hardcoded Max Level Values Detected
Several files still contain the old max level value
70. Please update these instances to use thecharacterMaxLevelconstant to ensure consistency across the codebase.
Maple2.Server.Core/Formulas/BaseStat.csMaple2.File.Ingest/MapperExtensions.csMaple2.Model/Error/UgcMapError.csMaple2.Model/Enum/ItemTag.csMaple2.Model/Enum/JobGroup.csMaple2.Model/Enum/ConditionType.csMaple2.Model/Enum/ActorState.csMaple2.Model/Game/Item/ItemType.csMaple2.Model/Enum/SpecialAttribute.csMaple2.Model/Enum/StringCode.csMaple2.Server.Game/Model/Field/Actor/FieldPet.csMaple2.Server.Game/Manager/ItemEnchantManager.cs🔗 Analysis chain
Line range hint
509-509: Character max level updated.The
characterMaxLevelconstant has been updated to 99. This change could have significant implications for various game systems such as experience gain, level-gated content, and character progression.To ensure this change is properly reflected throughout the codebase, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the old max level (assuming it was 70) echo "Searching for potential hardcoded old max level values:" rg --type csharp "\b70\b" --glob "!**/Constants.cs" echo "Searching for uses of characterMaxLevel constant:" rg --type csharp "characterMaxLevel"Length of output: 2150
Maple2.Model/Enum/Wedding.cs (3)
8-8: Consider renaming 'ForceDivorce' to 'ForcedDivorce' for grammatical consistencyFor grammatical accuracy, consider renaming
ForceDivorcetoForcedDivorce.Apply this diff if you agree:
Married = 2, ConsentualDivorce = 3, - ForceDivorce = 4, + ForcedDivorce = 4, DivorceCoolOff = 5, }
9-9: Clarify 'DivorceCoolOff' by renaming to 'DivorceCoolingOffPeriod'For improved clarity, consider renaming
DivorceCoolOfftoDivorceCoolingOffPeriodto better describe its purpose.Apply this diff if you agree:
ConsentualDivorce = 3, ForceDivorce = 4, - DivorceCoolOff = 5, + DivorceCoolingOffPeriod = 5, }
35-35: Consider renaming 'GroomBride' to 'BrideAndGroom' for clarityUsing
BrideAndGroommay be clearer and more descriptive of the combined entry type.Apply this diff if you agree:
Bride = 2, Groom = 4, - GroomBride = Groom | Bride, + BrideAndGroom = Groom | Bride, }Maple2.Server.World/Service/WorldService.Marriage.cs (1)
9-10: Add logging when the player is not foundWhen the player with
ReceiverIdis not found (line 9), no log message is recorded. Logging this event can aid in debugging and monitoring.Consider adding a log statement:
logger.Information("Player with ReceiverId {ReceiverId} not found.", request.ReceiverId);Maple2.Model/Metadata/ServerTable/ScriptConditionTable.cs (2)
25-33: Consider nullable types for optional properties inMaidDataIf certain properties in
MaidData, such asint ClosenessRank, might not have a value in some contexts, consider defining them as nullable types. This explicitly indicates that the property may not always have a value.public record MaidData( bool Authority, bool Expired, bool ReadyToPay, - int ClosenessRank, + int? ClosenessRank, TimeCondition ClosenessTime, TimeCondition MoodTime, TimeCondition DaysBeforeExpired );
35-40: Ensure consistent use of nullable types inWeddingDataIn
WeddingData, some properties are nullable while others are not. Review whether properties likeint MarriageDaysandTimeSpan CoolingOffshould be nullable to accurately reflect cases where they might be unset or inapplicable.public record WeddingData( MaritalStatus? UserState, HallStatus HallState, bool? HasReservation, - int MarriageDays, - TimeSpan CoolingOff); + int? MarriageDays, + TimeSpan? CoolingOff);This ensures consistency and makes it clear which properties are optional.
Maple2.Database/Model/WeddingHall.cs (1)
16-16: Consider renamingPublictoIsPublicfor clarityThe property
Publicon line 16 is a boolean indicating whether the wedding hall is public. To adhere to C# naming conventions and enhance code readability, consider renaming it toIsPublic.Maple2.Database/Storage/Game/GameStorage.Wedding.cs (1)
Line range hint
53-76: Potential issue with partner assignment in GetMarriage methodThe logic for assigning
Partner1andPartner2may be inverted. Whenpartner1.CharacterId == characterId, theInfoandMessageforPartner1are set topartner2andmarriage.Partner2Message, respectively. This might be unintentionally swapping the partners.Consider revising the assignment to correctly represent
Partner1andPartner2. Here's a suggested fix:Partner1 = new MarriagePartner { - Info = partner1.CharacterId == characterId ? partner2 : partner1, - Message = partner1.CharacterId == characterId ? marriage.Partner2Message : marriage.Partner1Message, + Info = partner1.CharacterId == characterId ? partner1 : partner2, + Message = partner1.CharacterId == characterId ? marriage.Partner1Message : marriage.Partner2Message, }, Partner2 = new MarriagePartner { - Info = partner1.CharacterId == characterId ? partner1 : partner2, - Message = partner1.CharacterId == characterId ? marriage.Partner1Message : marriage.Partner2Message, + Info = partner1.CharacterId == characterId ? partner2 : partner1, + Message = partner1.CharacterId == characterId ? marriage.Partner2Message : marriage.Partner1Message, },This adjustment ensures that
Partner1corresponds to the requesting character andPartner2to the other party.Maple2.Server.Game/PacketHandlers/WeddingHandler.cs (4)
47-76: Missing default case in switch statementIn the
Handlemethod's switch statement (lines 47-76), there is no default case to handle unexpected command values.Consider adding a default case to log or handle unknown commands:
default: // Handle unknown command session.Send(WeddingPacket.Error(WeddingError.s_wedding_result_err_unknown_command)); return;
227-228: Incomplete implementation inHandleInvitationFindUserThe method
HandleInvitationFindUserat lines 227-228 reads the invitee's name but does not contain any logic beyond that.It seems this method is incomplete. Would you like assistance in implementing the logic to find the user and proceed with sending an invitation? I can help draft the implementation or open a GitHub issue to track this task.
230-231: Incomplete implementation inHandleSendInvitationSimilarly,
HandleSendInvitationat lines 230-231 reads the invite count but lacks further logic.This method appears incomplete. Do you need help completing the implementation to send invitations based on the invite count? I can assist with writing the code or creating a GitHub issue for tracking.
12-12: Duplicate using directiveAt line 12, there is a
usingdirective forMaple2.Tools.Extensions, which might already be included or unnecessary.Ensure that all
usingdirectives are necessary and there are no duplicates to keep the code clean.Maple2.Server.Game/Manager/MarriageManager.cs (1)
600-602: Optimize the wedding hall change cost calculationThe method
GetWeddingHallChangeCostcalculates the cost difference when changing wedding halls:private long GetWeddingHallChangeCost(WeddingPackage.HallData currentHallData, WeddingPackage.HallData unconfirmedHallData) { return Math.Max(0, unconfirmedHallData.MeretCost - currentHallData.MeretCost); }Consider adding a comment to explain why
Math.Maxis used here, as it might not be immediately clear to future maintainers.Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
Line range hint
209-214: Potential issue:Replace("!", "")removes all occurrences of '!'In the
ParseToIntKeyValuePairmethod, usinginput.Replace("!", "")will remove all instances of'!'in the input string. If the intention is to remove only the leading'!', consider usinginput = input.Substring(1);to remove only the first character.Apply this diff to fix the issue:
private static KeyValuePair<int, bool> ParseToIntKeyValuePair(string input) { bool value = !input.StartsWith("!"); if (!value) { - input = input.Replace("!", ""); + input = input.Substring(1); } if (!int.TryParse(input, out int key)) { key = 0; } return new KeyValuePair<int, bool>(key, value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
Maple2.Server.World/Migrations/20241012062127_WeddingHall.Designer.csis excluded by!Maple2.Server.World/Migrations/*Maple2.Server.World/Migrations/20241012062127_WeddingHall.csis excluded by!Maple2.Server.World/Migrations/*Maple2.Server.World/Migrations/Ms2ContextModelSnapshot.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (33)
- Maple2.Database/Context/Ms2Context.cs (2 hunks)
- Maple2.Database/Model/Mail.cs (3 hunks)
- Maple2.Database/Model/WeddingHall.cs (1 hunks)
- Maple2.Database/Storage/Game/GameStorage.Wedding.cs (4 hunks)
- Maple2.Database/Storage/Metadata/TableMetadataStorage.cs (3 hunks)
- Maple2.File.Ingest/Maple2.File.Ingest.csproj (1 hunks)
- Maple2.File.Ingest/Mapper/ServerTableMapper.cs (6 hunks)
- Maple2.File.Ingest/Mapper/TableMapper.cs (3 hunks)
- Maple2.Model/Enum/Wedding.cs (2 hunks)
- Maple2.Model/Error/WeddingError.cs (1 hunks)
- Maple2.Model/Game/Mail.cs (3 hunks)
- Maple2.Model/Game/Sync/StateSync.cs (5 hunks)
- Maple2.Model/Game/User/Marriage.cs (2 hunks)
- Maple2.Model/Game/User/WeddingHall.cs (1 hunks)
- Maple2.Model/Metadata/Constants.cs (2 hunks)
- Maple2.Model/Metadata/ServerTable/ScriptConditionTable.cs (2 hunks)
- Maple2.Model/Metadata/Table/WeddingTable.cs (1 hunks)
- Maple2.Model/Metadata/TableMetadata.cs (1 hunks)
- Maple2.Server.Core/proto/channel/channel.proto (1 hunks)
- Maple2.Server.Core/proto/common.proto (1 hunks)
- Maple2.Server.Core/proto/world/world.proto (1 hunks)
- Maple2.Server.Game/Commands/DebugCommand.cs (1 hunks)
- Maple2.Server.Game/Manager/Items/InventoryManager.cs (1 hunks)
- Maple2.Server.Game/Manager/MarriageManager.cs (5 hunks)
- Maple2.Server.Game/PacketHandlers/UserSyncHandler.cs (1 hunks)
- Maple2.Server.Game/PacketHandlers/WeddingBillboardHandler.cs (1 hunks)
- Maple2.Server.Game/PacketHandlers/WeddingHandler.cs (3 hunks)
- Maple2.Server.Game/Packets/MarriagePacket.cs (0 hunks)
- Maple2.Server.Game/Packets/WeddingBillboardPacket.cs (1 hunks)
- Maple2.Server.Game/Packets/WeddingPacket.cs (1 hunks)
- Maple2.Server.Game/Service/ChannelService.Marriage.cs (1 hunks)
- Maple2.Server.Game/Util/ConditionUtil.cs (2 hunks)
- Maple2.Server.World/Service/WorldService.Marriage.cs (1 hunks)
💤 Files with no reviewable changes (1)
- Maple2.Server.Game/Packets/MarriagePacket.cs
🧰 Additional context used
🔇 Additional comments (62)
Maple2.Server.Game/Packets/WeddingBillboardPacket.cs (3)
1-5: LGTM: Imports are appropriate and concise.The imports cover all necessary dependencies for the
WeddingBillboardPacketclass functionality.
9-13: LGTM: Class structure is well-defined.The
WeddingBillboardPacketis appropriately defined as a static class, which is suitable for utility classes. The private enumCommandenhances type-safety and readability in theLoadmethod.
1-25: Overall, the implementation looks solid and aligns with the PR objectives.The
WeddingBillboardPacketclass effectively implements the functionality for creating wedding billboard packets. The code is well-structured, uses appropriate language features, and follows good practices. With the minor suggestion of adding a null check, this implementation should work well within the larger context of the wedding feature enhancements.Maple2.Server.Game/Service/ChannelService.Marriage.cs (2)
1-6: LGTM: Appropriate namespace and class declaration.The namespace
Maple2.Server.Game.Servicealigns well with the file path and project structure. The use of a partial class forChannelServiceis a good practice for organizing related functionality.
12-19: 🛠️ Refactor suggestion
⚠️ Potential issueAdd a default case to the switch statement and consider adding logging.
The switch statement clearly handles the
RemoveMarriageandRemoveWeddingHallcases. However, it's recommended to add a default case to handle unexpectedMarriageCasevalues. This would improve error handling and make the code more robust.Add a default case to the switch statement:
switch (request.MarriageCase) { case MarriageRequest.MarriageOneofCase.RemoveMarriage: session.Marriage.RemoveMarriage(); break; case MarriageRequest.MarriageOneofCase.RemoveWeddingHall: session.Marriage.RemoveWeddingHall(); break; + default: + throw new RpcException(new Status(StatusCode.InvalidArgument, $"Unsupported MarriageCase: {request.MarriageCase}")); }Consider adding logging statements before each operation to improve traceability:
switch (request.MarriageCase) { case MarriageRequest.MarriageOneofCase.RemoveMarriage: + logger.LogInformation($"Removing marriage for session {session.Id}"); session.Marriage.RemoveMarriage(); break; case MarriageRequest.MarriageOneofCase.RemoveWeddingHall: + logger.LogInformation($"Removing wedding hall for session {session.Id}"); session.Marriage.RemoveWeddingHall(); break; default: + logger.LogWarning($"Unsupported MarriageCase: {request.MarriageCase} for session {session.Id}"); throw new RpcException(new Status(StatusCode.InvalidArgument, $"Unsupported MarriageCase: {request.MarriageCase}")); }To ensure that the
RemoveMarriageandRemoveWeddingHallmethods are properly implemented, let's verify their existence in the codebase:✅ Verification successful
Add a default case to the switch statement and consider adding logging.
The
RemoveMarriageandRemoveWeddingHallmethods exist inMarriageManager.cs, ensuring that the switch statement correctly references them. Adding a default case will enhance error handling, and incorporating logging will improve traceability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of RemoveMarriage and RemoveWeddingHall methods # Test: Search for RemoveMarriage method echo "Searching for RemoveMarriage method:" rg --type csharp "RemoveMarriage\s*\([^)]*\)\s*\{" -g "!**/obj/**" -g "!**/bin/**" # Test: Search for RemoveWeddingHall method echo "Searching for RemoveWeddingHall method:" rg --type csharp "RemoveWeddingHall\s*\([^)]*\)\s*\{" -g "!**/obj/**" -g "!**/bin/**"Length of output: 545
Maple2.File.Ingest/Maple2.File.Ingest.csproj (1)
23-23: Verify the package update and its relevance to PR objectives.The
Maple2.File.Parser.Tadeuccipackage has been updated from version 2.1.30 to 2.1.311. This is a significant version jump that might introduce breaking changes or new features.
- Could you please confirm if this update is intentional?
- How does this package update relate to the wedding hall reservation functionalities mentioned in the PR objectives?
To ensure this update doesn't have unintended consequences, please run the following script to check for any other files that might be affected by this package update:
This script will help identify any files that might need attention due to the package update.
✅ Verification successful
Package update verified.
The
Maple2.File.Parser.Tadeuccipackage has been updated from version 2.1.30 to 2.1.311. Verification confirms that this update is isolated toMaple2.File.Ingest.csprojand does not impact other files in the codebase.Please confirm that this update is intentional and aligns with the PR objectives related to implementing wedding hall reservation functionalities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for files that might be affected by the Maple2.File.Parser.Tadeucci package update # Search for files that use types or methods from the Maple2.File.Parser.Tadeucci package echo "Files potentially affected by the package update:" rg --type csharp -l "using.*Maple2\.File\.Parser\.Tadeucci" || echo "No files found using the package directly." # Search for other project files that might reference this package echo -e "\nOther project files referencing the package:" rg --type xml -l "Maple2\.File\.Parser\.Tadeucci" -- *.csproj || echo "No other project files found referencing the package."Length of output: 602
Maple2.Server.Game/PacketHandlers/UserSyncHandler.cs (1)
42-44: Clarify the rationale behind skipping ServerTicks for WeddingEmotion stateThe introduction of a condition to skip reading
ServerTicksfor theActorState.WeddingEmotionstate alters the packet handling logic. While this change might be intentional and related to the wedding functionality improvements, it raises several questions:
- What is the specific reason for excluding
ServerTicksfor theWeddingEmotionstate?- How does this change align with the PR objective of implementing wedding hall reservations?
- Has the client-side code been updated to match this new packet structure for wedding emotions?
- Could this change potentially impact the synchronization of wedding-related actions?
Please provide more context on the motivation behind this change and confirm that it has been thoroughly tested with the client to ensure consistency.
To verify the impact of this change, please run the following script:
Maple2.Server.Core/proto/common.proto (1)
245-260: Great job on maintaining consistency and structure!The new
MarriageRequestandMarriageResponsemessage types are well-integrated into the existing protocol buffer definitions. They follow the established naming conventions and structural patterns, making the additions coherent with the rest of the file.These changes effectively support the PR objectives of implementing wedding-related functionalities, specifically the ability to remove marriages and wedding halls.
Maple2.Database/Model/Mail.cs (2)
21-21: LGTM: New property for wedding invitations added.The addition of the
WeddingInviteproperty aligns with the PR objectives for implementing wedding-related functionalities. The initialization with an empty string is a good practice to prevent potential null reference issues.
45-45: LGTM: Conversion operator updated to include WeddingInvite.The implicit conversion operator has been correctly updated to include the new
WeddingInviteproperty. This ensures that wedding invitation information is preserved when converting fromMaple2.Model.Game.MailtoMaple2.Model/Game/Mail.cs (3)
22-22: LGTM: New property for wedding invitations.The addition of the
WeddingInviteproperty is appropriate for implementing wedding-related functionalities. Initializing it with an empty string is a good practice to prevent null reference exceptions.
155-155: LGTM: Serialization of wedding invitation.The addition of
writer.WriteUnicodeString(WeddingInvite);in theWriteTomethod ensures that the wedding invitation information is properly serialized. This change is consistent with the newWeddingInviteproperty and supports the wedding-related functionalities being implemented.
Line range hint
1-176: Overall assessment: Good implementation of wedding invitation features.The changes to the
WeddingInviteproperty, theSetWeddingInvitemethod, and the update to theWriteTomethod all contribute to a cohesive implementation of the new feature.A minor improvement suggestion has been made regarding null checking in the
SetWeddingInvitemethod. Once addressed, this PR appears ready for merging.Maple2.Server.Game/Commands/DebugCommand.cs (1)
Line range hint
1-123: Overall improvement in packet handling robustnessThe changes to the
SendRawPacketCommandclass have significantly improved the input validation for raw packets. This enhancement contributes to the overall robustness of the debugging system by preventing malformed packets from being processed.Key improvements:
- Validation of hexadecimal string format for each byte.
- Minimum packet length check (although this needs a minor adjustment as noted in the previous comment).
These changes align well with the PR objectives of enhancing the system's functionality and reliability.
Maple2.Database/Context/Ms2Context.cs (3)
45-45: LGTM: WeddingHall DbSet property added correctly.The new
WeddingHallproperty has been added following the existing patterns in theMs2Contextclass. It's correctly marked as internal and uses the nullable annotation, which is consistent with other DbSet properties in this context.
78-78: LGTM: WeddingHall entity configuration added correctly.The
WeddingHallentity configuration has been added to theOnModelCreatingmethod following the existing patterns. This ensures that the new entity will be properly set up in the database schema.
45-45: Verify related WeddingHall implementations and migrations.The changes to add the
WeddingHallentity to the database context look good. To ensure full implementation:
- Verify that the
WeddingHallclass and itsConfiguremethod are properly implemented in theMaple2.Database.Modelnamespace.- Ensure that a database migration has been created to add the
WeddingHalltable to the schema.- Update any relevant repository or service classes to utilize the new
WeddingHallDbSet.To check for the
WeddingHallclass implementation and potential migration, you can run:Also applies to: 78-78
✅ Verification successful
WeddingHall integration verified successfully.
All necessary implementations and migrations for the
WeddingHallentity are present and correctly configured. No further actions are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for WeddingHall class implementation echo "Searching for WeddingHall class implementation:" rg -p "class WeddingHall" --type cs # Check for migration files echo "\nSearching for migration files related to WeddingHall:" rg -p "WeddingHall" --type cs --glob "**/Migrations/*.cs"Length of output: 1547
Maple2.Model/Game/Sync/StateSync.cs (3)
18-18: Improved enum naming for better clarityThe renaming of
Flag4toAnimationenhances code readability by explicitly stating the purpose of this flag. This change aligns well with the PR objectives and maintains backwards compatibility.
149-150: Deserialization logic updated consistentlyThe
ReadFrommethod has been correctly updated to use the newAnimationflag andAnimationNamevariable. This change maintains consistency with the earlier renaming and ensures proper deserialization.
101-102: Serialization logic updated consistentlyThe
WriteTomethod has been correctly updated to use the newAnimationflag andAnimationNamevariable. This change maintains consistency with the earlier renaming and ensures proper serialization.Maple2.Model/Error/WeddingError.cs (1)
1-5: LGTM: File header and namespace declaration.The file header, import statement, and namespace declaration are appropriate for the content of the file.
Maple2.Database/Storage/Metadata/TableMetadataStorage.cs (3)
48-48: LGTM: New private field for WeddingTable added correctly.The addition of the
weddingTablefield follows the established pattern in the class, usingLazy<T>for potential performance benefits through deferred loading.
106-106: LGTM: Public property for WeddingTable added correctly.The
WeddingTableproperty is implemented consistently with other table properties in the class, providing appropriate access to the privateweddingTablefield.
165-165: LGTM: WeddingTable initialized correctly in the constructor.The
weddingTableis initialized using theRetrieve<T>method, consistent with other table initializations. The use of"wedding*.xml"as the key allows for flexibility in handling multiple wedding-related XML files.Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
534-534: Excellent addition to filter out expired itemsThis change enhances the
Findmethod by excluding expired items from the search results. It's a crucial improvement that maintains the integrity of the inventory system and prevents potential issues with expired items being used or displayed.Maple2.Model/Metadata/Constants.cs (3)
Line range hint
1-1004: Summary of changes and potential impact.The changes to this file include:
- Addition of three new wedding coupon constants
- Addition of a new proposal item constant
- Update of the character max level to 99
These changes align with the PR objectives of implementing wedding hall reservations and related functionality. The update to the character max level could have wide-ranging effects on game balance and progression systems.
Recommendations:
- Ensure that all systems relying on the max character level are updated accordingly.
- Verify that the new wedding-related constants are used consistently in the implemented wedding functionality.
- Consider updating any documentation or player-facing information that references the old max level or lacks information about the new wedding features.
175-175: New proposal item constant added.A new constant
ProposalItemIdhas been added with the value 11600482. This is likely related to the wedding functionality being implemented.To verify the usage of this new constant, run the following script:
✅ Verification successful
Verification of New
ProposalItemIdConstantNo additional usages of
ProposalItemIdfound in the codebase beyond its declaration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new proposal item constant rg --type csharp "ProposalItemId"Length of output: 2150
105-107: New wedding coupon constants added.Three new constants for wedding coupons have been introduced:
Grade1WeddingCouponItemIdGrade2WeddingCouponItemIdGrade3WeddingCouponItemIdThese constants likely correspond to different tiers of wedding coupons in the game. Ensure that these item IDs are correctly referenced in the wedding-related functionality implemented in this PR.
To verify the usage of these new constants, run the following script:
✅ Verification successful
Verification Successful: New wedding coupon constants are correctly referenced.
- Used in
Maple2.Server.Game/Manager/MarriageManager.cs🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new wedding coupon constants rg --type csharp "Grade1WeddingCouponItemId|Grade2WeddingCouponItemId|Grade3WeddingCouponItemId"Length of output: 677
Maple2.Model/Metadata/Table/WeddingTable.cs (1)
5-6: The 'WeddingTable' record definition is well-structuredThe 'WeddingTable' record correctly encapsulates both 'Rewards' and 'Packages', providing a comprehensive model for wedding-related data. The use of
IReadOnlyDictionaryensures immutability, which is good for maintaining data integrity.Maple2.Database/Model/WeddingHall.cs (2)
65-65: Confirm automatic generation ofCreationTimeOn line 65, the
CreationTimeproperty is configured with.ValueGeneratedOnAdd(). Ensure that the database is set up to automatically generate this value upon entity creation, and that there are no conflicts with the property's setter in the code.Check if
CreationTimeis correctly set in the database upon adding a newWeddingHallrecord.
62-63:⚠️ Potential issueVerify the one-to-one relationship configuration in Entity Framework
The Entity Framework configuration on lines 62-63 aims to establish a one-to-one relationship between
WeddingHallandMarriage. The use ofOneToOne<WeddingHall, Marriage>()is unconventional and might not correctly configure the relationship. Ensure that the relationship is set up properly to avoid runtime issues.Run the following script to check the usage of relationship configurations involving
WeddingHallandMarriage:Maple2.Model/Game/User/WeddingHall.cs (2)
19-28: Consider Consistency in Property AccessibilityThe properties
Id,ReserverAccountId,ReserverCharacterId,ReserverName, andPartnerNameare marked asrequiredand haveset;accessors, whileMarriageIdhas aninit;accessor.Ensure that the use of
init;forMarriageIdis intentional and consistent with the design requirements. IfMarriageIdshould be immutable after object initialization,init;is appropriate. Otherwise, consider usingset;for consistency.
6-81: Ensure Compatibility with Target FrameworkThe code uses features like file-scoped namespaces (line 4) and
requiredproperties (lines 19-28), which are available in C# 10 and C# 11 respectively.Confirm that the project targets a framework version that supports these features (e.g., .NET 6.0 or later). If not, you may encounter compilation issues.
Maple2.Model/Game/User/Marriage.cs (4)
30-30: Serialization ofStatusas a byte is appropriateConverting the
MaritalStatusenum to a byte for serialization enhances efficiency and aligns with the expected data format.
66-66: Addition ofIsOnlineproperty enhances functionalityIntroducing the
IsOnlineproperty provides a clear and efficient way to check the partner's online status based on theInfoobject.
70-70:⚠️ Potential issueEvaluate the necessity of serializing
AccountIdIncluding
AccountIdin the serialized data may expose sensitive information. Ensure that serializingAccountIdis necessary for the client-side logic and complies with security guidelines. If onlyCharacterIdis required, consider omittingAccountIdto protect user data.
72-72:⚠️ Potential issueConfirm logic for writing partner's online status
The line
writer.WriteBool(!IsOnline);writes the inverse ofIsOnline, effectively indicating if the partner is offline. Verify whether the protocol expects a flag for offline status rather than online status. If so, consider renaming the property or adding a comment for clarity.Maple2.Server.Game/Packets/WeddingPacket.cs (8)
40-47: Verify the purpose of theWriteInt()call inUpdateHallmethodIn the
UpdateHallmethod, there is a call topWriter.WriteInt();without any arguments on line 44. This writes a default integer value (zero) to the packet. Verify whether this is intentional and necessary for the packet structure. If it's not required, consider removing it to avoid sending unintended data.If unnecessary, apply this diff to remove the line:
public static ByteWriter UpdateHall(WeddingHall hall) { var pWriter = Packet.Of(SendOp.Wedding); pWriter.Write<Command>(Command.UpdateHall); pWriter.WriteClass<WeddingHall>(hall); - pWriter.WriteInt(); return pWriter; }
49-55: Ensure the correct usage ofWeddingErrorin theErrormethodThe
Errormethod writes aWeddingErrorenum value to the packet. Confirm that theWeddingErrorenum correctly represents all possible error cases and that it aligns with the client-side expectations. This helps in ensuring that error handling is consistent and understandable.
104-110: Review the use ofWeddingErroras a parameter inDeclinedCancelReservationThe method
DeclinedCancelReservation(WeddingError reply)uses aWeddingErrorparameter namedreply. Since this method represents a declined cancellation request, verify whether sending an error code is appropriate here. If a different type of response is expected (e.g., a reason message or a different enum), consider updating the parameter type accordingly.If a different response is needed, you might adjust the method like this:
public static ByteWriter DeclinedCancelReservation(string reason) { var pWriter = Packet.Of(SendOp.Wedding); pWriter.Write<Command>(Command.DeclinedCancelReservation); - pWriter.Write<WeddingError>(reply); + pWriter.WriteUnicodeString(reason); return pWriter; }
112-118: Assess the necessity ofWeddingErrorinAcceptCancelReservationSimilar to
DeclinedCancelReservation, theAcceptCancelReservation(WeddingError reply)method includes aWeddingErrorparameter. Verify whether this is intended, as accepting a cancellation might not typically involve an error code. Ensure that the parameter accurately reflects the data needed by the client.
120-127: Confirm the use ofdifferenceCostinChangeReservationRequestThe
ChangeReservationRequestmethod includes adifferenceCostparameter. Verify that this value correctly represents the cost difference for changing the reservation and that it's calculated accurately elsewhere. This ensures that clients receive the correct information regarding any additional charges or refunds.
136-143: ValidatedifferenceCostinAcceptReservationChangeIn the
AcceptReservationChangemethod, ensure that thedifferenceCostparameter is correctly calculated and represents the final cost difference after the reservation change has been accepted. This helps maintain consistency and accuracy in billing-related functionalities.
67-72: Ensure consistency in packet construction across methodsIn the
Engagedmethod and others, packets are constructed by writing a command and then writing associated data. Review all methods to ensure that this pattern is consistently applied and that all necessary data is included in the correct order. This consistency is vital for proper packet parsing on the client side.
11-30: Review theCommandenum for completeness and accuracyEnsure that the
Commandenum includes all necessary commands and that their assigned byte values match the client's expectations. Additionally, check for any missing commands that may be required for full wedding functionality.Maple2.Database/Storage/Game/GameStorage.Wedding.cs (7)
6-6: Addition of WeddingHall alias improves code readabilityThe
using WeddingHall = Maple2.Model.Game.WeddingHall;directive simplifies the usage ofWeddingHallthroughout the code, enhancing readability.
29-29: Returning the marriage after commit ensures consistencyReturning
GetMarriage(partner1Id)after a successful commit allows the caller to immediately access the newly createdMarriageobject with all associated data.
32-41: SaveMarriage method correctly updates existing marriagesThe
SaveMarriagemethod checks if the marriage exists before updating, preventing disbanded marriages from being saved. This ensures data integrity.
43-51: DeleteMarriage method properly removes marriage recordsThe
DeleteMarriagemethod correctly finds and removes the specified marriage from the context, handling the case where the marriage does not exist.
99-112: CreateWeddingHall method initializes the wedding hall correctlyThe method properly sets up the
WeddingHallmodel with the necessary relationships and saves it to the context.
115-117: GetWeddingHall retrieves the wedding hall based on marriageThe method efficiently fetches the wedding hall associated with a specific marriage and converts it appropriately.
141-144: Verify time comparison precision in WeddingHallTimeIsAvailableWhen checking if a ceremony time is available, ensure that the precision of
DateTimecomparisons accounts for potential discrepancies due to milliseconds.Consider adjusting the comparison to account for possible differences in time precision.
return Context.WeddingHall.FirstOrDefault(hall => - hall.CeremonyTime == ceremonyDateTime) == null; + DbFunctions.TruncateTime(hall.CeremonyTime) == ceremonyDateTime.Date && + hall.CeremonyTime.Hour == ceremonyDateTime.Hour && + hall.CeremonyTime.Minute == ceremonyDateTime.Minute) == null;Alternatively, ensure that both times are standardized to the same precision before comparison.
Maple2.Server.Game/PacketHandlers/WeddingHandler.cs (2)
121-124: Verify the marital status check logicIn the
HandleReserveHallmethod, the condition at line 121 checks if the player's marital status is notMaritalStatus.Engaged. Ensure that this condition correctly identifies players who are eligible to reserve the wedding hall. If the logic is reversed, engaged players might be prevented from making reservations.Please confirm that
MaritalStatus.Engagedis the correct status for players who should be allowed to reserve the hall. If engaged players are supposed to reserve the hall, the condition should be:-if (session.Marriage.Marriage.Status != MaritalStatus.Engaged) { +if (session.Marriage.Marriage.Status == MaritalStatus.Engaged) {
220-223: Ensure proper time window after ceremony for entryAt lines 220-223, the code checks if
DateTime.Now > ceremonyTime.AddMinutes(30)to prevent entry after 30 minutes from the scheduled ceremony time. Verify that this condition aligns with the game design requirements.If players should not be able to enter after 30 minutes post-ceremony, the logic is correct. However, ensure that
ceremonyTimerepresents the actual start time and that time zones are properly handled.Maple2.Server.Game/Manager/MarriageManager.cs (2)
642-643: Ensure player info synchronization handles name changesIn the
SyncUpdatemethod, you check for changes in the partner's name and online status:if (name != partner.Info.Name || wasOnline != partner.Info.Online) { session.Send(WeddingPacket.UpdateMarriage(Marriage)); }Ensure that name changes are correctly propagated and that there are no side effects.
To verify, run the following script:
#!/bin/bash # Description: Verify that name changes trigger the appropriate updates rg --type cs 'UpdateMarriage' -A 5This will show where
UpdateMarriageis called and help confirm proper behavior.
562-565: Confirm packet handling for declined reservation changesA
TODOcomment indicates uncertainty about the packets sent when a reservation change is declined:// TODO: Confirm these packets. There seems to be no response. session.Send(WeddingPacket.DeclineReservationChange()); partner.Session.Send(WeddingPacket.DeclineReservationChange());Recommend verifying whether
WeddingPacket.DeclineReservationChange()correctly notifies both clients.To assist, run the following script to find all usages of
DeclineReservationChangein the codebase:This will help determine if the method is implemented correctly elsewhere or if additional handling is needed.
Would you like assistance in confirming the correct packet flow or opening an issue to track this?
✅ Verification successful
Packet handling for declined reservation changes confirmed
The
WeddingPacket.DeclineReservationChange()is correctly sent to both the initiating session and the partner's session, ensuring that both clients are notified appropriately.
- Maple2.Server.Game/Manager/MarriageManager.cs: Lines 562-565
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'WeddingPacket.DeclineReservationChange' in the codebase rg --type cs 'WeddingPacket\.DeclineReservationChange'Length of output: 2553
Script:
#!/bin/bash # Description: Find all usages of 'WeddingPacket.DeclineReservationChange' in the codebase rg --type cs "WeddingPacket\.DeclineReservationChange"Length of output: 292
Maple2.File.Ingest/Mapper/TableMapper.cs (2)
1527-1541: Verify property mappings forWeddingItemEnsure that the properties
itemID,count,grade, andnightRewardcorrectly correspond to theWeddingItemclass properties. Any mismatch may lead to incorrect data mapping.
1553-1555: EnsurePlannerIdis correctly assignedVerify that
package.planneris being correctly assigned toPlannerIdand that this property is intended to hold the planner's identifier. Ifplannercan be null or optional, consider adding null checks or default values.Maple2.File.Ingest/Mapper/ServerTableMapper.cs (4)
91-103: Correct usage ofParseToIntKeyValuePairmethodThe implementation correctly uses the
ParseToIntKeyValuePairmethod to parse quest and item conditions. This enhances code readability and consistency in handling key-value pairs.
116-124: Proper restructuring withMaidDataclassEncapsulating maid-related properties into the new
ScriptConditionMetadata.MaidDataclass improves code organization and maintainability. Good job on enhancing the code structure.
125-130: Proper restructuring withWeddingDataclassGrouping wedding-related properties into the new
ScriptConditionMetadata.WeddingDataclass enhances clarity and code organization. This makes the codebase more maintainable.
135-138: Consistent parsing of numeric conditionsThe consistent use of
ParseToIntKeyValuePairfor parsing conditions likeBuff,Meso,Level, andAchieveCompletedenhances code readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
Maple2.Model/Metadata/Table/WeddingTable.cs (1)
13-16: Well-structuredWeddingPackagerecord!The
WeddingPackagerecord is well-designed:
- It includes essential properties like
IdandPlannerId.- The
Hallsproperty usesIReadOnlyDictionary, ensuring immutability.One minor suggestion:
Consider adding XML documentation comments to describe the purpose of each property, especially
PlannerId. This would enhance code readability and maintainability.Example:
/// <summary> /// Represents a wedding package. /// </summary> /// <param name="Id">The unique identifier for the wedding package.</param> /// <param name="PlannerId">The identifier of the associated wedding planner.</param> /// <param name="Halls">A dictionary of wedding halls available in this package.</param> public record WeddingPackage( int Id, int PlannerId, IReadOnlyDictionary<int, WeddingPackage.HallData> Halls) { // ... }Maple2.Server.Game/PacketHandlers/WeddingHandler.cs (1)
47-76: Expanded command handlingThe
Handlemethod has been appropriately updated to include new cases for wedding-related commands, each calling its respective handling method. This is consistent with the new enum values and provides a clear structure for handling different wedding operations.For improved maintainability, consider using a dictionary-based approach or pattern matching (if using C# 8.0 or later) to map commands to their respective handlers. This could reduce the size of the switch statement and make it easier to add or modify handlers in the future.
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
Line range hint
209-229: New parsing methods improve type safety, but potential issue in string parsingThe introduction of
ParseToIntKeyValuePairandParseToStringKeyValuePairmethods improves type safety and makes the parsing logic more explicit. This is a good refactoring that enhances code clarity.However, there's a potential issue in the
ParseToStringKeyValuePairmethod:if (!value) { input = input.Substring(1); }This code removes only the first character if the input starts with '!'. If the intention is to remove all '!' characters (as done in
ParseToIntKeyValuePair), this should be changed to:if (!value) { input = input.Replace("!", ""); }Alternatively, if the intention is indeed to remove only the leading '!', the current implementation is correct, but it might be worth adding a comment explaining this behavior to prevent future confusion.
Maple2.Server.Game/Manager/MarriageManager.cs (3)
422-423: Address the TODO: Confirm and set the correct item rarityThere's a TODO comment indicating uncertainty about the rarity of the wedding coupon being created:
// TODO: Fix rarity ? Item? coupon = session.Field.ItemDrop.CreateItem(couponItemId, 1, 1);Please verify and set the appropriate rarity for the coupon item to ensure it behaves correctly in the game.
Do you need assistance determining the correct rarity value for the coupon item?
555-559: Address the TODO: Confirm packet responses for reservation changesA TODO comment suggests uncertainty about the packet responses when declining a reservation change:
// TODO: Confirm these packets. There seems to be no response. session.Send(WeddingPacket.DeclineReservationChange()); partner.Session.Send(WeddingPacket.DeclineReservationChange());Please verify that the correct packets are being sent and received to handle this scenario properly.
Would you like assistance in confirming the correct packet sequence for declining a reservation change?
Line range hint
610-612: Avoid potentialNullReferenceExceptionby checkingpartner.InfoIn both the
BeginListenandSyncUpdatemethods, the null-forgiving operator!is used withpartner.Info, which can suppress compiler warnings but may lead to runtime exceptions ifpartner.Infoisnull:// BeginListen method session.PlayerInfo.Listen(partner.Info!.CharacterId, listener); // SyncUpdate method bool wasOnline = partner.Info!.Online; string name = partner.Info.Name; partner.Info.Update(type, info);Consider adding null checks to ensure
partner.Infois notnullbefore accessing its properties.Apply this diff to add null checks:
// In BeginListen method +if (partner.Info == null) { + logger.Error("Partner info is null for CharacterId {CharacterId}", partner.CharacterId); + return; +} session.PlayerInfo.Listen(partner.Info.CharacterId, listener); // In SyncUpdate method +if (partner.Info == null) { + logger.Error("Partner info is null during SyncUpdate for CharacterId {CharacterId}", partner.CharacterId); + return true; +} bool wasOnline = partner.Info.Online; string name = partner.Info.Name; partner.Info.Update(type, info);Also applies to: 631-633
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- Maple2.File.Ingest/Maple2.File.Ingest.csproj (1 hunks)
- Maple2.File.Ingest/Mapper/ServerTableMapper.cs (6 hunks)
- Maple2.Model/Enum/Wedding.cs (2 hunks)
- Maple2.Model/Game/Sync/StateSync.cs (5 hunks)
- Maple2.Model/Metadata/ServerTable/ScriptConditionTable.cs (3 hunks)
- Maple2.Model/Metadata/Table/WeddingTable.cs (1 hunks)
- Maple2.Server.Game/Commands/DebugCommand.cs (1 hunks)
- Maple2.Server.Game/Manager/MarriageManager.cs (5 hunks)
- Maple2.Server.Game/PacketHandlers/WeddingHandler.cs (3 hunks)
- Maple2.Server.World/Service/WorldService.Marriage.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Maple2.File.Ingest/Maple2.File.Ingest.csproj
- Maple2.Model/Enum/Wedding.cs
- Maple2.Model/Game/Sync/StateSync.cs
- Maple2.Model/Metadata/ServerTable/ScriptConditionTable.cs
- Maple2.Server.Game/Commands/DebugCommand.cs
- Maple2.Server.World/Service/WorldService.Marriage.cs
🧰 Additional context used
🔇 Additional comments (13)
Maple2.Model/Metadata/Table/WeddingTable.cs (2)
5-6: Excellent restructuring of theWeddingTablerecord!The changes to the
WeddingTablerecord are well-implemented:
- The renaming from
WeddingRewardTabletoWeddingTablebetter reflects its expanded scope.- The addition of the
Packagesproperty alongsideRewardsprovides a comprehensive structure for wedding-related data.- The use of
IReadOnlyDictionaryfor both properties ensures immutability, which is a good practice for data integrity.These changes align well with the PR objectives of implementing wedding hall reservations.
17-31: 🛠️ Refactor suggestionWell-structured nested records, with room for improvement
The
HallDataandItemrecords are well-designed overall:
- The use of
IReadOnlyListforItemsandCompleteItemsensures immutability.- The nested structure provides a clear organization of related data.
Suggestions for improvement:
- Consider using an enum for the
Rarityproperty in theItemrecord instead ofshort. This would improve type safety and readability.public enum ItemRarity : short { Common = 0, Uncommon = 1, Rare = 2, Epic = 3, Legendary = 4 } public record Item( int ItemId, ItemRarity Rarity, int Amount, bool NightOnly);
Add XML documentation comments to describe the purpose of each property in both
HallDataandItemrecords. This would enhance code readability and maintainability.Consider using more descriptive names for
ItemsandCompleteItemsto clarify their distinct purposes.These suggestions align with previous review comments, particularly regarding the use of an enum for
Rarityand improving property documentation.Maple2.Server.Game/PacketHandlers/WeddingHandler.cs (7)
24-25: Improved enum naming for clarityThe change from "ReverseHall" to "ReserveHall" and "ReverseHallReply" to "ReserveHallReply" corrects a typo, improving the clarity and accuracy of the
Commandenum.
84-94: Improved error handling in proposal processThe
HandleProposemethod has been updated to useWeddingErrorfor more specific error handling and feedback. This change enhances the consistency of error handling across the wedding-related operations and provides clearer information to the user.The addition of condition updates (lines 93-94) is a good practice for tracking the proposal state.
126-127: Improved check for existing wedding hall reservationThe condition
if (session.Marriage.WeddingHall.Id != 0)is a more appropriate check for an existing wedding hall reservation compared to the previous implementation. This change addresses the issue mentioned in a past review comment and provides a more reliable way to determine if a reservation already exists.
152-182: Well-structured reservation cancellation processThe implementation of
HandleCancelReservation,CancelNoRefundReply, andHandleCancelReservationReplyprovides a comprehensive and well-structured approach to handling wedding hall reservation cancellations. The methods include appropriate checks, such as verifying the existence of a reservation before proceeding with cancellation.The use of
WeddingErrorfor error handling and confirmation is consistent with other parts of the codebase, promoting uniformity in error management across wedding-related operations.
184-200: Effective implementation of reservation change functionalityThe
HandleChangeReservationandHandleChangeReservationReplymethods provide a clear and effective way to manage changes to existing wedding hall reservations. The use ofWeddingErrorfor error handling is consistent with other parts of the codebase, maintaining a uniform approach to error management.The methods appropriately delegate the business logic to the
Marriageobject, which is a good practice for maintaining separation of concerns.
106-111:⚠️ Potential issuePotential logical error in proposal reply handling
The condition
if (response != WeddingError.s_wedding_propose_ok)on line 106 may not correctly identify a successful proposal acceptance. Typically, a successful operation is represented by an "OK" or "None" error code.Consider changing the condition to check for a success code, for example:
-if (response != WeddingError.s_wedding_propose_ok) { +if (response != WeddingError.none) {Ensure that
WeddingError.none(or the appropriate success code) is used consistently throughout the codebase to represent successful operations.
214-217:⚠️ Potential issueIncorrect logic for ceremony entry time window
The condition
if (DateTime.Now >= ceremonyTime.AddMinutes(-5))on line 214 is incorrect. This condition will prevent players from entering during the intended time window (within 5 minutes before the ceremony).To allow entry starting 5 minutes before the ceremony, change the condition to:
-if (DateTime.Now >= ceremonyTime.AddMinutes(-5)) { +if (DateTime.Now < ceremonyTime.AddMinutes(-5)) {This change will ensure that players can enter the ceremony within the intended time frame.
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (4)
91-91: Method renaming improves clarityThe change from
ParseToKeyValuePairtoParseToIntKeyValuePairenhances code readability by explicitly indicating the return type of the parsed key-value pair. This modification aligns well with the principle of making code self-documenting.Also applies to: 97-97
103-103: Improved data structure and parsing in ScriptConditionMetadataThe refactoring of
ScriptConditionMetadataconstructor improves the organization of data by introducingMaidDataandWeddingDatastructures. This change enhances code readability and maintainability. The use of specific parsing methods (ParseToIntKeyValuePairandParseToStringKeyValuePair) for different data types is a good practice that increases type safety.Also applies to: 116-130, 135-138
153-153: Consistent refactoring across related methodsThe changes in
MergeQuestScriptConditionsmirror those inMergeNpcScriptConditions, maintaining consistency across related methods. This parallel structure enhances code maintainability and reduces the likelihood of errors. The use ofParseToIntKeyValuePairand the introduction ofMaidDataandWeddingDatastructures are applied consistently.Also applies to: 159-159, 165-165, 178-192, 197-200
Line range hint
91-229: Overall improvement in data parsing and structureThe changes in this file represent a significant improvement in the parsing and structuring of game data. Key enhancements include:
- More specific parsing methods (
ParseToIntKeyValuePairandParseToStringKeyValuePair) that improve type safety.- Refactoring of
ScriptConditionMetadatato use nested structures (MaidDataandWeddingData), which improves code organization.- Consistent application of these changes across related methods (
MergeNpcScriptConditionsandMergeQuestScriptConditions).These modifications enhance code readability, maintainability, and type safety. The only point of concern is the potential inconsistency in handling '!' characters in
ParseToStringKeyValuePair, which should be reviewed and clarified if necessary.Overall, this refactoring effort significantly improves the quality of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
Maple2.Database/Storage/Game/GameStorage.Wedding.cs (9)
29-29: Approve change with performance consideration.The modification to return a full
Marriageobject instead of just an ID provides more information to the caller. However, this introduces an additional database call which might impact performance for high-volume operations.Consider optimizing by returning the created marriage object directly:
- return Commit() ? GetMarriage(partner1Id) : null; + return Commit() ? new Marriage(marriage, partner1, partner2) : null;This would require passing
partner1andpartner2objects to the method or retrieving them earlier in the method.
32-41: Approve addition with concurrency consideration.The new
SaveMarriagemethod is a good addition for updating existing marriages. It correctly checks for the existence of the marriage before updating and usesContext.TrySaveChanges()for error handling.Consider adding a concurrency check to handle potential conflicts if the marriage is updated simultaneously by multiple processes. You could use Entity Framework's concurrency tokens or implement optimistic concurrency control:
public bool SaveMarriage(Marriage marriage) { var existingMarriage = Context.Marriage.Find(marriage.Id); if (existingMarriage == null) { return false; } Context.Entry(existingMarriage).CurrentValues.SetValues(marriage); return Context.TrySaveChanges(); }
43-51: Approve addition with related data consideration.The new
DeleteMarriagemethod is well-structured for removing a marriage entry. It correctly checks for the existence of the marriage before deleting and usesContext.TrySaveChanges()for error handling.Consider handling related data cleanup, such as associated wedding halls or other dependent entities. You might want to implement a cascading delete or explicitly handle related data:
public bool DeleteMarriage(long marriageId) { using var transaction = Context.Database.BeginTransaction(); try { var marriage = Context.Marriage.Find(marriageId); if (marriage == null) { return false; } // Delete related data var relatedWeddingHalls = Context.WeddingHall.Where(wh => wh.MarriageId == marriageId); Context.WeddingHall.RemoveRange(relatedWeddingHalls); Context.Marriage.Remove(marriage); if (Context.TrySaveChanges()) { transaction.Commit(); return true; } transaction.Rollback(); return false; } catch { transaction.Rollback(); return false; } }This ensures that all related data is properly cleaned up when a marriage is deleted.
Line range hint
53-80: Approve addition with suggestion for simplification.The new
GetMarriagemethod effectively retrieves marriage information based on either character ID or wedding ID. It correctly fetches partner information and constructs aMarriageobject.The logic for assigning partners and their messages is a bit complex and might be prone to errors. Consider simplifying it:
public Marriage? GetMarriage(long characterId = 0, long weddingId = 0) { Model.Marriage? marriage = weddingId > 0 ? Context.Marriage.Find(weddingId) : Context.Marriage.FirstOrDefault(m => m.Partner1Id == characterId || m.Partner2Id == characterId); if (marriage == null) return null; PlayerInfo? partner1 = GetPlayerInfo(marriage.Partner1Id); PlayerInfo? partner2 = GetPlayerInfo(marriage.Partner2Id); if (partner1 == null || partner2 == null) return null; bool isPartner1Requester = partner1.CharacterId == characterId; return new Marriage { Id = marriage.Id, CreationTime = marriage.CreationTime.ToEpochSeconds(), ExpHistory = marriage.ExpHistory.Select<Model.MarriageExp, MarriageExp>(exp => exp).ToList(), Partner1 = new MarriagePartner { Info = isPartner1Requester ? partner2 : partner1, Message = isPartner1Requester ? marriage.Partner2Message : marriage.Partner1Message, }, Partner2 = new MarriagePartner { Info = isPartner1Requester ? partner1 : partner2, Message = isPartner1Requester ? marriage.Partner1Message : marriage.Partner2Message, }, Status = marriage.Status, }; }This simplification reduces the complexity and potential for errors in partner assignment.
99-112: Approve addition with suggestion for uniqueness check.The new
CreateWeddingHallmethod effectively creates a wedding hall associated with a marriage. The use of a transaction ensures data consistency.Consider adding a check to ensure that only one wedding hall exists per marriage:
public WeddingHall? CreateWeddingHall(WeddingHall hall, Marriage marriage) { if (Context.WeddingHall.Any(wh => wh.MarriageId == marriage.Id)) { return null; // or throw an exception } BeginTransaction(); Model.WeddingHall model = hall; model.MarriageId = marriage.Id; model.OwnerId = marriage.Partner1.CharacterId; model.CreationTime = DateTime.Now; Context.WeddingHall.Add(model); if (!SaveChanges()) { return null; } return Commit() ? GetWeddingHall(marriage) : null; }This ensures that each marriage can have only one associated wedding hall.
119-129: Approve addition with suggestion for edge case handling.The new
GetWeddingHallmethod effectively retrieves a wedding hall based on either hall ID or marriage ID. It correctly uses theGetMarriagemethod to fetch the associated marriage.Consider handling the case where both
hallIdandmarriageIdare 0:public WeddingHall? GetWeddingHall(long hallId = 0, long marriageId = 0) { if (hallId == 0 && marriageId == 0) { throw new ArgumentException("Either hallId or marriageId must be provided."); } Model.WeddingHall? model = hallId > 0 ? Context.WeddingHall.Find(hallId) : Context.WeddingHall.FirstOrDefault(hall => hall.MarriageId == marriageId); if (model == null) { return null; } Marriage? marriage = GetMarriage(weddingId: model.MarriageId); return marriage == null ? null : model.Convert(marriage); }This modification ensures that the method always has valid input and handles all possible scenarios.
131-139: Approve addition with suggestion for related entity updates.The new
DeleteWeddingHallmethod effectively removes a wedding hall entry. It correctly checks for the existence of the wedding hall before deleting and usesContext.TrySaveChanges()for error handling.Consider updating related entities that might reference this wedding hall. For example, you might want to update the associated marriage to reflect that the wedding hall has been deleted:
public bool DeleteWeddingHall(long hallId) { using var transaction = Context.Database.BeginTransaction(); try { var hall = Context.WeddingHall.Find(hallId); if (hall == null) { return false; } // Update associated marriage if necessary var marriage = Context.Marriage.Find(hall.MarriageId); if (marriage != null) { // Update marriage to reflect wedding hall deletion // For example: marriage.WeddingHallDeleted = true; Context.Marriage.Update(marriage); } Context.WeddingHall.Remove(hall); if (Context.TrySaveChanges()) { transaction.Commit(); return true; } transaction.Rollback(); return false; } catch { transaction.Rollback(); return false; } }This ensures that all related data is properly updated when a wedding hall is deleted.
141-144: Approve addition with suggestion for time buffer consideration.The new
WeddingHallTimeIsAvailablemethod effectively checks if a wedding hall is available at a specific time. It correctly converts the epoch time to DateTime and checks for existing bookings.Consider adding buffer times before and after the ceremony to avoid tight scheduling:
public bool WeddingHallTimeIsAvailable(long ceremonyTime) { DateTime ceremonyDateTime = ceremonyTime.FromEpochSeconds(); TimeSpan buffer = TimeSpan.FromHours(1); // Adjust buffer as needed return !Context.WeddingHall.Any(hall => hall.CeremonyTime >= ceremonyDateTime.Subtract(buffer) && hall.CeremonyTime <= ceremonyDateTime.Add(buffer)); }This modification ensures that there's adequate time between ceremonies for setup and cleanup.
146-159: Approve addition with suggestion for query optimization.The new
GetWeddingHallsmethod effectively retrieves all upcoming wedding halls. It correctly filters for future ceremonies and fetches associated marriage information.The current implementation may lead to an N+1 query problem by calling
GetMarriagefor each wedding hall. Consider optimizing the query to fetch all required data in a single database call:public IEnumerable<WeddingHall> GetWeddingHalls() { var entries = Context.WeddingHall .Where(hall => hall.CeremonyTime > DateTime.Now) .Include(hall => hall.Marriage) .ThenInclude(marriage => marriage.Partner1) .Include(hall => hall.Marriage.Partner2) .AsEnumerable(); foreach (var entry in entries) { var marriage = new Marriage { Id = entry.Marriage.Id, CreationTime = entry.Marriage.CreationTime.ToEpochSeconds(), ExpHistory = entry.Marriage.ExpHistory.Select<Model.MarriageExp, MarriageExp>(exp => exp).ToList(), Partner1 = new MarriagePartner { Info = entry.Marriage.Partner1, Message = entry.Marriage.Partner1Message, }, Partner2 = new MarriagePartner { Info = entry.Marriage.Partner2, Message = entry.Marriage.Partner2Message, }, Status = entry.Marriage.Status, }; yield return entry.Convert(marriage); } }This optimization reduces the number of database queries and improves performance, especially when dealing with a large number of wedding halls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Maple2.Database/Storage/Game/GameStorage.Wedding.cs (4 hunks)
🧰 Additional context used
📓 Learnings (1)
Maple2.Database/Storage/Game/GameStorage.Wedding.cs (1)
Learnt from: Zintixx PR: AngeloTadeucci/Maple2#279 File: Maple2.Database/Storage/Game/GameStorage.Wedding.cs:147-159 Timestamp: 2024-10-13T18:10:08.059Z Learning: In the `GameStorage` class in `Maple2.Database/Storage/Game/GameStorage.Wedding.cs`, the `GetMarriage` method is necessary to properly convert the `Marriage` object and initialize `Partner1` and `Partner2`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (8)
91-97: LGTM! Consider extracting repeated parsing logic.The changes look good. Using
ParseToIntKeyValuePairinstead ofParseToKeyValuePairimproves consistency and type safety. The addition of Maid and Wedding data structures enhances the detail of script conditions.To further improve the code, consider extracting the repeated parsing logic for quests and items into a separate method to reduce duplication.
Here's a suggestion for extracting the parsing logic:
private Dictionary<int, bool> ParseConditions(IEnumerable<string> conditions) { var result = new Dictionary<int, bool>(); foreach (string condition in conditions) { KeyValuePair<int, bool> parsed = ParseToIntKeyValuePair(condition); result.Add(parsed.Key, parsed.Value); } return result; } // Usage: var questStarted = ParseConditions(scriptCondition.quest_start); var questsCompleted = ParseConditions(scriptCondition.quest_complete);This refactoring would make the code more DRY and easier to maintain.
Also applies to: 103-130
153-159: LGTM! Consider refactoring to reduce duplication.The changes in MergeQuestScriptConditions are consistent with those in MergeNpcScriptConditions, which is good for maintaining uniformity across the codebase. The use of
ParseToIntKeyValuePairand the addition of Maid and Wedding data structures improve the code's consistency and expand its capabilities.Given the similarity between MergeNpcScriptConditions and MergeQuestScriptConditions, consider refactoring these methods to reduce code duplication. Here's a suggestion:
- Create a new method that encapsulates the common logic:
private ScriptConditionMetadata CreateScriptConditionMetadata( int id, int scriptId, ScriptType type, IEnumerable<string> questStart, IEnumerable<string> questComplete, IEnumerable<string> items, string[] itemCount, // ... other common parameters ) { var questStarted = ParseConditions(questStart); var questsCompleted = ParseConditions(questComplete); var itemList = ParseItems(items, itemCount); // ... create and return ScriptConditionMetadata } private List<KeyValuePair<ItemComponent, bool>> ParseItems(IEnumerable<string> items, string[] itemCount) { // ... implement item parsing logic }
- Update both MergeNpcScriptConditions and MergeQuestScriptConditions to use this new method:
scriptConditions.Add(scriptId, CreateScriptConditionMetadata( npcId, scriptId, ScriptType.Npc, scriptCondition.quest_start, scriptCondition.quest_complete, scriptCondition.item, scriptCondition.itemCount, // ... other parameters ));This refactoring would significantly reduce code duplication and make future maintenance easier.
Also applies to: 165-200
Line range hint
209-221: LGTM with suggestions for improved error handling.The new
ParseToIntKeyValuePairmethod is a good addition that centralizes the parsing logic for integer key-value pairs. However, there are a few areas where it could be improved:
- The method doesn't handle null or empty input strings.
- If parsing fails, it silently sets the key to 0, which could lead to unexpected behavior.
Consider updating the method to handle these cases more explicitly:
private static KeyValuePair<int, bool> ParseToIntKeyValuePair(string input) { if (string.IsNullOrEmpty(input)) { throw new ArgumentException("Input cannot be null or empty", nameof(input)); } bool value = !input.StartsWith("!"); if (!value) { input = input.Substring(1); } if (!int.TryParse(input, out int key)) { throw new FormatException($"Unable to parse '{input}' as an integer"); } return new KeyValuePair<int, bool>(key, value); }This version will throw appropriate exceptions for invalid inputs, making it easier to catch and handle errors at the call site.
222-229: LGTM with suggestions for consistency and input validation.The new
ParseToStringKeyValuePairmethod is a good addition that complements theParseToIntKeyValuePairmethod. It correctly handles the parsing of the boolean value from the input string.However, there are a couple of improvements that could be made:
- The method doesn't handle null or empty input strings, similar to
ParseToIntKeyValuePair.- For consistency, it might be beneficial to align the error handling approach with
ParseToIntKeyValuePair.Consider updating the method as follows:
private static KeyValuePair<string, bool> ParseToStringKeyValuePair(string input) { if (string.IsNullOrEmpty(input)) { throw new ArgumentException("Input cannot be null or empty", nameof(input)); } bool value = !input.StartsWith("!"); if (!value) { input = input.Substring(1); } return new KeyValuePair<string, bool>(input, value); }This version adds input validation and maintains consistency with the
ParseToIntKeyValuePairmethod, making the codebase more uniform and robust.
Line range hint
1116-1192: LGTM! Consider extracting XML parsing logic.The additions for StringBoard and StringBoardLink event types are straightforward and correct. The enhanced XML parsing for the AttendGift case adds more detailed functionality, which is good.
However, there's repeated XML parsing logic that could be extracted to improve readability and maintainability.
Consider creating a helper method for XML parsing to reduce repetition:
private XmlNode? ParseXmlValue(string value, string rootNodeName) { if (string.IsNullOrEmpty(value)) { return null; } var xml = new XmlDocument(); xml.LoadXml(value); return xml.FirstChild?.Name == rootNodeName ? xml.FirstChild : null; } // Usage: XmlNode? value1Node = ParseXmlValue(value1, "ms2"); XmlNode? value2Node = ParseXmlValue(value2, "ms2"); XmlNode? value3Node = ParseXmlValue(value3, "ms2"); if (value1Node != null) { // Parse value1Node } if (value2Node != null) { XmlNode? stringNode = value2Node.SelectSingleNode("string"); XmlNode? configNode = value2Node.SelectSingleNode("Config"); // ... rest of the parsing logic } // ... similar for value3NodeThis refactoring would make the code more DRY and easier to maintain, especially if you need to add more XML parsing in the future.
Line range hint
1204-1380: LGTM! Consider simplifying attribute checks.The additions to the ParseItemMergeOptionTable method expand the range of attributes that can be parsed, which is great for increasing the functionality of the system. The logic for handling these new attributes is consistent with the existing code.
However, there's some repetition in the attribute checking that could be simplified to make the code more maintainable.
Consider using sets or dictionaries to simplify the attribute checking:
private static readonly HashSet<string> BasicAttributes = new HashSet<string> { "str", "dex", "int", "luk", "hp", "hp_rgp", "hp_inv", "sp", "sp_rgp", "sp_inv", "ep", "ep_rgp", "ep_inv", "asp", "msp", "atp", "evp", "cap", "cad", "car", "ndd", "abp", "jmp", "pap", "map", "par", "mar", "wapmin", "wapmax", "dmg", "pen", "rmsp", "bap", "bap_pet" }; private static readonly HashSet<SpecialAttribute> RateBasedSpecialAttributes = new HashSet<SpecialAttribute> { SpecialAttribute.HpOnKill, SpecialAttribute.ReduceCooldown, // ... add other rate-based special attributes here }; // In the parsing logic: if (BasicAttributes.Contains(mergeOptionEntry.optionName)) { var basicAttribute = mergeOptionEntry.optionName.ToBasicAttribute(); // ... rest of the basic attribute parsing logic } else { var specialAttribute = mergeOptionEntry.optionName.ToSpecialAttribute(); bool isRateBased = RateBasedSpecialAttributes.Contains(specialAttribute); // ... use isRateBased to determine which parsing logic to use }This approach would make it easier to maintain the list of attributes and simplify the logic for determining how to parse each attribute.
Line range hint
1381-1421: LGTM! Consider grouping related fields for improved readability.The additions to the MeretMarketItemMetadata and the corresponding parsing logic in ParseMeretCustomShop are well-implemented. These changes expand the data model to support new features in the shop system, which is great for enhancing functionality.
The use of null-coalescing operators (??) to handle potential null values from the parent item is a good practice, keeping the code concise and safe.
To further improve readability, consider grouping related fields together in the MeretMarketItemMetadata constructor. For example:
return new MeretMarketItemMetadata( // Basic information Id: item.id, ParentId: parent?.id ?? 0, TabId: parent?.tabID ?? item.tabID, ItemId: parent?.itemID ?? item.itemID, Rarity: (byte)(parent?.grade ?? item.grade), // Quantity and duration Quantity: item.quantity, BonusQuantity: item.bonusQuantity, DurationInDays: item.durationDay, // Pricing and sales CurrencyType: (MeretMarketCurrencyType)(parent?.paymentType ?? item.paymentType), Price: item.price, SalePrice: item.salePrice == 0 ? item.price : item.salePrice, SaleTag: (MeretMarketItemSaleTag)item.saleTag, // Time-based properties SaleStartTime: ParseDateTime(saleStartTime), SaleEndTime: ParseDateTime(saleEndTime), PromoStartTime: ParseDateTime(promoStartTime), PromoEndTime: ParseDateTime(promoEndTime), // Requirements and restrictions JobRequirement: jobRequirement.Select(job => (JobCode)job).AsEnumerable().FilterFlags(), RequireMinLevel: parent?.minLevel ?? item.minLevel, RequireMaxLevel: parent?.maxLevel ?? item.maxLevel, RequireAchievementId: parent?.achieveID ?? item.achieveID, RequireAchievementRank: parent?.achieveGrade ?? item.achieveGrade, // Flags and additional properties RestockUnavailable: parent?.noRestock ?? item.noRestock, PcCafe: parent?.pcCafe ?? item.pcCafe, Giftable: parent?.giftable ?? item.giftable, ShowSaleTime: item.showSaleTime, // Visual elements Banner: item.banner, BannerTag: (MeretMarketBannerTag)item.bannerTag, PromoName: item.promoName ); // Helper method for parsing DateTime private long ParseDateTime(string? dateTimeString) => string.IsNullOrEmpty(dateTimeString) ? 0 : DateTime.ParseExact(dateTimeString, "yyyy-MM-dd-HH-mm-ss", CultureInfo.InvariantCulture).ToEpochSeconds();This grouping makes it easier to understand the different aspects of a MeretMarketItemMetadata at a glance.
Line range hint
1-1421: Overall LGTM with suggestions for future improvementsThe changes in this file significantly enhance the functionality of the ServerTableMapper, adding support for new game features and expanding existing ones. The additions are generally well-implemented and maintain consistency with the existing codebase.
Here are some high-level suggestions for future improvements:
Consider breaking down some of the larger methods (e.g., ParseGameEventData, ParseItemMergeOptionTable) into smaller, more focused methods to improve readability and maintainability.
Look for opportunities to extract common parsing logic into reusable helper methods, especially for XML parsing and attribute handling.
Consider introducing more robust error handling and logging throughout the file to make debugging easier in production environments.
As the file continues to grow, it might be worth considering splitting it into multiple files, each focusing on a specific aspect of the server table mapping (e.g., ShopMapper, EventMapper, etc.).
Consider adding unit tests for the parsing logic, especially for edge cases and new features, to ensure reliability as the codebase evolves.
These improvements would help maintain the code's quality and make it easier to extend and maintain in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Maple2.Server.World/Migrations/Ms2ContextModelSnapshot.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (3)
- Maple2.Database/Context/Ms2Context.cs (2 hunks)
- Maple2.File.Ingest/Mapper/ServerTableMapper.cs (6 hunks)
- Maple2.Server.Game/Commands/DebugCommand.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.Database/Context/Ms2Context.cs
- Maple2.Server.Game/Commands/DebugCommand.cs
🧰 Additional context used
🔇 Additional comments (1)
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
Line range hint
1193-1203: LGTM! Simple and consistent addition.The addition of the 'level' field to the OxQuizTable.Entry is straightforward and maintains consistency with the existing code structure. This change enhances the data model for OX Quiz entries without introducing unnecessary complexity.
Summary by CodeRabbit
New Features
WeddingInviteproperty to enhance mail functionalities related to wedding invitations.Bug Fixes
Documentation
Refactor
Chores