fix(recorder): migrate FiredMan EH to new owner on locality transfer - #118
fix(recorder): migrate FiredMan EH to new owner on locality transfer#118fank wants to merge 2 commits into
Conversation
When a player slotted into a pre-placed AI unit, their shots were never recorded. Per BIS wiki, addEventHandler "Local" only fires on machines where it was added; OCAP is server-side only, so the Local EH lives on the server. When locality transferred from server to a client, the server's EH fired with _isLocal=false (removing its own FiredMan EH), but the new owner never installed one — so FiredMan never triggered for the player. Refactor the install/remove blocks into reusable code values and, when the server loses locality, remoteExec the install to the new owner.
There was a problem hiding this comment.
Code Review
This pull request refactors the event handler management for unit locality changes in fnc_eh_fired_server.sqf, introducing dedicated blocks for installing and removing FiredMan and HandleDamage handlers. This ensures that recording follows the unit's owner, particularly when players take control of AI. A review comment points out that the installation block is not idempotent, which could lead to duplicate event handlers if a unit's ownership changes multiple times, and suggests incorporating a check to remove existing handlers before adding new ones.
The server's Local EH cleanup only runs on the server, so a client that previously owned a unit retains its FiredMan/HandleDamage handlers indefinitely. If the unit later returns to that client (S→A→S→A), the install block stacked a second EH on A and emitted duplicate :EVENT:PROJECTILE: records on every shot. Clear any pre-existing EH at the start of the install block.
|
@thegamecracks your #124 fixed this right? |
Oh, I didn't realize this PR existed before I started writing mine. Sorry!
This seems to match up with my reproduction steps, although it's more broad in its wording (implies non-JIP was affected too, which I tested to be safe prior to #124).
All of the theorizing I did turned out to be the exact same here 😅
This solution is different since I dropped the server-side Local EH in favour of letting all clients synchronize their own EHs. AFAIK, the server-side Local EH can't by itself handle cases where locality transfers between client-to-client (e.g. group leader changes between players) because it's not one of the parties involved in the transfer (wiki). |
Summary
Root cause
Per BIS wiki,
addEventHandler "Local"only fires on machines where it was added. OCAP is server-side only, so the Local EH lives on the server. The previous logic assumed the EH would fire on both old and new owners, but in practice:_isLocal=false→ removes its own FiredMan EH.:EVENT:PROJECTILE:ever reaches the extension.This matched a user report where vehicle kills (server-side
EntityKilled) and ACE3 medical events (server-local AI dying) were captured, but zero shots/hits were recorded across a 7-minute session with 10 confirmed kills.Fix
GVAR(ownerInstallBlock)and cleanup intoGVAR(ownerRemoveBlock)to eliminate three near-duplicate copies._isLocal=falsebranch, after cleaning up server EHs,remoteExecthe install block toowner _entity(which is the new owner at that point).Delivery pattern is unchanged — install code is still shipped as an inline code block via
remoteExec ["call", target], just sourced from aGVARinstead of being inlined three times. No newCfgRemoteExecwhitelisting required.Test plan
:EVENT:PROJECTILE:events appear in the extension log atloglevel: debug.