Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean up water logic #411

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

wipfli
Copy link
Collaborator

@wipfli wipfli commented Mar 7, 2025

When preparing water unittests I noticed that there are some things to clean up in the Water.java file. For example, if one checks for sf.hasTag("name") it is not necessary to later check again for sf.getTag("name") != null...

Comment on lines -40 to -41
var alkaline = 0;
var reservoir = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are unused variables

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties are in tilezen, should we keep them for now and add them to the output vector tiles?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should treat this as cleanup for now, and also investigate how the Tangram styles ever used the alkaline/reservoir properties, because if they were never symbolized we don't need them...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find the keyword alkaline in the tangrams:

https://github.com/search?q=org%3Atangrams+alkaline&type=code

lakes are there:

https://github.com/search?q=org%3Atangrams%20lake&type=code

reservoir are there too, but not as key but rather as kind value:

https://github.com/search?q=org%3Atangrams+reservoir&type=code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to remove the vector tile features properties alkaline and reservoir

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed also the OSM reservoir / alkaline

@wipfli
Copy link
Collaborator Author

wipfli commented Mar 7, 2025

CI seems to fail because it cannot find the geojson reader in planetiler. Strange...

Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  /home/runner/work/basemaps/basemaps/tiles/src/main/java/com/protomaps/basemap/feature/CountryCoder.java:[3,48] package com.onthegomap.planetiler.reader.geojson does not exist
Error:  /home/runner/work/basemaps/basemaps/tiles/src/main/java/com/protomaps/basemap/postprocess/Clip.java:[13,48] package com.onthegomap.planetiler.reader.geojson does not exist

@wipfli
Copy link
Collaborator Author

wipfli commented Mar 7, 2025

Now it says

[INFO] Downloading from nexus-snapshots: https://s01.oss.sonatype.org/content/repositories/snapshots/com/onthegomap/planetiler/planetiler-core/0.8.4-SNAPSHOT/planetiler-core-0.8.4-SNAPSHOT.pom
Warning:  The POM for com.onthegomap.planetiler:planetiler-core:jar:0.8.4-SNAPSHOT is missing, no dependency information available
Warning:  The POM for com.onthegomap.planetiler:planetiler-core:jar:tests:0.8.4-SNAPSHOT is missing, no dependency information available

@wipfli
Copy link
Collaborator Author

wipfli commented Mar 7, 2025

Since themeMinZoom is actually never used, there is another logic error which gets hidden by feature merge. The great lakes for example are always taken from the 10 m and the 50 m NE source layers. They appear only once in the final tiles because of feature merge post processing. But if we disable feature merge those appear twice:

image

@wipfli
Copy link
Collaborator Author

wipfli commented Mar 7, 2025

Using themeMinZoom again now

@wipfli wipfli marked this pull request as draft March 9, 2025 18:24
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