fix(core): harden config init, dedupe weapon tracking, style cleanup - #27
Merged
Merged
Conversation
- Move cleanup timer creation from OnMapStart to OnPluginStart so lifetime-based cleanup keeps working when the plugin is loaded or reloaded mid-map. OnMapStart/OnMapEnd are not fired for late-loaded plugins, so previously the timer never started until the next map change. It is now a single repeating timer for the plugin lifetime, freed in OnPluginEnd. - Read sm_weaponcleaner_max / sm_weaponcleaner_lifetime in OnConfigsExecuted instead of relying on the ConVar change hook happening to fire during AutoExecConfig. - Cache mp_freezetime once instead of calling FindConVar() every round_start, and guard against it being missing (avoids a native error on games without the ConVar). - Guard InsertWeapon against tracking the same weapon entity twice, which could leak a tracking slot and evict other weapons early (e.g. OnClientDisconnect simulating a drop that the engine also reports). - Bump version to 2.2.5 and refresh copilot-instructions.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The cleanup timer is still created without a no-mapchange flag, so it will be destroyed on map change and won’t be recreated after OnMapStart was removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes small, targeted fixes to WeaponCleaner’s initialization and weapon cleanup behavior, aiming to make lifetime-based cleanup and round-start logic more robust—especially when the plugin is loaded mid-map.
Changes:
- Move the repeating cleanup timer creation into
OnPluginStartand addOnPluginEndcleanup. - Read effective ConVar values after config execution via
OnConfigsExecuted(), and cache/guardmp_freezetime. - Prevent
InsertWeaponfrom tracking the same weapon entity reference twice; update Copilot instructions/version notes.
File summaries
| File | Description |
|---|---|
| addons/sourcemod/scripting/WeaponCleaner.sp | Adjusts timer lifecycle, config initialization timing, freeze-time lookup safety, and weapon tracking dedupe. |
| .github/copilot-instructions.md | Updates repository documentation (versions, tree, and modernization notes). |
Review details
- Files reviewed: 2/2 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.
…nk logic
- Retry FindConVar("mp_freezetime") in Event_RoundStart if the cached
handle is still null, so a late-registered convar isn't stuck at 0
freeze time for the plugin's whole lifetime.
- Extract the max-weapons shrink-and-kill logic into ApplyMaxWeapons()
and use it from both OnConVarChanged and OnConfigsExecuted, so the
two call sites can't drift out of sync.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SourceMod calls OnMapStart (and replays OnClientPutInServer) for plugins loaded mid-map, so the cleanup timer was never actually missing on late load. Move it back to OnMapStart/OnMapEnd instead of OnPluginStart/OnPluginEnd. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rushaway
changed the title
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
While reviewing the plugin I found a few bugs and rough edges. This PR fixes them with minimal, surgical changes.
1. Config values read before the config is executed (bug / fragility)
g_MaxWeaponsandg_MaxWeaponLifetimewere read fromConVar.IntValueimmediately afterCreateConVar, i.e. beforeAutoExecConfigruns. It only happened to work because the change hook fires when the generated cfg is executed. AddedOnConfigsExecuted()to read the effective values at the correct time (also runs on late load). TheOnPluginStartreads are kept as a safe default for the tiny window in between.2.
FindConVar("mp_freezetime")every round + no null guardEvent_RoundStartlooked upmp_freezetimeevery round and passed the result straight toGetConVarInt, which throws a native error if the ConVar does not exist. Now cached once inOnPluginStart, self-healed (re-looked-up) if it was still null (e.g. queried before the engine registered it), and null-checked before use. The cached handle stays valid for the plugin's lifetime —mp_freezetimeis a permanent engine convar, so its current value is always read live from.IntValueat eachround_start, including when a map (e.g. a ze-style multi-stage map) changes the cvar's value between rounds.3.
InsertWeaponcould track the same entity twice (bug)Nothing stopped the same weapon entref being inserted twice (e.g.
OnClientDisconnectsimulates a drop that the engine may also report viaSDKHook_WeaponDropPost).RemoveWeapononly clears the first match, so the duplicate leaks a tracking slot and can cause other weapons to be evicted early when the list looks "full". Added a dedupe check in the existing scan loop (no extra iteration cost).4. Max-weapons shrink logic deduplicated
Extracted the max-weapons shrink-and-kill logic into
ApplyMaxWeapons()and used it from bothOnConVarChangedandOnConfigsExecuted, so the two call sites can't drift out of sync.5. Housekeeping
2.2.4→2.2.5.if (/for (spacing throughout..github/copilot-instructions.md: fixed stale version numbers, the project tree, and the "Modernization Opportunities" section (theCloseHandle→deletemigration it described is already done).Testing
spcompavailable); relying on CI.Notes / not changed
OnMapStart/OnMapEndtimer setup is unchanged: SourceMod callsOnMapStart(and replaysOnClientPutInServer) for plugins loaded mid-map, so the repeating cleanup timer already starts correctly on a late load — no fix was needed there.OnWeaponSpawned→OnWeaponSpawnedPostis a redundant pass-through and the naming hints it may have been meant asSDKHook_SpawnPost. Left as-is to avoid a behavioural change without a test server.OnClientDisconnectiterates weapon slots0..4(magic number); left untouched.🤖 Generated with Claude Code