Conversation
User request: "Make unit test in PlacesTest.java that will correctly fail to expose the issue" Added three tests that demonstrate incorrect zoom behavior: - testLocalityNoPopulationNotVisibleAtZoom7: place=locality without population incorrectly appears at zoom 7 (should be invisible until zoom 12) - testLocalityNoPopulationMinZoom: has min_zoom=8 instead of expected 13 - testCityWithPopulationVisibleAtZoom7: verifies cities with population should be visible at zoom 7 (this one passes) The bug causes less-important places like "Place Mahiouindo" (place=locality) to appear ahead of more-important places like "Kétou" (place=city) at zoom 7. Root cause: pm:population not being passed to zoomsIndex on Places.java:257. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When the Places.java code was refactored to support Overture data, the zoom calculation logic was split into separate indexes (osmKindsIndex and zoomsIndex). However, the computed pm:population value from osmKindsIndex was not being passed to zoomsIndex, causing population-based zoom rules to fail. This caused places without population tags (like place=locality) to incorrectly appear at zoom 7 instead of zoom 12+, while more important places (like cities) were correctly prioritized. Changes: - Places.java:257,352: Include pm:population in computed tags passed to zoomsIndex - Places.java:348-355: Move population extraction before zoomsIndex evaluation in processOverture() to avoid compilation error - PlacesTest.java: Update test to check _minzoom value instead of feature visibility This fixes the issue where "Place Mahiouindo" (place=locality) appeared ahead of "Kétou" (place=city, pop=160k) at zoom 7. All PlacesTest tests now pass (17 tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
User request: "Look at the last two commits, I'm still seeing something wrong there with Kétou not appearing, and Place Mahiouindo there in its place." Commits a87aeb0 and efc53ba attempted to fix Places prioritization but introduced new bugs in the zoom level assignment. When the Places code was refactored to split logic between osmKindsIndex and zoomsIndex, the zoom rules were incorrectly set: 1. Line 129: ALL localities got minzoom=7 (should be 11 for generic localities) 2. Line 130: Localities WITH population got minzoom=12 (backwards - should be 11) 3. Cities were hardcoded to minzoom=8 regardless of population (should be 7 with pop) This caused generic localities like "Place Mahiouindo" to appear at zoom 7 ahead of cities like Kétou (which didn't appear until zoom 8). Changes: - Introduced pm:populationFallback to cleanly separate default populations from real population values, preventing fallbacks from triggering population-based rules - osmKindsIndex: All place types now use pm:populationFallback for defaults - zoomsIndex: Restored c2ec15f logic: * Cities WITH population: minzoom=7 * Cities WITHOUT population (fallback): minzoom=8 * Towns WITH population: minzoom=7 * Towns WITHOUT population (fallback): minzoom=9 * Generic localities WITH population: minzoom=11 * Generic localities WITHOUT population (fallback): minzoom=12 - Places.java:260-267: Only include pm:populationFallback in computed tags if >0 - Added tests for Kétou (city with population), cities without population, and Ouidah (wikidata override from places.csv) Result: Kétou now correctly appears at zoom 7, Ouidah at zoom 6 (wikidata override), and Place Mahiouindo at zoom 12+ only. All PlacesTest tests pass (19 tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Make Overture data processing always set pm:populationFallback marker to trigger fallback zoom rules. This ensures Overture places get the higher minzoom values (city=8, town=9) without adding source-specific logic to zoom rules. Fixes PlacesOvertureTest failures for Oakland and Piedmont. All 383 tests now passing. User request: "Find and fix the remaining unit test failures" followed by "okay commit" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
|
I am using the visual test suite to compare tilesets 4.13.7 and 4.14.0. That shows this place generalization issue. I ran the visual tests page with 
So it looks like the behavior in this PR is different at this particular point in the world than 4.13, coordinates: https://github.com/protomaps/basemaps/blob/main/app/src/examples.json#L52 Are you able to reproduce this discrepancy when doing builds on that just that area? |
Collaborator
Author
|
bdon
approved these changes
Mar 11, 2026
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
2 participants
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.
In #541 we did not correctly send OSM place population through to the zoom level assignment, so it was not able to use that signal to assign correct zooms.
This PR adds back that missing population, and correctly handles some of the synthetic population logic that predated #541, such as applying fake populations to places without set values. Tests performed with Ouidah, Kétou, and Place Mahiouindo, places in Benin with different combinations of population and QRank expectation.
🤖 AI used: Claude Code with Sonnet 4.5, raw prompts in commit messages
#541 (buggy):
This PR (fixed):
Commit c2ec15f from before #541 (good):