Remove hardcoded server constants and use parsed constants from Server.m2d / constants.xml#661
Open
mfranca0009 wants to merge 26 commits intoMS2Community:masterfrom
Open
Remove hardcoded server constants and use parsed constants from Server.m2d / constants.xml#661mfranca0009 wants to merge 26 commits intoMS2Community:masterfrom
mfranca0009 wants to merge 26 commits intoMS2Community:masterfrom
Conversation
…r constants that are processed during file ingest WIP.
…ring file ingest WIP
…ing file ingest #2 WIP
…ing file ingest MS2Community#3 WIP
…ning and non-IConvertible data types handling during constant parsing.
… nested foreach loop and indexing the property names to search them later when looping through the parsed XML constants data.
…w them to retrieve their values.
… for ConstantsTable.
…onfusion from two constructors and nethier defined as the JSON constructor.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Author
|
Fixing a server constant and NPCMetadata parameter that was deleted during merge conflict resolution; stand by please. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Maple2.Server.Game/Manager/FishingManager.cs (1)
242-255:⚠️ Potential issue | 🟠 MajorGuard server-configured tick values before
Random.Shared.Next.If
fisherBoreDurationis low/invalid (or reduced below 1), the computed ranges at Lines 252/254 can become invalid and throw at runtime.Suggested hardening
- int fishingTick = session.ServerTableMetadata.ConstantsTable.fisherBoreDuration; + int fishingTick = Math.Max(1, session.ServerTableMetadata.ConstantsTable.fisherBoreDuration); bool hasAutoFish = session.Player.Buffs.HasBuff(BuffEventType.AutoFish); // Fishing Success if (Random.Shared.Next(0, 10000) < selectedFish.BaitProbability) { if (!hasAutoFish && Random.Shared.Next(0, 10000) < session.ServerTableMetadata.ConstantsTable.fishFightingProp) { fishFightGame = true; } - fishingTick -= rodMetadata.ReduceTime; - fishingTick = Random.Shared.Next(fishingTick - fishingTick / 3, fishingTick); // Chance for early catch + fishingTick = Math.Max(1, fishingTick - rodMetadata.ReduceTime); + int earlyMin = Math.Max(0, fishingTick - fishingTick / 3); + fishingTick = Random.Shared.Next(earlyMin, fishingTick + 1); // Chance for early catch } else { - fishingTick = Random.Shared.Next(fishingTick + 1, fishingTick * 2); // If tick is over bore duration, it will fail + int failMin = fishingTick + 1; + int failMaxExclusive = Math.Max(failMin + 1, fishingTick * 2); + fishingTick = Random.Shared.Next(failMin, failMaxExclusive); // If tick is over bore duration, it will fail }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/FishingManager.cs` around lines 242 - 255, Guard against invalid/too-small tick ranges before calling Random.Shared.Next by clamping fishingTick to a safe minimum and ensuring the Next(min, max) arguments are valid: when computing fishingTick from session.ServerTableMetadata.ConstantsTable.fisherBoreDuration and rodMetadata.ReduceTime, enforce fishingTick >= 1 (or a sensible minimum like 1) after subtraction, and compute the lower/upper bounds (e.g., lower = Math.Max(1, fishingTick - fishingTick/3) and upper = Math.Max(lower+1, fishingTick)) before calling Random.Shared.Next; do the same for the failure branch (ensure upper > lower, e.g., min = Math.Max(1, fishingTick+1) and max = Math.Max(min+1, fishingTick*2)) so Random.Shared.Next never receives an invalid range.Maple2.Database/Storage/Game/GameStorage.Map.cs (1)
114-122:⚠️ Potential issue | 🟠 MajorAvoid mutating tracked rows when computing effective expiry order.
Line 118 mutates tracked
UgcMapentities within the foreach loop, then line 121 re-queries theIQueryablefor ordering. The subsequentOrderByexecutes against the database and selects based on originalExpiryTimevalues, not the adjusted in-memory values. Additionally, the modified tracked entities remain in the change tracker and will persist ifSaveChangesis called later in the same request scope.💡 Proposed fix
- public PlotInfo? GetSoonestPlotFromExpire(TimeSpan ugcHomeSaleWaitingTime) { - IQueryable<UgcMap> maps = Context.UgcMap.Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor); - foreach (UgcMap map in maps) { - if (map.OwnerId == 0) { - map.ExpiryTime = map.ExpiryTime.Add(ugcHomeSaleWaitingTime); - } - } - UgcMap? model = maps.OrderBy(map => map.ExpiryTime).FirstOrDefault(); + public PlotInfo? GetSoonestPlotFromExpire(TimeSpan ugcHomeSaleWaitingTime) { + List<UgcMap> maps = Context.UgcMap + .AsNoTracking() + .Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor) + .ToList(); + + UgcMap? model = maps + .OrderBy(map => map.OwnerId == 0 + ? map.ExpiryTime.Add(ugcHomeSaleWaitingTime) + : map.ExpiryTime) + .FirstOrDefault(); return model == null ? null : ToPlotInfo(model); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Database/Storage/Game/GameStorage.Map.cs` around lines 114 - 122, GetSoonestPlotFromExpire mutates tracked UgcMap.ExpiryTime for OwnerId==0 then re-queries, so the DB ordering ignores in-memory adjustments and changes may be persisted; instead avoid mutation by projecting an effective expiry and ordering by that projection (use Context.UgcMap / UgcMap, compute effectiveExpiry = (map.OwnerId == 0 ? map.ExpiryTime + ugcHomeSaleWaitingTime : map.ExpiryTime) in the query or via AsNoTracking and a Select), then pick the record with the smallest effectiveExpiry and construct/return the PlotInfo without altering the tracked UgcMap entities or calling SaveChanges.Maple2.Server.Game/Manager/CurrencyManager.cs (1)
156-168:⚠️ Potential issue | 🟠 MajorMigrate StarPoint and MesoToken max constants to ConstantsTable.
StarPointMaxandMesoTokenMaxare missing fromConstantsTableand still reference hardcodedConstant.*values while other currencies (ReverseCoin,MentorToken,MenteeToken) have been migrated. Add these properties toConstantsTableand update the references inCurrencyManager(lines 157-159, 165-167) to usesession.ServerTableMetadata.ConstantsTable.StarPointMaxandsession.ServerTableMetadata.ConstantsTable.MesoTokenMaxfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/CurrencyManager.cs` around lines 156 - 168, Currency max values StarPointMax and MesoTokenMax were not migrated to ConstantsTable; add int properties StarPointMax and MesoTokenMax to the ConstantsTable class and populate them in the server metadata, then update CurrencyManager (the switch handling for CurrencyType.StarPoint and CurrencyType.MesoToken) to replace Constant.StarPointMax and Constant.MesoTokenMax with session.ServerTableMetadata.ConstantsTable.StarPointMax and session.ServerTableMetadata.ConstantsTable.MesoTokenMax respectively so the code uses the centralized ConstantsTable values.
🧹 Nitpick comments (5)
Maple2.Model/Game/Cube/Nurturing.cs (1)
63-73: RenamenurturingEatGrowthinPlayfor clarity.This parameter is used for play progression but is named like eat progression. Renaming improves call-site readability and avoids constant-mix mistakes.
Proposed rename
-public bool Play(long accountId, int nurturingEatGrowth, int nurturingPlayMaxCount) { +public bool Play(long accountId, int nurturingPlayGrowth, int nurturingPlayMaxCount) { if (PlayedBy.Count >= nurturingPlayMaxCount) { return false; } @@ - Feed(nurturingEatGrowth); + Feed(nurturingPlayGrowth); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Game/Cube/Nurturing.cs` around lines 63 - 73, The Play method's parameter nurturingEatGrowth is misleading; rename it to something like nurturingPlayGrowth (or playProgress) in the Play(long accountId, int nurturingEatGrowth, int nurturingPlayMaxCount) signature and update all usages inside the method (e.g., the Feed call: Feed(nurturingEatGrowth) -> Feed(nurturingPlayGrowth) or Feed(playProgress)) and any callers that pass that argument to match the new name so the Play method and its interaction with Feed and callers remain consistent.Maple2.Server.Game/PacketHandlers/MesoMarketHandler.cs (1)
62-62: Use one source of truth for base-price debit.After validating
amount, debitamount(or a cachedbasePrice) on Line 99 to avoid split-source drift and keep the transaction self-consistent.🛠️ Suggested consistency cleanup
- if (amount != session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice) { + long basePrice = session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice; + if (amount != basePrice) { session.Send(MesoMarketPacket.Error(s_mesoMarket_error_invalidSaleMoney)); return; } @@ - session.Currency.Meso -= session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice; + session.Currency.Meso -= amount;Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/MesoMarketHandler.cs` at line 62, The code validates amount against session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice but later debits a hardcoded or different value; change the debit to use the same validated source to avoid drift: inside MesoMarketHandler, after the check that amount == session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice, cache that value (e.g., basePrice = session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice or use the validated amount variable) and use that cached value when calling the account/character debit logic (the code that currently subtracts from the player balance around the debit operation on Line ~99) so validation and debit use one consistent source (amount or basePrice) rather than two different literals/fields.Maple2.Server.Game/Manager/BlackMarketManager.cs (1)
66-66: GuardBlackMarketSellEndDaybefore computing expiry.Line 66 trusts metadata directly; a non-positive value would create immediately expired listings. Clamp to a safe minimum (or fail fast earlier).
🛠️ Suggested guard
- ExpiryTime = DateTime.Now.AddDays(session.ServerTableMetadata.ConstantsTable.BlackMarketSellEndDay).ToEpochSeconds(), + int sellDays = Math.Max(1, session.ServerTableMetadata.ConstantsTable.BlackMarketSellEndDay); + ExpiryTime = DateTime.Now.AddDays(sellDays).ToEpochSeconds(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/BlackMarketManager.cs` at line 66, The ExpiryTime assignment uses session.ServerTableMetadata.ConstantsTable.BlackMarketSellEndDay directly which can be non-positive and produce immediate expirations; guard by clamping that value to a safe minimum (e.g., 1 day) before calling DateTime.Now.AddDays(...).ToEpochSeconds() — replace the direct reference to BlackMarketSellEndDay in the ExpiryTime calculation with a validated value (e.g., var days = Math.Max(1, session.ServerTableMetadata.ConstantsTable.BlackMarketSellEndDay)) or explicitly check for <= 0 and handle/log/fail-fast, then use that validated days value when computing ExpiryTime.Maple2.Model/Game/User/StatAttributes.cs (1)
80-90: Consider: Magic strings could be centralized.The stat limit keys ("StatPointLimit_str", "StatPointLimit_dex", etc.) are duplicated here and in
ConfigManager. Consider extracting these to constants to prevent typo-related bugs.💡 Optional: Extract key constants
// In a shared location: public static class StatLimitKeys { public const string Strength = "StatPointLimit_str"; public const string Dexterity = "StatPointLimit_dex"; public const string Intelligence = "StatPointLimit_int"; public const string Luck = "StatPointLimit_luk"; public const string Health = "StatPointLimit_hp"; public const string CriticalRate = "StatPointLimit_cap"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Game/User/StatAttributes.cs` around lines 80 - 90, Centralize the repeated stat-limit magic strings by introducing shared constants and updating StatLimit to use them: extract keys like "StatPointLimit_str", "StatPointLimit_dex", "StatPointLimit_int", "StatPointLimit_luk", "StatPointLimit_hp", and "StatPointLimit_cap" into a single static class (used by ConfigManager and other callers), then replace the literal keys in the StatLimit(BasicAttribute type, IDictionary<string,int> statLimits) switch in StatAttributes.cs to reference those constants (e.g., StatLimitKeys.Strength, StatLimitKeys.Dexterity, etc.) to avoid duplication and typos.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
219-219: Minor: Redundant conversion for EndTick calculation.
TimeSpan.FromSeconds(x).TotalMillisecondsis equivalent tox * 1000. While functionally correct, it's slightly verbose.💡 Optional simplification
- EndTick = (FieldTick + (long) TimeSpan.FromSeconds(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime).TotalMilliseconds).Truncate32(), + EndTick = (FieldTick + (long)(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime * 1000)).Truncate32(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs` at line 219, Replace the verbose TimeSpan calculation for EndTick with a direct milliseconds multiplication: change the expression using TimeSpan.FromSeconds(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime).TotalMilliseconds to owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime * 1000 (cast to long as needed) so EndTick = (FieldTick + (long)(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime * 1000)).Truncate32(); update the expression in FieldManager.State where EndTick, FieldTick, and QuestPortalKeepTime are referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs`:
- Around line 2151-2157: The CleanConstantsInput logic is unsafe: replace the
direct int.Parse(input) and input[0] access with defensive checks and TryParse
to avoid exceptions for empty/malformed values; for the TimeSpan case (propName
"UgcHomeSaleWaitingTime") ensure input is not null/empty and use int.TryParse to
get seconds (fallback to 0 or a sensible default) before calling
TimeSpan.FromSeconds, and for the integer prefix-trimming branch (type ==
typeof(int)) check input is not null/empty and input.Length>1 before indexing
and consider validating that all characters are digits before removing a leading
'0' to avoid corrupting non-numeric values.
In `@Maple2.Model/Game/Cube/Nurturing.cs`:
- Around line 51-59: Feed currently only increments Stage once; change Feed to
handle crossing multiple thresholds by looping: after adding nurturingEatGrowth
to Exp, repeatedly check the threshold for the current Stage using
NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp and increment
Stage while Exp >= that threshold and Stage is less than
NurturingMetadata.RequiredGrowth.Last().Stage (or until Exp >=
NurturingMetadata.RequiredGrowth.Last().Exp), so Stage advances across all
reached thresholds in a single Feed call.
In `@Maple2.Model/Game/Mail.cs`:
- Around line 45-50: The Mail constructor currently allows negative
mailExpiryDays which yields an already-expired ExpiryTime; add an input guard at
the start of the public Mail(int mailExpiryDays) constructor to validate
mailExpiryDays (e.g., if (mailExpiryDays < 0) throw new
ArgumentOutOfRangeException(nameof(mailExpiryDays), "mailExpiryDays must be
non-negative");) so ExpiryTime is always set to now or a future time; modify the
Mail constructor (method symbol: Mail) to perform this check before computing
ExpiryTime.
In `@Maple2.Model/Game/Npc/Npc.cs`:
- Around line 14-19: The conditional chain in Npc.cs making the Metadata
assignment is unreachable because the first if
(metadata.Distance.LastSightRadius == 0) prevents the later height fallbacks
from executing; update the logic in the Npc constructor (where Metadata is set)
to compute each fallback value independently and then construct NpcMetadata
once: determine lastSightRadius = metadata.Distance.LastSightRadius == 0 ?
constLastSightRadius : metadata.Distance.LastSightRadius, lastSightHeightUp =
metadata.Distance.LastSightHeightUp == 0 ? constLastSightHeightUp :
metadata.Distance.LastSightHeightUp, and lastSightHeightDown =
metadata.Distance.LastSightHeightDown == 0 ? constLastSightHeightDown :
metadata.Distance.LastSightHeightDown, then call Metadata = new
NpcMetadata(metadata, lastSightRadius, lastSightHeightUp, lastSightHeightDown)
(or the appropriate NpcMetadata overload) so each missing dimension correctly
falls back to its constant.
In `@Maple2.Model/Metadata/ServerTable/ConstantsTable.cs`:
- Around line 120-121: Replace unsafe null-forgiving array defaults with empty
arrays for the ConstantsTable record: change properties like GetExpControlValue0
and GetExpControlValue1 (and all other array fields currently initialized with
"= null!") to use safe defaults such as Array.Empty<float>() (or
Array.Empty<T>() for the appropriate element type) so missing keys yield empty
arrays instead of risking NREs; update every listed occurrence in the record
(including the lines noted in the comment) to the empty-array pattern while
keeping types and property names intact.
In `@Maple2.Server.Game/Manager/Items/InventoryManager.cs`:
- Around line 49-63: The switch mapping inventory types to constants indexes can
throw if the ConstantsTable arrays (e.g., bagSlotTabGameCount,
bagSlotTabSkinCount, etc.) are null or shorter than expected; update the
InventoryManager initialization/expansion code that switches on InventoryType to
guard each array before indexing by checking for null and Length > 0 (or use
safe access with a default fallback), specifically around the usages of
session.ServerTableMetadata.ConstantsTable.* properties; if an array is missing
or too short return a sensible default slot count (0 or configured fallback) or
log an error and handle gracefully so no IndexOutOfRangeException occurs.
In `@Maple2.Server.Game/PacketHandlers/MeretMarketHandler.cs`:
- Around line 200-202: The relist path writes the raw client price to item.Price
(item.Price = price) bypassing the same clamping applied for initial listings;
update the relist branch in MeretMarketHandler (where item.Price is set during
relist) to clamp price to the configured min/max using the same constants used
for initial listings (e.g., UGCShopMinPrice/UGCShopMaxPrice or the equivalents
on session.ServerTableMetadata.ConstantsTable) before assigning to item.Price so
relisted items cannot exceed configured bounds; keep the existing
PromotionEndTime/ListingEndTime logic unchanged.
In `@Maple2.Server.Game/PacketHandlers/TaxiHandler.cs`:
- Around line 139-141: The code checks session.Currency.Meret against
ServerTableMetadata.ConstantsTable.MeratAirTaxiPrice but sends the wrong error
StringCode.s_err_lack_meso; update the NoticePacket.MessageBox call to use the
Meret-specific error constant (e.g., replace StringCode.s_err_lack_meso with
StringCode.s_err_lack_meret) so insufficient Meret payments show the correct
error; adjust any similar Meret taxi payment branches to reference
StringCode.s_err_lack_meret and keep the existing session/Currency and
ConstantsTable checks unchanged.
---
Outside diff comments:
In `@Maple2.Database/Storage/Game/GameStorage.Map.cs`:
- Around line 114-122: GetSoonestPlotFromExpire mutates tracked
UgcMap.ExpiryTime for OwnerId==0 then re-queries, so the DB ordering ignores
in-memory adjustments and changes may be persisted; instead avoid mutation by
projecting an effective expiry and ordering by that projection (use
Context.UgcMap / UgcMap, compute effectiveExpiry = (map.OwnerId == 0 ?
map.ExpiryTime + ugcHomeSaleWaitingTime : map.ExpiryTime) in the query or via
AsNoTracking and a Select), then pick the record with the smallest
effectiveExpiry and construct/return the PlotInfo without altering the tracked
UgcMap entities or calling SaveChanges.
In `@Maple2.Server.Game/Manager/CurrencyManager.cs`:
- Around line 156-168: Currency max values StarPointMax and MesoTokenMax were
not migrated to ConstantsTable; add int properties StarPointMax and MesoTokenMax
to the ConstantsTable class and populate them in the server metadata, then
update CurrencyManager (the switch handling for CurrencyType.StarPoint and
CurrencyType.MesoToken) to replace Constant.StarPointMax and
Constant.MesoTokenMax with
session.ServerTableMetadata.ConstantsTable.StarPointMax and
session.ServerTableMetadata.ConstantsTable.MesoTokenMax respectively so the code
uses the centralized ConstantsTable values.
In `@Maple2.Server.Game/Manager/FishingManager.cs`:
- Around line 242-255: Guard against invalid/too-small tick ranges before
calling Random.Shared.Next by clamping fishingTick to a safe minimum and
ensuring the Next(min, max) arguments are valid: when computing fishingTick from
session.ServerTableMetadata.ConstantsTable.fisherBoreDuration and
rodMetadata.ReduceTime, enforce fishingTick >= 1 (or a sensible minimum like 1)
after subtraction, and compute the lower/upper bounds (e.g., lower = Math.Max(1,
fishingTick - fishingTick/3) and upper = Math.Max(lower+1, fishingTick)) before
calling Random.Shared.Next; do the same for the failure branch (ensure upper >
lower, e.g., min = Math.Max(1, fishingTick+1) and max = Math.Max(min+1,
fishingTick*2)) so Random.Shared.Next never receives an invalid range.
---
Nitpick comments:
In `@Maple2.Model/Game/Cube/Nurturing.cs`:
- Around line 63-73: The Play method's parameter nurturingEatGrowth is
misleading; rename it to something like nurturingPlayGrowth (or playProgress) in
the Play(long accountId, int nurturingEatGrowth, int nurturingPlayMaxCount)
signature and update all usages inside the method (e.g., the Feed call:
Feed(nurturingEatGrowth) -> Feed(nurturingPlayGrowth) or Feed(playProgress)) and
any callers that pass that argument to match the new name so the Play method and
its interaction with Feed and callers remain consistent.
In `@Maple2.Model/Game/User/StatAttributes.cs`:
- Around line 80-90: Centralize the repeated stat-limit magic strings by
introducing shared constants and updating StatLimit to use them: extract keys
like "StatPointLimit_str", "StatPointLimit_dex", "StatPointLimit_int",
"StatPointLimit_luk", "StatPointLimit_hp", and "StatPointLimit_cap" into a
single static class (used by ConfigManager and other callers), then replace the
literal keys in the StatLimit(BasicAttribute type, IDictionary<string,int>
statLimits) switch in StatAttributes.cs to reference those constants (e.g.,
StatLimitKeys.Strength, StatLimitKeys.Dexterity, etc.) to avoid duplication and
typos.
In `@Maple2.Server.Game/Manager/BlackMarketManager.cs`:
- Line 66: The ExpiryTime assignment uses
session.ServerTableMetadata.ConstantsTable.BlackMarketSellEndDay directly which
can be non-positive and produce immediate expirations; guard by clamping that
value to a safe minimum (e.g., 1 day) before calling
DateTime.Now.AddDays(...).ToEpochSeconds() — replace the direct reference to
BlackMarketSellEndDay in the ExpiryTime calculation with a validated value
(e.g., var days = Math.Max(1,
session.ServerTableMetadata.ConstantsTable.BlackMarketSellEndDay)) or explicitly
check for <= 0 and handle/log/fail-fast, then use that validated days value when
computing ExpiryTime.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Line 219: Replace the verbose TimeSpan calculation for EndTick with a direct
milliseconds multiplication: change the expression using
TimeSpan.FromSeconds(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime).TotalMilliseconds
to owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime * 1000
(cast to long as needed) so EndTick = (FieldTick +
(long)(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime *
1000)).Truncate32(); update the expression in FieldManager.State where EndTick,
FieldTick, and QuestPortalKeepTime are referenced.
In `@Maple2.Server.Game/PacketHandlers/MesoMarketHandler.cs`:
- Line 62: The code validates amount against
session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice but later debits
a hardcoded or different value; change the debit to use the same validated
source to avoid drift: inside MesoMarketHandler, after the check that amount ==
session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice, cache that value
(e.g., basePrice =
session.ServerTableMetadata.ConstantsTable.MesoMarketBasePrice or use the
validated amount variable) and use that cached value when calling the
account/character debit logic (the code that currently subtracts from the player
balance around the debit operation on Line ~99) so validation and debit use one
consistent source (amount or basePrice) rather than two different
literals/fields.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
Maple2.Database/Model/Mail.csMaple2.Database/Storage/Game/GameStorage.Map.csMaple2.Database/Storage/Metadata/ServerTableMetadataStorage.csMaple2.File.Ingest/Mapper/NpcMapper.csMaple2.File.Ingest/Mapper/ServerTableMapper.csMaple2.Model/Common/ServerTableNames.csMaple2.Model/Game/Cube/Nurturing.csMaple2.Model/Game/Mail.csMaple2.Model/Game/Npc/Npc.csMaple2.Model/Game/User/StatAttributes.csMaple2.Model/Metadata/Constants.csMaple2.Model/Metadata/NpcMetadata.csMaple2.Model/Metadata/ServerTable/ConstantsTable.csMaple2.Model/Metadata/ServerTableMetadata.csMaple2.Server.Game/Commands/PlayerCommand.csMaple2.Server.Game/Manager/BlackMarketManager.csMaple2.Server.Game/Manager/BuddyManager.csMaple2.Server.Game/Manager/BuffManager.csMaple2.Server.Game/Manager/ConfigManager.csMaple2.Server.Game/Manager/CurrencyManager.csMaple2.Server.Game/Manager/ExperienceManager.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.csMaple2.Server.Game/Manager/FishingManager.csMaple2.Server.Game/Manager/ItemMergeManager.csMaple2.Server.Game/Manager/Items/InventoryManager.csMaple2.Server.Game/Manager/Items/StorageManager.csMaple2.Server.Game/Manager/SkillManager.csMaple2.Server.Game/Manager/TradeManager.csMaple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.csMaple2.Server.Game/Model/Field/Actor/FieldNpc.csMaple2.Server.Game/Model/Field/Actor/FieldPet.csMaple2.Server.Game/Model/Field/Actor/FieldPlayer.csMaple2.Server.Game/Model/Field/Tombstone.csMaple2.Server.Game/PacketHandlers/BeautyHandler.csMaple2.Server.Game/PacketHandlers/ClubHandler.csMaple2.Server.Game/PacketHandlers/FunctionCubeHandler.csMaple2.Server.Game/PacketHandlers/GuildHandler.csMaple2.Server.Game/PacketHandlers/HomeBankHandler.csMaple2.Server.Game/PacketHandlers/HomeDoctorHandler.csMaple2.Server.Game/PacketHandlers/ItemLockHandler.csMaple2.Server.Game/PacketHandlers/JobHandler.csMaple2.Server.Game/PacketHandlers/MeretMarketHandler.csMaple2.Server.Game/PacketHandlers/MesoMarketHandler.csMaple2.Server.Game/PacketHandlers/NpcTalkHandler.csMaple2.Server.Game/PacketHandlers/PartyHandler.csMaple2.Server.Game/PacketHandlers/QuestHandler.csMaple2.Server.Game/PacketHandlers/SystemShopHandler.csMaple2.Server.Game/PacketHandlers/TaxiHandler.csMaple2.Server.Game/PacketHandlers/UserChatHandler.csMaple2.Server.Game/Session/GameSession.csMaple2.Server.Game/Util/ChatUtil.csMaple2.Server.Login/PacketHandlers/CharacterManagementHandler.csMaple2.Server.World/Containers/PartyLookup.csMaple2.Server.World/Containers/PartyManager.csMaple2.Server.World/Program.csMaple2.Server.World/WorldServer.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
2179-2184:⚠️ Potential issue | 🟠 Major
CleanConstantsInputstill has throw-on-malformed-input paths.
int.Parse(input)andinput[0]are still unsafe for ingest-time data and can crash constants loading.Suggested fix
if (type == typeof(TimeSpan) && propName == "UgcHomeSaleWaitingTime") { - input = TimeSpan.FromSeconds(int.Parse(input)).ToString(); // TODO: may not be correct conversion to TimeSpan + if (!int.TryParse(input, NumberStyles.Integer, CultureInfo.InvariantCulture, out int seconds)) { + seconds = 0; + } + input = TimeSpan.FromSeconds(seconds).ToString("c", CultureInfo.InvariantCulture); } // Remove prefix 0 on integers since they do not convert properly -if (type == typeof(int) && input[0] == '0' && input.Length > 1) { +if (type == typeof(int) && input.Length > 1 && input[0] == '0' && input.All(char.IsDigit)) { input = input.Remove(0, 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs` around lines 2179 - 2184, CleanConstantsInput currently uses int.Parse and direct indexing (input[0]) which can throw on malformed ingest data; update the TimeSpan handling for property "UgcHomeSaleWaitingTime" to TryParse the input as an integer (e.g., int.TryParse) before calling TimeSpan.FromSeconds and fallback to a safe default or error path, and replace the unsafe input[0] check in the int branch (type == typeof(int)) with a length check and/or TryParse to ensure the string is non-empty and numeric before trimming a leading '0' (use char checks or TryParse) so malformed inputs do not throw in CleanConstantsInput or related methods.
🧹 Nitpick comments (1)
Maple2.Model/Metadata/Constants.cs (1)
122-137: Avoid keeping server-tunable values in two sources.This block reintroduces many runtime-tunable settings as compile-time constants. With
ServerTableMetadata.ConstantsTablenow in place, this can drift and bypass runtime configuration. Prefer keepingConstantfor invariants and reading tunables from the parsed table at usage sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Metadata/Constants.cs` around lines 122 - 137, This file reintroduces runtime-tunable settings as compile-time constants (e.g., MailQuestItems, MaxClosetMaxCount, MaxClosetTabNameLength, BlockSize, SouthEast, NorthEast, NorthWest, SouthWest, UGCShopProfitFee, UGCShopProfitDelayDays, PartyFinderListingsPageCount, ProposalItemId, ClubMaxCount, InitialTierExcessRestockCurrency) — replace these compile-time consts with runtime lookups from the parsed ServerTableMetadata.ConstantsTable (or expose them as non-const properties that read the table) and leave only true invariants as const; update callers to read from ServerTableMetadata.ConstantsTable.<Get*>() or the equivalent accessor so runtime configuration controls these tunables instead of hard-coded values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs`:
- Around line 2152-2157: The loop over parser.ParseConstants() calls
constant.value.Trim() which can throw if constant.value is null or empty; before
calling Trim() guard the value (e.g., check string.IsNullOrWhiteSpace or use a
null-coalescing fallback) and handle the empty case (skip the entry or pass an
empty string) so CleanConstantsInput receives a safe string; update the foreach
block that references parser.ParseConstants(), constant.value,
CleanConstantsInput, propertyLookup and trimmedKey to perform this null/empty
check and proceed accordingly.
- Around line 2189-2225: The SetValue/HandleNonIConvertibleTypes flow must
defensively parse and avoid setting invalid values: update
HandleNonIConvertibleTypes to use TryParse variants for TimeSpan
(TimeSpan.TryParseExact/TryParse with CultureInfo), floats (float.TryParse with
CultureInfo), and numeric array element conversions (use appropriate TryParse
for elementType like int.TryParse/short.TryParse/long.TryParse/float.TryParse
/double.TryParse based on elementType) and return null or leave value unchanged
when any parse fails; ensure array element parsing skips/aborts on invalid
segments rather than throwing. In SetValue (method SetValue), after calling
HandleNonIConvertibleTypes(prop, ref value) re-check if value is null or
otherwise invalid and return early before calling prop.SetValue, and keep using
Convert.ChangeType only when value is confirmed valid and convertible.
In `@Maple2.Model/Metadata/NpcMetadata.cs`:
- Around line 21-24: The NpcMetadata record’s corpse field (NpcMetadataCorpse)
was omitted from the primary constructor parameters and from the copy/clone
constructors (e.g., the copy constructor NpcMetadata(NpcMetadata other, float
lastSightRadius)), causing corpse metadata to be dropped; update the NpcMetadata
record signature to include the corpse parameter (e.g., Corpse of type
NpcMetadataCorpse) and ensure every constructor and constructor-chaining
invocation (including NpcMetadata(NpcMetadata other, float lastSightRadius) and
other copy overloads around the shown blocks) passes other.Corpse through so the
corpse metadata is preserved on cloning/ingest.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Around line 146-148: The runtime fallback for NPC last-sight defaults is wrong
because Npc.cs currently checks LastSightRadius == 0 first, preventing the
height defaults from ever being applied; update the fallback logic in the Npc
class (where LastSightRadius, LastSightHeightUp, LastSightHeightDown are
initialized) to not short-circuit on radius—instead, apply per-field fallbacks:
if LastSightHeightUp == 0 set it to
ServerTableMetadata.ConstantsTable.NpcLastingSightHeightUp, if
LastSightHeightDown == 0 set it to
ServerTableMetadata.ConstantsTable.NpcLastingSightHeightDown, and if
LastSightRadius == 0 set it to
ServerTableMetadata.ConstantsTable.NpcLastingSightRadius (or combine into a
single initialization method that checks each property independently) so the
heights injected by callers like FieldNpc constructor are honored.
---
Duplicate comments:
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs`:
- Around line 2179-2184: CleanConstantsInput currently uses int.Parse and direct
indexing (input[0]) which can throw on malformed ingest data; update the
TimeSpan handling for property "UgcHomeSaleWaitingTime" to TryParse the input as
an integer (e.g., int.TryParse) before calling TimeSpan.FromSeconds and fallback
to a safe default or error path, and replace the unsafe input[0] check in the
int branch (type == typeof(int)) with a length check and/or TryParse to ensure
the string is non-empty and numeric before trimming a leading '0' (use char
checks or TryParse) so malformed inputs do not throw in CleanConstantsInput or
related methods.
---
Nitpick comments:
In `@Maple2.Model/Metadata/Constants.cs`:
- Around line 122-137: This file reintroduces runtime-tunable settings as
compile-time constants (e.g., MailQuestItems, MaxClosetMaxCount,
MaxClosetTabNameLength, BlockSize, SouthEast, NorthEast, NorthWest, SouthWest,
UGCShopProfitFee, UGCShopProfitDelayDays, PartyFinderListingsPageCount,
ProposalItemId, ClubMaxCount, InitialTierExcessRestockCurrency) — replace these
compile-time consts with runtime lookups from the parsed
ServerTableMetadata.ConstantsTable (or expose them as non-const properties that
read the table) and leave only true invariants as const; update callers to read
from ServerTableMetadata.ConstantsTable.<Get*>() or the equivalent accessor so
runtime configuration controls these tunables instead of hard-coded values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Maple2.File.Ingest/Mapper/NpcMapper.csMaple2.File.Ingest/Mapper/ServerTableMapper.csMaple2.Model/Metadata/Constants.csMaple2.Model/Metadata/NpcMetadata.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.csMaple2.Server.Game/Manager/Items/InventoryManager.csMaple2.Server.Game/Model/Field/Actor/FieldNpc.csMaple2.Server.Game/Session/GameSession.csMaple2.Server.World/Program.csMaple2.Server.World/WorldServer.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs
- Maple2.Server.Game/Manager/Items/InventoryManager.cs
- Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs
- Maple2.File.Ingest/Mapper/NpcMapper.cs
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
Outdated
Show resolved
Hide resolved
… back GlobalCubeSkillIntervalTime and merged into XML parsing functionality, while also adding back NpcMetadataCorpse parameter in NpcMetadata record.
…and it's much cleaner than what was being used to pass constant values for lasting sight radius, height up, and height down.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (3)
2152-2157:⚠️ Potential issue | 🟠 MajorGuard
constant.valuebeforeTrim()to avoid ingest crashes.Line 2156 calls
constant.value.Trim()directly; null values will throw and abort constants loading.Proposed fix
foreach ((string key, Constants.Key constant) in parser.ParseConstants()) { string trimmedKey = key.Trim(); if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) continue; + string rawValue = constant.value?.Trim() ?? string.Empty; + if (rawValue.Length == 0) continue; string cleanValue = CleanConstantsInput( - constant.value.Trim(), + rawValue, trimmedKey, property.PropertyType ); SetValue(property, constants, cleanValue); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs` around lines 2152 - 2157, The loop over parser.ParseConstants() calls constant.value.Trim() without null-check, which can throw; update the foreach handling in ServerTableMapper.cs (the block that uses parser.ParseConstants(), propertyLookup and CleanConstantsInput) to guard constant.value before trimming—either skip entries with null constant.value or coalesce to an empty string (e.g., use (constant.value ?? string.Empty) or an explicit null check) before calling Trim(), then pass the resulting non-null cleanValue into CleanConstantsInput so CleanConstantsInput never receives a null reference.
2189-2228:⚠️ Potential issue | 🟠 MajorDefensive conversion is incomplete in
SetValue/HandleNonIConvertibleTypes.
TimeSpan.Parse,float.Parse, andConvert.ChangeTypeon array elements can still throw on malformed inputs; alsoSetValuedoes not re-checkvalueafter normalization beforeprop.SetValue.Proposed fix
private void SetValue(PropertyInfo prop, object? obj, object? value) { if (obj == null && value == null || value == null) return; HandleNonIConvertibleTypes(prop, ref value); + if (value == null) return; bool isConvertible = typeof(IConvertible).IsAssignableFrom(prop.PropertyType); prop.SetValue(obj, isConvertible ? Convert.ChangeType(value, prop.PropertyType, CultureInfo.InvariantCulture) : value); } private object? HandleNonIConvertibleTypes(PropertyInfo prop, ref object? value) { if (value == null) return value; @@ - value = TimeSpan.Parse((string)value, CultureInfo.InvariantCulture); + if (!TimeSpan.TryParse((string) value, CultureInfo.InvariantCulture, out TimeSpan parsedTimeSpan)) { + value = null; + return value; + } + value = parsedTimeSpan; @@ - for (int i = 0; i < segments.Length; i++) { - object convertedValue = Convert.ChangeType(segments[i].Trim(), elementType); - destinationArray.SetValue(convertedValue, i); - } + for (int i = 0; i < segments.Length; i++) { + string segment = segments[i].Trim(); + try { + object convertedValue = Convert.ChangeType(segment, elementType, CultureInfo.InvariantCulture); + destinationArray.SetValue(convertedValue, i); + } catch { + value = null; + return value; + } + } @@ - value = new Vector3(float.Parse(parts[0], CultureInfo.InvariantCulture), - float.Parse(parts[1], CultureInfo.InvariantCulture), - float.Parse(parts[2], CultureInfo.InvariantCulture)); + if (!float.TryParse(parts[0], NumberStyles.Float, CultureInfo.InvariantCulture, out float x) || + !float.TryParse(parts[1], NumberStyles.Float, CultureInfo.InvariantCulture, out float y) || + !float.TryParse(parts[2], NumberStyles.Float, CultureInfo.InvariantCulture, out float z)) { + value = null; + return value; + } + value = new Vector3(x, y, z);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs` around lines 2189 - 2228, The SetValue/HandleNonIConvertibleTypes flow must defensively handle parse/convert failures and ensure value is safe before calling prop.SetValue: update HandleNonIConvertibleTypes to use TryParse (TimeSpan.TryParse/TryParseExact, float.TryParse with CultureInfo.InvariantCulture) and wrap Convert.ChangeType for array elements in try/catch or TryConvert logic (skip/assign defaults for malformed segments) and return null or the original value on failure; then in SetValue re-check value for null/invalid after calling HandleNonIConvertibleTypes and only call prop.SetValue when value is valid and convertible (use IsAssignableFrom/IConvertible check and try/catch around Convert.ChangeType to log or handle conversion exceptions). Ensure references include the methods SetValue and HandleNonIConvertibleTypes, checks on prop.PropertyType (TimeSpan, Vector3, IsArray), and use CultureInfo.InvariantCulture consistently.
2179-2185:⚠️ Potential issue | 🟠 Major
CleanConstantsInputstill has unsafe parsing/indexing paths.Line 2180 uses
int.Parse(input)and Line 2183 indexesinput[0]without empty/malformed checks, so bad constants can throw.Proposed fix
if (type == typeof(TimeSpan) && propName == "UgcHomeSaleWaitingTime") { - input = TimeSpan.FromSeconds(int.Parse(input)).ToString(); // TODO: may not be correct conversion to TimeSpan + if (!int.TryParse(input, NumberStyles.Integer, CultureInfo.InvariantCulture, out int seconds)) { + seconds = 0; + } + input = TimeSpan.FromSeconds(seconds).ToString("c", CultureInfo.InvariantCulture); } // Remove prefix 0 on integers since they do not convert properly -if (type == typeof(int) && input[0] == '0' && input.Length > 1) { - input = input.Remove(0, 1); +if (type == typeof(int) && input.Length > 1 && input[0] == '0') { + input = input.TrimStart('0'); + if (input.Length == 0) input = "0"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs` around lines 2179 - 2185, In CleanConstantsInput, guard the unsafe int.Parse and input[0] indexing: for the TimeSpan branch (type == typeof(TimeSpan) && propName == "UgcHomeSaleWaitingTime") use int.TryParse on input and only call TimeSpan.FromSeconds(parsed).ToString() when TryParse succeeds (otherwise leave input unchanged or handle error); for the integer branch (type == typeof(int)) first check that input is not null/empty and consists of digits (or use int.TryParse) and input.Length > 1 before checking input[0] and removing a leading '0' so malformed or empty constants cannot throw; reference the CleanConstantsInput method and the branches that check propName "UgcHomeSaleWaitingTime" and type == typeof(int).Maple2.Model/Metadata/ServerTable/ConstantsTable.cs (1)
6-959:⚠️ Potential issue | 🟠 MajorAdd ingest completeness validation to avoid silent bad defaults.
Line [6]-Line [959] relies on permissive defaults for almost every constant. In
Maple2.File.Ingest/Mapper/ServerTableMapper.cs:ParseConstants, unknown keys are skipped, so mapping drift can produce a “valid”ConstantsTablewith incorrect runtime values and no hard failure.🛡️ Suggested fail-fast/logging guard (in parser)
private ConstantsTable ParseConstants() { var constants = new ConstantsTable(); Dictionary<string, PropertyInfo> propertyLookup = typeof(ConstantsTable).GetProperties() .ToDictionary(p => p.Name.Trim(), p => p, StringComparer.OrdinalIgnoreCase); + HashSet<string> populated = new(StringComparer.OrdinalIgnoreCase); foreach ((string key, Constants.Key constant) in parser.ParseConstants()) { string trimmedKey = key.Trim(); - if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) continue; + if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) { + // log unknown key for ingest diagnostics + continue; + } string cleanValue = CleanConstantsInput( constant.value.Trim(), trimmedKey, property.PropertyType ); SetValue(property, constants, cleanValue); + populated.Add(property.Name); } + + // Fail fast on a minimal required set used by core flows. + string[] required = ["characterMaxLevel", "MaxBuddyCount", "MaxBlockCount"]; + string[] missing = required.Where(k => !populated.Contains(k)).ToArray(); + if (missing.Length > 0) { + throw new InvalidDataException($"Missing required constants: {string.Join(", ", missing)}"); + } return constants; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Metadata/ServerTable/ConstantsTable.cs` around lines 6 - 959, ParseConstants in ServerTableMapper.cs currently skips unknown keys and lets ConstantsTable be constructed with permissive defaults, causing silent mapping drift; update ParseConstants to perform ingest completeness validation by comparing parsed key set against the expected ConstantsTable field/property names (the constructor parameters for ConstantsTable) and fail-fast on discrepancies (missing required keys or unexpected/unmapped keys) by throwing or returning a clear error/log before returning the constructed ConstantsTable; ensure the validation references the ConstantsTable constructor parameter list or a hard-coded expected-key set so defaults cannot mask missing values and surface a meaningful error when mapping is incomplete.
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
221-224: Use a single constants source for quest portal spawn values.This method reads portal constants from
owner.Session.ServerTableMetadata.ConstantsTable, while nearby field-spawn logic reads fromFieldManager.ServerTableMetadata.ConstantsTable. Using one source inFieldManageravoids config-source drift for portal dimensions/lifetime/model.♻️ Proposed refactor
+ var constants = ServerTableMetadata.ConstantsTable; var portal = new Portal(NextLocalId(), metadata.MapId, metadata.PortalId, PortalType.Quest, PortalActionType.Interact, - npc.Position.Offset(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalDistanceFromNpc, npc.Rotation), npc.Rotation, - new Vector3(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalDistanceFromNpc, owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalDimensionY, - owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalDimensionZ), owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalDistanceFromNpc, + npc.Position.Offset(constants.QuestPortalDistanceFromNpc, npc.Rotation), npc.Rotation, + new Vector3(constants.QuestPortalDistanceFromNpc, constants.QuestPortalDimensionY, + constants.QuestPortalDimensionZ), constants.QuestPortalDistanceFromNpc, 0, true, false, true); var fieldPortal = new FieldQuestPortal(owner, this, NextLocalId(), portal) { Position = portal.Position, Rotation = portal.Rotation, - EndTick = (FieldTick + (long) TimeSpan.FromSeconds(owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepTime).TotalMilliseconds).Truncate32(), + EndTick = (FieldTick + (long) TimeSpan.FromSeconds(constants.QuestPortalKeepTime).TotalMilliseconds).Truncate32(), StartTick = FieldTickInt, - Model = owner.Session.ServerTableMetadata.ConstantsTable.QuestPortalKeepNif, + Model = constants.QuestPortalKeepNif, };Also applies to: 229-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs` around lines 221 - 224, The portal spawn code that constructs new Portal (the local variable portal) is reading quest portal constants from owner.Session.ServerTableMetadata.ConstantsTable; change these lookups to use FieldManager.ServerTableMetadata.ConstantsTable (the same ConstantsTable used by nearby field-spawn logic) for all quest portal parameters (distance, dimensions, lifetime/model etc.) in the Portal constructor sites (the block creating portal at lines around the Portal(...) call and the similar block at lines 229–231) so both field and portal logic use the same single constants source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Model/Metadata/ServerTable/ConstantsTable.cs`:
- Line 305: The code embeds hardcoded fallback values in ConstantsTable (e.g.,
the field timeResetDead initialized to 180 and similar literals at the other
locations), which should be removed so the canonical Server.m2d/constants.xml is
the single source of truth; replace the literal initializers in ConstantsTable
with logic that reads the values from the constants XML (use the existing
constants lookup/parsing helper used elsewhere in this class), and if a key is
missing either propagate an error or log and fail fast (do not silently keep the
hardcoded numeric like 180, 5000, -50, 100); update the fields (e.g.,
timeResetDead and the other fields with the mentioned literals) to be assigned
from the parsed constants dictionary (or validated via TryGet/HasKey) rather
than hardcoded defaults.
---
Duplicate comments:
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs`:
- Around line 2152-2157: The loop over parser.ParseConstants() calls
constant.value.Trim() without null-check, which can throw; update the foreach
handling in ServerTableMapper.cs (the block that uses parser.ParseConstants(),
propertyLookup and CleanConstantsInput) to guard constant.value before
trimming—either skip entries with null constant.value or coalesce to an empty
string (e.g., use (constant.value ?? string.Empty) or an explicit null check)
before calling Trim(), then pass the resulting non-null cleanValue into
CleanConstantsInput so CleanConstantsInput never receives a null reference.
- Around line 2189-2228: The SetValue/HandleNonIConvertibleTypes flow must
defensively handle parse/convert failures and ensure value is safe before
calling prop.SetValue: update HandleNonIConvertibleTypes to use TryParse
(TimeSpan.TryParse/TryParseExact, float.TryParse with
CultureInfo.InvariantCulture) and wrap Convert.ChangeType for array elements in
try/catch or TryConvert logic (skip/assign defaults for malformed segments) and
return null or the original value on failure; then in SetValue re-check value
for null/invalid after calling HandleNonIConvertibleTypes and only call
prop.SetValue when value is valid and convertible (use
IsAssignableFrom/IConvertible check and try/catch around Convert.ChangeType to
log or handle conversion exceptions). Ensure references include the methods
SetValue and HandleNonIConvertibleTypes, checks on prop.PropertyType (TimeSpan,
Vector3, IsArray), and use CultureInfo.InvariantCulture consistently.
- Around line 2179-2185: In CleanConstantsInput, guard the unsafe int.Parse and
input[0] indexing: for the TimeSpan branch (type == typeof(TimeSpan) && propName
== "UgcHomeSaleWaitingTime") use int.TryParse on input and only call
TimeSpan.FromSeconds(parsed).ToString() when TryParse succeeds (otherwise leave
input unchanged or handle error); for the integer branch (type == typeof(int))
first check that input is not null/empty and consists of digits (or use
int.TryParse) and input.Length > 1 before checking input[0] and removing a
leading '0' so malformed or empty constants cannot throw; reference the
CleanConstantsInput method and the branches that check propName
"UgcHomeSaleWaitingTime" and type == typeof(int).
In `@Maple2.Model/Metadata/ServerTable/ConstantsTable.cs`:
- Around line 6-959: ParseConstants in ServerTableMapper.cs currently skips
unknown keys and lets ConstantsTable be constructed with permissive defaults,
causing silent mapping drift; update ParseConstants to perform ingest
completeness validation by comparing parsed key set against the expected
ConstantsTable field/property names (the constructor parameters for
ConstantsTable) and fail-fast on discrepancies (missing required keys or
unexpected/unmapped keys) by throwing or returning a clear error/log before
returning the constructed ConstantsTable; ensure the validation references the
ConstantsTable constructor parameter list or a hard-coded expected-key set so
defaults cannot mask missing values and surface a meaningful error when mapping
is incomplete.
---
Nitpick comments:
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Around line 221-224: The portal spawn code that constructs new Portal (the
local variable portal) is reading quest portal constants from
owner.Session.ServerTableMetadata.ConstantsTable; change these lookups to use
FieldManager.ServerTableMetadata.ConstantsTable (the same ConstantsTable used by
nearby field-spawn logic) for all quest portal parameters (distance, dimensions,
lifetime/model etc.) in the Portal constructor sites (the block creating portal
at lines around the Portal(...) call and the similar block at lines 229–231) so
both field and portal logic use the same single constants source.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Maple2.File.Ingest/Mapper/ServerTableMapper.csMaple2.Model/Game/Npc/Npc.csMaple2.Model/Metadata/NpcMetadata.csMaple2.Model/Metadata/ServerTable/ConstantsTable.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
…sTable record pointed out by CodeRabbit.
…g used found by CodeRabbit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Model/Metadata/ServerTable/ConstantsTable.cs`:
- Around line 673-687: Add runtime validation for the bag-slot arrays in the
ConstantsTable record: ensure each array (bagSlotTabGameCount,
bagSlotTabSkinCount, bagSlotTabSummonCount, bagSlotTabMaterialCount,
bagSlotTabMasteryCount, bagSlotTabLifeCount, bagSlotTabQuestCount,
bagSlotTabGemCount, bagSlotTabPetCount, bagSlotTabActiveSkillCount,
bagSlotTabCoinCount, bagSlotTabBadgeCount, bagSlotTabMiscCount,
bagSlotTabLapenShardCount, bagSlotTabPieceCount) is non-null, has Length >= 2
and that element[0] (base) is >= 0 and <= element[1] (max); expose this as a
ValidateInvariants() method on the ConstantsTable record and call it from
ParseConstants() (in ServerTableMapper / Maple2.File.Ingest/Mapper) immediately
after mapping so malformed data throws a clear exception during load rather than
causing IndexOutOfRangeException at runtime (include descriptive exception
messages mentioning which array failed).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Model/Metadata/ServerTable/ConstantsTable.csMaple2.Server.Game/PacketHandlers/TaxiHandler.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/PacketHandlers/TaxiHandler.cs
Zintixx
requested changes
Mar 3, 2026
Maple2.Server.Login/PacketHandlers/CharacterManagementHandler.cs
Outdated
Show resolved
Hide resolved
…nally, remove Convert.ChangeType usage, create a reflection-based approach to access TryParse for generic objects.
Zintixx
requested changes
Mar 3, 2026
Zintixx
requested changes
Mar 6, 2026
Collaborator
AngeloTadeucci
left a comment
AngeloTadeucci
left a comment
There was a problem hiding this comment.
Outside of Zin's comments and some weird line breaks it looks good!
…aces other than just the initialization of StatAttributes.
…d line breaks per Tadeucci's feedback
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Maple2.Server.Game/PacketHandlers/MeretMarketHandler.cs (1)
202-204:⚠️ Potential issue | 🟠 MajorClamp relist prices with the same min/max as initial listings.
The first listing path clamps
price, but the relist path still writes the raw client value on Line 202. That leaves a relist-only path to persist out-of-range UGC prices.Suggested fix
- item.Price = price; + item.Price = Math.Clamp(price, Constants.UGCShopSellMinPrice, Constants.UGCShopSellMaxPrice); item.PromotionEndTime = promote ? DateTime.Now.AddHours(Constants.UGCShopAdHour).ToEpochSeconds() : 0; item.ListingEndTime = DateTime.Now.AddDays(Constants.UGCShopSaleDay).ToEpochSeconds();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/MeretMarketHandler.cs` around lines 202 - 204, The relist path is assigning the raw client price to item.Price (item.Price = price) without clamping, allowing out-of-range UGC prices; modify the relist logic in MeretMarketHandler (where item.Price is set on relist) to apply the same clamping used for initial listings (use the same min/max values or existing clamp helper) before assigning item.Price so relisted items cannot persist prices outside the allowed range.
🧹 Nitpick comments (4)
Maple2.Server.Game/Model/Field/Tombstone.cs (1)
11-15: Region name "Autofac Autowired" is misleading; ReSharper comment is unnecessary.This property isn't autowired by Autofac—it's simply a computed property navigating through object references. Additionally,
MemberCanBePrivate.Globalsuppression is redundant sinceConstantsis alreadyprivate.Consider simplifying:
♻️ Suggested simplification
- `#region` Autofac Autowired - // ReSharper disable MemberCanBePrivate.Global private ConstantsTable Constants => Owner.Session.ServerTableMetadata.ConstantsTable; - // ReSharper restore All - `#endregion`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Model/Field/Tombstone.cs` around lines 11 - 15, The region name "Autofac Autowired" and the surrounding ReSharper suppression are misleading and unnecessary for the private computed property Constants; remove the region block and the ReSharper comments and keep a simple private Constants property that returns Owner.Session.ServerTableMetadata.ConstantsTable (i.e., delete the "#region Autofac Autowired", the "// ReSharper ..." comments and "#endregion" so the file contains just the private Constants => Owner.Session.ServerTableMetadata.ConstantsTable declaration).Maple2.File.Ingest/Utils/GenericHelper.cs (2)
20-20: Return value is never used.The method modifies
valuevia therefparameter, but the return value is never consumed by the caller. Consider changing the return type tovoidfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Utils/GenericHelper.cs` at line 20, The method HandleNonIConvertibleTypes currently returns object? but callers ignore that value; change its signature from "private static object? HandleNonIConvertibleTypes(PropertyInfo prop, ref object? value)" to "private static void HandleNonIConvertibleTypes(PropertyInfo prop, ref object? value)", remove any return expressions inside the method (instead just set ref value as needed) and update any internal logic that assumed a return to rely on the ref parameter; this keeps callers unchanged while making the intent clear.
9-9: Simplify redundant null check.The condition
(obj == null && value == null) || value == nullis logically equivalent to justvalue == null. The first clause is redundant.♻️ Suggested simplification
- if (obj == null && value == null || value == null) return; + if (value == null) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Utils/GenericHelper.cs` at line 9, The null-check is redundant: replace the condition checking "(obj == null && value == null) || value == null" with a single check for "value == null" in the method inside the GenericHelper class where the variables obj and value are used; update the if-statement so it simply returns when value is null to remove the unnecessary clause and keep behavior identical.Maple2.Model/Metadata/Constants.cs (1)
207-212: Avoid keepingNpcLastSight*as a second source of truth.These values are already ingested from server metadata, so leaving hardcoded copies here means runtime NPC sight behavior can drift from the parsed data. It would be better to route the remaining consumer through the metadata path, or fail fast if that metadata is unavailable.
Based on learnings, Metadata is read-only game data loaded from XML at startup, so these runtime defaults should ideally come from that single metadata source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Metadata/Constants.cs` around lines 207 - 212, The three hardcoded constants NpcLastSightRadius, NpcLastSightHeightUp, and NpcLastSightHeightDown in Constants.cs are a second source of truth; remove these constants and change callers to fetch the values from the runtime metadata path (use the NpcMetadataDistance/Metadata read-only XML-loaded structure used for server table/constants.xml) and if that metadata entry is missing, fail fast or throw a clear exception so consumers cannot silently use stale defaults; update any code referencing NpcLastSight* to call the metadata accessor (or a small wrapper method that returns the metadata values and throws if absent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs`:
- Around line 2152-2156: The loop over parser.ParseConstants() silently skips
unknown or renamed keys causing missing configuration; change behavior in the
foreach that uses parser.ParseConstants(), propertyLookup, CleanInput and
GenericHelper.SetValue so that when propertyLookup.TryGetValue(trimmedKey, out
PropertyInfo? property) returns false you do not continue silently but instead
record the unmapped trimmedKey (and original constant.value) and fail ingest:
either throw a descriptive exception listing all unmapped keys after parsing or
log an error and abort processing, ensuring the error message names the missing
constant keys and the related ConstantsTable target so ingest fails rather than
producing silent defaults.
In `@Maple2.File.Ingest/Utils/GenericHelper.cs`:
- Around line 40-47: The Vector3 parsing block in GenericHelper.cs currently
returns the original string when parts.Length != 3 which causes SetValue to
later try to assign a string to a Vector3 property; change the error handling so
malformed input is handled explicitly: inside the prop.PropertyType ==
typeof(Vector3) branch, validate parts.Length and on mismatch either throw a
clear ArgumentException/FormatException (including the offending value and
prop.Name) or assign a safe default (e.g., Vector3.zero) and log the error
before returning; ensure SetValue receives a Vector3 (not the original string)
in all code paths.
- Line 81: Fix the typo in the XML/comment inside GenericHelper.cs: replace
"CulturueInfo" with the correct "CultureInfo" in the fallback comment (the
comment near the GenericHelper class/method that reads "// Fallback without
CulturueInfo provided, in case the type does not have a CultureInfo overload.").
- Around line 33-36: The loop currently calls TryParseObject and then does
destinationArray.SetValue(parseResult ?? default, i), which passes null into
value-type arrays; change it to compute a type-appropriate default when parsing
fails (e.g., var defaultValue = elementType.IsValueType ?
Activator.CreateInstance(elementType) : null) and then call
destinationArray.SetValue(parseResult ?? defaultValue, i). Use the existing
TryParseObject, elementType, destinationArray and segments identifiers so the
fix is localized to the loop.
In `@Maple2.Server.Game/Commands/PlayerCommand.cs`:
- Around line 136-138: The error branches that validate the player's level and
prestige (the if that checks "level < 1 || level > Constants.characterMaxLevel"
and the analogous prestige check) currently write to ctx.Console.Error but don't
set ctx.ExitCode; update both branches to set ctx.ExitCode = 1 before returning
so the process signals failure to callers (ensure you add the same ctx.ExitCode
= 1 in both the level validation block and the prestige validation block).
In `@Maple2.Server.World/Containers/PartyManager.cs`:
- Around line 405-407: Add a new constant Constants.PartyVoteKickDurationSeconds
to the ConstantsTable and replace the incorrect use of
Constants.PartyVoteReadyDurationSeconds inside the Task.Factory.StartNew lambda
in PartyManager (the Thread.Sleep call used for vote kick timeout) so the
vote-kick timeout uses the new PartyVoteKickDurationSeconds constant instead of
PartyVoteReadyDurationSeconds.
---
Duplicate comments:
In `@Maple2.Server.Game/PacketHandlers/MeretMarketHandler.cs`:
- Around line 202-204: The relist path is assigning the raw client price to
item.Price (item.Price = price) without clamping, allowing out-of-range UGC
prices; modify the relist logic in MeretMarketHandler (where item.Price is set
on relist) to apply the same clamping used for initial listings (use the same
min/max values or existing clamp helper) before assigning item.Price so relisted
items cannot persist prices outside the allowed range.
---
Nitpick comments:
In `@Maple2.File.Ingest/Utils/GenericHelper.cs`:
- Line 20: The method HandleNonIConvertibleTypes currently returns object? but
callers ignore that value; change its signature from "private static object?
HandleNonIConvertibleTypes(PropertyInfo prop, ref object? value)" to "private
static void HandleNonIConvertibleTypes(PropertyInfo prop, ref object? value)",
remove any return expressions inside the method (instead just set ref value as
needed) and update any internal logic that assumed a return to rely on the ref
parameter; this keeps callers unchanged while making the intent clear.
- Line 9: The null-check is redundant: replace the condition checking "(obj ==
null && value == null) || value == null" with a single check for "value == null"
in the method inside the GenericHelper class where the variables obj and value
are used; update the if-statement so it simply returns when value is null to
remove the unnecessary clause and keep behavior identical.
In `@Maple2.Model/Metadata/Constants.cs`:
- Around line 207-212: The three hardcoded constants NpcLastSightRadius,
NpcLastSightHeightUp, and NpcLastSightHeightDown in Constants.cs are a second
source of truth; remove these constants and change callers to fetch the values
from the runtime metadata path (use the NpcMetadataDistance/Metadata read-only
XML-loaded structure used for server table/constants.xml) and if that metadata
entry is missing, fail fast or throw a clear exception so consumers cannot
silently use stale defaults; update any code referencing NpcLastSight* to call
the metadata accessor (or a small wrapper method that returns the metadata
values and throws if absent).
In `@Maple2.Server.Game/Model/Field/Tombstone.cs`:
- Around line 11-15: The region name "Autofac Autowired" and the surrounding
ReSharper suppression are misleading and unnecessary for the private computed
property Constants; remove the region block and the ReSharper comments and keep
a simple private Constants property that returns
Owner.Session.ServerTableMetadata.ConstantsTable (i.e., delete the "#region
Autofac Autowired", the "// ReSharper ..." comments and "#endregion" so the file
contains just the private Constants =>
Owner.Session.ServerTableMetadata.ConstantsTable declaration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f34138f-3b4e-4f4e-b101-4da6e5bb23e8
📒 Files selected for processing (44)
Maple2.File.Ingest/Mapper/ServerTableMapper.csMaple2.File.Ingest/Utils/GenericHelper.csMaple2.Model/Metadata/Constants.csMaple2.Server.Game/Commands/PlayerCommand.csMaple2.Server.Game/Manager/BlackMarketManager.csMaple2.Server.Game/Manager/BuddyManager.csMaple2.Server.Game/Manager/BuffManager.csMaple2.Server.Game/Manager/ConfigManager.csMaple2.Server.Game/Manager/CurrencyManager.csMaple2.Server.Game/Manager/ExperienceManager.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.csMaple2.Server.Game/Manager/FishingManager.csMaple2.Server.Game/Manager/ItemMergeManager.csMaple2.Server.Game/Manager/Items/InventoryManager.csMaple2.Server.Game/Manager/Items/StorageManager.csMaple2.Server.Game/Manager/SkillManager.csMaple2.Server.Game/Manager/TradeManager.csMaple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.csMaple2.Server.Game/Model/Field/Actor/FieldPet.csMaple2.Server.Game/Model/Field/Actor/FieldPlayer.csMaple2.Server.Game/Model/Field/Tombstone.csMaple2.Server.Game/PacketHandlers/BeautyHandler.csMaple2.Server.Game/PacketHandlers/ClubHandler.csMaple2.Server.Game/PacketHandlers/FunctionCubeHandler.csMaple2.Server.Game/PacketHandlers/GuildHandler.csMaple2.Server.Game/PacketHandlers/HomeBankHandler.csMaple2.Server.Game/PacketHandlers/HomeDoctorHandler.csMaple2.Server.Game/PacketHandlers/ItemLockHandler.csMaple2.Server.Game/PacketHandlers/JobHandler.csMaple2.Server.Game/PacketHandlers/MeretMarketHandler.csMaple2.Server.Game/PacketHandlers/MesoMarketHandler.csMaple2.Server.Game/PacketHandlers/NpcTalkHandler.csMaple2.Server.Game/PacketHandlers/PartyHandler.csMaple2.Server.Game/PacketHandlers/QuestHandler.csMaple2.Server.Game/PacketHandlers/SystemShopHandler.csMaple2.Server.Game/PacketHandlers/TaxiHandler.csMaple2.Server.Game/PacketHandlers/UserChatHandler.csMaple2.Server.Login/PacketHandlers/CharacterManagementHandler.csMaple2.Server.World/Containers/GlobalPortalManager.csMaple2.Server.World/Containers/PartyLookup.csMaple2.Server.World/Containers/PartyManager.csMaple2.Server.World/Program.csMaple2.Server.World/WorldServer.cs
✅ Files skipped from review due to trivial changes (1)
- Maple2.Server.World/Containers/GlobalPortalManager.cs
🚧 Files skipped from review as they are similar to previous changes (16)
- Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs
- Maple2.Server.World/Program.cs
- Maple2.Server.Game/Manager/ItemMergeManager.cs
- Maple2.Server.Game/PacketHandlers/HomeBankHandler.cs
- Maple2.Server.Game/Manager/BuffManager.cs
- Maple2.Server.Game/Manager/FishingManager.cs
- Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs
- Maple2.Server.Game/Manager/TradeManager.cs
- Maple2.Server.Game/PacketHandlers/NpcTalkHandler.cs
- Maple2.Server.Game/Manager/BuddyManager.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs
- Maple2.Server.Game/Manager/BlackMarketManager.cs
- Maple2.Server.Game/PacketHandlers/UserChatHandler.cs
- Maple2.Server.Game/Manager/SkillManager.cs
- Maple2.Server.Game/PacketHandlers/SystemShopHandler.cs
Comment on lines
+2152
to
+2156
| foreach ((string key, Constants.Key constant) in parser.ParseConstants()) { | ||
| string trimmedKey = key.Trim(); | ||
| if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) continue; | ||
| string cleanValue = CleanInput(constant.value.Trim(), trimmedKey, property.PropertyType); | ||
| GenericHelper.SetValue(property, constants, cleanValue); |
Contributor
There was a problem hiding this comment.
Don't silently drop constants that fail to map.
Line 2154 currently turns an unknown or renamed constants.xml key into a quiet default on ConstantsTable. With runtime code now consuming this table for inventory sizes, delete timers, name limits, and price bounds, that becomes a late runtime failure or wrong gameplay config instead of an ingest-time failure.
Suggested fix
+ var unmappedKeys = new List<string>();
foreach ((string key, Constants.Key constant) in parser.ParseConstants()) {
string trimmedKey = key.Trim();
- if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) continue;
+ if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) {
+ unmappedKeys.Add(trimmedKey);
+ continue;
+ }
string cleanValue = CleanInput(constant.value.Trim(), trimmedKey, property.PropertyType);
GenericHelper.SetValue(property, constants, cleanValue);
}
+ if (unmappedKeys.Count > 0) {
+ throw new InvalidOperationException($"Unmapped constants: {string.Join(", ", unmappedKeys)}");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs` around lines 2152 - 2156, The
loop over parser.ParseConstants() silently skips unknown or renamed keys causing
missing configuration; change behavior in the foreach that uses
parser.ParseConstants(), propertyLookup, CleanInput and GenericHelper.SetValue
so that when propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)
returns false you do not continue silently but instead record the unmapped
trimmedKey (and original constant.value) and fail ingest: either throw a
descriptive exception listing all unmapped keys after parsing or log an error
and abort processing, ensuring the error message names the missing constant keys
and the related ConstantsTable target so ingest fails rather than producing
silent defaults.
Comment on lines
+33
to
+36
| for (int i = 0; i < segments.Length; i++) { | ||
| TryParseObject(elementType, segments[i].Trim(), out object? parseResult); | ||
| destinationArray.SetValue(parseResult ?? default, i); | ||
| } |
Contributor
There was a problem hiding this comment.
Array element parse failure will throw at runtime for value type arrays.
If TryParseObject fails (returns false), parseResult is null. The expression parseResult ?? default evaluates to null (since default here is default(object?)). Calling destinationArray.SetValue(null, i) on a value type array (e.g., int[]) throws ArgumentException.
Consider using the element type's default value on parse failure:
🔧 Proposed fix
for (int i = 0; i < segments.Length; i++) {
- TryParseObject(elementType, segments[i].Trim(), out object? parseResult);
- destinationArray.SetValue(parseResult ?? default, i);
+ if (TryParseObject(elementType, segments[i].Trim(), out object? parseResult)) {
+ destinationArray.SetValue(parseResult, i);
+ } else {
+ destinationArray.SetValue(elementType.IsValueType ? Activator.CreateInstance(elementType) : null, i);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (int i = 0; i < segments.Length; i++) { | |
| TryParseObject(elementType, segments[i].Trim(), out object? parseResult); | |
| destinationArray.SetValue(parseResult ?? default, i); | |
| } | |
| for (int i = 0; i < segments.Length; i++) { | |
| if (TryParseObject(elementType, segments[i].Trim(), out object? parseResult)) { | |
| destinationArray.SetValue(parseResult, i); | |
| } else { | |
| destinationArray.SetValue(elementType.IsValueType ? Activator.CreateInstance(elementType) : null, i); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Ingest/Utils/GenericHelper.cs` around lines 33 - 36, The loop
currently calls TryParseObject and then does
destinationArray.SetValue(parseResult ?? default, i), which passes null into
value-type arrays; change it to compute a type-appropriate default when parsing
fails (e.g., var defaultValue = elementType.IsValueType ?
Activator.CreateInstance(elementType) : null) and then call
destinationArray.SetValue(parseResult ?? defaultValue, i). Use the existing
TryParseObject, elementType, destinationArray and segments identifiers so the
fix is localized to the loop.
Comment on lines
+40
to
+47
| if (prop.PropertyType == typeof(Vector3)) { | ||
| string[] parts = ((string) value).Split(','); | ||
| if (parts.Length != 3) return value; | ||
| float.TryParse(parts[0], CultureInfo.InvariantCulture, out float x); | ||
| float.TryParse(parts[1], CultureInfo.InvariantCulture, out float y); | ||
| float.TryParse(parts[2], CultureInfo.InvariantCulture, out float z); | ||
| value = new Vector3(x, y, z); | ||
| } |
Contributor
There was a problem hiding this comment.
Vector3 parsing silently leaves original string value on malformed input.
When parts.Length != 3, the method returns early without modifying value. The caller (SetValue) then attempts to assign the original string to a Vector3 property, which will throw. Consider logging or handling this case more explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Ingest/Utils/GenericHelper.cs` around lines 40 - 47, The Vector3
parsing block in GenericHelper.cs currently returns the original string when
parts.Length != 3 which causes SetValue to later try to assign a string to a
Vector3 property; change the error handling so malformed input is handled
explicitly: inside the prop.PropertyType == typeof(Vector3) branch, validate
parts.Length and on mismatch either throw a clear
ArgumentException/FormatException (including the offending value and prop.Name)
or assign a safe default (e.g., Vector3.zero) and log the error before
returning; ensure SetValue receives a Vector3 (not the original string) in all
code paths.
| return success; | ||
| } | ||
|
|
||
| // Fallback without CulturueInfo provided, in case the type does not have a CultureInfo overload. |
Contributor
There was a problem hiding this comment.
Typo in comment: "CulturueInfo" → "CultureInfo".
- // Fallback without CulturueInfo provided, in case the type does not have a CultureInfo overload.
+ // Fallback without CultureInfo provided, in case the type does not have a CultureInfo overload.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fallback without CulturueInfo provided, in case the type does not have a CultureInfo overload. | |
| // Fallback without CultureInfo provided, in case the type does not have a CultureInfo overload. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Ingest/Utils/GenericHelper.cs` at line 81, Fix the typo in the
XML/comment inside GenericHelper.cs: replace "CulturueInfo" with the correct
"CultureInfo" in the fallback comment (the comment near the GenericHelper
class/method that reads "// Fallback without CulturueInfo provided, in case the
type does not have a CultureInfo overload.").
Comment on lines
+136
to
138
| if (level < 1 || level > Constants.characterMaxLevel) { | ||
| ctx.Console.Error.WriteLine($"Invalid level: {level}. Must be between 1 and {Constants.characterMaxLevel}."); | ||
| return; |
Contributor
There was a problem hiding this comment.
Set a failure exit code on rejected level input.
Both branches print an error and return, but they never set ctx.ExitCode = 1, so invalid player level / player prestige invocations can still look successful to scripts.
Suggested fix
if (level < 1 || level > Constants.characterMaxLevel) {
ctx.Console.Error.WriteLine($"Invalid level: {level}. Must be between 1 and {Constants.characterMaxLevel}.");
+ ctx.ExitCode = 1;
return;
}
@@
if (level < 1 || level > Constants.AdventureLevelLimit) {
ctx.Console.Error.WriteLine($"Invalid level: {level}. Must be between 1 and {Constants.AdventureLevelLimit}.");
+ ctx.ExitCode = 1;
return;
}Also applies to: 206-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Commands/PlayerCommand.cs` around lines 136 - 138, The
error branches that validate the player's level and prestige (the if that checks
"level < 1 || level > Constants.characterMaxLevel" and the analogous prestige
check) currently write to ctx.Console.Error but don't set ctx.ExitCode; update
both branches to set ctx.ExitCode = 1 before returning so the process signals
failure to callers (ensure you add the same ctx.ExitCode = 1 in both the level
validation block and the prestige validation block).
Comment on lines
405
to
+407
| Task.Factory.StartNew(() => { | ||
| // TODO: The duration is wrong. | ||
| Thread.Sleep(TimeSpan.FromSeconds(Constant.PartyVoteReadyDurationSeconds)); | ||
| Thread.Sleep(TimeSpan.FromSeconds(Constants.PartyVoteReadyDurationSeconds)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for vote kick related constants in metadata or constants files
rg -i "votekick|vote.*kick" --type-add 'xml:*.xml' --type xml
rg -i "votekick|vote.*kick" --type csRepository: MS2Community/Maple2
Length of output: 3517
🏁 Script executed:
# Find and examine the PartyManager.cs file around lines 405-407
fd "PartyManager.cs" -x cat -n {} | sed -n '400,410p'Repository: MS2Community/Maple2
Length of output: 706
🏁 Script executed:
# Search for Constants definitions related to Party and Duration
rg "PartyVote|Duration.*Seconds" --type cs -B 2 -A 2 | head -50Repository: MS2Community/Maple2
Length of output: 3736
🏁 Script executed:
# Find Constants or ConstantsTable files
fd -e cs | xargs rg "class Constants|class.*ConstantsTable" -lRepository: MS2Community/Maple2
Length of output: 47
🏁 Script executed:
# Find ConstantsTable definition
rg "class ConstantsTable" --type cs -A 30Repository: MS2Community/Maple2
Length of output: 45
🏁 Script executed:
# Search for all duration-related properties in Constants
rg "PartyVote.*Duration|Duration.*Party" --type csRepository: MS2Community/Maple2
Length of output: 765
🏁 Script executed:
# Look at the broader context of the PartyManager VoteKick method
fd "PartyManager.cs" -path "*/Maple2.Server.World/*" -x cat -n | sed -n '390,425p'Repository: MS2Community/Maple2
Length of output: 231
🏁 Script executed:
# Read ConstantsTable.cs to see all Party-related duration constants
cat -n Maple2.Model/Metadata/ServerTable/ConstantsTable.cs | grep -i "party\|duration" -A 1 -B 1Repository: MS2Community/Maple2
Length of output: 5691
🏁 Script executed:
# Get the exact context of PartyManager.cs VoteKick method
fd "PartyManager.cs" -path "*/Maple2.Server.World/Containers/*" -x cat -n | sed -n '375,425p'Repository: MS2Community/Maple2
Length of output: 231
🏁 Script executed:
# Get the exact VoteKick method code
cat -n Maple2.Server.World/Containers/PartyManager.cs | sed -n '375,425p'Repository: MS2Community/Maple2
Length of output: 2050
Create a separate PartyVoteKickDurationSeconds constant for vote kick timeout.
The TODO at line 406 is valid: PartyVoteReadyDurationSeconds is semantically incorrect for vote kick duration. Currently, both ready check and kick votes reuse the same timeout constant. A separate PartyVoteKickDurationSeconds should be added to ConstantsTable and used here instead of PartyVoteReadyDurationSeconds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.World/Containers/PartyManager.cs` around lines 405 - 407, Add a
new constant Constants.PartyVoteKickDurationSeconds to the ConstantsTable and
replace the incorrect use of Constants.PartyVoteReadyDurationSeconds inside the
Task.Factory.StartNew lambda in PartyManager (the Thread.Sleep call used for
vote kick timeout) so the vote-kick timeout uses the new
PartyVoteKickDurationSeconds constant instead of PartyVoteReadyDurationSeconds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove hardcoded server constants and merge constants from Xml.m2d that were not present in Server.m2d into Server.m2d.
Solves #317.
Summary by CodeRabbit
Release Notes
New Features
Refactoring