Safer entity iteration, real success reporting, and better feedback for ForceInput - #24
Open
Rushaway wants to merge 1 commit into
Open
Safer entity iteration, real success reporting, and better feedback for ForceInput#24Rushaway wants to merge 1 commit into
Rushaway wants to merge 1 commit into
Conversation
…er feedback - Snapshot matching entities into an ArrayList before firing inputs. Firing an input while iterating FindEntityByClassname() is unsafe because an input can create or remove entities mid-iteration; this could crash the server or make the command act on entities it just spawned. - Check the AcceptEntityInput() return value instead of always printing "Input successful.". Commands now report how many entities/players the input actually applied to, and how many failed. - Reply when nothing matched the selector / crosshair instead of silently doing nothing. - sm_forceinputplayer: size aTargetList as MAXPLAYERS + 1, gate on IsClientInGame(), and pass the real target to LogAction(). - !target: allow player slot 1 (was excluded by `entity <= 1`). - Add command descriptions to RegAdminCmd() so they show up in `sm help`. - Use the `args` parameter instead of GetCmdArgs(); minor cleanup. Bump version to 2.2.0. Compiles clean on SourcePawn 1.12.0.7210. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
A few edge cases still produce misleading behavior (stale variant parameters, invalid HammerID parsing safety, and inaccurate success/failure counts when targets become invalid or aren’t in-game).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the ForceInputs SourceMod plugin to make entity targeting safer (by snapshotting matches before firing inputs), improve correctness of success/failure reporting, and provide clearer admin feedback for no-op situations.
Changes:
- Snapshot matching entities into an
ArrayListbefore firing inputs to avoid unsafe iteration while inputs can create/remove entities. - Add
FireInput()/LogInput()helpers and switch to checkingAcceptEntityInput()return values with aggregated success/failure summaries. - Improve UX via command descriptions in
sm help, clearer no-match/no-target replies, and bump version to2.2.0.
File summaries
| File | Description |
|---|---|
| addons/sourcemod/scripting/ForceInputs.sp | Refactors input firing/logging and implements safer entity iteration plus better success/failure and no-op feedback. |
| .github/copilot-instructions.md | Updates documented current plugin version to match the bump. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+40
to
+46
| bool FireInput(int entity, const char[] input, const char[] parameter, int activator, int caller) | ||
| { | ||
| if(parameter[0]) | ||
| SetVariantString(parameter); | ||
|
|
||
| return AcceptEntityInput(entity, input, activator, caller); | ||
| } |
Comment on lines
99
to
103
| for(int i = 0; i < TargetCount; i++) | ||
| { | ||
| if(!IsValidEntity(aTargetList[i])) | ||
| if(!IsClientInGame(aTargetList[i])) | ||
| continue; | ||
|
|
Comment on lines
+204
to
207
| if(sArguments[0][0] == '#') // HammerID | ||
| { | ||
| int HammerID = StringToInt(sArguments[0][1]); | ||
| int iHammerID = StringToInt(sArguments[0][1]); | ||
|
|
Comment on lines
+238
to
+243
| for(int i = 0; i < hEntities.Length; i++) | ||
| { | ||
| int entity = EntRefToEntIndex(hEntities.Get(i)); | ||
|
|
||
| if(entity == INVALID_ENT_REFERENCE || !IsValidEntity(entity)) | ||
| continue; |
Comment on lines
+33
to
+34
| RegAdminCmd("sm_forceinput", Command_ForceInput, ADMFLAG_ROOT, "Force an input on entities by classname/targetname/HammerID (supports !self, !target, #<HammerID> and * wildcards)"); | ||
| RegAdminCmd("sm_forceinputplayer", Command_ForceInputPlayer, ADMFLAG_ROOT, "Force an input on one or more players"); |
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
This PR fixes a few latent bugs in
ForceInputs.spand improves user feedback. It compiles cleanly with zero warnings on SourcePawn1.12.0.7210.Bugs fixed
1. Unsafe entity iteration (potential crash / wrong targets)
Command_ForceInputfired inputs while iteratingFindEntityByClassname()for the#<HammerID>, classname and wildcard selectors. An input can create or destroy entities synchronously (Kill, entity-spawning inputs, templates, …), which invalidates the running enumeration — the exact class of problem this plugin's history is about ("Modified … to fix crash issues"). It could also apply the input to entities that were spawned by the same command.Now the matching entities are collected into an
ArrayList(as entity references) first, and the input is fired on that snapshot.2.
AcceptEntityInput()return value ignoredEvery path printed
"[SM] Input successful."unconditionally, even when the input name was invalid or the entity rejected it. The return value is now checked. Multi-entity / multi-player commands report a summary instead of spamming one line per entity:[SM] Input "Foo" applied to 12 entities.[SM] Input "Foo" applied to 9 of 12 entities, 3 failed.[SM] No entities matched "func_nope".3. Silent no-ops
sm_forceinput nonexistent Kill, an unknown HammerID, and!targetaimed at the sky / world all did nothing with no reply. They now tell the admin what happened.4.
!targetcould not target player slot 1The check was
entity <= 1, which also excludes the first player slot. Changed toentity < 1(worldspawn / no-hit still rejected).5.
sm_forceinputplayernitsaTargetListwas sizedMAXPLAYERSand passedMAXPLAYERSas the max — nowMAXPLAYERS + 1/sizeof(...).IsClientInGame()(wasIsValidEntity()), since forcing an input on a not-in-game client entity is meaningless.LogAction()now receives the real target client instead of-1.Improvements
RegAdminCmd()calls now include descriptions, so both commands show useful text insm help.FireInput()/LogInput()helpers remove the copy-pastedSetVariantString+AcceptEntityInput+ log blocks (4 near-identical copies).argsparameter instead ofGetCmdArgs().2.2.0.Behaviour changes worth a look during review
"Input successful."replies are replaced by a single summary line.!targeton player slot 1 now works.🤖 Generated with Claude Code