⚡ Bolt: Optimize GregFacilityModule.GetRackCount using NetworkMap#177
⚡ Bolt: Optimize GregFacilityModule.GetRackCount using NetworkMap#177mleem97 wants to merge 1 commit into
Conversation
|
👋 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. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| 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
This PR optimizes the GetRackCount method by replacing an FindObjectsOfType search with a direct lookup in the NetworkMap device count cache. This is a significant performance improvement for large facility maps.
While the implementation includes necessary defensive checks for the NetworkMap instance and its data, the reliance on a hardcoded index (index 2) creates a brittle dependency on the internal data structure of the NetworkMap. Additionally, although the optimization is logically sound, the PR lacks unit tests to verify that the fallback logic correctly handles null instances or undersized arrays. Codacy reports that the PR is up to standards, though cyclomatic complexity has increased slightly (+3) due to the new defensive branching.
About this PR
- The PR lacks unit or integration tests to verify the new optimization and its fallback mechanisms. Given the importance of facility metrics, we should ensure the fallback to
FindObjectsOfTypeworks as intended whenNetworkMapis unavailable or returns unexpected data. - The optimization relies on a hardcoded index (2) for retrieving rack counts. This creates a brittle dependency on the internal implementation of
Il2Cpp.NetworkMap. If the order of device types in the network map changes, this logic will break silently or return incorrect data.
Test suggestions
- Verify GetRackCount returns index 2 from NetworkMap when the instance and data are valid.
- Verify GetRackCount falls back to FindObjectsOfType when NetworkMap.instance is null.
- Verify GetRackCount falls back to FindObjectsOfType when GetNumberOfDevices returns null.
- Verify GetRackCount falls back to FindObjectsOfType when the returned array length is less than 3.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify GetRackCount returns index 2 from NetworkMap when the instance and data are valid.
2. Verify GetRackCount falls back to FindObjectsOfType when NetworkMap.instance is null.
3. Verify GetRackCount falls back to FindObjectsOfType when GetNumberOfDevices returns null.
4. Verify GetRackCount falls back to FindObjectsOfType when the returned array length is less than 3.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if (global::Il2Cpp.NetworkMap.instance != null) | ||
| { | ||
| var counts = global::Il2Cpp.NetworkMap.instance.GetNumberOfDevices(); | ||
| if (counts != null && counts.Length > DEVICE_INDEX_RACKS) | ||
| { | ||
| return counts[DEVICE_INDEX_RACKS]; | ||
| } | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The instance property is accessed multiple times, which can be inefficient in IL2CPP/Unity environments where singleton accessors often involve native lookups. Localizing the reference clarifies the logic and reduces overhead.
| if (global::Il2Cpp.NetworkMap.instance != null) | |
| { | |
| var counts = global::Il2Cpp.NetworkMap.instance.GetNumberOfDevices(); | |
| if (counts != null && counts.Length > DEVICE_INDEX_RACKS) | |
| { | |
| return counts[DEVICE_INDEX_RACKS]; | |
| } | |
| } | |
| var map = global::Il2Cpp.NetworkMap.instance; | |
| if (map != null) | |
| { | |
| var counts = map.GetNumberOfDevices(); | |
| if (counts != null && counts.Length > DEVICE_INDEX_RACKS) | |
| { | |
| return counts[DEVICE_INDEX_RACKS]; | |
| } | |
| } |
💡 What: Optimize
GregFacilityModule.GetRackCount()to useNetworkMap.instance.GetNumberOfDevices()instead ofFindObjectsOfType<Rack>().🎯 Why:
UnityEngine.Object.FindObjectsOfTypeis O(N) over all active loaded objects, which can cause frame drops and GC spikes when called frequently. The network map maintains this count internally and allows O(1) lookups.📊 Impact: Turns an O(N) allocation-heavy lookup into an O(1) array access. Massively reduces overhead when called on frame updates or frequently during gameplay.
🔬 Measurement: Check the CPU usage using a profiler during calls to GetRackCount(). It should now take practically no time and cause 0 byte allocations.
PR created automatically by Jules for task 17838429299308485425 started by @mleem97