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

Add bbox plugin #14

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Add bbox plugin #14

merged 3 commits into from
Mar 28, 2024

Conversation

pka
Copy link
Contributor

@pka pka commented Mar 26, 2024

Adaption of the t-rex plugin for bbox. Replaced the toml dependency, because its installation was too cumbersome for me.

Not tested yet with shortbread_gen and will try to make a simpler implementation like the tilekiln plugin.

@joto
Copy link
Contributor

joto commented Mar 26, 2024

Can you have a look at the errors https://github.com/osm2pgsql-dev/osm2pgsql-themepark/actions/runs/8433369573/job/23096899346#step:4:79 ?

You can also run these locally using luacheck.

@pka
Copy link
Contributor Author

pka commented Mar 26, 2024

Now tested with shortbread_gen.

@pka pka marked this pull request as ready for review March 26, 2024 16:41
@pka
Copy link
Contributor Author

pka commented Mar 26, 2024

In the tilekiln implementation, the buffer size seems to be zoom level dependent. Should that also be supported by other implementations?

Copy link
Contributor

@joto joto left a comment

Choose a reason for hiding this comment

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

A few tiny issues to fix, but apart from that it looks good to me. I haven't tested the result, but I guess you have. :-)

{
name = 'db',
postgis = {
url = "postgres:///db"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this hardcoding a database name? Not sure how this works with bbox, but maybe leave this empty so it picks up database connection from PG* env vars if they are available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BBOX currently requires a configuraton, but it can be overwritten with env vars. A possbile way is to add comments at the begin of the config file explaining this.

Is it possible to get the used db parameters for adding a matching PG url?

Copy link
Contributor

Choose a reason for hiding this comment

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

The libpq library from PostgreSQL looks at some PG* env variables, this should work more or less everwhere unless that library is not used. So having no database config options is totally okay for osm2pgsql and should be for bbox also.

So there are several different ways of specifiying which database to use to osm2pgsql, maybe we can expose this information to Lua. But should we expose the original command line option or what's used in the end (taking env vars into account)? I have to think about this some more.

lua/themepark/plugins/bbox.lua Outdated Show resolved Hide resolved
end

local config = {
webserver = { server_addr = '0.0.0.0:8080' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Default should probably be 127.0.0.1 to not open up a server to the world.

This is probably something we want to make configurable. The t-rex plugin also needs those settings. But okay for now. We can do that after this has been merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 127.0.0.1 (0.0.0.0 is needed for Docker setups).

return keys1
end

local function dump_toml(o, indent_size, parents)
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to pull this out later and re-use it in the t-rex config plugin also.

lua/themepark/plugins/bbox.lua Outdated Show resolved Hide resolved
geometry_field = info.geom_column,
geometry_type = string.upper(info.geom_type),
srid = plugin.themepark.options.srid,
-- simplify = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't set simplify by default, escpecially when using osm2pgsql-gen.

@joto
Copy link
Contributor

joto commented Mar 28, 2024

I am merging this now. We can sort out the details of how to specify database access params later.

@joto joto merged commit cec2320 into osm2pgsql-dev:master Mar 28, 2024
1 check passed
@pka pka deleted the bbox-config-gen branch March 28, 2024 21:04
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