Conversation
…ame from comparison The state comparison at captureLoop:170 never worked due to two bugs: 1. Read from "unitData" but wrote to "OCAP_unitData" (QGVARMAIN mismatch) 2. Frame number at index 8 changed every frame, making comparison always unequal Fix uses a 0 placeholder for frame during comparison and stores a deep copy. This eliminates redundant extension calls for unchanged units (dead, stationary).
Summary of ChangesHello @fank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces several performance optimizations to the recording system. By caching frequently accessed exclusion lists, fixing a critical unit data deduplication bug, and optimizing data collection loops, the changes aim to reduce CPU overhead and network traffic, particularly for dead or stationary units, leading to a more efficient and responsive recording process. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces several performance optimizations by caching frequently accessed data and refactoring loops for efficiency. Key changes include caching vehicle/marker exclusion lists, fixing unit data deduplication, caching player scores and unit types, and optimizing array iterations. These changes align with the goal of reducing redundant computations and improving overall recording performance.
| GVAR(excludeKindList) = parseSimpleArray EGVAR(settings,excludeKindFromRecord); | ||
| GVAR(excludeClassList) = parseSimpleArray EGVAR(settings,excludeClassFromRecord); | ||
|
|
||
| GVAR(excludeMarkerList) = if (!isNil QEGVAR(settings,excludeMarkerFromRecord)) then { | ||
| parseSimpleArray EGVAR(settings,excludeMarkerFromRecord) | ||
| } else { | ||
| [] | ||
| }; |
There was a problem hiding this comment.
Caching the exclusion lists (excludeKindList, excludeClassList, excludeMarkerList) at initialization is a significant performance improvement. This prevents repeated parsing of these settings in performance-critical loops, such as the captureLoop and marker handlers. This aligns with the general rule of caching expensive operations.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| private _localUnits = 0; private _localAlive = 0; private _remoteUnits = 0; private _remoteAlive = 0; | ||
| { if (side _x isEqualTo _s) then { | ||
| if (local _x) then { _localUnits = _localUnits + 1; if (alive _x) then { _localAlive = _localAlive + 1 } } | ||
| else { _remoteUnits = _remoteUnits + 1; if (alive _x) then { _remoteAlive = _remoteAlive + 1 } }; | ||
| }} forEach _allUnits; | ||
|
|
||
| private _localDead = 0; private _remoteDead = 0; | ||
| { if (side _x isEqualTo _s) then { | ||
| if (local _x) then { _localDead = _localDead + 1 } else { _remoteDead = _remoteDead + 1 }; | ||
| }} forEach _allDeadMen; | ||
|
|
||
| private _localGroups = 0; private _remoteGroups = 0; | ||
| { if (side _x isEqualTo _s) then { | ||
| if (local _x) then { _localGroups = _localGroups + 1 } else { _remoteGroups = _remoteGroups + 1 }; | ||
| }} forEach _allGroups; | ||
|
|
||
| private _localVeh = 0; private _remoteVeh = 0; private _localWH = 0; private _remoteWH = 0; | ||
| { if (side _x isEqualTo _s) then { | ||
| private _isWH = _x isKindOf "WeaponHolderSimulated"; | ||
| if (local _x) then { if (_isWH) then { _localWH = _localWH + 1 } else { _localVeh = _localVeh + 1 } } | ||
| else { if (_isWH) then { _remoteWH = _remoteWH + 1 } else { _remoteVeh = _remoteVeh + 1 } }; | ||
| }} forEach _vehicles; | ||
|
|
||
| _sideData pushBack [ | ||
| [count _localUnits, {alive _x} count _localUnits, count _localDead, count _localGroups, count _localVeh, count _localWH], | ||
| [count _remoteUnits, {alive _x} count _remoteUnits, count _remoteDead, count _remoteGroups, count _remoteVeh, count _remoteWH] | ||
| [_localUnits, _localAlive, _localDead, _localGroups, _localVeh, _localWH], | ||
| [_remoteUnits, _remoteAlive, _remoteDead, _remoteGroups, _remoteVeh, _remoteWH] | ||
| ]; |
There was a problem hiding this comment.
The refactoring of the telemetry loop to use single-pass counting instead of chained select calls is a significant performance improvement. The original code iterated over _allUnits, _allDeadMen, _allGroups, and _vehicles multiple times for each side. The new approach iterates once per collection and categorizes/counts within that single pass, drastically reducing redundant iterations. This directly addresses the general rule about avoiding multiple iterations for mutually exclusive categories.
References
- To optimize performance when counting items in an array that fall into two mutually exclusive categories, count one category directly and derive the count of the second by subtracting from the total. Avoid iterating over the array twice.
| if (_x getVariable [QGVARMAIN(unitData), []] isNotEqualTo _unitData) then { | ||
| _x setVariable [QGVARMAIN(unitData), +_unitData]; | ||
| _unitData set [8, GVAR(captureFrameNo)]; | ||
| [":SOLDIER:STATE:", _unitData] call EFUNC(extension,sendData); | ||
| _x setVariable [QGVARMAIN(unitData), _unitData]; | ||
| }; |
There was a problem hiding this comment.
The fix for unitData state deduplication is critical. By comparing against QGVARMAIN(unitData) and correctly setting the captureFrameNo before sending, redundant extension calls for dead/stationary units are eliminated. This directly addresses a high-impact performance issue.
| { | ||
| if ((str _marker) find _x >= 0) exitWith { | ||
| _isExcluded = true; | ||
| }; | ||
| } forEach GVAR(excludeMarkerList); |
There was a problem hiding this comment.
By using the pre-cached GVAR(excludeMarkerList), the expensive parseSimpleArray call is removed from this frequently executed loop, leading to improved performance.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| }; | ||
| private _toExcludeClass = false; | ||
| private _classList = parseSimpleArray EGVAR(settings,excludeClassFromRecord); | ||
| private _classList = GVAR(excludeClassList); |
There was a problem hiding this comment.
Similar to the excludeKindList, caching EGVAR(settings,excludeClassFromRecord) into GVAR(excludeClassList) and using the cached variable here avoids repeated parsing, leading to better performance in this per-frame handler.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| { | ||
| if ((str _marker) find _x > -1) exitWith { | ||
| _isExcluded = true; | ||
| }; | ||
| } forEach GVAR(excludeMarkerList); |
There was a problem hiding this comment.
The change to use GVAR(excludeMarkerList) here is consistent with the other marker handlers and provides the same performance benefits by avoiding repeated parseSimpleArray calls.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| { | ||
| if ((_marker) find _x >= 0) exitWith { | ||
| _isExcluded = true; | ||
| }; | ||
| } forEach GVAR(excludeMarkerList); |
There was a problem hiding this comment.
This change ensures that the initial marker collection also benefits from the cached exclusion list, preventing unnecessary parsing during mission initialization.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| private _vic = _x; | ||
| private _toExcludeKind = false; | ||
| private _kindList = parseSimpleArray EGVAR(settings,excludeKindFromRecord); | ||
| private _kindList = GVAR(excludeKindList); |
There was a problem hiding this comment.
Replacing parseSimpleArray EGVAR(settings,excludeKindFromRecord) with the cached GVAR(excludeKindList) significantly improves performance by avoiding repeated parsing of the exclusion list within the per-frame handler. This aligns with the general rule of caching expensive operations in performance-critical code.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| ["Wheeled_APC_F","Tracked_APC","APC_Wheeled_01_base_F","APC_Wheeled_02_base_F", | ||
| "APC_Wheeled_03_base_F","APC_Tracked_01_base_F","APC_Tracked_02_base_F","APC_Tracked_03_base_F"] | ||
| findIf {_this isKindOf _x} != -1 |
There was a problem hiding this comment.
Replacing the count anti-pattern with findIf for early-exit in APC classification is a good optimization. count iterates through the entire array, while findIf stops as soon as a match is found, which is more efficient for this use case.
References
- In performance-critical code such as per-frame handlers, cache the results of expensive operations (e.g.,
parseSimpleArray) in local variables if the result is used multiple times, such as for both an emptiness check and a subsequent loop.
| private _scores = getPlayerScores _x; | ||
| private _scoresStr = _x getVariable [QGVAR(lastScoresStr), ""]; | ||
| if (_scores isNotEqualTo (_x getVariable [QGVAR(lastScores), []])) then { | ||
| _scoresStr = _scores joinString ","; | ||
| _x setVariable [QGVAR(lastScores), _scores]; | ||
| _x setVariable [QGVAR(lastScoresStr), _scoresStr]; | ||
| }; |
There was a problem hiding this comment.
Caching getPlayerScores _x and its string representation (_scoresStr) prevents redundant calls to getPlayerScores and joinString if the scores haven't changed. This is a good performance improvement, especially in a per-frame handler.
1 participant
Summary
countanti-pattern withfindIffor early-exit in APC classificationselectwith single-pass counting in telemetry loopTest Plan
hemtt buildpasses on every commit (44 SQF files compiled, 3 PBOs built):SOLDIER:STATE:calls