Fix area thresholds using floating point division#620
Open
candux wants to merge 1 commit into
Open
Conversation
Landuse post-processing and the shared Area name-stripping threshold used scaled area formulas such as:
400 / (4096 * 4096) * (256 * 256)
30000 / (4096 * 4096) * (256 * 256)
In Java these are evaluated with integer arithmetic because every operand is an integer literal. A one-line JShell check shows the bug:
printf 'System.out.println(400 / (4096 * 4096) * (256 * 256));\nSystem.out.println(30000 / (4096 * 4096) * (256 * 256));\n/exit\n' | jshell -q
jshell> 0
jshell> 0
The fixed expressions force floating point division by making the numerator a double literal:
printf 'System.out.println(400d / (4096 * 4096) * (256 * 256));\nSystem.out.println(30000d / (4096 * 4096) * (256 * 256));\n/exit\n' | jshell -q
jshell> 1.5625
jshell> 117.1875
The visible production effect today is in Landuse: the min-area filter at z<15 was disabled because the computed minArea was 0. The Area.java change fixes the same latent integer arithmetic in the shared name-stripping threshold, but current callers do not emit name tags through that branch, so it produced no visible Saarland PMTiles delta in this comparison.
git blame traces both bad expressions to commit 1c38928 (Brandon Liu, 2023-03-28 21:26:42 +0800, "finish port of area logic"). That same commit added Buildings.java calling Area.filterArea(items, 0), so the shared name-stripping threshold has been zero since Area.filterArea was introduced.
Add a Landuse regression test at z14 so a sub-threshold polygon is dropped while a larger polygon survives. This exercises the coordinate-space threshold directly and would fail if the formula regressed to integer division.
Saarland PMTiles comparison against origin/main using saarland-latest.osm.pbf:
- landuse archive: 16,101,245 bytes before, 16,073,949 bytes after (-27,296)
- landuse decoded features: 219,609 before, 218,167 after (-1,442)
- landuse feature deltas by zoom: z8 -15, z9 -29, z10 -97, z11 -174, z12 -273, z13 -420, z14 -434; z6, z7, and z15 unchanged
- buildings archive: 20,368,010 bytes before and after, identical SHA256
- buildings decoded features: 1,007,114 before and after
Verification: mvn -q -Dtest=LanduseTest test, mvn -q package, mvn -q spotless:check, git diff --check, and before/after Saarland PMTiles scans.
AI assistance: this change was prepared with help from OpenAI Codex.
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.
Landuse post-processing and the shared Area name-stripping threshold used scaled area formulas such as:
In Java these are evaluated with integer arithmetic because every operand is an integer literal. A one-line JShell check shows the bug:
The fixed expressions force floating point division by making the numerator a double literal:
The visible production effect today is in Landuse: the min-area filter at z<15 was disabled because the computed minArea was 0. The Area.java change fixes the same latent integer arithmetic in the shared name-stripping threshold, but current callers do not emit name tags through that branch, so it produced no visible Saarland PMTiles delta in this comparison.
git blame traces both bad expressions to commit 1c38928 (Brandon Liu, 2023-03-28 21:26:42 +0800, "finish port of area logic"). That same commit added Buildings.java calling Area.filterArea(items, 0), so the shared name-stripping threshold has been zero since Area.filterArea was introduced.
Add a Landuse regression test at z14 so a sub-threshold polygon is dropped while a larger polygon survives. This exercises the coordinate-space threshold directly and would fail if the formula regressed to integer division.
Saarland PMTiles comparison against origin/main using saarland-latest.osm.pbf:
landuse archive: 16,101,245 bytes before, 16,073,949 bytes after (-27,296)
landuse decoded features: 219,609 before, 218,167 after (-1,442)
landuse feature deltas by zoom: z8 -15, z9 -29, z10 -97, z11 -174, z12 -273, z13 -420, z14 -434; z6, z7, and z15 unchanged
buildings archive: 20,368,010 bytes before and after, identical SHA256
buildings decoded features: 1,007,114 before and after
Verification: mvn -q -Dtest=LanduseTest test, mvn -q package, mvn -q spotless:check, git diff --check, and before/after Saarland PMTiles scans.
AI assistance: this change was prepared with help from OpenAI Codex.