Bugfix area calculations in the NavGenerator#7104
Conversation
Two fixes to label area calculations: - Correct constant for converting ogrid units to KM - Correct DFS algorithm for summing leaf sizes Also renamed some functions and variables associated with tree and section area calculations - these are actually calculating area proportions not KM squared values.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors nav-label area calculations in ChangesNav-Label Area Calculation Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/sim/NavGenerator.lua (1)
1526-1526:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCull threshold
0.16may need recalibration after area-calculation corrections.The two bugs previously cancelled non-uniformly (DFS double-counting is proportional to each leaf's neighbor count, not a flat 4×), so
NavLabelMetadata.Areaper label will shift by a connectivity-dependent factor after this fix. The0.16 km²threshold was tuned against the old (imperfectly-cancelled) values — double-check that it remains the intended cutoff with the now-correct area values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/sim/NavGenerator.lua` at line 1526, The hardcoded cull threshold 0.16 in the conditional that checks metadata.Area (alongside metadata.NumberOfExtractors and metadata.NumberOfHydrocarbons) was tuned against the old buggy area calculation and may no longer be valid; re-evaluate and adjust this cutoff by running area-distribution tests on NavLabelMetadata.Area after the area-calculation fix, then replace the literal 0.16 in the if (metadata.Area < 0.16 and metadata.NumberOfExtractors == 0 and metadata.NumberOfHydrocarbons == 0) check with an updated threshold or a configurable constant (e.g., cullAreaThreshold) and document the chosen value and test results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lua/sim/NavGenerator.lua`:
- Line 1526: The hardcoded cull threshold 0.16 in the conditional that checks
metadata.Area (alongside metadata.NumberOfExtractors and
metadata.NumberOfHydrocarbons) was tuned against the old buggy area calculation
and may no longer be valid; re-evaluate and adjust this cutoff by running
area-distribution tests on NavLabelMetadata.Area after the area-calculation fix,
then replace the literal 0.16 in the if (metadata.Area < 0.16 and
metadata.NumberOfExtractors == 0 and metadata.NumberOfHydrocarbons == 0) check
with an updated threshold or a configurable constant (e.g., cullAreaThreshold)
and document the chosen value and test results.
|
It just needs a changelog snippet |
3 participants
Patches two bugs in lua/sim/NavGenerator.lua affecting NavLabelMetadata Area calculations.
These were combining to make plausibly correct but slightly wrong numbers.
This commit also changes some of the variable naming for Area calculations within the NavTree - these currently represent proportions of area covered (i.e. not absolute values). By doing the calculation on ogrid sizes we can also avoid all conversion to KM in that part of the code, which makes for a teeny-tiny performance gain.
Summary by CodeRabbit