fix: DeleteOutput parsing, late load support, config reload cache - #34
Open
Rushaway wants to merge 3 commits into
Open
fix: DeleteOutput parsing, late load support, config reload cache#34Rushaway wants to merge 3 commits into
Rushaway wants to merge 3 commits into
Conversation
- RestoreLevel(): fix `DeleteOutput` entries that specify a target and input (e.g. "m_OnUser4 score10,ApplyScore"). FindCharInString() returns an offset relative to the `sValue[Target]` substring, but it was used as an absolute index, corrupting the output-name portion of the buffer. The parameter guard also re-tested `Input` instead of the parameter offset (copy/paste bug), so it never took the "no parameter" path. Only the output-only and output+target forms worked before. - Support late loads / `sm plugins reload`: AskPluginLoad2() received `late` but ignored it, and OnMapStart() (the only place the config loads) does not fire until the next map. Store the flag and load on OnPluginStart() when late. - Split config loading into LoadMapConfig() so `sm_savelevel_reload` no longer wipes the saved-level cache (OnMapStart() recreating g_PlayerLevels dropped levels of players who already disconnected). The reload command now reports whether a config was actually loaded instead of always claiming success. - GetLevel(): guard the `math` entry parser against a missing comma so a malformed config line skips that entry instead of triggering a negative array-index runtime error that aborts level detection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Failed config reloads must preserve the last known-good configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes SaveLevel parsing and improves late-load, config reload, and malformed-config handling.
Changes:
- Corrects
DeleteOutputparsing and malformedmathhandling. - Adds late-load support and centralized config loading.
- Preserves the player cache during reloads and reports failures.
File summaries
| File | Summary | Findings |
|---|---|---|
addons/sourcemod/scripting/SaveLevel.sp |
Updates parsing, loading, and reload behavior. | Moderate (2 votes): failed reloads can discard the active configuration instead of preserving the last known-good config. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Removed unused late load variable and related logic.
LoadMapConfig() unconditionally deleted g_Config before attempting to load the replacement file. If the file was missing or failed to parse, g_Config was left null for the rest of the map even though Command_ReloadConfig() reported failure, silently disabling level saving/restoration. Load into a temporary KeyValues and only swap it into g_Config once it has been fully validated. A new `keepOnFailure` parameter distinguishes the two callers: OnMapStart() (false) still clears g_Config on failure, since the previous map's config is meaningless on a new map; the reload admin command (true) now leaves the active configuration untouched when the reload itself fails. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Late-load and plugin-reload support remains incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
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
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.
Summary
Fixes a parsing bug that breaks a shipped map config, plus two robustness/UX improvements around config (re)loading.
1.
DeleteOutputwith a target/input is broken (bug)In
RestoreLevel(),DeleteOutputvalues are"<output> <target>[,<input>[,<parameter>]]". The parser:FindCharInString(sValue[Target], ','), which returns an offset relative to thesValue[Target]substring, then uses it as an absolute index (sValue[Input] = 0). This writes the terminator into the middle of the output name.if(Input == -1)— a copy/paste of the previous check.Inputis already known non-negative here, so the "target + input, no parameter" branch was never reachable.Net effect: only the
output-only andoutput + targetforms ever worked."DeleteOutput" "m_OnUser4 score10,ApplyScore"inZE_FFVII_Mako_Reactor_V6_B08.cfgtruncates the output name and deletes the wrong thing (or nothing).Fix: make the offsets absolute consistently and test the parameter offset.
2. No late-load /
sm plugins reloadsupport (improvement)AskPluginLoad2()takeslatebut ignores it, andOnMapStart()is the only place the map config is loaded. Reloading the plugin mid-map (or any late load) leaves every map "not supported" until the next map change.Fix: store the flag and call the loader from
OnPluginStart()when late (guarded so it does not double-run on a normal boot).3.
sm_savelevel_reloadwipes the saved-level cache and always claims success (bug)Command_ReloadConfigcalledOnMapStart(), which doesdelete g_PlayerLevels; g_PlayerLevels = new StringMap();. Reloading a config therefore drops the saved levels of players who already disconnected. It also printed "Map config file has been reloaded." even when the file was missing or failed to parse.Fix: config loading moves into
LoadMapConfig()(only touchesg_Config).OnMapStart()still resets the cache (new map = stale data). The reload command reports the real result. This also matches the architecture already described in.github/copilot-instructions.md, which references aLoadMapConfig()that did not exist.4. Malformed
mathconfig line → runtime error (minor)In
GetLevel(), amathentry missing a comma makesFindCharInStringreturn-1, thensValue[-1] = 0throws an array-index runtime error that aborts level detection for the whole map. Added a guard so the bad entry is skipped.Testing
spcomp1.12.x) to compile.DeleteOutputparser by hand against everyDeleteOutputline inaddons/sourcemod/configs/savelevel/(m_OnUser1 leveling_counter,m_OnUser4 score10,ApplyScore,m_OnUser2 leveling_counter,m_OnUser3 map_wandlevels): output-only and output+target behaviour is unchanged, output+target+input now resolves correctly.🤖 Generated with Claude Code