OpenMapTiles 3.15.0 SNAPSHOT (3) #160
Merged
Conversation
This file contains 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
…to match content of OMT PR 1489)
…ding on feature size (matching OMT PR 1579)
…er available at Z8
…va_code_regeneration Java code regenerated from latest OMT YML
Update readme links
handle duplicate route relations
Add indigenous land boundaries
…lapse Convert semi-colon separated house numbers to a range
The difference is between OpenMapTiles/master (OMT) and planetiler-openmaptiles/omt_3_15_0 (PT-OMT) (e.g. development versions). The point is, that while PT-OMT was using limit of "8km" for Z9-Z11, OMT is using limit "ST_Length(geometry) > 8000 / POWER(2, zoom_level - 9) AND zoom_level BETWEEN 9 AND 11". Some further differences still visible, hence further commits expected.
... (as per OMT PR 1486) But FERRY_MIN_PIXEL_SIZE is "too much" in the contexct of Planetiler, since it is applied within tiles, hence causes gaps in lines if a line "strikes a little" certain tile. Hence we will need to divert a little.
hence the results are much closer to what OMT is doing for Z4-Z9
ferry test polygon with area 1 now qualifies for min. zoom 5
…d_on_review-6 Updates based on review - round 5 - correction 1
…d_on_review-6 Updates based on review - round 5 - correction 2
msbarry
reviewed
May 2, 2024
| attemptNeLakeIdMapping(element); | ||
| } catch (GeometryException e) { | ||
| e.log(stats, "omt_water", | ||
| "Unable to process intersections", config.logJtsExceptions()); |
There was a problem hiding this comment.
Can you clarify in this error log that we are "unable to add OSM ID to natural earth water feature" so when people see errors they know we might just be missing those attributes, as opposed to missing entire water features.
There was a problem hiding this comment.
Adjusted.
| if (geom.isValid()) { | ||
| lakeInfo.geom = geom; | ||
| } else { | ||
| LOGGER.warn("Fixing geometry of NE lake {}", feature.getLong("ne_id")); |
There was a problem hiding this comment.
Ahh interesting I didn't expect invalid geometries from natural earth... that would cause problems though. This log could probably just be info/debug
There was a problem hiding this comment.
Adjusted.
| intersection = neGeom.intersection(geom); | ||
| } catch (TopologyException e) { | ||
| stats.dataError("omt_water_intersection"); | ||
| LOGGER.debug("omt_water_intersection, NE ID: {}, OSM ID: {}", |
There was a problem hiding this comment.
Maybe add a little bit more context, like "fixed invalid OSM water polygon {} when adding ID to natural earth {}"
There was a problem hiding this comment.
I've removed this one based on previous comments. And adjusted the GeometryException bellow.
Comment on lines
223
to
224
| LOGGER.debug("omt_water_intersecting, OSM ID: {}", | ||
| element.source().id(), e); |
There was a problem hiding this comment.
Maybe add a little bit more context, like "fixed invalid OSM water polygon {} when looking up natural earth features"
There was a problem hiding this comment.
I've removed that one based on our exchange in Slack + the note bellow this one.
| stats.dataError("omt_water_intersecting"); | ||
| LOGGER.debug("omt_water_intersecting, OSM ID: {}", | ||
| element.source().id(), e); | ||
| final var geomFixed = GeometryFixer.fix(geom); |
There was a problem hiding this comment.
can you overwrite geom here so that fillOsmIdIntoNeLake doesn't need to fix the geometry again?
There was a problem hiding this comment.
Adjusted.
msbarry
reviewed
May 2, 2024
| } catch (TopologyException e) { | ||
| stats.dataError("omt_water_intersecting"); | ||
| LOGGER.debug("omt_water_intersecting, OSM ID: {}", | ||
| element.source().id(), e); |
There was a problem hiding this comment.
also let's omit the exception here and on line 253 below - there's no sense digging into JTS exceptions unless we're sure the input is valid.
There was a problem hiding this comment.
Makes sense, removed.
since we are not able to raise issues with JTS if we do stuff with invalid/unfixed polygons
…illOsmIdIntoNeLake()
…ons" This reverts commit 7440973.
This is same trick as done for NE lakes before. Thanks to that we then do not need to handle TopologyExceptions and do retries.
…d_on_review-6 Updates based on review - round 5 - correction 3
|
Regarding latest batch of "adjusted" comments from me: You can see that only now, since only now I've pushed and merged those changes. Sorry for the delay and confusion. |
msbarry
approved these changes
May 3, 2024
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| Copyright (c) 2021, MapTiler.com & OpenMapTiles contributors. | |||
| Copyright (c) 2023, MapTiler.com & OpenMapTiles contributors. | |||
There was a problem hiding this comment.
I notice you've got 2023 in a few of these headers
There was a problem hiding this comment.
Ah, @TomPohys was asking about the same.
TL;DR: IANAL hence I tend to put there a year of "most recent substantial change". This particular file has such in 2023.
Longer: Looking through various IP FAQs and Open Source licensing FAQs, etc. there seems to be varying degree of advice, from "does not matter" to "really important", with various tactics about when and how to change that value. And since I'm not going to hire a layer (which is IMHO important if we truly need a legal advice about for example what "substantial change" means), then I outline my rationale:
- Extreme position 1: Bump the copyright year every year even when no changes were done = IMHO totally bogus and IANAL but legally unenforceable.
- Extreme position 1: Put there a year at the beginning and then never bump it, even if the file goes through substantial changes all the time = IMHO also wrong, but sort of "wronging myself" (e.g. possibly loosing protections which I'm entitled to, but hey, copyright protection is supposed to run even several years after my death, so ...).
Hence I opt to be "somewhere in between", e.g. bump the year when substantial changes occur.
| if (!geom.isValid()) { | ||
| geom = GeometryFixer.fix(geom); | ||
| stats.dataError("omt_fix_water_before_ne_intersect"); | ||
| LOGGER.debug("Fixing geometry of OSM element {} before attempt to add ID to natural earth water feature", element.source().id()); |
There was a problem hiding this comment.
CI is complaining about this line, looks like you need to run ./scripts/format.sh
There was a problem hiding this comment.
Thanks, adjusted.
…d_on_review-6 Updates based on review - round 5 - correction 4
|
Merged. Release planned for Monday. E.g. if no serious showstopper is found until than, this will be 3.15. |
ofalvai
pushed a commit
to ofalvai/planetiler-cycling-infra
that referenced
this pull request
Jul 12, 2024
* clean-up: mvn spotless:apply
* Long ferries (as per OMT PR 1486)
* regenerate-openmaptiles.sh b3d67ed5b327c9059aeea0b3304772c6b4c8c7e9 (to match content of OMT PR 1489)
* Add aboriginal lands (as per OMT PR 1489)
* handle duplicate route relations (to match OMT PR 1501)
* regenerate-openmaptiles.sh master, to match several OMT PRs which adjusted only YML
* URLs in comments adjusted to match OMT PR 1560
* Convert separated addresses to dashed addresses
* add brunnel (and layer) attributes only for certain zoomlevels, depending on feature size (matching OMT PR 1579)
* unit test testInterstateMotorway(): brunnel tag for test line no longer available at Z8
* unit test testInterstateMotorway() clean-up: Z13 was tested twice
* minor clean-up: fixed unit test naming
* partial fix for differences in transportation_name layer
The difference is between OpenMapTiles/master (OMT) and
planetiler-openmaptiles/omt_3_15_0 (PT-OMT) (e.g. development versions).
The point is, that while PT-OMT was using limit of "8km" for Z9-Z11, OMT
is using limit "ST_Length(geometry) > 8000 / POWER(2, zoom_level - 9)
AND zoom_level BETWEEN 9 AND 11".
Some further differences still visible, hence further commits expected.
* further adjustments to better match what is done with ferries in OMT
... (as per OMT PR 1486)
But FERRY_MIN_PIXEL_SIZE is "too much" in the contexct of Planetiler,
since it is applied within tiles, hence causes gaps in lines if a line
"strikes a little" certain tile. Hence we will need to divert a little.
* ferry minLength tweak + clean-up
* mvn spotless:apply
* fixed minor typo
* minor reformatting
* ferry line length filter replaced with min. zoom calculation
hence the results are much closer to what OMT is doing for Z4-Z9
* testFerry() adjusted to match previous commit
ferry test polygon with area 1 now qualifies for min. zoom 5
* clea-up of unused stuff + mvn spotless:apply
* mvn spotless:apply
* added TODO node for follow-up pull-request/simplification
* clean-up: common getMinZoom() code moved to Utils
* minzoom clipping for brunnel was adjusted do Z9-Z12 -> test adjusted too
* clean-up
* use same tolerance for all transportation items, like OSM does
* clean-up, since ferry and non-ferry procesing is now same
* we need regenerate to work with master branch for now
* first sub-class search for agg_stop simplified a little
* contains() used instead of indexOf() for better readability
* numbers as list, not array, so that getFirst() and getLast() can be used
* better trimming and filtring of housenumbers
* adjusted handling of large house numbers
* several unit tests collapsed to one with @ParameterizedTest + @CsvSource
* AGG_STOP_SUBCLASS_ORDER simplified from Map to List
* fixed major omission from previous commit
* clamp() used to replace min()&max() combo
* agg_stop now implemented
* fixed typo in the error message
* prepare IE and GB boundary geometry outside of synchronized{}
* fixed typo in the error message
* mvn spotless:apply
* switch statements for IE and GB route networks simplified
* avoid RouteNetwork->String mapping, not needed for anyMatch()
* fix: attr. brunnel optional based on size on Z4-Z11, attr. layer optional between Z9-Z11
* tolerance change in transportation reverted, added note to README as per why
* fix: monzoom for sea&co. is Z0-Z14 based on area, for the rest it is Z3-Z14 again based on area
* clean-up: avoid doing area->side->area, do just area
* regenerate-openmaptiles.sh 6c31841f4674f15e15afde346a060cf7c22e6cdd (to match content of OMT PR 1591)
* relevant process() functions adjusted to match changes in transportation/mapping.yaml
* regenerate-openmaptiles.sh master, instead of 6c31841f4674f15e15afde346a060cf7c22e6cdd (to match content of OMT PR 1591, in a cleaner way)
* introduce duplicate housenumber filtering (matching OMT PR 1391)
* (less related) clean-up: use isEmpty() instead if size check
* testContainsHousenumber UT adjusted, since duplicate housenumber filtering is reducing amount of house numbers
* use combination of uic_ref, name, network and operator as key for agg_stop sets
If we rely on only on `uic_ref` we group together also stations which are
too far apart (even different cities). With this combo results seem OK,
e.g. all grouped stations are within around 950m (1000 pixels at Z14) of
each other (1000 being used in `PARTITION BY LabelGrid(...` in
`layers/poi/poi.sql` in OpenMapTiles).
* agg_stop comparison made more explicit, since we want to match same exact one
* mvn spotless:apply
* name now important for agg_stop processing, hence name:es (ab)used for unit tests
* agg_stop: simplified processing of nearest station
Results still same, only ordering is different:
- previously: agg_stop=1 first
- now: FIFO
* agg_stop: forther code simplification
* fixed major typo introduced in previous merge
* setMinPixelSize() + setMinZoom() used instead of areaToMinZoom()
* clean-up: unused stuff removed
* mvn spotless:apply
* setAttrWithMinSize() used instead of getBrunnelMinzoom()
getFerryMinzoom() kept since we'd like to replicate `sql_filter: ST_Length(...` from OMT
* getMinZoomForLength() no longer used, hence removed
* MIN_LENGTH value halved, to partially counter the effect of Planetiler applying the limit before merging
* clean-up: LOG2 not used, hence removed
* Revert "MIN_LENGTH value halved, to partially counter the effect of Planetiler applying the limit before merging"
This reverts commit 8fb6707.
* use 256px as buffer pixel override instead of MIN_LENGTH
Make sure we have enough room (=whole next tile) when checking MIN_LENGTH,
to avoid pieces of otherwise "long enough" lines to be excluded (thus
creating "holes" or missing ends) in some tiles just because length in that
particular tile is bellow limit. Given that MIN_LENGTH values translated to
pixels are quite big (compared to {@code BUFFER_SIZE}), such missing pieces
would be quite noticeable.
This improves items mainly in Z12-Z13.
* added BY_TEMP_HAS_NAME comparator to avoid its repeated construction during run-time
* duplicate houcenumber processing simplified further
* clean-up: get(0) replaced with getFirst()
* clean-up: CPU-intensive prepare() moved out of synchronized block
* regenerate-openmaptiles.sh 3cf77e2a542d8a369bb08bf2538cdde0b3effb2b (to match content of OMT PR 1423)
* unit test adjusted for POI office class changes
* regenerate-openmaptiles.sh master (to match content of OMT PR 1544)
* added charging_station implementation matching OMT PR 1544
* use setMinPixelSizeBelowZoom() instead of uniAreaToMinZoom()
* use setMinPixelSizeBelowZoom() instead of getFerryMinzoom()
* fixed unit test, to match recent tweaks
* regenerate-openmaptiles.sh master (to match content of OMT PR 1605)
* Fix university office/amenity collision (to match OSM PR 1607)
* Remove expressway from ramps
* OsmMarinePoint+ne_10m_geography_marine_polys join via name limited to 50km distance
* regenerate-openmaptiles.sh master (to match content of OMT PR 1604)
* Revert "Add aboriginal lands (as per OMT PR 1489)"
This reverts commit 899a0c5.
* added handling of aboriginal_lands/OsmBoundaryPolygon into Boundary and Place layers
* big islands can now get rank lower than 3
* Add e-road and a-road for transportation z4
* testPolishHighwayIssue165() adjusted: e-road now takes precendence
* regenerate-openmaptiles.sh master (to match content of OMT PR 1627)
* regenerate-openmaptiles.sh fff7110aeb61882abfafe22d1618fbe6181d96cb (to match content of OMT PR 1620)
* Expanded road route attributes (to match OMT PR 1620)
* unrelated clean-up: use getFirst()
* adjusted handling or null and empty ref
* regenerate-openmaptiles.sh master (to match content of OMT PR 1620)
* pointOnSurface() used instead of centroid() to make sure the point is within the boundary
* adjusted deduplication of route_<n>_<something> attributes
* clean-up of some previous adjustments of importantMarinePoints
* clean-up: removed unused imports
* mvn spotless:apply
* OsmWaterPolygon minzoom calculation reverted to v3.14 and then adjusted for new 1/4 threshold
* part of process(Tables.OsmWaterPolygon, ...) moved to setupOsmWaterPolygonFeature()
* if feature with centerline has minzoom bellow Z9, use also a label point between minzoom and Z8
* bay point vs. centerline difference mentioned in the README
* testWaterNameBay for small bay adjusted, testWaterNameBay for big bay added
* testWaterNameLakeline and testWaterNameMultipleLakelines tweaked to use smaller test areas
* mvn spotless:apply
* added unit test which checks minzoom for admin_level=2 disputed border (derived from way)
* fixed missing admin_level=2 disputed borders at Z3
* landuse fix for Z5: include all features from ne_50m_urban_areas
* minDist and buffer tweaked for residential areas for Z6-Z8 to better match OpenMapTiles
* landuse fix for Z5: unit test updated
* bumped copyright statements
* added/adjusted acknowledgments
* fixed handling of Baikonur border in Kazachstan
* fixed handling of Baikonur border in Kazachstan (2)
* OSM boundaries only for Z5+, non-disputed for Z4+
* OSM boundaries only for Z5+, non-disputed for Z4+ - unit test adjusted
* OSM Lake IDs at low zooms
* testLakeZoomLevels(): NE lakes removed, checked now in testLakeNaturalEarth*()
* Use ne_10m_admin_0_boundary_lines_land_disputed to fix missing South Sudan boundary
* testNaturalEarthCountryBoundaries() extended to cover adjusted handling of ne_10m_admin_0_boundary_lines_land
* OSM lake area limit increased 2x to match smallest NE lake
* OSM lake area limit increased 2x even further to match smallest observed during Planet processing
* clean-up: SMALLEST_OSM_LAKE_AREA renamed to OSM_ID_MATCH_AREA_LIMIT to better match what it is
* mvn spotless:apply
* properly handle center line vs. point also for lakes
* clean-up: min() dropped since nothing sets minzoomCL to value higher than MINZOOM_BAY
* Revert e-road and a-road.
* Revert e-road and a-road. (follow-up)
* added unit test to cover: More roads for z4.
* improved handling of disputed KE:SS border: will work even if left/right reversed
* new PolygonIndex.getIntersecting() used for OSM lake ID matching + increased unit test coverage
* neLakeIndexes simplified into neLakeIndex
* test several NE lakes also in testLakeNaturalEarthByName()
* clean-up: synchronized NOT needed for PolygonIndex.put()
* neLakeNameMaps will stay as is, otherwise same names from several NE tables will clash
* neAllLakeInfos stays, since neLakeNameMaps contains only some and hard(er) to get all from neLakeIndex
* added test for NE lake ame collision: bigger one gets to be matched
* simplified NE->OSM lake matching: ignore small OSM lakes right away
* minor speed-up: compare intersection area instead of intersction area ratio
* fixed minor typo
* minor clean-up
* minor clean-up after previous changes
* adjusted handling of TopologyException to match previous tweaks
* mvn spotless:apply
* tweaked error message, to better mach cases when TopologyException occurs
* adjusted trunks: Z6+ by default, some from Z5, some even from Z4
* adjusted motorways: only some at Z4
* adjusted transportation: clip also construction links to Z9+
* unit tests adjusted to the recent changes
* use merge() to avoid problems with concurrent updates
* synchronized mergeId() added to avoid problems with concurrent updates
* clean-up after cc797f9
* mvn spotless:apply
* label point in water_name refactored using setMinPixelSizeBelowZoom()
* clean-up: LOG2 removed since no longer used
* refactored handling of TopologyException for easier troubleshooting of problematic polygons
* fix invalid NE lake geometries + further TopologyException handling tweaks
* TopologyException handling removed, since no longer needed after adding NE geometry fixing
* clean-up after previous changes
* OpenMapTiles 3.15 was released so we can use proper tag now
* regenerate-openmaptiles.sh v3.15 to match official OpenMapTiles release
* Revert "clean-up after previous changes"
This reverts commit ee70382.
* Revert "TopologyException handling removed, since no longer needed after adding NE geometry fixing"
This reverts commit d4fe59c.
* minor clean-up
* handle TopologyException also from intersects()
* further tweaks for TopologyException handling
* warnings demoted to debugs since we're going to re-try
* bumped copyright statements
* bumped copyright statements
* print errors+stack-traces only after retries with fixed polygons
since we are not able to raise issues with JTS if we do stuff with
invalid/unfixed polygons
* message about fixing NE lake geometry demoted: warn -> debug
* omt_water message adjusted to make it more clear what it relates to
* do NOT store fixed geom in separate variable to avoid 2nd fixing in fillOsmIdIntoNeLake()
* one more error message rephrased for clearer context
* Revert "print errors+stack-traces only after retries with fixed polygons"
This reverts commit 7440973.
* strip stack-trace from TopologyException debug messages
* fix OSM element geometry if not valid before trying to match NE lake
This is same trick as done for NE lakes before.
Thanks to that we then do not need to handle TopologyExceptions and do
retries.
* clean-up
* clean-up: mvn spotless:apply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up on #144 , contains bugfixes for problems found while comparing tilesets generated with 3.15.0 SNAPSHOT of OpenMapTiles and
planetiler-openmaptilesand also final bump of version from3.14.0to3.15.0: