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

Remove download-osm make-dc, gen bbox #297

Merged
merged 3 commits into from
Sep 28, 2020

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Jun 11, 2020

Simplify download-osm to only generate bbox value, without the overly complex (and unneeded) docker-compose config file.

New functionality:

  • --bbox will generate a simple text file containing the bounding box string. The value will be either computed based on the geofabrik's catalog, or for other sources it will continue using osmconvert
  • download-osm bbox <pbf-file> <bbox-file> will parse pbf file to generate the same text file with bbox string
  • The --output param is now required when used with --bbox

Removed functionality:

  • --make-dc parameter and make-dc command.

Simplify `download-osm` to only generate bbox value, without the overly complex (and unneeded) docker-compose config file.

New functionality:
* --bbox will generate a simple .env file with `BBOX=<....>` string. The value will be either computed based on the geofabrik's catalog, or for other sources it will continue using `osmconvert`
* `download-osm bbox <pbf-file> <env-file>` will parse pbf file to generate the same .env file with bbox string
* The --output param is now required when used with --bbox
@nyurik nyurik requested a review from TomPohys June 11, 2020 23:50
@TomPohys
Copy link
Member

Thanks @nyurik, looks great!

@nyurik nyurik merged commit 223dd07 into openmaptiles:master Sep 28, 2020
@nyurik nyurik deleted the download-osm-bbox branch September 28, 2020 20:37
@zstadler
Copy link
Contributor

zstadler commented Sep 29, 2020

Thanks @nyurik for simplifying the interface between download-osm and openmaptiles!

There are significant difference between the the polygon bbox of openstreetmap.fr and the bbox computed by osmconvert.
For example, the actual bbox of the israel_and_palestine polygon is 34.05,29.435,35.915,33.355 while the computed bbox is 25.1488889,22.0000374,36.8999418,33.4835140.
This is 18x increase in size that impacts the tile generation time.

openstreetmap.fr has its polygons at https://download.openstreetmap.fr/polygons/$(area}.poly.

Would it be possible to use them in download-osm for accurate bbox generation?

For example, https://download.openstreetmap.fr/polygons/europe/monaco.poly is

polygon
1
	7.406	43.732
	7.41	43.738
	7.419	43.743
	7.42	43.745
	7.425	43.746
	7.425	43.749
	7.429	43.753
	7.433	43.753
	7.436	43.756
	7.441	43.755
	7.443	43.752
	7.457	43.745
	7.537	43.538
	7.537	43.535
	7.535	43.533
	7.501	43.512
	7.497	43.514
	7.415	43.722
	7.408	43.725
	7.405	43.728
	7.406	43.732
END
END

For our purpose, we only care about the <lon> <lat> lines. All END lines as well as single number lines, like 1 and 2 should be ignored.

@nyurik
Copy link
Member Author

nyurik commented Sep 29, 2020

@zstadler sure, we could improve the bbox generation further. Let me know if you want to submit a PR for that.

@zstadler
Copy link
Contributor

@nyurik,

Unfortunately, python is not my forte...

zstadler added a commit to zstadler/openmaptiles that referenced this pull request Oct 1, 2020
Currently, the `BBOX` setting in `.env` is ignored for all areas except `planet`.
On the other hand, the `planet` area is an overkill for any `BBOX` setting other than the default - `-180.0,-85.0511,180.0,85.0511`.

With this PR, `quickstart.sh` would not override a modified `BBOX` value in `.env`. 

Also, this provides a way to avoid the pessimistic `BBOX` computation for `osmfr` extracts, as described in openmaptiles/openmaptiles-tools#297 (comment)
@zstadler
Copy link
Contributor

zstadler commented Oct 2, 2020

@nyurik,

Unfortunately, python is not my forte...

https://github.com/ustroetz/polygon2osm/blob/master/polygon2geojson.py converts a .poly file to geojson

@nyurik
Copy link
Member Author

nyurik commented Oct 2, 2020

@zstadler #326

TomPohys pushed a commit to openmaptiles/openmaptiles that referenced this pull request Oct 15, 2020
* Allow setting `BBOX` to be set in `.env` file

Currently, the `BBOX` setting in `.env` is ignored for all areas except `planet`.
On the other hand, the `planet` area is an overkill for any `BBOX` setting other than the default - `-180.0,-85.0511,180.0,85.0511`.

With this PR, `quickstart.sh` would not override a modified `BBOX` value in `.env`. 

Also, this provides a way to avoid the pessimistic `BBOX` computation for `osmfr` extracts, as described in openmaptiles/openmaptiles-tools#297 (comment)

#### Currently
- If the user does not do anything, `quickstart.sh` and `make generate-tiles` create an `mbtiles` file for the full extent of the data source. This applies to `planet` and other data sources.
- If a user sets the `BBOX` value in `.env` and a `planet` data source is used, `quickstart.sh` and `make generate-tiles` create an `mbtiles` file for the extent set in the `.env` file.
- If a user sets the `BBOX` value in `.env` and a non-`planet` data source is used, it is ignored - `quickstart.sh` and `make generate-tiles` create an `mbtiles` file for the full extent of the data source.

#### Problem statement
While users of a `planet` data source have a simple way to override the default extend of the tile generation, users of other data sources have no simple way of doing that. In fact, for such users the `BBOX` setting in the `.env` file is ignored and therefore misleading.

#### Proposal
- If the user does not do anything, `quickstart.sh` and `make generate-tiles` create an `mbtiles` file for the full extent of the data source. This applies to `planet` and other data sources.
- If a user sets the `BBOX` value in `.env`, `quickstart.sh` and `make generate-tiles` create an `mbtiles` file for the extent set in the `.env` file. This applies to `planet` and other data sources.
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.

3 participants