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 worst-case openmaptiles mvts #159

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

Conversation

msbarry
Copy link
Collaborator

@msbarry msbarry commented Jun 15, 2024

Add some of the biggest openmaptiles in the world generated by planetiler. I made some utilities for reducing tiles sizes in onthegomap/planetiler#669 and onthegomap/planetiler#673 and applied them to the openmaptiles profile in openmaptiles/planetiler-openmaptiles#112 (mostly merging features with the same attributes into multigeometries and sorting by hilbert index). This PR includes both the optimized and unoptimized tiles. Ideally mlt should generate smaller tiles without optimizations so those application-layer optimizations aren't necessary anymore.

TODO:

  • These tiles should have the same license as the rest of the openmaptiles MVT's but it's not clear to me where that license lives?
  • Update the files in test/expected/omt - I tried running just bless but it doesn't seem to be implemented?
  • The java encoder throws an exception for several of these tiles, should I check them in anyway? Or try to fix the exceptions first?

@springmeyer
Copy link
Collaborator

These should be super useful, thank you for pulling them together.

My slight preference would be to put them into a new subdirectory in order to make it easy to distinguish / remember they are different from the vanilla OMT that have already been contributed.

Update the files in test/expected/omt - I tried running just bless but it doesn't seem to be implemented?

The encode.jar can be used to created expected fixtures. But there is not tooling quiet yet. The just bless command was adde recently without hooking it up to anything so far. I've been experimenting with a slightly improved structure to the ./expected directory + some automation to get it updated over in #125, but that is not yet merged.

The java encoder throws an exception for several of these tiles

Many of the existing OMT tiles currently hit a decoder bug, which @mactrem fixed earlier today: #158. So you might try with that PR? You say it is the encoder that throws: if that is the case, would be really useful to see the error + put the error in a separate ticket and we'll take a look at fixing it asap.

should I check them in anyway? Or try to fix the exceptions first?

I'm open to either. A primary priority recently for me, working with @mactrem, has been to make sure the encoder and decoder handle diverse tiles robustly and without error. So, its good timing to fix what you uncovered. If you are interested in taking a look yourself, go for it! But also great to ticket and one of us can look next week too.

@nyurik
Copy link
Member

nyurik commented Jun 15, 2024

  • Licensing for the tests is still a todo - see License of tiles in the test/fixtures/ folder #126
  • So is just bless -- I know this is an expected feature (you discovered it without it being documented anywhere, just as I was hoping it would be), but we have not formalized a utility to convert a single MVT->MLT from CLI. Once that is done (help wanted), it will be easy to do a simple bash script to recursively (and in parallel) convert all tiles in all dirs, save them to the test/output, and bless them (the diffing and blessing parts are already done)

@pnorman
Copy link

pnorman commented Jun 15, 2024

I have also made various torturous tiles, some of which cause errors in maplibre with over 64k vertices. Would those be of interest?

If examining various odd conversion cases, consider extents that are not of the form 2^n.

@msbarry
Copy link
Collaborator Author

msbarry commented Jun 17, 2024

Thanks @springmeyer, are you thinking a folder like test/fixtures/omt-large?

Merging master appeared to fix most of the exceptions but there is still at least one more. I'll open up a new issue with that one.

@msbarry
Copy link
Collaborator Author

msbarry commented Jun 17, 2024

@pnorman are those "naturally occurring" tiles or artificial? Seems useful to include somewhere to make sure it's robust to a lot of corner cases but not sure if the goal of these test fixtures is to be real world examples or include artificial ones. Any thoughts here @springmeyer @nyurik @mactrem ?

@pnorman
Copy link

pnorman commented Jun 17, 2024

@pnorman are those "naturally occurring" tiles or artificial? Seems useful to include somewhere to make sure it's robust to a lot of corner cases but not sure if the goal of these test fixtures is to be real world examples

They were real-world cases where I had to work around mapbox/maplibre behavior.

@springmeyer springmeyer mentioned this pull request Jun 20, 2024
2 tasks
@springmeyer
Copy link
Collaborator

Thanks @springmeyer, are you thinking a folder like test/fixtures/omt-large?

Yeah, that was my thinking. But I've had a chance now to look into testing in general in the repo and I have a new thought which is to not integrate these new tiles right now. Our testing framework needs an overhaul first. So, I propose letting this hang out for a week or so until I've had a chance to work on getting the tests more ready for extra tiles like these.

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.

4 participants