Increase coords consistency and clamp zoom level to numerically feasible range #273
Conversation
| key[i + 1] = *part; | ||
| pub fn new(zoom_level: ZoomLevel, x: u32, y: u32) -> Self { | ||
| // use morton enconding / Z-ordering to build linear quadtree index | ||
| let encoded_coords = morton_encoding::morton_encode([x, y]); |
There was a problem hiding this comment.
Interesting approach. As this is used a lot, can we benchmark the encoding/decoding OR do you have a guess on how fast this is?
There was a problem hiding this comment.
It compiles to ~20 branchless instructions of x86 and should be really fast: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=6693a9f5ac38027b1fefe470f4208428 (select ASM in the ... menu on the top left)
EDIT: And it should make the comparison operation a lot cheaper.
Yes. Works on my Windows 10 machine. |
maplibre/src/coords.rs
Outdated
| /// [here](https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames#Zoom_levels). | ||
| /// This implementation allows 30 because this covers all tile sizes that can be represented | ||
| /// with signed 32-bit integers. | ||
| pub(crate) const MAX_ZOOM: u8 = 30; |
There was a problem hiding this comment.
This should probably be an associated constant of ZoomLevel, i.e. ZoomLevel::MAX.
maplibre/src/coords.rs
Outdated
|
|
||
| impl std::ops::Add<u8> for ZoomLevel { | ||
| type Output = ZoomLevel; | ||
| pub(crate) fn max_tile_coord(&self) -> u32 { |
There was a problem hiding this comment.
| pub(crate) fn max_tile_coord(&self) -> u32 { | |
| pub fn max_tile_coord(&self) -> u32 { |
maplibre/src/coords.rs
Outdated
| // check for out of bound access | ||
| let TileCoords { | ||
| x, y, z | ||
| } = self.into_tile(TileAddressingScheme::XYZ)?; |
There was a problem hiding this comment.
Quadkeys should work on WorldTileCoords. Hardcoding here the addressing scheme seems odd
There was a problem hiding this comment.
The only purpose here is to reuse the bound check code. Quite hacky. I will refactor it a bit to make that clear.
|
Format can be fixed with |
Co-authored-by: Max Ammann <max@maxammann.org>
nyurik
force-pushed
the
coord_zoom_overflow
branch
from
f692f04
to
7d69e1c
Compare
|
Rebased on main, and cargo fmt-ed it |
|
@terrorfisch I updated this to the lastest main. If you want to go over it again, then I can review this one. Regardin the morton-encoding dependency: I think it would be nice to include the efficiency aspect (compiled down to 20 instructions) in the source code as comment. |
|
@terrorfisch Same here, I would still love to merge this :) |
ZoomLevelto0..31when converted from floating pointZoomWorldTileCoordsto supported zoom level rangemorton_encodingcrate. This change is here because the previous QuadKey depended on the ZoomLevel type.Fixes crash from #161 by making the "backend" more robust.
Does not implemented the desired zoom limit. This is done in #180
💻 Examples
You can arbitrarily zoom in/out in the demo without panic.
🚨 Test instructions
Launch demo and start zooming :)
✔️ PR Todo
I am not satisfied with these points yet and lack the domain knowledge to proceed:
get_childrenusage in rendererWorldTileCoordsViewRegion::iteris the correct way to go