-
Notifications
You must be signed in to change notification settings - Fork 38
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
Implement screen coord translation for MVT writer #150
Conversation
Hi @pka, i'm ok with changing signatures, as long as we obey the versioning rules (bump the minor). Otherwise we could go the easier route and simply not change the existing method, but instead add a new That said, I am not too certain about the API itself. If we take PostGIS's MVT as a guide, it has 2 components: envelope and extent. The extent is a positive integer (same for both X and Y), and should probably be named |
Thanks for pointing to Postgis. My starting point was the Morecantile API, on which the rewrite of tile-grid as an OGC API Tiles implementation was based. OGC has a type I'm for changing the signature with an appropriate version bump and I'm fine with changing |
What speaks against |
thx, yes, i do think The conversion of any given geo-object (i.e. geojson) to MVT is a tricky subject because of how it is possible to do it in multiple ways. So, given a geojson, a user may want to:
So for the first case, |
We should stay with the minimal useful conversion function and not try to write a full |
I agree that (at least initially) geozero should focus on the MVP. The current mvp essentially has no re-projection, so it doesn't do proper conversion to a planar coordinate system, possibly resulting in an error. Your modification adds simple scaling that assumes that the initial coordinates are in the 0th tile of the planar projection, and you simply scale it to the right WRT concatenations, AFAIK, you can only concatenate MVT Layers (because they are at the top of the MVT spec). I used that trick in OpenMapTiles to generate MVTs directly from PostGIS, concatenating multiple WRT f64 - yep, makes sense, i guess we will have to wait for the georust to support proper multi-type geotypes, including the ones with integers, and also migrate geozero to those types? |
P.S. I think the original |
Good point regarding geographic coordinates. Projecting to planar coordinates is indeed an important step which is not integrated in geozero yet. I made some experiments with processing chains a long time ago, but they weren't successful. |
I did a few minor optimizations in #152 that would be merged into this PR first, and then I can merge this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Going forward, perhaps we should create a separate "MVT wrapper" that could be created on top of an MVT object, and somehow apply all the re-projection logic?
* On a second though, I think `to_mvt_unscaled` is more accurate. `raw` here would be confusing. * There is no need to store bounds and do additional math ops on it. Can precompute a few things earlier on. * Default can be auto-derived
Contains a breaking change in the ToMvt signatures. @nyurik Is that ok for you?