fix: detect and offset overlapping sky map labels - #944
Conversation
jaydeetay
left a comment
jaydeetay
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the refactor into computeScreenPosition/drawLabel is cleanly done, the resolver's center-based AABB model matches how label quads are actually drawn, and the sequential placement can't oscillate or loop infinitely (zero-size boxes are safe too, since the overlap test uses strict inequalities). A few issues need addressing before this can merge, though.
1. Cross-layer overlaps aren't handled (main gap)
Each layer (stars, constellations, planets, Messier objects, …) has its own LabelObjectManager (SkyRenderer.createLabelManager), and the collision pass runs inside a single manager's drawInternal. So star labels are deduplicated against other star labels, but a planet label and a star label never enter the same resolveCollisions call — and a planet passing near a bright named star is a common case for #764. Fixing this needs frame-scoped state shared across managers, e.g.:
- Make
LabelCollisionResolvera shared instance holding the accumulated placed rectangles, with abeginFrame()that clears it and aplace(LabelPosition)that resolves one label against everything placed so far (your loop is already sequential, so this is mostly moving the accumulator into a field). SkyRenderer.onDrawFramecallsbeginFrame()before the manager draw loop, andcreateLabelManagerhands the shared instance to eachLabelObjectManager.- Priority then falls out of draw order: earlier layers keep their positions, later layers get displaced.
2. Rotation is ignored
Labels are rotated around their center by the phone's up-angle (gl.glRotatef(...) in drawLabel), but the resolver tests axis-aligned boxes in unrotated screen space. With the device held at an angle the real footprints are rotated rectangles, so the check both misses real overlaps and reports false ones — and the displacement is screen-space "down" rather than along the label's own down direction, so a displaced label can end up beside or above its star. At minimum this deserves a comment; better is to test and offset in the rotated label frame.
3. Frame-to-frame instability — needs device testing
Which label gets displaced depends on list order, which comes from sky-region iteration order and changes as the user pans. Two overlapping labels can swap displaced/undisplaced roles between frames, and displacement is a discrete jump of a full label height, so labels may visibly pop as the view drifts across an overlap boundary. There's no hysteresis. Since this couldn't be tested on a device, someone should watch it live on a dense star field before merge — this class of solution often looks fine in unit tests and jittery in motion.
4. Allocation churn on the render thread
Every frame now allocates two ArrayLists, one LabelPosition per visible label, one extra LabelPosition per collision iteration, and one Vector3 per drawn label — on top of the two Vector3s per label the old path already created. At 60 fps that's thousands of small objects per second feeding GC in the render loop. Suggested fixes: reuse the lists as fields (cleared per frame), make LabelPosition mutable or use parallel float arrays, and pass (float x, float y) to drawLabel instead of wrapping in a fresh Vector3. (The O(n²) pairwise check itself is fine at the label counts involved. Nice touch that unmoved labels return the same instance rather than a copy.)
5. Ambiguous star–label association
A label pushed a full height downward lands directly under the other star's label, with nothing indicating which star it belongs to. That may be an acceptable tradeoff for #764, but worth an explicit decision (alternatives: hide the lower-priority label, or a leader line).
Smaller points
- New files should be Kotlin per AGENTS.md — both
LabelCollisionResolverand its test are Java. A Kotlin data class would also eliminate theLabelPositiongetter boilerplate. - The test duplicates the production
overlapsformula verbatim, so the cluster assertions are tautological — if the formula were wrong the test copy would agree with it. Asserting explicit expected coordinates (as the pair test does) is stronger. - Resolver test coverage is otherwise good (no-overlap, pair, cluster, empty/single, iteration limit — the
y == 0expectation checks out by hand).
|
The previous comment was generated by Claude, obviously. Here's a video of the fix in action. The main issue seems to be that the labels from different layers are unaware of each other, so the collision detection is only per-layer. From a UX point of view, the labels jumping is also quite jarring and labels can move far from the targets they are representing. I'm not sure yet of the right answer here (from a UX point of view). One thing to bear in mind is that the next release of Sky Map is probably the last release of the current code base - a complete rewrite is on the way. So I'm not sure how much to invest in fixing this right now. Uploading screen-20260727-221924-1785187141146.mp4… |
Addresses @jaydeetay's review. The resolver is now a single instance owned by SkyRenderer, cleared once per frame in onDrawFrame and handed to every LabelObjectManager, so a planet label and a star label resolve against each other instead of only against their own layer. Draw order decides which label keeps its anchor. Overlap testing and displacement now happen in the label's own rotated frame, so a displaced label moves along its own down direction rather than screen-space down. Placed positions are reused from a pooled list rather than allocating two ArrayLists and a LabelPosition per label per frame. Both files are Kotlin per AGENTS.md, and the tests assert explicit expected coordinates instead of re-deriving the overlap formula.
|
53a7533 addresses most of this. Taking them in order.
Smaller points: both files are Kotlin now, and the tests assert explicit expected coordinates rather than re-deriving the overlap formula, so they would catch a wrong formula. Still open, and I would rather flag than paper over:
Verified with the fdroid debug unit suite: 208 tests, 0 failures. |
2 participants
Description
When two sky objects are close together on screen, their text labels can render on top of each other and become unreadable; the only workaround today is rotating the map until they no longer overlap. This adds a lightweight collision-detection pass in the label renderer: before drawing, each label's already-known on-screen pixel footprint (from
LabelMaker.LabelData) is checked against labels already placed this frame, and an overlapping label is offset so both stay readable. Only a small number of labels are visible on screen at once, so the pairwise check is inexpensive.The reporter asked for exactly this in the issue and named the relevant renderer files.
Fixes #764
Type of Change
Checklist
./stardroid-v1/gradlew :app:test(see Notes for Reviewers)Notes for Reviewers
This change adds a
LabelCollisionResolverplus a unit test for it. I was not able to run the full Android Gradle build (./stardroid-v1/gradlew :app:test) or exercise it on a device in my environment, so please rely on CI to confirm the tests pass before merging. The collision logic is self-contained and does not change the projection or culling paths, only the on-screen placement of already-visible labels.AI was used for assistance.