Repository navigation
Find the nearest floor edge without reading every edge - #254
Merged
Merged
Conversation
Dragging an agent snaps its cone out of wall ink and back onto the floor (standablePointNear). When the point was off the floor, _pulledIn looked for the nearest edge of the floor footprint by measuring every edge of it: 9,000 to 13,500 edges on Lotus, Breeze, Bind and Ascent, with an Offset allocation and a modulo per edge. On the web, where no native library runs, that cost 384 us per drag frame in a browser CPU profile of the drag harness (5.7% of busy time). The footprint already files its edges in rows one unit tall for its containment test. The nearest-edge search now reads those rows outward from the point's own and stops once the nearest edge found is no farther than the rows not yet read; an edge outside them is more than that far. Ties still go to the first edge in ring order, and a non-finite point reads every edge in order as before, so the chosen edge, foot and snapped point are the same. Edge endpoints are kept in one Float64List that the containment test and the search both read. Measured on the four maps above: - Browser drag (CPU profile, 4 maps x 8 s): 384 -> 83 us per frame under standablePointNear. - Browser, per call on drag paths through walls: mean 190-310 -> 10-17 us, p99 575-935 -> 60-115 us. - VM, per call: mean 170-220 -> 10-17 us, p99 790-1170 -> 80-120 us. An old-against-new comparison over 299,234 points on all 26 map sides (drag paths, points beside wall and floor vertices, off-map, NaN and infinite points) found no difference in the snapped point. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change caches footprint edge coordinates and indexes edges by vertical row. It updates containment and nearest-edge queries to use that cache, and changes visibility stepping to pass footprints into edge selection. ChangesFootprint-based visibility stepping
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No concrete issue remains that calls for a change before merging. Normal checks are still appropriate. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
At 1e16 scale, rounding let an edge just beyond the rows read compute the same distance as the one found, and reading every edge would have chosen it. The search now stops only once the nearest edge is strictly nearer than the unread rows, with room for rounding. A footprint too tall to split into rows (coordinates near 1e308) reads every edge, as before, instead of throwing. Both cases are now in svg_standable_point_test. Found in review by Astra. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A tall edge is filed in every row it spans, so a point far beside the floor measured it once per row read: 1,001 times for one edge of a floor 1,000 units tall. Each search now skips edges it has already measured; the edge it picks is the same. Found in review by Greptile. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On the web, keeping an agent out of walls while it's dragged cost 384 µs per drag frame. It now costs 83 µs, and agents land on exactly the same points as before.
What was slow
SvgHeightVisibility.standablePointNearruns on every drag frame. It moves the agent's cone out of wall ink and back onto the floor. When the point was just off the floor,_pulledInlooked for the nearest floor edge by measuring every edge of the floor outline: 9,000–13,500 edges per map, with anOffsetallocation and a modulo per edge. On the web, where there's no native library, that was 5.7% of busy time in a browser CPU profile of a cone drag (Lotus, Bind, Ascent, Breeze, 8 s each).What changed
The floor footprint already files its edges into 1-unit rows for its containment test. The nearest-edge search now reads those rows outward from the point's own row. It stops once the nearest edge found is no farther than the rows not yet read. An unread edge is more than that far away, because rows are filed with a 0.05 margin, so the stop is exact. Ties still go to the first edge in ring order. A NaN, infinite or huge point still scans every edge in order, as before. Edge endpoints now live in one
Float64Listthat both the containment test and the search read, with no per-edgeOffsetor%.Numbers
Desktop runs the same Dart, so it gets the same speedup.
Same results
An old-against-new comparison over 299,234 points on all 26 map sides found no difference in the snapped point. The points included drag paths through walls, points beside wall and floor corners, off-map points, and NaN and infinite points; about 30% got moved and 35% returned null. The same comparison inside the browser build found no difference on 8 sides.
What's not covered
_blockingWallAtand the ground-height lookup. Each would be its own change.Testing
flutter test: 1,763 passed, 9 skipped (the native-library tests that needICARUS_SVG_NATIVE_LIBRARY). The standable, footprint and vision tests that pin this behaviour pass unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
No outstanding findings block merging.
Summary
The PR searches footprint rows outward for the nearest floor edge instead of measuring every edge. It now measures each edge at most once per lookup, fixing the previously reported repeated work. No outstanding findings remain.
Reviews (2) · Last reviewed commit: "Measure each floor edge once per nearest..."