Fix bbox to xyz roundtrip computation #1059

nyurik · 2023-12-10T05:15:20Z

Add tile-grid as a dependency to use its webmercator tile math.

nyurik · 2023-12-10T05:17:58Z

@sharkAndshark please take a look at this function - i think you may know more about it. See if you know how to fix it?

sharkAndshark · 2023-12-10T05:19:43Z

I'm trying to understand what's the bug here.

nyurik · 2023-12-10T05:35:49Z

@sharkAndshark this test fails, but I was expecting it should succeed. See https://github.com/maplibre/martin/actions/runs/7155799338/job/19484815026 :

---- tests::test_xyz_to_bbox stdout ----
thread 'tests::test_xyz_to_bbox' panicked at martin-tile-utils/src/lib.rs:336:9:
assertion `left == right` failed
  left: (1, 26, 3, 28)
 right: (1, 3, 2, 5)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The Y coordinate gets decoded as an inverse.

nyurik · 2023-12-10T05:42:48Z

P.S. Note that both tile-grid and utiles have this functionality, and eventually we should adapt it for our needs, but for now I could use the round-trip-able xyz->bbox->xyz

nyurik

Do we really want to introduce a new tms concept into all this? I was hoping we can keep the whole system using the same xyz, and only the mbtiles crate to know to invert it last minute.

sharkAndshark · 2023-12-10T15:29:13Z

Agree. Use TMS only when it's mbtiles make things simplier. I will roll back and check the y axis schema of our tests cases here (even othe places) carefully.

martin-tile-utils/src/lib.rs

nyurik · 2023-12-10T19:57:17Z

Agree. Use TMS only when it's mbtiles make things simplier. I will roll back and check the y axis schema of our tests cases here (even othe places) carefully.

There are several ways to implement the "tile ID" concept:

some struct + enum. The struct has X,Y,Z, and enum has TMS/WTS values. This is not a good approach for multiple reasons: it is easy to mess up if struct matches the enum (e.g. to accidentally treat x/y/z as tms whereas it should have been wts, etc. It also makes it impossible to have non-x/y/z style IDs, like what PMTiles, S2, and H3 uses.
have one ID struct for the whole program -- X/Y/Z, whose values are understood to be the same everywhere. Then only mbtiles crate needs to know that it should use TMS internally. The only drawback is that will not easily support other IDs, e.g. if we ever want to start using H3 for tile access.
use enum with variables, e.g. enum TileID { WTS { x: u32, y: u32, z: u8 }, TMS { x: u32, y: u32, z: u8 }, H3 { ... } } -- this would support all ID types, but it would be more difficult to coordinate different systems with different ID schemas. Eventually we may go here, but not just yet.

nyurik · 2023-12-10T20:04:29Z

P.S. We really should not implement this ourselves. Instead, we should rely on the other crates as mentioned in #1059 (comment)

sharkAndshark · 2023-12-11T03:44:58Z

Hi @nyurik Could I split this to these three tasks.

Fix tests
For the functions in martin-tiles-util, make xyz schema as default and implicitly. Invert Y before calling when it's MBTiles
Use tile-grid or utiles directly

nyurik · 2023-12-11T04:04:52Z

I think the first two should be done together. As far as I can see, most code is already xyz, so should be minimal changes. The 3rd is optional, but it might be easier to just use tile grid directly from the start, and not bother with fixing the first two? Not sure.

sharkAndshark · 2023-12-11T09:14:57Z

Before to do anything else, I think we need to talk about this:

assert_relative_eq!(bbox[0], -168.74999999999955, epsilon = f64::EPSILON * 2.0);
assert_relative_eq!(bbox[1], 74.01954331150218, epsilon = f64::EPSILON * 2.0);
assert_relative_eq!(bbox[2], -146.2499999999996, epsilon = f64::EPSILON * 2.0);
assert_relative_eq!(bbox[3], 81.09321385260832, epsilon = f64::EPSILON * 2.0);

// Fixme: These are totally wrong
let xyz = bbox_to_xyz(bbox[0], bbox[1], bbox[2], bbox[3], 5);
assert_eq!(xyz, (1, 3, 2, 5));

assertion `left == right` failed
left: (1, 3, 3, 6)
 right: (1, 3, 2, 5)

It's difficult to handle when it's corner of tile bbox. Should we introduce a tolerance here? I'm still thinking of it. Any help? @nyurik

nyurik · 2023-12-11T17:24:28Z

Thanks! This is still much better than before, and I am actually Ok with it being "slightly" different - as this is a floating point rounding error. I think our testing should be like this:

convert xyz to bbox and validate against hard-coded values
convert back to xyz, and validate, but here you want to make sure that bounds are either equal, or bigger by no more than 1 tile. In your example above:

let xyz = bbox_to_xyz(bbox[0], bbox[1], bbox[2], bbox[3], 5);
// these numbers might be slightly wrong?
assert!(xyz.left == 0 || xyz.left == 1);
assert!(xyz.top == 2 || xyz.top == 3);
assert!(xyz.right == 2 || xyz.right == 3);
assert!(xyz.bottom == 5 || xyz.bottom == 6);

sharkAndshark · 2023-12-12T15:55:54Z

either equal, or bigger by no more than 1 tile

@nyurik Do you actually mean buffered by? Then the test code would be like:

// For left top corner
assert!(tile_col == some_value || tile_col == some_value + 1)
assert!(tile_row == some_value || tile_row == some_value - 1)

// For right down corner
assert!(tile_col == some_value || tile_col == some_value  +1)
assert!(tile_row == some_value || tile_row == some_value + 1)

nyurik · 2023-12-12T17:40:17Z

martin-tile-utils/src/lib.rs

-    let max_value = (1_u32 << zoom) - 1;
-    (x.min(max_value), y.min(max_value))
+pub fn tile_index(lon: f64, lat: f64, zoom: u8) -> (u64, u64) {
+    let tms = tms().lookup("WebMercatorQuad").unwrap();


We need to move tms creation into static lazy init block. See https://docs.rs/lazy_static/latest/lazy_static/ - it is already being used for some other dependencies, so might as well use that too. This way we won't do it on each call.

I use OnceLock instead, is it Ok?
I tried many times but the lazy_static crate didn't work on my computer. Maybe it's because my rust version( I've already run rustup update) I'm not sure.

im ok with the oncelock (eventually everything will migrate to the core i think), but you do realize you are already using lazy_static as part of insta, console, and other crates which are used in martin?

TBH I didn't realized... 😂😂

Still a lot to learn.

By the way, seems that tile-grid crate used OnceCell internall.

/// Global registry of tile matrix sets pub fn tms() -> &'static TileMatrixSets { static TMS: OnceCell<TileMatrixSets> = OnceCell::new(); &TMS.get_or_init(|| { let mut sets = TileMatrixSets::new(); let tms = vec![ #[cfg(feature = "projtransform")] include_str!("../data/CanadianNAD83_LCC.json"), //include_str!("../data/CDB1GlobalGrid.json"), // Error("missing field `coalesc`", line: 19, column: 67) #[cfg(feature = "projtransform")] include_str!("../data/EuropeanETRS89_LAEAQuad.json"), //include_str!("../data/GNOSISGlobalGrid.json"), // Error("missing field `coalesc`", line: 31, column: 66) #[cfg(feature = "projtransform")] include_str!("../data/UPSAntarcticWGS84Quad.json"), #[cfg(feature = "projtransform")] include_str!("../data/UPSArcticWGS84Quad.json"), #[cfg(feature = "projtransform")] include_str!("../data/UTM31WGS84Quad.json"), include_str!("../data/WebMercatorQuad.json"), include_str!("../data/WGS1984Quad.json"), //include_str!("../data/WorldCRS84Quad.json"), // conflicts with WGS1984Quad include_str!("../data/WorldMercatorWGS84Quad.json"), ] .into_iter() .map(|data| TileMatrixSet::from_json(&data).unwrap()) .collect::<Vec<_>>(); sets.register(tms, false).unwrap(); // user_tms_dir = os.environ.get("TILEMATRIXSET_DIRECTORY", None) // if user_tms_dir: // tms_paths.extend(list(pathlib.Path(user_tms_dir).glob("*.json"))) sets }) }

martin-tile-utils/src/lib.rs

sharkAndshark · 2023-12-13T06:09:16Z

martin-tile-utils/src/lib.rs

-    let max_value = (1_u32 << zoom) - 1;
-    (x.min(max_value), y.min(max_value))
+pub fn tile_index(lon: f64, lat: f64, zoom: u8) -> (u64, u64) {
+    let tms = tms().lookup("WebMercatorQuad").unwrap();


I use OnceLock instead, is it Ok?
I tried many times but the lazy_static crate didn't work on my computer. Maybe it's because my rust version( I've already run rustup update) I'm not sure.

martin/src/bin/martin-cp.rs

nyurik · 2023-12-14T18:10:42Z

will review & merge shortly, thx for all the hard work!

nyurik · 2023-12-15T05:45:40Z

thanks for fixing this!!!

nyurik · 2023-12-15T07:01:47Z

@sharkAndshark sadly, this solution still has some weird bug, and does not fully solve general use case. It seems the xy_tile somehow limits the computation to ~z24(?).

Notice that beyond z24, all values are identical

#[test]
fn test_box() {
    fn tst(left: f64, bottom: f64, right: f64, top: f64, zoom: u8) -> String {
        let (x0, y0, x1, y1) = bbox_to_xyz(left, bottom, right, top, zoom);
        format!("({x0}, {y0}, {x1}, {y1})")
    }

    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 0), @"(0, 0, 0, 0)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 1), @"(0, 1, 0, 1)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 2), @"(0, 3, 0, 3)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 3), @"(0, 7, 0, 7)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 4), @"(0, 14, 1, 15)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 5), @"(0, 29, 2, 31)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 6), @"(0, 58, 5, 63)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 7), @"(0, 116, 11, 126)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 8), @"(0, 233, 23, 253)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 9), @"(0, 466, 47, 507)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 10), @"(1, 933, 94, 1014)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 11), @"(3, 1866, 188, 2029)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 12), @"(6, 3732, 377, 4059)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 13), @"(12, 7465, 755, 8119)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 14), @"(25, 14931, 1510, 16239)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 15), @"(51, 29863, 3020, 32479)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 16), @"(102, 59727, 6041, 64958)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 17), @"(204, 119455, 12083, 129917)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 18), @"(409, 238911, 24166, 259834)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 19), @"(819, 477823, 48332, 519669)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 20), @"(1638, 955647, 96665, 1039339)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 21), @"(3276, 1911295, 193331, 2078678)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 22), @"(6553, 3822590, 386662, 4157356)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 23), @"(13107, 7645181, 773324, 8314713)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 24), @"(26214, 15290363, 1546649, 16629427)");
    
    // All these are incorrect
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 25), @"(33554431, 33554431, 33554431, 33554431)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 26), @"(67108863, 67108863, 67108863, 67108863)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 27), @"(134217727, 134217727, 134217727, 134217727)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 28), @"(268435455, 268435455, 268435455, 268435455)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 29), @"(536870911, 536870911, 536870911, 536870911)");
    assert_snapshot!(tst(-179.43749999999955,-84.76987877980656,-146.8124999999996,-81.37446385260833, 30), @"(1073741823, 1073741823, 1073741823, 1073741823)");
}

sharkAndshark · 2023-12-15T07:12:27Z

Weired. I'm debuging.

nyurik requested a review from sharkAndshark December 10, 2023 05:15

nyurik assigned nyurik and sharkAndshark and unassigned nyurik Dec 10, 2023

nyurik marked this pull request as draft December 10, 2023 05:15

sharkAndshark approved these changes Dec 10, 2023

View reviewed changes

nyurik mentioned this pull request Dec 10, 2023

Feature: mbtiles copy --bbox ... to copy tiles within a bbox only #1060

Merged

nyurik commented Dec 10, 2023

View reviewed changes

sharkAndshark force-pushed the fix-bbox-xyz branch from 2c5c2cd to 103704c Compare December 10, 2023 18:45

sharkAndshark approved these changes Dec 10, 2023

View reviewed changes

martin-tile-utils/src/lib.rs Outdated Show resolved Hide resolved

sharkAndshark force-pushed the fix-bbox-xyz branch 3 times, most recently from b751d32 to a94b489 Compare December 12, 2023 15:44

nyurik commented Dec 12, 2023

View reviewed changes

sharkAndshark force-pushed the fix-bbox-xyz branch 2 times, most recently from c913956 to 18f73cb Compare December 13, 2023 05:23

sharkAndshark reviewed Dec 13, 2023

View reviewed changes

martin/src/bin/martin-cp.rs Outdated Show resolved Hide resolved

sharkAndshark marked this pull request as ready for review December 13, 2023 07:47

sharkAndshark approved these changes Dec 14, 2023

View reviewed changes

nyurik and others added 9 commits December 14, 2023 21:58

Fix bbox to xyz roundtrip

b1eeaa0

Try to fix

3fe72ba

wip

dddd8e4

wip

ef2bda3

Minior improvement

45285a3

Allow buffered by 1 tile for bbox test

058e9a1

Remove comment

9468ea7

Allow cast_possible_truncation

0525085

Continue with u32 for simplicity, may revisit

bda8e7d

nyurik changed the title ~~[WIP] Fix bbox to xyz roundtrip~~ Fix bbox to xyz roundtrip Dec 15, 2023

nyurik changed the title ~~Fix bbox to xyz roundtrip~~ Fix bbox to xyz roundtrip computation Dec 15, 2023

nyurik force-pushed the fix-bbox-xyz branch from e58df7e to bda8e7d Compare December 15, 2023 05:42

nyurik enabled auto-merge (squash) December 15, 2023 05:43

nyurik merged commit 886358e into maplibre:main Dec 15, 2023
18 checks passed

nyurik deleted the fix-bbox-xyz branch December 15, 2023 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bbox to xyz roundtrip computation #1059

Fix bbox to xyz roundtrip computation #1059

nyurik commented Dec 10, 2023 •

edited

Loading

nyurik commented Dec 10, 2023

sharkAndshark commented Dec 10, 2023

nyurik commented Dec 10, 2023

nyurik commented Dec 10, 2023

nyurik left a comment

sharkAndshark commented Dec 10, 2023 •

edited

Loading

nyurik commented Dec 10, 2023

nyurik commented Dec 10, 2023

sharkAndshark commented Dec 11, 2023 •

edited

Loading

nyurik commented Dec 11, 2023

sharkAndshark commented Dec 11, 2023 •

edited

Loading

nyurik commented Dec 11, 2023 •

edited

Loading

sharkAndshark commented Dec 12, 2023 •

edited

Loading

nyurik Dec 12, 2023

sharkAndshark Dec 13, 2023

nyurik Dec 13, 2023

sharkAndshark Dec 13, 2023

sharkAndshark Dec 13, 2023

sharkAndshark Dec 13, 2023

sharkAndshark Dec 13, 2023

nyurik commented Dec 14, 2023

nyurik commented Dec 15, 2023

nyurik commented Dec 15, 2023

sharkAndshark commented Dec 15, 2023

Fix bbox to xyz roundtrip computation #1059

Fix bbox to xyz roundtrip computation #1059

Conversation

nyurik commented Dec 10, 2023 • edited Loading

nyurik commented Dec 10, 2023

sharkAndshark commented Dec 10, 2023

nyurik commented Dec 10, 2023

nyurik commented Dec 10, 2023

nyurik left a comment

Choose a reason for hiding this comment

sharkAndshark commented Dec 10, 2023 • edited Loading

nyurik commented Dec 10, 2023

nyurik commented Dec 10, 2023

sharkAndshark commented Dec 11, 2023 • edited Loading

nyurik commented Dec 11, 2023

sharkAndshark commented Dec 11, 2023 • edited Loading

nyurik commented Dec 11, 2023 • edited Loading

sharkAndshark commented Dec 12, 2023 • edited Loading

nyurik Dec 12, 2023

Choose a reason for hiding this comment

sharkAndshark Dec 13, 2023

Choose a reason for hiding this comment

nyurik Dec 13, 2023

Choose a reason for hiding this comment

sharkAndshark Dec 13, 2023

Choose a reason for hiding this comment

sharkAndshark Dec 13, 2023

Choose a reason for hiding this comment

sharkAndshark Dec 13, 2023

Choose a reason for hiding this comment

sharkAndshark Dec 13, 2023

Choose a reason for hiding this comment

nyurik commented Dec 14, 2023

nyurik commented Dec 15, 2023

nyurik commented Dec 15, 2023

sharkAndshark commented Dec 15, 2023

nyurik commented Dec 10, 2023 •

edited

Loading

sharkAndshark commented Dec 10, 2023 •

edited

Loading

sharkAndshark commented Dec 11, 2023 •

edited

Loading

sharkAndshark commented Dec 11, 2023 •

edited

Loading

nyurik commented Dec 11, 2023 •

edited

Loading

sharkAndshark commented Dec 12, 2023 •

edited

Loading