Configurable tiling layout

[akesmarki]

Issue

[#4562]

Tasklist

• Add tests
• Add #fixes with the issue number that this PR addresses
• Update the docs with any new request parameters or changes to behavior described
• Update the changelog
• If you made changes to the lua files, update the taginfo too.

Requirements / Relations

We build on the recently introduced config singleton.
As described in the linked issue, this PR is about the configurable tiling scheme/layout.
The new keys in valhalla.json are optional, and the fallback when they are not specified is the Valhalla/OSM default layout.

Attila Kesmarki - [email protected] on behalf of NNG LLC.

[nilsnolde]

Thanks, we always wanted to have configurable tile sizes. However, why the coordinates? Personally I wouldn't like to have that in the code, bcs it's redundant: either you clip the PBF before, or you can use valhalla_build_extract to produce extracts from a pre-built tileset. I can't imagine any other requirement that would warrant adding this, but maybe I'm missing smth?

[akesmarki]

When I cut the coded numbers from the source, I didn't want to just go halfway. But you are right, the same result can be achieved with other tools.
I will remove the minpt key.

[dnesbitt61]

I have seen issues with some tile sizes if I recall. I seem to remember tile sizes have to be evenly divisible into 1.
0.125, 0.25, 0.5 work, but
0.3, 0.6, 0.75 I think cause issues.
Perhaps this should be tested with some of these tile size variations to make sure they work. If there are limitations they should be spelled out somewhere.

[dnesbitt61]

Also, if tile bounds are provided what happens in the OSM pbf extends outside the bounds? Is this handled properly or does this cause a crash in valhalla_build_tiles?

[dnesbitt61]

It seems better to always use a full earth tiling system (rather than allowing min, max lat,lon for the tiles) to create consistent tile Ids.

[dnesbitt61]

What protects using a different tile system in the config file than what the data contains? I think the config should be part of mjolnir for use when creating data. However, the actual data should contain the tiling information for use when reading/using data. Otherwise, what protection is there against someone using a config file that doesn't match the data.

[danthe42]

I agree @dnesbitt61, that's a great idea and out safest option, thanks !

Maybe, by extending the GraphTileHeader with a new field: total number of columns in the tile grid, on that given level would be enough. In that case the tilesize can be calculated easily as 360/total_columns so we don't need to use float type.

Also, the storing of the total number of tiles in a row like this will guarantee that the tile sizes won't cause problems in the last column/row (what you wrote about above: "0.3, 0.6, 0.75 I think cause issues.")

[kevinkreiser]

getting these values from config doesnt seem like the right idea to me. as @dnesbitt61 said it invites the possibility of having a config that doesnt match the actual tiles.

if we want to make this configurable, we have to:

1. validate the config doesnt break the rules (evenly divisble adjacent levels)
2. the config is only used to build tiles (ie it is a mjolnir config)
3. the service and library discover the tilesize from the tiles themselves via graphtileheader

[kevinkreiser]

that last one is a deal breaker in that it would require a major version change. we would have to add something to the graphtileheader and add code to graphreader et al to read that. we can do that as a new feature in valhalla v3 but if we make tiles that arent using the default sizes and old valhalla v3 code wants to read those, it will break because it wont know anything about the configurable sizes and having to get that info from graphtileheader. sadly to actually do this we need to do it in a new major version

[akesmarki]

Yes, old valhalla v3 code would not be able to use the new tiles. Can't we handle this feature as a new, experimental one and warn the users about this ?

@kevinkreiser , could you advice how to progress with this ?

• Should this PR be cancelled now, and later the design/implementation of the final graphtile-embedded tilesizes can be discussed, when preparing to switch to v4.
• Or, would it worth to include the configurable tilesize feature in some form (based on this PR) in the current, v3 series as a temporary solution, so, at least we could start experimenting it ?

[akesmarki]

Anyway, I've modified my PR to include the first 2 items from the list:

1. validate the config doesnt break the rules (evenly divisble adjacent levels)
2. the config is only used to build tiles (ie it is a mjolnir config)

Even if it won't get in the official valhalla for now, this is a good temporary solution until the end of the v3 line.

[nilsnolde]

Hm, but this modification doesn't solve the initial problem of breaking a consuming application if the tiles were built non-standard, right? Then what's the point if there can't be a graph reader being able to read them?

Thanks, we always wanted to have configurable tile sizes.

I was very confused here haha I thought about hierarchy limits, not tile dimensions.

[nilsnolde]

I said it a few times before, it's also part of our docs (I realize likely no one reads the CONTRIBUTING.md though;)): it's always safest to add an issue before putting in the work. We had quite a few even larger PRs we needed to reject which is a shame and doesn't feel very good, also for us, when quite a bit of work went into them.

[akesmarki]

Well, there would be 2 use cases:

• I believe all future releases from master would be able to convert OSM maps with a custom layout if this is merged. Am I wrong ?
• Also, we could produce graph tiles from other sources with custom layout.

The feature is on our forked branch, we can use it, so I won't feel bad if you refuse to accept this contribution.

[nilsnolde]

Jep then you could build tiles that way, but you won’t be able to read the same tiles right? The graph reader will still expect the default resolution. That’s where the real “experimenting” is though isn’t it? Just building stuff without using it seems pointless. Or am I missing smth?

[akesmarki]

Nope, I could produce graph tiles using a custom tile grid, and use the valhalla-app viewer/tester to calculate routes on it when I specified the same custom layout in the valhalla.json.
So it seems that the graph reader and the whole valhalla_service used this custom layout correctly.

[nilsnolde]

Huh interesting.. then I’d be clearly missing smth😄

[akesmarki]

Any insights about this Pull Request by the maintainers ? Should I change it somehow, cancel it, or should it be put "on hold" ?

[kevinkreiser]

i mean i am truly shocked that we've been so diligent that all you needed to change was the tile hierarchy to get it to work! in terms of merging it into master i guess i wouldnt be against it if the default is the current configuration. this way no one gets screwed over unless they decide to do it to themselves haha

to prove that this actually works for literally everything id like to see a commit where we change the default (in all the test configs) to be some other valid configuration. if we can do that and everything works then we can revert just that testing change, add back a test that exorcises a non default configuration, and review and merge it. specifically i mean changing mainly this to some other valid configuration: https://github.com/valhalla/valhalla/blob/master/test/test.cc#L123

like @nilsnolde im just shocked that its possible we havent made any other assumptions around this particular design choice and that its all only accessed from this one place. a rare moment where we didn't make a mistake and kept our selves disciplined haha

[nilsnolde]

what I'd like to change is the coding style and would ask to make it more valhalla like:

• function names either camel case with the first letter capitalized or snake case
• no bSizesInConfigOk, tilerowsvector etc indicating the type as part of the variable name
• variable names always snake case (we'd like that for functions too actually, but somehow java-ish google style crept in at some point)