⚡ Bolt: Optimize GetRackCount to use O(1) device array lookup#186
⚡ Bolt: Optimize GetRackCount to use O(1) device array lookup#186mleem97 wants to merge 1 commit into
Conversation
Replaced `FindObjectsOfType<Il2Cpp.Rack>()` in `GregFacilityModule.GetRackCount()` with `NetworkMap.instance.GetNumberOfDevices()`, using index `2` for racks and providing a graceful fallback. This reduces CPU time from O(N) over all scene objects to an O(1) array access.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 high |
🟢 Metrics 5 complexity · 0 duplication
Metric Results Complexity 5 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The optimization successfully implements an O(1) lookup using the NetworkMap device array, but the use of a hardcoded index (2) is brittle and highly susceptible to breaking during game updates.
Furthermore, the Codacy analysis indicates the PR is not up to standards. This is primarily due to an empty catch block that swallows exceptions, which prevents observability into whether the optimized path is actually functioning or silently failing back to the O(N) method. Finally, there is a total lack of automated tests to verify the lookup logic or the fallback mechanism.
About this PR
- The PR lacks unit or integration tests to verify the new O(1) lookup logic and its fallback mechanism. Testing is required to ensure the fallback logic triggers correctly when the NetworkMap is unavailable or the array bounds are exceeded.
Test suggestions
- Missing recommended test scenario: Verify GetRackCount returns the correct value from NetworkMap at index 2
- Missing recommended test scenario: Verify GetRackCount falls back to FindObjectsOfType when NetworkMap.instance is null
- Missing recommended test scenario: Verify GetRackCount falls back to FindObjectsOfType when the devices array is too short (Length <= 2)
- Missing recommended test scenario: Verify GetRackCount returns 0 if both NetworkMap and fallback methods return no data or null
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: Verify GetRackCount returns the correct value from NetworkMap at index 2
2. Missing recommended test scenario: Verify GetRackCount falls back to FindObjectsOfType when NetworkMap.instance is null
3. Missing recommended test scenario: Verify GetRackCount falls back to FindObjectsOfType when the devices array is too short (Length <= 2)
4. Missing recommended test scenario: Verify GetRackCount returns 0 if both NetworkMap and fallback methods return no data or null
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| } | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
🔴 HIGH RISK
Empty catch blocks can mask underlying issues and make troubleshooting difficult. Since this logic interfaces with Il2Cpp types which are subject to runtime changes, it is recommended to either log the exception or add a comment explaining that the fallback logic below is intended to handle any failures here.
| catch { } | |
| catch (System.Exception) | |
| { | |
| // Fallback to O(N) FindObjectsOfType if the optimized NetworkMap access fails | |
| } |
| internal GregFacilityModule(GregApiContext ctx) => _ctx = ctx; | ||
|
|
||
| // Device index for racks in NetworkMap.GetNumberOfDevices() array | ||
| private const int DEVICE_INDEX_RACKS = 2; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The hardcoded index 2 is brittle and relies on the internal ordering of the NetworkMap device array. If the game updates and changes this order, GetRackCount will return incorrect values without warning. Check if there is an enum or named constant in the Il2Cpp.NetworkMap metadata that should be used instead.
Try running the following prompt in your coding agent:
Search the Il2Cpp metadata for NetworkMap and GetNumberOfDevices to find any associated enums for device types, then replace the magic number 2 with the appropriate enum value.
|
|
||
| // Graceful fallback to FindObjectsOfType if NetworkMap is not available | ||
| var fallbackRacks = UnityEngine.Object.FindObjectsOfType<global::Il2Cpp.Rack>(); | ||
| return fallbackRacks != null ? fallbackRacks.Length : 0; |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Unity's FindObjectsOfType<T>() returns an empty array if no matches are found, not null. The null check is redundant.
| return fallbackRacks != null ? fallbackRacks.Length : 0; | |
| return fallbackRacks.Length; |
💡 What: Replaced
FindObjectsOfType<Il2Cpp.Rack>()inGregFacilityModule.GetRackCount()withNetworkMap.instance.GetNumberOfDevices(), using the proper index2for racks and providing a fallback.🎯 Why:
FindObjectsOfTypeis O(N) over all loaded objects and blocks the main thread, causing severe performance issues in scenes with many objects.📊 Impact: Reduces
GetRackCountCPU time from O(N) to O(1), improving frame rates and lowering GC pressure when polled.🔬 Measurement: Profile the application during high rack count scenes;
GetRackCountshould show negligible CPU overhead.PR created automatically by Jules for task 4322514682969428915 started by @mleem97