Repository navigation
Guard the scope tracking removal in ScopeOut - #88
michaelinfaith wants to merge 1 commit into
Conversation
46eff67 moved the removal to the top of ScopeOut so the tracking is dropped even if the player can't receive the scope out. 34353af then added a TryGetValue guarded removal at the old place, after the CanReceiveGSS check, and left the unguarded index at the top. That index still throws a KeyNotFoundException when ScopeOut runs for an entity that is no longer in _scopedPlayersByEntity. Keep the removal at the top, guarded with TryGetValue, and drop the duplicate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eXpl0it3r
left a comment
eXpl0it3r
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up! The duplicate is indeed just a leftover from #73 being written against the code before 46eff67, and keeping the removal at the top with the guard is what both changes intended.
Since #67 is pretty much superseded at this point, could you also guard the last remaining direct index in Tick?
var currentlyScoped = _scopedPlayersByEntity[entity.EntityId];That's essentially the Tick hunk from #67, so a TryGetValue with a continue. With that, there's no direct index into _scopedPlayersByEntity left and I can close #67. If you take it over, it would be nice to credit Kyr27 for it in the commit message.
Also, please remove the Co-Author attribution, we're not being paid to advertise for Anthropic. 😉
| // Remove the tracking even if the player can't receive the scope out. | ||
| // The entity can already be removed. Refer to the same guard in ScopeIn. | ||
| if (_scopedPlayersByEntity.TryGetValue(entity.EntityId, out var scopedPlayers)) |
There was a problem hiding this comment.
As far as I can tell, this can't really throw in the current code. ScopeOut is only called from the scope check in Tick, right after _scopedPlayersByEntity[entity.EntityId] was already indexed for the same entity, and from OnRemovedEntity, before the key gets removed. SpectateServerCommand calls it as well, but the command isn't registered.
So the guard is fine as a defensive measure, but can you adjust the commit message and PR description, so it doesn't read like a crash we actually hit? Or maybe remove the comment all together.
2 participants
46eff67 moved the
_scopedPlayersByEntityremoval to the top ofScopeOut, so the tracking is dropped even when the player can't receive the scope out. 34353af (from #73) then added aTryGetValue-guarded removal at the old place, after theCanReceiveGSScheck, and the unguarded index at the top stayed. That index still throwsKeyNotFoundExceptionwhenScopeOutruns for an entity that is no longer in the map.This keeps the removal at the top, guarded with
TryGetValue, and drops the duplicate. Behaviour is otherwise unchanged.Related: #67 adds similar guards in
Tick,FlushViewChangesToScoped,SendToScopedandOnRemovedEntity, but not inScopeOut.Tested: builds without warnings. My local stack runs an earlier version of this fix (it only deleted the unguarded line) on a branch that also has #73 merged; this exact version has only been built.
Written with AI assistance (Claude Code); I reviewed it.
🤖 Generated with Claude Code