Skip to content

Conversation

@migurski
Copy link
Contributor

@migurski migurski commented Dec 29, 2025

Follow up to #537.

Migrate from plain Java conditionals to MultiExpression rules when applying POI minZoom. Removes most dense logic from processOsm(), passes all existing tests with no changes required.

To recreate wayArea and height span logic, introduce range expressions for rules:

  • withinRange(attr, lower, upper) – true when named attribute is between lower (inclusive) and upper (exclusive) bounds
  • atLeast(attr, lower) – true when named attribute is greater than or equal to lower (inclusive)

To separate into multiple rules passes to for readability (kinds followed by zooms for either points or polyons), introduce SourceFeatureWithComputedTags wrapper for source features with a mutable tags map.

@migurski migurski marked this pull request as ready for review December 29, 2025 18:39
@wipfli
Copy link
Collaborator

wipfli commented Jan 4, 2026

Good work, thanks for opening this pull request @migurski. Some thoughts from my side:
withinRange should always have a lower and an upper bound such that it is always a range. If we need lessThan or greaterThan operators then let us introduce them explicitly.
Additional tags can be added directly to existing source features. I think I did this somewhere in Roads.java or Place.java to get country-specific logic. The additional new class with extra tags s overkill seems overkill to me. Keep up the good work!

@migurski
Copy link
Contributor Author

migurski commented Jan 5, 2026

Good work, thanks for opening this pull request @migurski. Some thoughts from my side: withinRange should always have a lower and an upper bound such that it is always a range. If we need lessThan or greaterThan operators then let us introduce them explicitly.

In this case, we’ll generally always need them for the unbounded right-hand side of a range. I’ll update this PR with an atLeast operator.

Additional tags can be added directly to existing source features. I think I did this somewhere in Roads.java or Place.java to get country-specific logic. The additional new class with extra tags s overkill seems overkill to me. Keep up the good work!

I thought this was the case and even had it passing unit tests, but ran into a bunch of errors when I actually tried doing it in a real processing run:

0:00:38 ERR [osm_pass2:process] - Error processing OSM Way 1452790668
java.lang.UnsupportedOperationException
	at java.base/java.util.AbstractMap.put(AbstractMap.java:213)
	at com.onthegomap.planetiler.reader.WithTags.setTag(WithTags.java:147)
	at com.protomaps.basemap.layers.Pois.calculateDimensions(Pois.java:248)
	at com.protomaps.basemap.layers.Pois.processOsm(Pois.java:259)
	at com.onthegomap.planetiler.ForwardingProfile.processFeature(ForwardingProfile.java:217)
	at com.onthegomap.planetiler.reader.osm.OsmReader.render(OsmReader.java:471)
	at com.onthegomap.planetiler.reader.osm.OsmReader.lambda$pass2$7(OsmReader.java:363)
	at com.onthegomap.planetiler.worker.WorkerPipeline$Builder.lambda$addWorker$0(WorkerPipeline.java:249)
	at com.onthegomap.planetiler.worker.Worker.lambda$new$0(Worker.java:41)
	at com.onthegomap.planetiler.worker.Worker.lambda$new$1(Worker.java:68)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1825)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1090)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:614)
	at java.base/java.lang.Thread.run(Thread.java:1474)

The unsupported operation is why I switched to using a HashMap in 8821873.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 5, 2026

@wipfli
Copy link
Collaborator

wipfli commented Jan 5, 2026

@wipfli
Copy link
Collaborator

wipfli commented Jan 5, 2026

Above is how an additional _country tag is added to a source feature in Places.java

@migurski
Copy link
Contributor Author

migurski commented Jan 6, 2026

Above is how an additional _country tag is added to a source feature in Places.java

I saw this pattern elsewhere and used it, so I’m unsure why my own usage resulted in the UnsupportedOperationException above. Creating the new wrapper with a HashMap solved this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants