Parse Adjustments on new XML format #51
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce comprehensive support for a new "New" XML data format across the Maple2 file parser and its test suite. This includes new serializers, parser methods, and data classes for items, skills, maps, quests, NPCs, riding, tables, and additional effects. Locale and language handling are now parameterized, and test coverage is expanded and refactored for the new format. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant XmlSerializer
participant DataClass
Client->>Parser: new Parser(xmlReader, language)
Client->>Parser: ParseNew()
Parser->>XmlSerializer: Deserialize("new xml file")
XmlSerializer-->>Parser: DataClass instance (e.g., ItemDataNew, SkillNew)
Parser->>Client: Parsed data (Id, Name, Data)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Zintixx
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (17)
Maple2.File.Parser/Xml/Skill/Combo.cs (1)
17-26: Consider modernizing array initialization for consistency.The new
ComboNewclass follows the appropriate pattern for supporting the new XML format. However, the array initialization still usesArray.Empty<int>()syntax, which is inconsistent with the modernization seen in other files (e.g.,ItemOption.cs).Consider updating the array initialization to match the modern pattern:
- [M2dArray] public int[] inputSkill = Array.Empty<int>(); - [M2dArray] public int[] outputSkill = Array.Empty<int>(); + [M2dArray] public int[] inputSkill = []; + [M2dArray] public int[] outputSkill = [];Maple2.File.Parser/Xml/Table/ItemOptionVariation.cs (1)
72-88: Consider initializing the list field for consistency.The new
ItemOptionVariationNewRootclass follows established XML parsing patterns and has a clean structure. However, for consistency with other changes in this PR, consider initializing the list field.[XmlRoot("ms2")] public class ItemOptionVariationNewRoot { - [XmlElement] public List<Option> option; + [XmlElement] public List<Option> option = [];This ensures non-null default values and maintains consistency with the initialization pattern used throughout the codebase.
Maple2.File.Parser/Xml/Map/Map.cs (1)
13-27: LGTM! Good defensive programming with potential performance consideration.The initialization of all class fields with new instances is excellent defensive programming that prevents null reference exceptions. The use of
new()syntax is clean and modern.Consider the performance implications: if these
MapDataobjects are created frequently but not all fields are always used, you might want to implement lazy initialization for some properties to reduce memory overhead.Maple2.File.Parser/Xml/Skill/SubConditionTarget.cs (1)
37-46: New simplified class for updated XML format.The
SubConditionTargetNewclass focuses on core buff-related fields, which appears intentional for the new XML format. However, consider documenting why certain fields (likehasSkillID,eventCondition,targetCheckRange, etc.) were excluded from the new version.Consider adding XML documentation to clarify the purpose and scope of the simplified class:
+/// <summary> +/// Simplified version of SubConditionTarget for new XML format. +/// Focuses on core buff-related conditions only. +/// </summary> public partial class SubConditionTargetNew {Maple2.File.Tests/AdditionalEffectParserTest.cs (1)
27-30: Consider clarifying the test method name.The method name
TestAdditionalEffectParserNewwithLocale.KRmight be confusing. Consider renaming to something likeTestAdditionalEffectParserNewKRorTestAdditionalEffectParserNewFormatto clearly indicate what's being tested.- public void TestAdditionalEffectParserNew() { + public void TestAdditionalEffectParserNewFormat() {Maple2.File.Parser/Xml/Skill/ChangeSkill.cs (1)
22-24: Consider removing commented-out code.The commented-out properties (
changeSkillLevel,originSkillID,originSkillLevel) suggest these are intentionally omitted from the new format. If these properties are definitively not needed, consider removing the commented code to keep the codebase clean.- // [M2dArray] public int[] changeSkillLevel = Array.Empty<int>(); - // [XmlAttribute] public int originSkillID; - // [XmlAttribute] public short originSkillLevel = 1;Maple2.File.Parser/AdditionalEffectParser.cs (1)
34-46: Remove the commented debug code.The method implementation looks correct and follows the established pattern. However, there's commented debug code that should be removed.
Apply this diff to clean up the commented code:
- //if (data.id != 10200201) continue;Maple2.File.Parser/NpcParser.cs (1)
51-65: Improve variable naming for consistency.The method implementation is correct but the variable name
rootKrat line 55 is misleading since this is parsing the new format, not Korean-specific data.Apply this diff to improve variable naming:
- var rootKr = npcNewSerializer.Deserialize(reader) as NpcDataListNew; - Debug.Assert(rootKr != null); - - foreach (NpcDataRootNew item in rootKr.npcs) { + var root = npcNewSerializer.Deserialize(reader) as NpcDataListNew; + Debug.Assert(root != null); + + foreach (NpcDataRootNew item in root.npcs) {Maple2.File.Parser/Xml/Skill/RegionSkill.cs (1)
40-66: New class successfully added but consider code duplication implications.The
RegionSkillNewclass correctly implements the new XML format by duplicating relevant fields fromRegionSkillwhile omitting sensor-specific properties (sensorStartDelay,sensorSplashStartDelay,sensorForceInvokeByInterval). This suggests the new format doesn't support these sensor features.Code duplication concern: This creates significant duplication with 25+ identical field declarations. While this may be the most appropriate approach for XML serialization scenarios requiring precise schema control, consider the maintenance burden where changes to common fields must be applied to both classes.
Alternative approaches to consider in future iterations:
- Shared base class for common fields
- Composition pattern to reduce duplication
- Conditional serialization attributes
However, for XML serialization requiring strict schema control, separate classes often provide the cleanest solution.
Maple2.File.Parser/Xml/AdditionalEffect/BasicProperty.cs (2)
11-11: Consider consistent field initialization pattern.The explicit initialization of these fields to their default values (0 for int, false for bool) is inconsistent with other fields in the class that rely on implicit initialization. In C#, fields are automatically initialized to their default values.
Either initialize all fields consistently or remove the explicit default initializations to maintain consistency:
- [XmlAttribute] public int buffSubType = 0; // 0,1,2,4,6,8,16,32,64,128,256,512,1024 + [XmlAttribute] public int buffSubType; // 0,1,2,4,6,8,16,32,64,128,256,512,1024Also applies to: 15-16, 42-42, 46-46, 48-48
53-53: Initialize the new field for consistency within this change.Since other fields in this change are being explicitly initialized, consider initializing this new field as well for consistency.
- [XmlAttribute] public int attackPossibleIndividualEffectID; + [XmlAttribute] public int attackPossibleIndividualEffectID = 0;Maple2.File.Parser/Xml/Skill/TriggerSkill.cs (1)
35-64: Consider reducing code duplication between skill classes.The new
ActionSkillandActionAdditionalclasses share many fields withTriggerSkill, creating maintenance challenges. Consider extracting common fields into a base class or shared component.Example approach using a base class:
public abstract class BaseSkillAction { [XmlAttribute] public uint delay; [XmlAttribute] public int removeDelay; [XmlAttribute] public int interval; [XmlAttribute] public bool immediateActive; [XmlAttribute] public int fireCount = 1; [XmlAttribute] public bool nonTargetActive; [XmlAttribute] public bool useDirection; [XmlAttribute] public bool onlySensingActive; [XmlAttribute] public bool dependOnCasterState; [XmlAttribute] public bool independent; [XmlAttribute] public bool chain; [XmlAttribute, DefaultValue(150.0f)] public float chainDistance = 150.0f; [M2dArray] public int[] linkSkillID = Array.Empty<int>(); }Maple2.File.Parser/Xml/Skill/Kinds.cs (2)
39-56: Review the extensive commenting of XML attributes.Many previously active XML attributes have been commented out in the new version. This suggests significant schema changes between the old and new XML formats.
Consider removing the commented-out attributes entirely rather than keeping them as comments, unless they serve as documentation for migration purposes:
- //[XmlAttribute] public int type; // 0,1,2,3 - //[XmlAttribute] public int subType; // 0,1,2,3,4,5,6,7,8,9 - // [XmlAttribute] public string state = string.Empty; // gos* - // [XmlAttribute] public bool spRecoverySkill; - // [XmlAttribute] public int motionType; // 0 - // [XmlAttribute] public string emotion = string.Empty; - // [XmlAttribute] public float offsetHPBar; - // [XmlAttribute] public bool weaponDependency; - // [XmlAttribute] public bool releaseStunState;This would improve code clarity and reduce maintenance burden.
63-69: Consider removing commented client-ignored fields.Since these fields are already marked as "Ignored by client" and are now commented out entirely, consider removing them to reduce code clutter.
Maple2.File.Tests/SkillParserTest.cs (1)
32-33: Clean up commented-out code.These event handler subscriptions appear to be debugging code. Either uncomment them if they're needed for the tests, or remove them entirely.
- // parser.skillNewSerializer.UnknownElement += TestUtils.UnknownElementHandler; - //parser.skillNewSerializer.UnknownAttribute += TestUtils.UnknownAttributeHandler;Maple2.File.Parser/Xml/Skill/Skill.cs (1)
168-172: Document the property name changes in the new format.The property names have changed (e.g.,
pauseProperty→pause,rangeProperty→range). Consider adding XML comments to document these naming convention changes to help developers understand the mapping between old and new formats.+ /// <summary> + /// Pause property (previously pauseProperty in the old format) + /// </summary> [XmlElement] public PauseProperty pause; + /// <summary> + /// Range property (previously rangeProperty in the old format) + /// </summary> [XmlElement] public RegionSkillNew range;Maple2.File.Parser/Xml/AdditionalEffect/AdditionalEffect.cs (1)
94-129: Consider adding XML documentation for complex array properties.The
Additionalclass contains many array properties whose purpose might not be immediately clear. Adding XML documentation would improve maintainability.public partial class Additional { + /// <summary> + /// Array of skill IDs to be triggered by this additional effect + /// </summary> [M2dArray] public int[] skillID = Array.Empty<int>(); + /// <summary> + /// Corresponding skill levels for each skill ID + /// </summary> [M2dArray] public short[] level = Array.Empty<short>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (80)
Maple2.File.Parser/AdditionalEffectParser.cs(2 hunks)Maple2.File.Parser/Enum/Locale.cs(1 hunks)Maple2.File.Parser/ItemOptionParser.cs(5 hunks)Maple2.File.Parser/ItemParser.cs(1 hunks)Maple2.File.Parser/MapParser.cs(2 hunks)Maple2.File.Parser/Maple2.File.Parser.csproj(1 hunks)Maple2.File.Parser/NpcParser.cs(3 hunks)Maple2.File.Parser/QuestParser.cs(3 hunks)Maple2.File.Parser/RidingParser.cs(2 hunks)Maple2.File.Parser/ScriptParser.cs(5 hunks)Maple2.File.Parser/SkillParser.cs(3 hunks)Maple2.File.Parser/TableParser.cs(18 hunks)Maple2.File.Parser/Xml/AdditionalEffect/AdditionalEffect.cs(1 hunks)Maple2.File.Parser/Xml/AdditionalEffect/BasicProperty.cs(2 hunks)Maple2.File.Parser/Xml/AdditionalEffect/DotProperty.cs(1 hunks)Maple2.File.Parser/Xml/AdditionalEffect/ModifySkillCooldown.cs(1 hunks)Maple2.File.Parser/Xml/AdditionalEffect/MotionProperty.cs(1 hunks)Maple2.File.Parser/Xml/AdditionalEffect/RemoveRechargingCount.cs(1 hunks)Maple2.File.Parser/Xml/AdditionalEffect/Spread.cs(1 hunks)Maple2.File.Parser/Xml/AdditionalEffect/StatusProperty.cs(2 hunks)Maple2.File.Parser/Xml/Common/ItemOptionNew.cs(1 hunks)Maple2.File.Parser/Xml/Item/Customize.cs(2 hunks)Maple2.File.Parser/Xml/Item/Cutting.cs(1 hunks)Maple2.File.Parser/Xml/Item/Install.cs(1 hunks)Maple2.File.Parser/Xml/Item/Item.cs(1 hunks)Maple2.File.Parser/Xml/Item/Property.cs(2 hunks)Maple2.File.Parser/Xml/Item/Slot.cs(4 hunks)Maple2.File.Parser/Xml/ItemOption.cs(4 hunks)Maple2.File.Parser/Xml/ItemOptionConstant.cs(2 hunks)Maple2.File.Parser/Xml/ItemOptionRandom.cs(1 hunks)Maple2.File.Parser/Xml/Map/Map.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Collisions.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Corpse.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Crystal.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Dummy.cs(2 hunks)Maple2.File.Parser/Xml/Npc/Model.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Npc.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Ride.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Skill.cs(1 hunks)Maple2.File.Parser/Xml/Quest/Condition.cs(1 hunks)Maple2.File.Parser/Xml/Quest/Quest.cs(2 hunks)Maple2.File.Parser/Xml/Quest/Require.cs(1 hunks)Maple2.File.Parser/Xml/Quest/Reward.cs(1 hunks)Maple2.File.Parser/Xml/Riding/Riding.cs(1 hunks)Maple2.File.Parser/Xml/Script/NpcScript.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Basic.cs(2 hunks)Maple2.File.Parser/Xml/Skill/ChangeSkill.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Combo.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Kinds.cs(1 hunks)Maple2.File.Parser/Xml/Skill/MagicControl.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Property/DamageProperty.cs(2 hunks)Maple2.File.Parser/Xml/Skill/RegionSkill.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Sensor.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Skill.cs(2 hunks)Maple2.File.Parser/Xml/Skill/StateAttribute.cs(1 hunks)Maple2.File.Parser/Xml/Skill/SubConditionTarget.cs(1 hunks)Maple2.File.Parser/Xml/Skill/TriggerSkill.cs(2 hunks)Maple2.File.Parser/Xml/Table/GachaInfo.cs(1 hunks)Maple2.File.Parser/Xml/Table/IndividualItemDrop.cs(1 hunks)Maple2.File.Parser/Xml/Table/ItemMergeOption.cs(2 hunks)Maple2.File.Parser/Xml/Table/ItemOptionVariation.cs(1 hunks)Maple2.File.Parser/Xml/Table/JobTable.cs(2 hunks)Maple2.File.Parser/Xml/Table/SetItemOption.cs(1 hunks)Maple2.File.Parser/Xml/Table/StatString.cs(1 hunks)Maple2.File.Tests/AdditionalEffectParserTest.cs(3 hunks)Maple2.File.Tests/FunctionCubeParserTest.cs(3 hunks)Maple2.File.Tests/ItemOptionParserTest.cs(7 hunks)Maple2.File.Tests/ItemParserTest.cs(4 hunks)Maple2.File.Tests/MapParserTest.cs(4 hunks)Maple2.File.Tests/NifParserTest.cs(0 hunks)Maple2.File.Tests/NpcParserTest.cs(5 hunks)Maple2.File.Tests/QuestParserTest.cs(5 hunks)Maple2.File.Tests/RidingParserTest.cs(4 hunks)Maple2.File.Tests/ScriptParserTest.cs(6 hunks)Maple2.File.Tests/SkillParserTest.cs(4 hunks)Maple2.File.Tests/TableParserKRTest.cs(0 hunks)Maple2.File.Tests/TableParserNewTest.cs(1 hunks)Maple2.File.Tests/TableParserTest.cs(2 hunks)Maple2.File.Tests/TestUtils.cs(0 hunks)Maple2.File.Tests/XBlockParserTest.cs(0 hunks)
💤 Files with no reviewable changes (4)
- Maple2.File.Tests/NifParserTest.cs
- Maple2.File.Tests/TestUtils.cs
- Maple2.File.Tests/XBlockParserTest.cs
- Maple2.File.Tests/TableParserKRTest.cs
🧰 Additional context used
🧬 Code Graph Analysis (17)
Maple2.File.Parser/Xml/Npc/Corpse.cs (1)
Maple2.File.IO/Nif/Endian.cs (1)
Vector3(81-87)
Maple2.File.Parser/Xml/Npc/Model.cs (1)
Maple2.File.IO/Nif/Endian.cs (1)
Vector3(81-87)
Maple2.File.Parser/Xml/Item/Install.cs (1)
Maple2.File.IO/Nif/Endian.cs (1)
Vector3(81-87)
Maple2.File.Parser/Xml/Item/Customize.cs (1)
Maple2.File.Parser/Xml/Npc/Dummy.cs (1)
Transform(23-27)
Maple2.File.Parser/Xml/Item/Property.cs (1)
Maple2.File.Parser/Xml/Npc/Exp.cs (1)
Exp(5-7)
Maple2.File.Parser/Xml/Skill/Sensor.cs (2)
Maple2.File.Parser/Xml/Table/JobTable.cs (2)
Skill(48-56)Skill(93-106)Maple2.File.IO/Nif/Endian.cs (1)
Vector3(81-87)
Maple2.File.Tests/ScriptParserTest.cs (5)
Maple2.File.Parser/Tools/Filter.cs (1)
Filter(9-47)Maple2.File.Tests/TestUtils.cs (2)
TestUtils(11-52)TestUtils(19-43)Maple2.File.Parser/ScriptParser.cs (2)
ScriptParser(12-86)ScriptParser(20-27)Maple2.File.Parser/Xml/Script/NpcScript.cs (1)
NpcScriptNew(20-22)Maple2.File.Parser/Xml/Script/QuestScript.cs (1)
QuestScript(12-16)
Maple2.File.Parser/Xml/Table/ItemOptionVariation.cs (4)
Maple2.File.Parser/Xml/ItemOptionRandom.cs (1)
XmlRoot(4-7)Maple2.File.Parser/Xml/ItemOption.cs (1)
XmlRoot(8-11)Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (3)
XmlRoot(8-11)XmlRoot(71-74)Option(36-66)Maple2.File.Parser/Xml/Table/ItemOptionPick.cs (1)
XmlRoot(7-10)
Maple2.File.Parser/AdditionalEffectParser.cs (3)
Maple2.File.IO/M2dReader.cs (3)
M2dReader(10-72)M2dReader(14-37)PackFileEntry(39-41)Maple2.File.Parser/PetParser.cs (1)
IEnumerable(25-42)Maple2.File.Parser/Xml/AdditionalEffect/AdditionalEffect.cs (2)
AdditionalEffectDataNew(58-92)AdditionalEffectLevelDataNew(49-56)
Maple2.File.Parser/RidingParser.cs (3)
Maple2.File.Parser/Xml/Riding/Riding.cs (1)
RidingNew(28-30)Maple2.File.IO/M2dReader.cs (1)
GetString(64-67)Maple2.File.Parser/Tools/Sanitizer.cs (2)
Sanitizer(5-105)RemoveEmpty(85-87)
Maple2.File.Parser/SkillParser.cs (2)
Maple2.File.IO/M2dReader.cs (3)
M2dReader(10-72)M2dReader(14-37)PackFileEntry(39-41)Maple2.File.Parser/Xml/Skill/Skill.cs (1)
SkillNew(187-193)
Maple2.File.Parser/NpcParser.cs (4)
Maple2.File.IO/M2dReader.cs (4)
M2dReader(10-72)M2dReader(14-37)PackFileEntry(39-41)GetString(64-67)Maple2.File.Parser/Xml/Npc/Npc.cs (2)
NpcDataNew(52-54)NpcDataRootNew(47-50)Maple2.File.Parser/Xml/Npc/Dummy.cs (1)
EffectDummy(7-9)Maple2.File.Parser/Tools/Sanitizer.cs (2)
Sanitizer(5-105)SanitizeNpc(45-59)
Maple2.File.Parser/Xml/Table/StatString.cs (3)
Maple2.File.Parser/Xml/ItemOptionRandom.cs (1)
XmlRoot(4-7)Maple2.File.Parser/Xml/ItemOptionConstant.cs (2)
XmlRoot(8-11)XmlRoot(30-33)Maple2.File.Parser/Xml/Table/ItemOptionVariation.cs (2)
XmlRoot(7-20)XmlRoot(23-70)
Maple2.File.Parser/Xml/Skill/Basic.cs (10)
Maple2.File.Parser/Xml/Skill/Kinds.cs (1)
KindsNew(38-70)Maple2.File.Parser/Xml/Skill/Combo.cs (1)
ComboNew(17-26)Maple2.File.Parser/Xml/Skill/ChangeSkill.cs (1)
ChangeSkillNew(17-27)Maple2.File.Parser/Xml/Skill/AutoTargeting.cs (1)
AutoTargeting(5-10)Maple2.File.Parser/Xml/Skill/Push.cs (1)
Push(5-8)Maple2.File.Parser/Xml/Skill/StateAttribute.cs (1)
StateAttributeNew(34-60)Maple2.File.Parser/Xml/Skill/Sensor.cs (1)
Sensor(7-18)Maple2.File.Parser/Xml/Skill/MagicControl.cs (1)
MagicControlNew(12-17)Maple2.File.Parser/Xml/Quest/Condition.cs (1)
Condition(5-7)Maple2.File.Parser/Xml/AdditionalEffect/AdditionalEffect.cs (1)
Condition(131-202)
Maple2.File.Parser/MapParser.cs (7)
Maple2.File.IO/M2dReader.cs (5)
M2dReader(10-72)M2dReader(14-37)PackFileEntry(39-41)XmlReader(43-45)GetString(64-67)Maple2.File.Parser/Xml/Map/Map.cs (2)
MapDataRootNew(37-40)MapData(12-28)Maple2.File.Parser/QuestParser.cs (3)
IEnumerable(29-44)IEnumerable(46-59)Dictionary(61-80)Maple2.File.Parser/NpcParser.cs (3)
IEnumerable(36-49)IEnumerable(51-65)Dictionary(29-34)Maple2.File.Parser/SkillParser.cs (3)
IEnumerable(28-40)IEnumerable(42-56)Dictionary(58-71)Maple2.File.Parser/ItemParser.cs (3)
IEnumerable(26-36)IEnumerable(38-49)Dictionary(51-57)Maple2.File.Parser/Tools/Sanitizer.cs (4)
Sanitizer(5-105)SanitizeMap(38-43)RemoveEmpty(85-87)SanitizeBool(79-83)
Maple2.File.Parser/ItemParser.cs (4)
Maple2.File.IO/M2dReader.cs (4)
M2dReader(10-72)M2dReader(14-37)PackFileEntry(39-41)XmlReader(43-45)Maple2.File.Parser/Xml/Item/Item.cs (3)
ItemDataNew(55-85)ItemData(12-42)ItemDataRootNew(50-53)Maple2.File.Parser/MapParser.cs (3)
IEnumerable(29-43)IEnumerable(45-59)Dictionary(61-67)Maple2.File.Parser/PetParser.cs (1)
IEnumerable(25-42)
Maple2.File.Parser/Xml/Quest/Quest.cs (11)
Maple2.File.Parser/Xml/Quest/Basic.cs (1)
Basic(7-28)Maple2.File.Parser/Xml/Quest/Notify.cs (1)
Notify(7-11)Maple2.File.Parser/Xml/Quest/Require.cs (1)
Require(7-37)Maple2.File.Parser/Xml/Quest/Condition.cs (1)
Condition(5-7)Maple2.File.Parser/Xml/Quest/Reward.cs (1)
Reward(8-39)Maple2.File.Parser/Xml/Quest/ProgressMap.cs (1)
ProgressMap(5-7)Maple2.File.Parser/Xml/Quest/Guide.cs (1)
Guide(7-13)Maple2.File.Parser/Xml/Quest/Dispatch.cs (1)
Dispatch(5-10)Maple2.File.Parser/Xml/Quest/Goto.cs (2)
GotoNpc(5-9)GotoDungeon(11-15)Maple2.File.Parser/Xml/Quest/Remote.cs (1)
Remote(7-11)Maple2.File.Parser/Xml/Quest/SummonPortal.cs (1)
SummonPortal(5-8)
🔇 Additional comments (146)
Maple2.File.Parser/Xml/Item/Install.cs (1)
29-29: Initialize Vector3 to default to avoid null referencesSetting
physXdimension = defaultensures theVector3struct is always initialized (to 0,0,0) and prevents potential null reference issues during XML deserialization. SinceVector3is a value type,defaultis effective here.Maple2.File.Parser/Xml/AdditionalEffect/DotProperty.cs (1)
8-8: Validate XmlSerializer compatibility for long typeChanging
valuefrominttolongsupports larger damage values. Please verify that existing XML data and theXmlSerializercorrectly handlelongattributes and consider adding tests for values exceedingint.MaxValue.Maple2.File.Parser/Enum/Locale.cs (1)
3-10: Strongly typed locale enum introduction looks goodIntroducing the
Localeenum for supported locales (KR, NA, TW, TH, JP, CN) centralizes locale handling and replaces string literals, improving type safety and maintainability.Maple2.File.Parser/Maple2.File.Parser.csproj (1)
16-16: Ensure version consistency across projectsThe package version has been bumped to
2.3.0. Please verify that all related projects and deployment pipelines have been updated to reflect this version change, including any test or CI configuration files.Maple2.File.Parser/Xml/Common/ItemOptionNew.cs (1)
6-6:Details
✅ Verification successful
LGTM: Class renaming aligns with new XML format support.
The rename from
ItemOptionKRtoItemOptionNewis consistent with the PR objective to differentiate between old and new XML formats. The implementation remains functionally identical.Verify that all references to the old
ItemOptionKRclass have been updated throughout the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old ItemOptionKR class name # Expected: No occurrences of ItemOptionKR should remain echo "Searching for remaining ItemOptionKR references..." rg -i "ItemOptionKR" --type cs echo "Verifying ItemOptionNew is properly referenced..." rg -i "ItemOptionNew" --type cs -A 2 -B 2Length of output: 6080
All references to the old class have been removed; renaming verified.
Search results show zero occurrences of
ItemOptionKRand confirm thatItemOptionNewis consistently used in:
- Maple2.File.Tests/TableParserNewTest.cs
- Maple2.File.Parser/TableParser.cs
- Maple2.File.Parser/ItemOptionParser.cs
- Maple2.File.Parser/Xml/Common/ItemOptionNew.cs
- Maple2.File.Parser/Xml/Table/SetItemOption.cs
No further action required—ready to merge.
Maple2.File.Parser/Xml/Npc/Model.cs (1)
13-13: LGTM: Explicit default initialization improves safety.Initializing the
Vector3 offsetfield with= defaultis a good practice that ensures the field has a predictable default value (0,0,0) and prevents potential null reference issues during XML deserialization.Maple2.File.Parser/Xml/Npc/Corpse.cs (1)
14-14: LGTM: Consistent with initialization pattern.The explicit initialization of
Vector3 rotation = defaultfollows the same defensive programming pattern seen in other files and ensures predictable default values during XML deserialization.Maple2.File.Parser/Xml/Quest/Condition.cs (1)
6-6: LGTM: Modern collection initialization prevents null references.The initialization of
List<Navigation> navi = []uses modern C# collection expression syntax and ensures the field is never null. This prevents null reference exceptions and follows best practices for collection fields in XML-serializable classes.Maple2.File.Parser/Xml/Npc/Crystal.cs (1)
7-7: LGTM: Consistent collection initialization pattern.The initialization of
List<Crystal> crystal = []follows the same pattern as other collection fields in this refactoring, using modern C# syntax while ensuring the field is never null during XML deserialization.Maple2.File.Parser/Xml/Quest/Require.cs (1)
35-35: LGTM: Good defensive programming practice.Initializing the
dayOfWeekarray field withArray.Empty<Enum.DayOfWeek>()prevents null reference exceptions and follows the consistent pattern used for other array fields in this class.Maple2.File.Parser/Xml/Table/IndividualItemDrop.cs (1)
69-69: LGTM: Appropriate renaming for new XML format support.The class rename from
IndividualItemDropRootKRtoIndividualItemDropRootNewaligns with the PR objective to support new XML format without locale dependency. This naming convention is more appropriate and consistent with the broader refactoring effort.Maple2.File.Tests/TableParserTest.cs (2)
2-2: LGTM: Proper enum import for locale handling.Adding the
Maple2.File.Parser.Enumnamespace import enables the use of theLocaleenum instead of hardcoded strings, improving code maintainability.
15-16: LGTM: Improved locale and language handling.The changes enhance the parser initialization by:
- Using
Locale.NA.ToString()instead of hardcoded strings (more maintainable)- Adding explicit
"en"language parameter to support locale-aware parsingThese changes align with the PR objective to make parsing independent of locale settings.
Maple2.File.Parser/Xml/Skill/MagicControl.cs (1)
12-17: LGTM: Appropriate new class for updated XML format.The
MagicControlNewclass correctly follows the established pattern for supporting the new XML format. The key change frombool enabletoint enablesuggests the new XML format uses integer values, which is handled appropriately while maintaining the same structure for other fields.Maple2.File.Parser/Xml/Npc/Ride.cs (1)
11-12: LGTM! Explicit default initialization improves reliability.The explicit initialization of
rideTranslationandrideRotationwithdefaultvalues is a good practice that ensures these Vector3 fields are properly initialized during deserialization. This aligns with the broader pattern of explicit default initialization across the codebase and prevents potential null reference issues.Maple2.File.Parser/Xml/Table/GachaInfo.cs (2)
2-2: Required import for new locale-aware attributes.The
M2dXmlGeneratorimport is necessary for the[M2dFeatureLocale]attribute used below.
12-12: LGTM! Implementing IFeatureLocale enables locale-specific functionality.The implementation of
IFeatureLocaleinterface aligns with the broader refactoring to support locale-aware parsing, allowing the system to select appropriate data based on locale settings.Maple2.File.Parser/Xml/AdditionalEffect/RemoveRechargingCount.cs (1)
1-9: LGTM! Well-structured new data class for additional effects.The
RemoveRechargingCountclass follows consistent patterns with other XML data classes in the codebase:
- Proper namespace and imports
- Appropriate use of
[XmlAttribute]for serialization- Simple, focused responsibility
- Consistent naming conventions
This fits well into the expanded additional effects system being introduced.
Maple2.File.Parser/Xml/AdditionalEffect/ModifySkillCooldown.cs (1)
1-9: LGTM! Excellent use of modern C# practices.The
ModifySkillCooldownclass demonstrates good practices:
- Efficient initialization: Using
Array.Empty<T>()instead ofnew T[0]is more memory-efficient- Explicit initialization: Prevents potential null reference issues during deserialization
- Appropriate attributes:
[M2dArray]is correctly used for array XML serialization- Logical field types:
int[]for skill IDs andfloat[]for cooldown values makes sense semanticallyThis follows the consistent pattern established in the expanded additional effects system.
Maple2.File.Parser/Xml/AdditionalEffect/Spread.cs (1)
6-10: Well-structured XML serialization class.The
Spreadclass is properly designed with appropriate XML attributes and elements. The field types and naming follow consistent patterns for XML serialization.Maple2.File.Parser/Xml/ItemOption.cs (2)
17-347: Excellent modernization using C# 12 collection expressions.The transition from
Array.Empty<T>()to[]syntax provides several benefits:
- Each field gets a new empty array instance rather than sharing static references
- More concise and readable syntax
- Consistent with modern C# practices
- Safer for scenarios where arrays might be modified
This change aligns well with the broader effort to ensure proper field initialization across XML data classes.
531-706: Consistent modernization pattern applied to SpecialRate arrays.The same beneficial modernization pattern is properly applied to all SpecialRate float arrays, maintaining consistency across the codebase.
Maple2.File.Parser/Xml/Item/Customize.cs (2)
16-18: Good defensive initialization to prevent null references.Initializing complex type fields with
new()ensures these properties are never null by default, improving robustness during XML deserialization and reducing potential NullReferenceExceptions.
38-38: Consistent collection initialization pattern.Using
[]syntax for thetransformlist initialization aligns with the modernization effort seen across other XML data classes and ensures the collection is never null.Maple2.File.Parser/Xml/Skill/Combo.cs (1)
20-22: Verify that commented fields are intentionally omitted.The commented-out fields (
comboOriginSkill,npcComboSkillID,npcComboSkillLevel) suggest these are intentionally excluded from the new XML format. Please confirm this is the intended behavior and consider removing the commented code if it won't be needed in the future.Maple2.File.Parser/Xml/Script/NpcScript.cs (1)
16-22: LGTM! Consistent naming convention update.The renaming from
NpcScriptListKr/NpcScriptKRtoNpcScriptListNew/NpcScriptNewis consistent with the broader refactoring effort to support the new XML format. The class structure and inheritance hierarchy are properly maintained.Maple2.File.Parser/Xml/Npc/Skill.cs (1)
7-10:Details
✅ Verification successful
Verify C# version compatibility for empty array literals.
The use of empty array literals (
[]) is a good improvement for ensuring non-null default values. However, this syntax requires C# 12 or later.Please verify that the project targets C# 12 or later:
🏁 Script executed:
#!/bin/bash # Check the target framework and C# language version in project files fd -e csproj -e props -e targets . --exec cat {} \; | grep -E "(TargetFramework|LangVersion|LanguageVersion)" || echo "No explicit C# version found in project files" # Check for global.json or Directory.Build.props that might specify C# version cat global.json 2>/dev/null | jq '.sdk' || echo "No global.json found" fd -name "Directory.Build.props" --exec cat {} \; 2>/dev/null || echo "No Directory.Build.props found"Length of output: 930
Confirmed C# 12+ support for empty array literals
Project files explicitly target net8.0 with
LangVersionset to12(and in one caselatestmajor), so the[]syntax is fully supported. No further action needed.Maple2.File.Parser/Xml/Map/Map.cs (1)
32-40: Consistent renaming and proper initialization.The renaming from
MapDataRootKRtoMapDataRootNewaligns with the broader refactoring effort. The initialization of both thefieldDatalist and_environmentfield follows the established pattern for ensuring non-null defaults.Maple2.File.Parser/Xml/ItemOptionConstant.cs (2)
31-31: LGTM: Consistent renaming for new XML format support.The class rename from
ItemOptionConstantRootKRtoItemOptionConstantRootNewaligns with the PR objective to differentiate XML formats independent of locale.
45-45: LGTM: Property type updated to match new class naming.The property type change from
ItemOptionConstantDataKRtoItemOptionConstantDataNewis consistent with the class renaming pattern.Maple2.File.Parser/Xml/Item/Property.cs (3)
8-8: Good addition: New rank attribute for enhanced item properties.The
rankattribute with a default value of 1 is properly defined for XML serialization and provides a sensible default.
43-43: Excellent: Defensive programming to prevent null references.Initializing
globalRePackingScrollIDto an empty array prevents potential null reference exceptions when this field is accessed.
49-50: Excellent: Null safety improvements for nested objects.Initializing
sellandexpfields with new instances prevents null reference exceptions and ensures these objects are always available for use, even if not explicitly set during XML deserialization.Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (4)
72-73: LGTM: Consistent renaming and improved initialization.The rename from
ItemMergeOptionRootKRtoItemMergeOptionRootNewfollows the established pattern. The list initialization to[]improves null safety.
76-76: LGTM: Class renamed to support new XML format.The
MergeOptionNewclass maintains the same structure as the original while supporting the new XML format naming convention.
86-86: Good: Improved initialization and type consistency.The option list initialization and type change to
OptionNewmaintains consistency with the new naming pattern.
89-103:Details
❓ Verification inconclusive
Note the structural differences in OptionNew class.
The
OptionNewclass has a different structure compared to the originalOptionclass:
- Uses individual
idx*_maxattributes instead of arrays- Has a
minattribute instead of weight-based options- Removed the indexer property and static option flag
This suggests the new XML format has a simpler range-based structure. Ensure this aligns with the actual new XML schema.
Let me verify that this structure matches the expected new XML format:
🏁 Script executed:
#!/bin/bash # Description: Check for sample XML files that use the new format to validate the structure # Expected: Find XML files that demonstrate the new option structure echo "Searching for new XML format examples..." fd -e xml . | head -10 | xargs grep -l "idx.*_max\|mergeOption" 2>/dev/null || echo "No XML samples found in repository" echo "Searching for parser usage of new classes..." rg -A 5 "ItemMergeOptionRootNew|OptionNew"Length of output: 10236
Verify New OptionNew Structure Against XML Schema
The
OptionNewclass now:
- Defines separate
idx0_max–idx9_maxfloat attributes (instead of a float array)- Includes a single
minint attribute (replacing the previous weight-based fields)- Omits the indexer property and static‐option flag
Because there are no sample XML files in the repo, please confirm that your external
table/itemmergeoptionbase.xml<option>elements use these exact attribute names and counts (optionName, min, idx0_max … idx9_max). Consider adding a minimal XML sample to your unit tests to guard against future schema drift.Maple2.File.Tests/FunctionCubeParserTest.cs (3)
2-2: Good addition of enum import for type safety.Adding the
Localeenum import aligns with the PR's goal of improving locale handling consistency across the codebase.
13-14: Excellent refactoring to use Locale enum.Using
Locale.NAinstead of magic strings improves type safety and maintainability. Thelocale.ToString()call ensures consistent string representation.
27-34: Well-structured conditional assertions for different locales.The switch statement properly handles locale-specific expected counts (427 for NA, 433 for KR). This consolidation reduces code duplication while maintaining test accuracy for different locales.
Maple2.File.Parser/Xml/Skill/Property/DamageProperty.cs (2)
2-2: Good import addition for M2dArray support.The
M2dXmlGeneratornamespace import is necessary for the[M2dArray]attribute used in the new class.
44-82:Details
❓ Verification inconclusive
Verify the type changes from bool to int are intentional.
The new
DamagePropertyNewclass introduces important changes:
damageEventDisableandstateHitIgnorechanged frombooltoint(lines 76-77)- Added new
stateHitOnlyarray with[M2dArray]attribute (line 78)- Commented out
handattribute (line 46)These type changes suggest the new XML format supports more granular values than simple boolean flags.
Please verify that these type changes align with the actual new XML schema requirements:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of DamagePropertyNew to understand the context # and verify the type changes are used correctly # Search for DamagePropertyNew usage rg -A 5 -B 5 "DamagePropertyNew" # Search for references to the changed fields to understand expected values rg -A 3 -B 3 "damageEventDisable|stateHitIgnore|stateHitOnly"Length of output: 3707
Verify XML schema alignment for DamagePropertyNew changes
I’ve confirmed in
Maple2.File.Parser/Xml/Skill/Property/DamageProperty.csthat:
damageEventDisableandstateHitIgnorewere changed frombooltoint(lines 76–77)- A new
stateHitOnlyarray was added with[M2dArray](line 78)- The
handattribute was commented out (line 46)Please ensure the updated XML schema (XSD or sample data) now:
- Defines
damageEventDisableandstateHitIgnoreas integer attributes (to support multiple values)- Declares
stateHitOnlyas an array of integers with the same tag/name- Omits the
handattribute entirely in the new formatKey locations:
- Maple2.File.Parser/Xml/Skill/Property/DamageProperty.cs (DamagePropertyNew class, lines 44–82)
- Maple2.File.Parser/Xml/Skill/Skill.cs (usage of
public DamagePropertyNew damage;)Maple2.File.Parser/Xml/Quest/Reward.cs (1)
28-31: Excellent defensive programming improvement.Initializing the
List<Item>fields with empty arrays using the[]syntax prevents null reference exceptions and follows modern C# best practices. This change ensures these collections are always safe to enumerate or manipulate without null checks.Maple2.File.Tests/MapParserTest.cs (4)
2-2: Good addition of enum import for consistency.Adding the
Localeenum import maintains consistency with the refactoring pattern applied across other test files.
13-15: Excellent explicit locale and language parameterization.Using
Locale.NAand explicit"en"language parameter provides clear control over test configuration and improves maintainability compared to implicit locale handling.
33-36: Good separation of old and new XML format testing.Renaming to
TestMapParserNewand usingParseNew()method clearly distinguishes between testing the original and new XML formats. The use ofLocale.KRwith"kr"language parameter aligns with testing the new format.Also applies to: 44-44
55-68: Well-structured consolidation of locale-specific tests.The conditional assertions based on language parameter (1152 for "en", 1282 for "kr") effectively consolidate previously separate tests while maintaining accuracy for different locales.
Maple2.File.Parser/Xml/Skill/Sensor.cs (1)
1-18: Well-structured sensor data class implementation.The
Sensorclass is properly designed with appropriate XML serialization attributes and field types. The use ofVector3with the custom[M2dVector3]attribute aligns with the codebase patterns, and the partial class declaration allows for future extensibility.Maple2.File.Tests/AdditionalEffectParserTest.cs (2)
13-14: Good migration to Locale enum.The change from hardcoded locale strings to the
Localeenum improves type safety and maintainability.
33-38: Verify the expected count change rationale.The expected count changed from 6070 to 5881. Please ensure this change reflects the actual difference between old and new XML format data rather than a regression.
Maple2.File.Parser/Xml/Item/Item.cs (1)
13-42: Excellent field initialization pattern.Initializing all complex type fields with
new()at declaration is a great practice that prevents null reference exceptions and ensures consistent object state.Maple2.File.Parser/Xml/AdditionalEffect/MotionProperty.cs (1)
39-61: Well-designed overlapping effect structure.The new nested classes for
OverlapKeepEffectPropertyprovide a clear and well-structured representation of overlapping effects with distinct start, keep, and end phases. The XML serialization attributes are properly applied, and the helpful comment clarifies this is specific to the new XML format.Maple2.File.Tests/RidingParserTest.cs (4)
2-2: Good addition of Locale enum import.Adding the
Localeenum import standardizes locale handling across the codebase and eliminates hardcoded string usage.
13-14: Excellent refactoring to use Locale enum.Replacing hardcoded locale strings with
Locale.NA.ToString()improves maintainability and reduces the chance of typos in locale names.Also applies to: 29-30
44-47: Consistent naming convention update.The method rename from
TestRidingParserKrtoTestRidingParserNewand the switch fromParseKr()toParseNew()aligns well with the PR's goal of supporting a new XML format independent of locale settings. The change fromRidingKRtoRidingNewreturn type is also consistent.Also applies to: 50-50
54-58: Good defensive programming with passenger null checks.Adding explicit null checks for passengers and their individual elements improves test robustness and helps catch potential data integrity issues.
Maple2.File.Parser/Xml/Table/SetItemOption.cs (2)
33-35: Consistent class renaming aligns with new XML format support.The renaming from
SetItemOptionRootKRtoSetItemOptionRootNewis consistent with the PR's goal of supporting a new XML format independent of locale.
37-37:Details
✅ Verification successful
Verify ItemOptionNew class exists.
The
Partclass now inherits fromItemOptionNewinstead ofItemOptionKR. Please ensure that theItemOptionNewclass is properly defined in the codebase.Also applies to: 44-44
🏁 Script executed:
#!/bin/bash # Search for ItemOptionNew class definition ast-grep --pattern 'class ItemOptionNew $_'Length of output: 69684
ItemOptionNew class confirmed
The
ItemOptionNewclass is defined at:
- Maple2.File.Parser/Xml/Common/ItemOptionNew.cs:6
No further changes needed.
Maple2.File.Parser/Xml/Skill/ChangeSkill.cs (1)
17-27: New ChangeSkillNew class follows good practices.The new class structure looks good with proper use of
Array.Empty<T>()for initialization and addition of thechangeSkillCheckEffectCompareFuncproperty for enhanced functionality.Maple2.File.Tests/ItemParserTest.cs (4)
1-3: Excellent standardization of locale and language handling.The addition of
Localeenum usage and explicit language parameters ("en","kr") to theItemParserconstructor improves maintainability and makes language selection explicit rather than implicit.Also applies to: 14-16, 36-38, 57-59
31-31: Different item counts may indicate format differences.The NA locale test expects 35,309 items while the new KR format expects 35,970 items. This difference likely reflects the new XML format containing additional items, which aligns with the PR's goal of supporting an updated format.
Also applies to: 52-52
35-35: Consistent method naming with overall refactoring.The rename from
TestItemParserKrtoTestItemParserNewmaintains consistency with the shift from locale-specific to format-specific naming conventions.
24-24:Details
✅ Verification successful
Verify Parse() method signature change.
The change from
Parse<ItemDataRoot>()toParse()in the first test method while the second test usesParseNew()suggests different parsing approaches. Please ensure this is intentional and both methods return compatible data.Also applies to: 46-46
🏁 Script executed:
#!/bin/bash # Search for ItemParser Parse methods to verify signatures ast-grep --pattern 'class ItemParser { $$$ Parse($_) { $$$ } $$$ }'Length of output: 81
🏁 Script executed:
#!/bin/bash # Locate the ItemParser implementation file item_parser_file=$(rg -l "class ItemParser") echo "ItemParser implementation at: $item_parser_file" # List all public Parse and ParseNew method signatures echo -e "\nPublic Parse methods:" rg -n "public .*Parse" -n "$item_parser_file" echo -e "\nGeneric Parse overloads:" rg -n "Parse<" -n "$item_parser_file" echo -e "\nParseNew methods:" rg -n "ParseNew" -n "$item_parser_file"Length of output: 1107
🏁 Script executed:
#!/bin/bash # Verify Parse and ParseNew signatures in the ItemParser implementation parser="Maple2.File.Parser/ItemParser.cs" if [ ! -f "$parser" ]; then echo "Error: $parser not found"; exit 1 fi echo "=== Generic Parse<T> overloads ===" grep -E -n "public .*Parse<" "$parser" echo -e "\n=== Non-generic Parse() methods ===" grep -E -n "public .*Parse\(" "$parser" | grep -v "Parse<" echo -e "\n=== ParseNew methods ===" grep -E -n "ParseNew" "$parser"Length of output: 676
Both Parse() and ParseNew() signatures are consistent
Confirmed that in Maple2.File.Parser/ItemParser.cs both
public IEnumerable<(int Id, string Name, ItemData Data)> Parse()public IEnumerable<(int Id, string Name, ItemData Data)> ParseNew()
exist and return the same tuple sequence. No genericParse<T>()overload is present, so the tests’ use ofParse()andParseNew()is intentional and compatible.Maple2.File.Parser/AdditionalEffectParser.cs (2)
12-12: LGTM!The new serializer field is correctly declared to handle the new XML format.
18-18: LGTM!The serializer initialization is correct and properly targets the new data type.
Maple2.File.Parser/QuestParser.cs (4)
19-19: LGTM!The language parameter addition enables proper internationalization support and follows the consistent pattern across other parsers.
Also applies to: 21-23
18-18: LGTM!The new serializer setup correctly targets the new data types and simplifies the previous conditional logic.
Also applies to: 25-26
46-59: LGTM!The
ParseNew()method correctly implements parsing for the new XML format by deserializing from a single "questdata.xml" file and yieldingQuestDataNewobjects.
63-63: LGTM!The dynamic language-based file filtering properly replaces the hardcoded "en" locale and enables proper internationalization.
Maple2.File.Parser/Xml/Table/JobTable.cs (3)
72-74: LGTM!The new root class properly targets the updated
JobTableNewtype and maintains the correct XML structure.
76-88: LGTM!The new job table class correctly updates the structure with simplified attributes and proper XML element mapping for the skills property.
90-107: LGTM!The new skills class structure is well-designed and the addition of the
isLearnboolean attribute provides valuable information for skill management.Maple2.File.Parser/NpcParser.cs (3)
19-19: LGTM!The language parameter addition enables proper internationalization support and follows the consistent pattern across other parsers.
Also applies to: 21-23
18-18: LGTM!The new serializer correctly targets the
NpcDataListNewtype for the new XML format.Also applies to: 26-26
30-30: LGTM!The dynamic language-based NPC name file selection properly enables internationalization.
Maple2.File.Parser/SkillParser.cs (5)
7-7: LGTM: Added enum import for locale handling.Good addition to support the new
Localeenum usage across the codebase.
17-18: LGTM: Added new serializer and language field for enhanced XML format support.The addition of
skillNewSerializersupports the new XML format while maintaining backward compatibility, and thelanguagefield enables proper internationalization.
20-26: LGTM: Constructor properly parameterized for language support.The constructor changes correctly:
- Accept a language parameter for internationalization
- Initialize both old and new serializers for format compatibility
- Store the language for use in localized file loading
This aligns well with the PR objective of making parsing independent of locale settings.
42-56: LGTM: Method rename and implementation align with new XML format support.The changes from
ParseKrtoParseNewand the switch from "skill/" to "skilldata/" file paths correctly implement support for the new XML format. The return type change toSkillNewmaintains type safety while supporting the enhanced data structure.
60-60: LGTM: Language parameterization enables proper localization.Using the language parameter in the file path construction allows for dynamic loading of localized skill names, which is essential for multi-language support.
Maple2.File.Parser/Xml/Table/StatString.cs (1)
1-22: LGTM: Well-structured XML model classes for new format.The implementation follows established patterns:
- Proper XML serialization attributes (
[XmlRoot],[XmlElement],[XmlAttribute])- Clear namespace organization
- Appropriate data types for the properties
- Good documentation indicating the specific XML files these classes target
The structure aligns with other XML model classes in the codebase and supports the new XML format as intended.
Maple2.File.Parser/Xml/AdditionalEffect/StatusProperty.cs (3)
44-77: LGTM: New class maintains compatibility while supporting new XML format.The
StatusPropertyNewclass appropriately duplicates the original structure while ensuring thebooltype fordeathResistanceHPis used consistently. This pattern maintains backward compatibility while supporting the new XML schema.
79-86: LGTM: New special ability classes follow established patterns.The
SpecialAbilityNewandSpecialAbilityEntryclasses are well-structured with proper XML serialization attributes and follow the established naming and design patterns for the new XML format support.
28-28:Details
❓ Verification inconclusive
Verify the type change from
longtoboolfordeathResistanceHP.The type change from
longtoboolrepresents a significant schema modification. Please ensure this aligns with the actual XML data structure in the new format and won't cause deserialization issues.
🏁 Script executed:
#!/bin/bash # Description: Search for deathResistanceHP usage in XML files to verify the expected data type # Search for deathResistanceHP attribute usage in the codebase rg -A 3 -B 3 "deathResistanceHP" --type xmlLength of output: 192
🏁 Script executed:
#!/bin/bash # Refined search for deathResistanceHP in XML files (ignoring ripgrep’s --type filter) rg -g '*.xml' -n "deathResistanceHP"Length of output: 185
Confirm schema change for deathResistanceHP
I wasn’t able to find any XML schema or sample files referencing
deathResistanceHPin the repo, so it’s unclear whether it’s actually represented as a boolean in your new XML format. Please double-check that:
- The XML schema (XSD/DTD) or sample data has changed this attribute from a numeric value to a true/false flag.
- Any existing XML deserialization logic or tests are updated to reflect the new boolean type.
Without a matching XML reference to verify against, I recommend manually confirming that this type change is intentional and safe.
Maple2.File.Tests/ScriptParserTest.cs (3)
2-2: LGTM: Added enum import for locale handling.Good addition to support the new
Localeenum usage in tests.
13-16: LGTM: Proper locale and language parameterization.The use of
Locale.NAenum and explicit language string"en"correctly implements the new parameterized approach for parser testing.
80-80: LGTM: Updated parser method calls for new format.The calls to
ParseNpcNew()andParseQuestNew()correctly use the new parsing methods that support the enhanced XML format, and the expected return types (NpcScriptNew) align with the updated data structures.Also applies to: 95-95
Maple2.File.Parser/Xml/ItemOptionRandom.cs (3)
5-6: LGTM! Clean systematic renaming for new XML format support.The class renaming from
ItemOptionRandomRootKRtoItemOptionRandomRootNewand the field name change fromrankstooptionsimproves semantic clarity while supporting the new XML schema.
9-9: Consistent XML schema migration with improved naming.The changes from
ItemOptionRandomKRtoItemOptionRandomNewand the XML element update from"rank"to"v"are well-aligned with the new format requirements. The field renaming fromrankstooptionsprovides better semantic meaning.Also applies to: 17-17
20-23: Data class successfully migrated to new format.The
ItemOptionRandomDataNewclass maintains all the original field structure while supporting the new XML schema. The extensive name attribute comment provides valuable documentation of supported stat types.Maple2.File.Parser/Xml/Npc/Dummy.cs (3)
8-8: Excellent defensive programming - prevents null reference exceptions.Initializing the
dummylist to an empty collection prevents potential null reference exceptions during XML deserialization and subsequent usage.
14-15: Good practice: Initialize complex objects to prevent null references.Initializing
attachInfoandtransformwithnew()ensures these objects are always available, preventing null reference exceptions when accessing their properties.
24-25: Clear and explicit Vector3 initialization.Explicitly initializing
translateandrotationtodefaultmakes the intent clear and ensures consistent behavior across all instances.Maple2.File.Parser/RidingParser.cs (2)
15-15: New serializer properly added for new XML format support.The addition of
ridingNewSerializerwithRidingNewRoottype correctly supports the new XML format while maintaining the existing serializer for backward compatibility.Also applies to: 21-21
54-66: New parsing method successfully implements new format support.The
ParseNew()method correctly implements parsing for the new XML format. The logic is consistent with the existingParse()method but uses the appropriate new types.Note on code duplication: While this creates some duplication with the existing
Parse()method, this appears to be an intentional architectural choice to maintain clean separation between old and new format support during the migration period.Maple2.File.Parser/MapParser.cs (6)
7-7: LGTM: Necessary import for enum usage.The addition of the
Enumnamespace import supports the use of enums in the codebase, which is consistent with the broader refactoring mentioned in the PR objectives.
18-19: LGTM: Well-structured serializer and language field additions.The addition of
MapNewSerializerfor the new XML format and thelanguagefield for localization support follows good separation of concerns and enables the dual format support described in the PR objectives.
21-27: LGTM: Constructor properly parameterized for language support.The constructor changes correctly:
- Accept the language parameter for localization
- Initialize both old and new serializers for dual format support
- Store the language for use in parsing methods
This aligns with the PR goal of making XML format detection independent of locale settings.
32-32: LGTM: Consistent filtering pattern.The explicit filtering for "map/" prefix maintains consistency with other parsers in the codebase as shown in the relevant code snippets from ItemParser, NpcParser, etc.
45-59: LGTM: Well-implemented new format parsing with proper sanitization.The
ParseNew()method correctly:
- Reads from the consolidated "table/fielddata.xml" file
- Applies appropriate sanitization (
RemoveEmptyandSanitizeBool)- Uses the new serializer for
MapDataRootNew- Iterates through the
fieldDatacollection properly- Maintains the same return signature for consistency
The sanitization steps are particularly important for cleaning potentially malformed XML data before deserialization.
62-62: LGTM: Dynamic localized file path construction.The use of string interpolation to construct the localized file path (
$"{language}/mapname.xml") properly leverages the language parameter to support multiple locales as intended by the PR.Maple2.File.Parser/Xml/Riding/Riding.cs (3)
14-20: LGTM: Defensive initialization prevents null reference exceptions.Initializing all fields with
new()is excellent defensive programming that eliminates the risk of null reference exceptions when accessing these properties. This pattern ensures objects are always in a valid state upon instantiation.
24-26: LGTM: Consistent naming convention for new XML format.The renaming from
RidingKRRoottoRidingNewRootand updating the field type toRidingNewaligns with the broader refactoring pattern across the codebase to move away from locale-specific naming ("KR") to format-specific naming ("New").
28-30: LGTM: Proper inheritance and field initialization.The
RidingNewclass correctly:
- Inherits from
Ridingto extend functionality- Initializes the
passengerslist to an empty collection using modern C# syntax[]- Maintains proper XML serialization attributes
This follows good object-oriented design principles.
Maple2.File.Parser/Xml/Skill/Basic.cs (2)
3-3: LGTM: Necessary namespace import for new functionality.The addition of the
AdditionalEffectnamespace import is required to support the newConditionelement used in theBasicNewclass, ensuring all dependencies are properly resolved.
13-27: LGTM: Comprehensive expansion of skill data model with consistent naming.The
BasicNewclass expansion is well-designed:New attributes:
mainType,paramStr,param1provide additional skill configuration options- Proper default initialization (
paramStr = string.Empty)Updated elements:
- Correctly uses "New" versions of related classes (
KindsNew,StateAttributeNew)- Maintains XML serialization attributes consistently
New elements:
- Rich set of new skill components (
ComboNew,ChangeSkillNew,AutoTargeting,Push,Sensor,MagicControlNew,Condition)- All referenced classes are properly defined based on the relevant code snippets
This expansion significantly enhances the skill data model while maintaining consistency with the broader "New" format refactoring pattern.
Maple2.File.Parser/Xml/Npc/Npc.cs (4)
14-40: LGTM: Excellent defensive programming with comprehensive field initialization.Initializing all public fields with
new()is outstanding defensive programming that:
- Eliminates null reference exceptions
- Ensures objects are always in a valid, usable state
- Provides consistent behavior across all NPC data instances
- Follows modern C# best practices
The comprehensive initialization covers all data structures from basic properties to complex nested objects, ensuring robust data handling.
43-45: LGTM: Consistent naming and proper collection initialization.The renaming from
NpcDataListKRtoNpcDataListNewaligns with the broader refactoring pattern, and initializing thenpcslist to an empty collection using modern syntax[]ensures the list is always ready for use without null checks.
47-50: LGTM: Proper data container structure with defensive initialization.The
NpcDataRootNewclass correctly:
- Maintains the XML attribute for
id- Initializes the
environmentfield to prevent null references- Provides a clean container structure for individual NPC data entries
This design supports the new XML format's structure effectively.
52-54: LGTM: Clean inheritance with proper collection initialization.The
NpcDataNewclass properly:
- Extends the base
NpcDataclass to inherit all initialized fields- Adds its own
effectdummylist initialized to an empty collection- Maintains the XML serialization attributes for proper deserialization
This design promotes code reuse while supporting the enhanced data structure requirements of the new format.
Maple2.File.Parser/Xml/Npc/Collisions.cs (1)
8-8: Good defensive programming practices!Initializing collections to empty lists and vectors to default values prevents null reference exceptions and ensures predictable behavior.
Also applies to: 13-14, 19-19, 23-24
Maple2.File.Parser/Xml/Skill/TriggerSkill.cs (1)
16-16: Clarify the skillOwner field behavior.The comment "if 0 then 1?" indicates uncertainty about the field's behavior. This ambiguity should be resolved and properly documented.
Please verify the intended behavior when
skillOwneris 0 and update the comment to be more descriptive.Also applies to: 58-58
Maple2.File.Parser/ItemParser.cs (1)
26-49: Well-structured implementation for dual XML format support!The separation between
Parse()andParseNew()methods clearly distinguishes between the old and new XML formats, aligning perfectly with the PR objectives to support the new XML format without relying on locale settings.Maple2.File.Tests/QuestParserTest.cs (6)
2-2: Good addition of enum namespace.Adding the
Maple2.File.Parser.Enumnamespace improves type safety by enabling the use of theLocaleenum.
13-15: Improved type safety with Locale enum.The migration from hardcoded locale strings to the
Localeenum enhances type safety and reduces the likelihood of typos. The explicit language parameter ("en") makes the test intent clearer.
44-44: Verify ParseNew() method signature and return type.The method call has changed from
Parse()toParseNew()and the expected return type is nowQuestDataNew. Ensure this matches the actual parser implementation.
67-74: Well-structured language-specific test logic.The switch statement approach for handling different language expectations is clean and maintainable. It consolidates language-specific logic in a single test method while maintaining clear expectations for each locale.
50-50:Details
❓ Verification inconclusive
Verify the expected count for the new format.
The expected count has changed from a previous value to 6961 for the new KR format. Please verify this count is accurate for the new data format to prevent test failures.
🏁 Script executed:
#!/bin/bash # Search for any documentation or comments about the expected quest count rg -A 3 -B 3 "6961|quest.*count" --type csLength of output: 1317
🏁 Script executed:
#!/bin/bash # Display the contents of the QuestParserTest to understand how the count is derived sed -n '1,200p' Maple2.File.Tests/QuestParserTest.csLength of output: 2846
#!/bin/bash set -e # 1. Locate TestUtils and inspect XmlReader implementation rg -n "class TestUtils" -A 20 -B 0 --type cs # 2. Find where XmlReader is defined or used in TestUtils rg -n "XmlReader" -A 5 -B 5 --type cs # 3. Locate the QuestParser implementation rg -l "class QuestParser" --type cs # 4. Display the first 300 lines of QuestParser to see ParseNew logic FILE=$(rg -l "class QuestParser" --type cs) echo "=== QuestParser file: $FILE ===" head -n 300 "$FILE" # 5. List any XML files related to quests to estimate the number of entries echo "=== XML files containing 'quest' in their path ===" find . -type f -iname '*.xml' | grep -i quest | sed -n '1,50p'
Confirm new KR quest count (6961)
Please verify that the
Assert.AreEqual(6961, count)inTestQuestParserNewaccurately reflects the total number of quests in the KR “Live” dataset. Without matching this against the actual XML source, the test may break if entries are added or missing.Points to check:
- The XML files loaded by
Filter.Load(TestUtils.XmlReader, "KR", "Live")contain exactly 6961 unique<Quest>entries.QuestParser.ParseNew()is not filtering out or duplicating any quests.- Any recent data updates (added/removed quests) have been regenerated and included in the test fixtures.
33-36:Details
✅ Verification successful
Verify the test method rename aligns with parser changes.
The test method has been renamed from
TestQuestParserKrtoTestQuestParserNewand now usesLocale.KRwith language "kr". This should align with the parser implementation changes.
🏁 Script executed:
#!/bin/bash # Verify that QuestParser.ParseNew() method exists and returns QuestDataNew ast-grep --pattern $'class QuestParser { $$$ ParseNew($$$) { $$$ } $$$ }'Length of output: 86
🏁 Script executed:
#!/bin/bash # Locate the QuestParser implementation file quest_file=$(rg -l "class QuestParser" -g "*.cs") echo "QuestParser file: $quest_file" # Show lines around the class definition rg -n "class QuestParser" -C 5 "$quest_file" # List all Parse* methods in that file rg -n "Parse" -C 5 "$quest_file"Length of output: 731
🏁 Script executed:
#!/bin/bash # Enhanced verification: properly split file list and inspect each file mapfile -t files < <(rg -l "class QuestParser" -g "*.cs") for file in "${files[@]}"; do echo "=== Inspecting $file ===" rg -n "class QuestParser" -C 5 "$file" rg -n "Parse" -C 5 "$file" doneLength of output: 166
🏁 Script executed:
#!/bin/bash # Properly iterate over files containing QuestParser rg -l "class QuestParser" -g "*.cs" | while IFS= read -r file; do echo "=== Inspecting $file ===" rg -n "class QuestParser" -C 5 "$file" rg -n "Parse" -C 5 "$file" rg -n "ParseNew" -C 5 "$file" doneLength of output: 8406
TestQuestParserNew correctly matches QuestParser.ParseNew
Verified that
Maple2.File.Parser/QuestParser.csdefines:
public IEnumerable<(int Id, string Name, QuestDataNew Data)> ParseNew()And that
Maple2.File.Tests/QuestParserTest.cs’sTestQuestParserNewinvokesparser.ParseNew()and asserts onQuestDataNew. No further changes required.Maple2.File.Parser/Xml/Skill/Kinds.cs (3)
38-38: Consistent naming with the new schema version.The class rename from
KindsKRtoKindsNewaligns with the broader refactoring pattern across the codebase to support the new XML format.
61-61: Good addition of new attribute.The new
superArmorApplyFieldTypeboolean attribute appears to be a clean addition for the new XML schema.
52-52:Details
✅ Verification successful
Verify the type change for immediateActive.
The
immediateActivefield has changed frombooltoint. This is a significant type change that could affect serialization and deserialization.
🏁 Script executed:
#!/bin/bash # Search for usage of immediateActive to understand the impact of type change rg -A 2 -B 2 "immediateActive" --type csLength of output: 2563
ImmediateActive Type Change Verified
TheimmediateActivefield in Kinds.cs was intentionally updated frombooltoint, while TriggerSkill.cs continues to use aboolfor its ownimmediateActive. These are separate XML-attribute mappings and carry different semantics, so the change is safe as long as the XML for<Kinds>uses integer values (e.g. 0/1).Please ensure:
• The XML data for<Kinds>populatesimmediateActivewith integers.
• Any related schema or documentation is updated to reflect this.Files reviewed:
- Maple2.File.Parser/Xml/Skill/Kinds.cs
- Maple2.File.Parser/Xml/Skill/TriggerSkill.cs
Maple2.File.Parser/ItemOptionParser.cs (5)
92-112: Well-structured addition of new serializers.The addition of new serializers for the "New" data types follows a consistent pattern and maintains backward compatibility by keeping the existing serializers. This approach allows for gradual migration.
132-144: Improved file organization with consolidated table approach.The new
ParseConstantNew()method uses a single consolidated table file (table/itemoptionconstant.xml) instead of multiple individual files with suffixes. This simplification improves maintainability and reduces file I/O operations.
162-175: Consistent implementation pattern for new parsing methods.The
ParseRandomNew()method follows the same pattern asParseConstantNew()with proper error handling usingDebug.Assertand appropriate filtering logic.
247-255: Clean implementation of new variation parsing.The new
ParseVariationNew()method maintains consistency with the existing pattern while using the updated data types. The implementation is clean and follows established conventions.
193-203:Details
✅ Verification successful
Verify the new data types exist.
The method uses
ItemMergeOptionRootNewandMergeOptionNewtypes. Ensure these new data classes are properly defined in the corresponding XML namespace.
🏁 Script executed:
#!/bin/bash # Verify the new data types are defined ast-grep --pattern $'class ItemMergeOptionRootNew { $$$ }' ast-grep --pattern $'class MergeOptionNew { $$$ }'Length of output: 115
🏁 Script executed:
#!/bin/bash # Search for any references or definitions of the types across the codebase rg --fixed-strings "ItemMergeOptionRootNew" -n . rg --fixed-strings "MergeOptionNew" -n .Length of output: 1282
Verified new data type definitions
Definitions for
ItemMergeOptionRootNew(line 72) andMergeOptionNew(line 76) are present inMaple2.File.Parser/Xml/Table/ItemMergeOption.cs. No further action required.Maple2.File.Parser/ScriptParser.cs (5)
6-6: Good addition of enum namespace for type safety.Adding the
Maple2.File.Parser.Enumnamespace enables the use of strongly typed enums, improving code safety and maintainability.
18-27: Excellent language parameterization.The addition of the
languageparameter to the constructor enables dynamic language handling, making the parser more flexible and reusable across different locales. The separation of serializers for old and new formats maintains backward compatibility.
61-70: Consistent method renaming pattern.The renaming from
ParseQuestKrtoParseQuestNewfollows the established pattern, though the return type remains the same. The use of consolidatedquestscript_final.xmlfile improves organization.
72-85: Improved dynamic language handling.The
ParseStringsmethod now dynamically constructs the file prefix using the language parameter instead of hardcoded values. This makes the method more flexible and reduces code duplication.
39-48:Details
❓ Verification inconclusive
Verify the new data types and file structure.
The method has been renamed from
ParseNpcKrtoParseNpcNewand now returnsNpcScriptNewinstead of the previous type. It also uses a consolidated filenpcscript_final.xmlinstead of individual files.
🏁 Script executed:
#!/bin/bash # Verify the new data types exist ast-grep --pattern $'class NpcScriptListNew { $$$ }' ast-grep --pattern $'class NpcScriptNew { $$$ }' # Check if the consolidated file exists in the codebase fd "npcscript_final.xml"Length of output: 132
Verify new data types and resource file
I wasn’t able to locate the following in the codebase, so please confirm that they’ve been added and are correctly named/included:
- Class definitions for
NpcScriptListNewandNpcScriptNew- The consolidated XML resource file
npcscript_final.xml(e.g., under your project’s resources or data folder)If these types or the XML file haven’t been created or added yet, please update the project accordingly to avoid runtime errors.
Maple2.File.Tests/ItemOptionParserTest.cs (2)
2-2: Good addition of Locale enum import.Adding the
Localeenum import enables consistent locale handling throughout the test class.
76-99: Well-structured test with proper assertions.The test method properly validates both parsing methods and includes expected count assertions, which provides good test coverage.
Maple2.File.Parser/Xml/Item/Slot.cs (2)
14-16: Excellent defensive programming with proper initialization.The comprehensive initialization of collections and objects prevents null reference exceptions and follows good defensive programming practices. This is particularly important for XML deserialization where properties might not be populated.
Also applies to: 35-39, 53-53, 58-59, 84-85
8-8:Details
✅ Verification successful
Verify C# 12 collection expression compatibility.
The code uses collection expression syntax
= []which is a C# 12 feature. Ensure your project targets .NET 8+ to support this syntax.
🏁 Script executed:
#!/bin/bash # Description: Check the target framework version in project files # Find and examine project files for target framework fd -e csproj -e fsproj -e vbproj --exec cat {} \; | grep -i "targetframework\|langversion"Length of output: 675
No action required: C# 12 collection expressions are supported
All project files target net8.0 with<LangVersion>12</LangVersion>(orlatestmajor), so the= []syntax is fully supported.Maple2.File.Tests/NpcParserTest.cs (3)
13-15: Consistent locale and language parameter handling.Good use of the Locale enum and proper language parameter passing to the NpcParser constructor.
33-51: Well-implemented test for new XML format parsing.The test method properly validates the new parsing functionality with appropriate assertions and expected counts.
67-74: Efficient conditional logic for multi-locale testing.The switch statement allows testing different locales within a single test method while maintaining proper expected count validation.
Maple2.File.Tests/SkillParserTest.cs (1)
13-15: LGTM! Good use of Locale enum for type safety.The change from hardcoded strings to the
Localeenum improves type safety and maintainability.Maple2.File.Parser/Xml/AdditionalEffect/AdditionalEffect.cs (2)
43-92: Well-structured implementation of the new XML format.The new classes provide a clear hierarchy and improved property naming (e.g.,
BasicProperty→Basic). The structure supports the new XML format effectively.
186-202: Good use of nested classes for type safety.The nested classes within
Condition(e.g.,HasNotAdditionalEffectGroup,RequireEffect,WeaponNew,MapCodes) provide clear type definitions and improve code organization.Maple2.File.Parser/Xml/Quest/Quest.cs (1)
18-39: LGTM! Good defensive programming practice.The initialization of all fields with default instances prevents null reference exceptions and aligns well with the refactoring from "KR" to "New" naming convention to support the new XML format.
Also applies to: 58-64
Maple2.File.Parser/TableParser.cs (3)
49-49: Well-structured refactoring to support language parameterization.The addition of language parameter and new serializers properly sets up the infrastructure for supporting the new XML format while maintaining backward compatibility.
Also applies to: 103-103, 106-106, 108-108, 144-144, 198-198, 201-201
243-243: Verify the intentional differentiation between locale and language usage.The code uses
languageparameter for string table lookups (e.g., stringfishname.xml, interactname.xml) while continuing to uselocalefor game data files. This differentiation appears intentional but should be verified to ensure consistency with the new XML format requirements.Can you confirm that this differentiation is intentional? If so, consider adding a comment explaining when to use
localevslanguageto help future maintainers.Also applies to: 349-349, 487-487, 537-542, 737-737, 763-763, 1274-1274, 1350-1356
1521-1541: Clean implementation of stat string parsing methods.The new
ParseStatStringandParseSpecialAbilityStringmethods follow the established pattern and appropriately reuse the same serializer since both XML files likely share the same structure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Maple2.File.Parser/Xml/Skill/Skill.cs (2)
141-170: 🛠️ Refactor suggestionConsider a more structured migration approach for the new XML format.
While the duplication between
SkillAttackDataandSkillAttackDataNewwas already flagged in previous reviews, the structural changes suggest this is part of a systematic migration. The property name simplifications (removing "Property" suffixes) and type updates (e.g.,RegionSkill→RegionSkillNew) indicate a deliberate schema evolution.Consider implementing a phased migration strategy:
- Create a common interface or base class for shared behavior
- Add a version identifier to distinguish between formats
- Plan deprecation timeline for the legacy classes
+public interface ISkillAttackData { + string point { get; set; } + int pointGroupID { get; set; } + int targetCount { get; set; } + // ... other common properties +} + +public partial class SkillAttackData : ISkillAttackData { + // existing implementation +} + +public partial class SkillAttackDataNew : ISkillAttackData { + // new implementation +}
216-218: 🛠️ Refactor suggestionAddress commented-out code blocks.
Multiple properties remain commented out without explanation. This was flagged in previous reviews but the issue persists.
Either:
- Remove the commented code if it's obsolete
- Uncomment and properly integrate if still needed
- Add documentation explaining why it's preserved in this state
- // [XmlAttribute] public bool IgnoreOptimalHideEndEffect; - // [XmlAttribute] public bool IgnoreOptimalHideSplashInvokeEffect; - // [XmlAttribute] public bool IgnoreOptimalHideSplashEndEffect;Also applies to: 226-226, 229-231, 238-238
🧹 Nitpick comments (2)
Maple2.File.Parser/Xml/Skill/Skill.cs (2)
173-177: Update misleading comment about KR version.The comment indicates this is for "KR version" files, but the PR objectives state that this change aims to differentiate between formats "without relying on the locale setting." This comment appears inconsistent with the goal of locale-independence.
-// KR version: ./data/xml/skilldata/%04d.xml +// New format: ./data/xml/skilldata/%04d.xml
202-246: Consider separating motion and attack concerns.The
SkillMotionDataNewclass combines motion properties (sequenceName, effects, movement) with attack data (SkillAttackDataNew list), violating the single responsibility principle. This makes the class harder to understand and maintain.Consider splitting this into separate classes:
+public partial class SkillMotionDataNew { + [XmlElement] public SkillMotionPropertyNew motionProperty; + [XmlElement] public List<SkillAttackDataNew> attack; +} + +public partial class SkillMotionPropertyNew { + // Motion-specific properties only + [XmlAttribute] public string sequenceName = string.Empty; + // ... other motion properties +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.File.Parser/Xml/Skill/Skill.cs(2 hunks)Maple2.File.Tests/ScriptParserTest.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.File.Tests/ScriptParserTest.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Maple2.File.Parser/Xml/Skill/Skill.cs (2)
179-185: Clean structure for the new skill format.The
SkillNewclass provides a clean foundation for the new XML format with appropriate use of the feature locale system and consistent naming conventions.
187-200: Excellent structural improvements in the new format.The
SkillLevelDataNewclass demonstrates good architectural decisions by:
- Flattening the hierarchy with level-specific attributes (sp, ep, cooldown, etc.)
- Reducing complexity by eliminating unnecessary nesting
- Maintaining clean separation of concerns with focused child elements
This should improve both performance and maintainability compared to the original structure.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores