-
-
Notifications
You must be signed in to change notification settings - Fork 38
Add tilegrid 4326 #92
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
Conversation
…rid in extent calculations
- Used for slippy tile operations
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.
Thank you for this pull request! We've been trying to find a good Tile
<=> Projection
relationship for a while now and this TileGrid
concept is really good. I have few changes for convention and cleaning up the api. Thank you!
) | ||
|
||
// TileGrid contains the tile layout, including ability to get WGS84 coordinates for tile extents | ||
type TileGrid interface { |
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.
From my understanding the TileGrid
is what defines the rules for the tiles and how they're placed. If this is correct, I think the following changes should be made:
- the name should be changed to
TileScheme
which alludes to the rules which the tiles are created and laid out. "Grid" makes me thing of a structure which already exists (in memory); like a "dish" vs a "recipe" - the implementation should use the
Tile
type wherever possible. By doing this, theTile
andTileGrid
types can be simplified and some functions moved to stand alone withfunc(TileGrid, Tile ....
signatures. I'm thinking the new types and signatures would look like this:
type TileGrid interface {
// returns the srid of the projection of this tiling scheme
// the geom types from the other methods will be in this projection.
SRID() int
// this should return the (exclusive) maximum tile in this zoom. AKA:
// Tile{z, MaxX+1, MaxY+1}.
// it is exclusive so this function can be easily used for boundary checks
Size(z uint) Tile
// converts from a point (in the tile scheme's projection) and zoom to a tile.
// ok will be false if the point is not valid in the projection
FromNative(z uint, pt geom.Point) (t *Tile, ok bool)
// Retuns the tile's upper left point. ok will be false if the tile is not valid.
// <strike>one thing to note about implementing this is that you can compare tiles directly:
// ok := tileScheme.Max(tile.Z) >= tile</strike>
// second thing to note is that the
ToNative(Tile) (pt geom.Point, ok bool)
}
// no need for the srid because f can be made a closure
func RangeFamilyAt(TileGrid, Tile, f func(*Tile) error) error
// Tile.ExtentXXXX aren't needed because of ToNative
// NewTileLatLon aren't needed beacuse of FromNative
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.
I believe that we'd still need a mechanism to support getting a tile's extent in both projected as well as WGS84 units to support downstream usage within Tegola. Part of the reasoning for separating tiling logic out to support equirectangular tile schemes is in support of tile rendering as "4326" and a non-square tile grid. But, again, I very well could be missing some of the vision for these changes. I'd love to chat further so that I'm on the same page!
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.
I was thinking that the conversion from one coordinate system to another should be decoupled from slippy
(it was not before, but it is something we were looking to change). The implementer of Grid
works only with its native units and then the calling code can use the proj
package to convert to WGS84 or other projection systems.
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.
Gotcha. Makes sense.
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.
@ear7h re: you can compare tiles directly
, I didn't realize that structs in golang offered an ordering mechanism (or operator overloading) but am having trouble a mechanism to support this construct. Is there something I'm missing or misunderstanding?
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.
@meilinger My mistake, that doesn't work 😬 They can be compared for equality but not ordering.
// Tile2WebY returns the side of the tile in the +y side | ||
func Tile2WebY(zoom uint, n uint) float64 { | ||
// Tile2WebY returns the side of the tile in the +y side in webmercator | ||
func Tile2WebY(zoom uint, n uint, srid uint) float64 { |
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.
The extra arg isn't really necessary here. If it's being used elsewhere to satisfy func (uint, uint, uint) float64
signature, a closure should be made there. Since there there isn't a type requirement in this package it shouldn't be forced here.
// Tile2WebX returns the side of the tile in the -x side | ||
func Tile2WebX(zoom uint, n uint) float64 { | ||
// Tile2WebX returns the side of the tile in the -x side in webmercator | ||
func Tile2WebX(zoom uint, n uint, srid uint) float64 { |
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.
The extra arg isn't really necessary here. If it's being used elsewhere to satisfy func (uint, uint, uint) float64 signature, a closure should be made there. Since there there isn't a type requirement in this package it shouldn't be forced here.
// TODO (@ear7h): perhaps rethink this | ||
func Pixels2Webs(zoom uint, pixels uint) float64 { | ||
return WebMercatorMax * 2 / math.Exp2(float64(zoom)) * float64(pixels) / MvtTileDim | ||
func PixelsToProjectedUnits(zoom uint, pixels uint, srid uint) float64 { |
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.
This could be renamed FromPixels
Pixels2Native
and have a func (TileGrid, zoom, pixels uint) float
signature
YIndex2Lat(zoom, y uint) (lat float64) | ||
} | ||
|
||
func GetGrid(srid uint) TileGrid { |
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.
For convention, this function should be renamed NewGrid
. Also, as a clarification, are there external limitations (other programs, a spec, convention etc.) which prevent the usage of square tiles with 4326 projection?
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.
I believe that, from my experience, when someone mentions "4326" as the projection (which it is not), they are typically referring to equirectangular projection using lat/lon and WGS84 geoid upon a 2x1 tile ratio scheme (using square tiles, but 2x1 tile extent). But that is completely notional, not part of a spec and my experience is only a single data-point in usages.
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.
I think I understand what's going on bit better now, thank you for the explanation. I always get confused with srid, projections, and datums. From what I've found after a brief search it seem like some programs expect the behavior you describe. So, I think the functionality here is good.
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.
I hear ya, I still get confused as to difference of them as well.
} | ||
|
||
type grid struct { | ||
tileExtentRatio float64 |
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.
Could you document what this ratio is? x/y or y/x
Awesome, @ear7h! Thanks for the comprehensive feedback! Will address momentarily. |
@meilinger Hey, if you haven't started working on this, I'd like to take over and get it finished up. That way, we'll have more time to work through the PRs sitting in tegola 😅 |
Sounds good, I got a little way through but feel free to knock it out.
…On Mon, Dec 2, 2019, 7:02 PM Julio ***@***.***> wrote:
@meilinger <https://github.com/meilinger> Hey, if you haven't started
working on this, I'd like to take over and get it finished up. That way,
we'll have more time to work through the PRs sitting in tegola 😅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92?email_source=notifications&email_token=AABQEK2F5PQSWK3PFNPKHZDQWWVZ3A5CNFSM4JMJGVB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFXWWIY#issuecomment-560950051>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQEK3HZYUNSW6OY5FHT2DQWWVZ3ANCNFSM4JMJGVBQ>
.
|
👍 |
* first cut of tile grid * slippy tiles can more elegantly handle multiple projection systems
* first cut of tile grid * slippy tiles can more elegantly handle multiple projection systems
* first cut of tile grid * slippy tiles can more elegantly handle multiple projection systems
This will need to be used in concert w/ a PR (# to be updated) against
tegola
when I complete the conflicts from the current rebase against v0.11.x