Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow easier modification of the OpenStreetMap-carto style #243

Merged
merged 27 commits into from
Mar 31, 2022
Merged

Allow easier modification of the OpenStreetMap-carto style #243

merged 27 commits into from
Mar 31, 2022

Conversation

galewis2
Copy link
Collaborator

@galewis2 galewis2 commented Jan 25, 2022

Now, the style isn't built into the image, but is now either hosted in a volume/local directory or worse case, fetched at runtime.

It uses a wildcard to find critical style elements (like index.sql, lua file, etc) so there's some uncontrolled flexibility in different file names.

Would replace #42 and #97

Still need to add some documentation, but here's the existing work.

@galewis2 galewis2 marked this pull request as draft January 25, 2022 00:37
@galewis2 galewis2 marked this pull request as ready for review January 25, 2022 19:57
@galewis2
Copy link
Collaborator Author

This is based on the other PR I have, which improves the updating experience.

run.sh Outdated
@@ -75,13 +91,14 @@ if [ "$1" = "import" ]; then
fi

# Import data
sudo -u renderer osm2pgsql -d gis --create --slim -G --hstore --tag-transform-script /home/renderer/src/openstreetmap-carto/openstreetmap-carto.lua --number-processes ${THREADS:-4} -S /home/renderer/src/openstreetmap-carto/openstreetmap-carto.style /data.osm.pbf ${OSM2PGSQL_EXTRA_ARGS:-}
sudo -u renderer osm2pgsql -d gis --create --slim -G --hstore --tag-transform-script /home/renderer/src/openstreetmap-carto/*.lua --number-processes ${THREADS:-4} -S /home/renderer/src/openstreetmap-carto/*.style /data.osm.pbf ${OSM2PGSQL_EXTRA_ARGS:-}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if I have multiple styles/transform scripts?

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 designed this to assume there's only one style/transform script, or just take the first one.

In hindsight, I will make a variable but give it defaults.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made improvements, now you can specify the style element names as necessary. See the readme

@dbahrdt
Copy link
Collaborator

dbahrdt commented Feb 10, 2022

As general question:
How do we want to merge request? Do we squash the changes before merging? If not then I would strongly recommend to have useful commits. E.g. no "WIP" commits and the like.

@Overv
Copy link
Owner

Overv commented Feb 11, 2022

@dbahrdt Yeah, I think all PRs should use squash merges as a general rule.

@galewis2
Copy link
Collaborator Author

galewis2 commented Feb 14, 2022

@dbahrdt Yea, I always do squash commits to keep main tidy. I expect the dev branches to be polluted with non-useful commits as it's not really helpful or necessary to have useful commits on the dev branches.

@galewis2 galewis2 marked this pull request as draft February 17, 2022 20:00
@galewis2 galewis2 requested a review from dbahrdt February 18, 2022 14:59
@galewis2 galewis2 marked this pull request as ready for review February 18, 2022 15:48
@tommasodelorenzo
Copy link

@galewis2 . Ok thanks.
I would bother you with an additional quick question: if I make a small change to my style, is there a simple way to apply those changes without re-running the import?
The fact is that when running the import I get errors such as

+ sudo -u postgres createuser renderer
createuser: error: creation of new role failed: ERROR:  role "renderer" already exists

So I have to remove all data and re-run the import. This takes a lot of time.

@galewis2 galewis2 mentioned this pull request Feb 21, 2022
@galewis2 galewis2 closed this Feb 24, 2022
@galewis2 galewis2 deleted the patch-2 branch February 24, 2022 14:12
@galewis2 galewis2 restored the patch-2 branch February 24, 2022 14:12
@galewis2 galewis2 reopened this Feb 24, 2022
run.sh Outdated
exit 1
fi

set -x

if [ ! "$(ls -A /home/renderer/src/openstreetmap-carto)" ]; then

git clone --single-branch --branch v5.3.1 https://github.com/gravitystorm/openstreetmap-carto.git --depth 1 /home/renderer/src/openstreetmap-carto
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should include a copy of these files instead of downloading them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Especially as we are cloning this repo both on import and on run

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

run.sh Show resolved Hide resolved
@nielsole
Copy link
Collaborator

nielsole commented Feb 25, 2022

So I have to remove all data and re-run the import. This takes a lot of time.

The lua script transforms the data during import. The indexes/views might be different which could lead to performance problems if the script is not rerun.
I guess if you just changed the mml you should be able to omit the import step. Either way: best to test your styles on a small dataset :)
EDIT: Ah, this question was already answered in a separate issue: #249 (comment)

exit 1
fi

set -x

if [ ! "$(ls -A /home/renderer/src/openstreetmap-carto)" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

if [ ! -d /home/renderer/src/openstreetmap-carto ]; then

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if [ ! "$(ls -A /home/renderer/src/openstreetmap-carto)" ]; then
if [ ! -d /home/renderer/src/openstreetmap-carto ]; then

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 reverted this back as I need to confirm that openstreetmap-carto is empty, not just the directory exists (like you specify a bad path or something)

Copy link
Collaborator

@nielsole nielsole left a comment

Choose a reason for hiding this comment

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

Great PR!
I haven't setup custom styling myself yet, so my assessments may be off.
I think this could be further improved:

I am a big fan of incremental improvement, so maybe we can mark this as "beta"/experimental or something in the README and include 1-2 styles that are known to work and then merge it?

Co-authored-by: Robin C. Ladiges <Istador@users.noreply.github.com>
@galewis2 galewis2 changed the title Allow flexibility in style Allow easier modification of the OpenStreetMap-carto style Feb 28, 2022
Dockerfile Outdated Show resolved Hide resolved
@galewis2
Copy link
Collaborator Author

galewis2 commented Mar 15, 2022

Great PR! I haven't setup custom styling myself yet, so my assessments may be off. I think this could be further improved:

* README: Where can I find other styles? (Just one other example would already be great)

* better support lack of lua script: e.g. https://github.com/cyclosm/cyclosm-cartocss-style doesn't have a lua script and `--tag-transform-script` is optional

* Better support for multiple SQL scripts e.g.: https://github.com/OpenRailwayMap/OpenRailwayMap-CartoCSS/blob/master/SETUP.md has three SQL scripts that need to be executed in order

I am a big fan of incremental improvement, so maybe we can mark this as "beta"/experimental or something in the README and include 1-2 styles that are known to work and then merge it?

I specifically for now targeted this PR to make it easier to modify the existing OpenStreetMap Carto style with small changes, with small in place changes like labelling, colors, etc.

I did add a comment in the readme that openstreetmap-carto and styles like that are supported.

@galewis2 galewis2 requested a review from nielsole March 15, 2022 16:59
nielsole
nielsole previously approved these changes Mar 27, 2022
@galewis2
Copy link
Collaborator Author

@nielsole Could I get another approval, just had to do a conflict resolve after #248 , thx!

@galewis2 galewis2 requested a review from nielsole March 28, 2022 13:56
Copy link
Collaborator

@nielsole nielsole left a comment

Choose a reason for hiding this comment

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

sorry for taking another 3 days for reapproval

@galewis2 galewis2 merged commit 0f229a0 into Overv:master Mar 31, 2022
@galewis2 galewis2 deleted the patch-2 branch March 31, 2022 11:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants