feat(recorder): improve automatic recording lifecycle - #126
YetheSamartaka wants to merge 3 commits into
Conversation
Added: - Auto-restart and connection event buffering - New CBA settings to configure everything these changes are touching Changed: - Player counting and Steam ID handling
|
Linked with OCAP2/extension#202 |
thegamecracks
left a comment
There was a problem hiding this comment.
Small review from me since I noticed old code removed by #125 came back up, despite not triggering a merge conflict :)
Co-authored-by: thegamecracks <61257169+thegamecracks@users.noreply.github.com>
thegamecracks
left a comment
There was a problem hiding this comment.
Thanks for the edit!
|
Ratatoskr reviewed this pull request. Changes requested on Finished 2026-10-02 21:58 UTC. |
fank
left a comment
There was a problem hiding this comment.
Request changes: 2 findings to fix before merge (auto-restart ignores autoStart; stale buffered events replayed at frame 0), plus 3 minor notes.
Review details
I reviewed head fdb88445f1e96c60d2f6eab7cdf369ce06aa4877 against main, reading the new functions together with fnc_startRecording, fnc_exportData, fnc_captureLoop, fnc_init, the extension callback in extension/fnc_initSession.sqf, and the v1 export in the linked OCAP2/extension#202.
The core flow holds up. Events are buffered while startTime is nil or sessionReady is false. They are flushed after sessionReady and before captureLoop, and exportData resets startTime and sessionReady, so the restart re-registers through newMission. Pausing keeps startTime set, so events during a pause still go out immediately, as they did before. Headless-client exclusion via allPlayers - entities "HeadlessClient_F" is correct, and it now matches the CBA_fnc_players check that fnc_eh_disconnected.sqf already uses for the empty-server save.
Must fix
1. Auto-restart ignores OCAP_settings_autoStart and is on by default. (addons/recorder/fnc_autoRestartMonitor.sqf:19-29, addons/recorder/XEH_preInit.sqf:91-113)
The monitor checks autoRestartAfterEmpty, the pending flag, the client state and minPlayerCount. It never checks GVAR(autoStart). Here is a server that turned off auto-start so admins decide what gets recorded:
- An admin starts a recording by hand. Everyone leaves, and
saveOnEmptyexports it. autoRestartAfterEmptyPendingis now true.- Later, enough players join for something unrelated. The monitor starts a new recording without anyone asking for it.
- When they leave and
minMissionTimeis met, that recording is saved and uploaded to the web.
Before this PR, nothing restarted after an empty-server save. This changes behaviour for every existing server and creates recordings that the operator explicitly opted out of. Please gate the restart on GVAR(autoStart) (the snapshot taken in fnc_init.sqf:80), or default autoRestartAfterEmpty to false. The first option seems more natural, since the setting is described as re-arming auto-start.
2. The buffer is unbounded, and replays stale events at frame 0 of whichever recording starts next. (addons/recorder/fnc_recordPlayerConnectionEvent.sqf:26-30, addons/recorder/fnc_flushPlayerConnectionEvents.sqf:19-38)
After an export, every connect and disconnect is appended to connectedPlayerNamesBuffer until a recording starts. Nothing bounds the buffer or clears it. On a persistent server that window can last hours or days, for example:
autoRestartAfterEmptyorautoStartis off;- or the player count stays below
minPlayerCount(default 15) while people drift in and out.
When the next recording starts, all of those events are emitted at once, at the same frame, with the real timing lost. The recording then claims that players who left hours ago, during a different session, connected and disconnected at its start.
Before the first recording this matches existing behaviour, because events went out with captureFrameNo == 0. After an export, though, this is new data that never belonged to the recording. A simple fix: at flush time, emit connected only for players who are currently connected (from allPlayers, minus HCs), and drop the buffer. A cap or a clear-on-export alone would still replay irrelevant churn.
Minor / non-blocking
- Frame 0 is the "forever" sentinel, not the first recorded frame.
fnc_captureLoop.sqf:58reserves frame 0 for "not yet recording". OCAP2/extension#202 maps internal frame 0 to v1 frame-1(frameToV1). So "replay them at frame 0" actually lands before the recording's first frame (v10). Pre-start connection events already behaved this way, so this may be intended. If the goal is for them to show at the start of the timeline, frame 1 may be what you want. I haven't checked how the web player renders-1events, so I'm unsure whether this is visible. - The restart can start a recording on an empty server. With
excludeHeadlessClientsFromAutoStart = false, the empty-server save still fires, becauseCBA_fnc_playersexcludes HCs. But the monitor then counts the HCs. IfminPlayerCountis no higher than the HC count, a new recording starts within about 10 seconds with no humans present.saveOnEmptythen won't fire again until a human joins and leaves. This is an edge case with a non-default setting, but a note in the setting description, or always excluding HCs in the restart path, would avoid it. - Naming and duplication.
connectedPlayerNamesBufferholds both connect and disconnect events, with UIDs, so the name is misleading. The event payload code is duplicated betweenfnc_recordPlayerConnectionEvent.sqf:33-45andfnc_flushPlayerConnectionEvents.sqf:22-35. HavingrecordPlayerConnectionEventtake an optional frame and calling it from the flush would keep the Steam ID handling in one place.
Tests / checks
- The repository has no SQF test harness. CI is only
hemtt check/hemtt build, andgh pr checksreports no checks on this PR. HEMTT isn't available in my review environment, so I did not build or lint it, and nothing here is pinned by tests. A manual test on a dedicated server is worth recording in the PR, covering:- empty-server save → rejoin → auto-restart;
- the same flow with
autoStart = false; - connect/disconnect churn between recordings, then inspecting the exported events.
Earlier review discussions
- thegamecracks' thread on
fnc_eh_connected.sqf(re-introducedadminUIcontrolcall): already resolved by its author. I checked it against the current head:fnc_eh_connected.sqfno longer callsadminUIcontrol. It isn't my thread, so I took no action on it. No other unresolved threads exist.
Added:
Changed: