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 tile statistics to mbtiles #986

Merged
merged 30 commits into from
Nov 13, 2023
Merged

Add tile statistics to mbtiles #986

merged 30 commits into from
Nov 13, 2023

Conversation

sharkAndshark
Copy link
Collaborator

@sharkAndshark sharkAndshark commented Nov 5, 2023

Try to fix #964

  • Add tile statistics to mbtiles tools
  • Add test
  • Use 4326 instead of 3857 for tile bounds
  • Add document
  • Use size-format to prettify output
  • Statistics struct Refactor
  • Cleanup and reformat

@sharkAndshark sharkAndshark force-pushed the mbstats branch 14 times, most recently from 29c1fba to 8c35ffa Compare November 7, 2023 13:08
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
@sharkAndshark

This comment was marked as outdated.

mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
@nyurik
Copy link
Member

nyurik commented Nov 9, 2023

P.S. It is usually better to use git merge main, rather than using rebase. Rebase tends to loose tracking of the review comments.

@sharkAndshark
Copy link
Collaborator Author

P.S. It is usually better to use git merge main, rather than using rebase. Rebase tends to loose tracking of the review comments.

Thx! Didn't know that before : )

@nyurik
Copy link
Member

nyurik commented Nov 11, 2023

Forgot to mention - I am not certain if you are affected by this, but mbtiles invert the Y value (tile_row) -- the Y coordinate of a tile corresponds to 2^zoom - 1 - tile_row in mbtiles.

@sharkAndshark
Copy link
Collaborator Author

Thx for the mention. I did some research and I hope it's right.
MBTiles spec says it follow TMS, so the the Y of tile index increase from down to top.

Schema Y
XYZ From top to down
TMS From down to top

Tile Origin and Bbox of 3857

Origin Bbox
(-20037508.34, -20037508.34) [-20037508.34, -20037508.34, 20037508.34, 20037508.34]

Length of one single tile

(20037508.34 * 2) / (2^zoom_level)

MinX,MaxX

MinX = OriginX + X * TileLength
MaxX = MinX + TileLength

MinY,MaxY

MinY = OriginY + Y * TileLength
MaxY = MinY + TileLength
image

where
for<'e> &'e mut T: SqliteExecutor<'e>,
{
let file_size = query!(
Copy link
Collaborator Author

@sharkAndshark sharkAndshark Nov 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that PathBuf couldn't work with file:something?mode=memory together. Using sql query for the file size instead of PathBuf now. @nyurik

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, seems reasonable. The alternative would be to have Option for the size, and set it to None if unable to get it from the filesystem.

@sharkAndshark sharkAndshark marked this pull request as ready for review November 12, 2023 15:32
@wipfli
Copy link

wipfli commented Nov 12, 2023

@bdon you did something similar for pmtiles, right?

docs/src/54-mbtiles-validation.md Outdated Show resolved Hide resolved
mbtiles/Cargo.toml Outdated Show resolved Hide resolved
mbtiles/Cargo.toml Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
where
for<'e> &'e mut T: SqliteExecutor<'e>,
{
let file_size = query!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, seems reasonable. The alternative would be to have Option for the size, and set it to None if unable to get it from the filesystem.

mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
docs/src/52-mbtiles-stats.md Outdated Show resolved Hide resolved
Comment on lines 100 to 102
if self.count != 0 {
let smallest = SizeFormatterBinary::new(self.smallest.expect("The smallest tile size of all zoom levels shouldn't be None when the tiles count of all zoom level is not 0"));
let largest = SizeFormatterBinary::new(self.largest.expect("The largest tile size of all zoom levels shouldn't be None when the tiles count of all zoom level is not 0"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no point for all this complex expect strings which will never happen. Might as well do a simpler variant:

Suggested change
if self.count != 0 {
let smallest = SizeFormatterBinary::new(self.smallest.expect("The smallest tile size of all zoom levels shouldn't be None when the tiles count of all zoom level is not 0"));
let largest = SizeFormatterBinary::new(self.largest.expect("The largest tile size of all zoom levels shouldn't be None when the tiles count of all zoom level is not 0"));
if let (Some(smallest), Some(largest)) = (self.smallest, self.largest) {
let smallest = SizeFormatterBinary::new(smallest);
let largest = SizeFormatterBinary::new(largest);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is The largest tile size of all zoom levels shouldn't be None Ok? Or just "Couldn't be None".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to do any asserts (.expected(...)) - you can simply check if both values are not None, and if so, use them.

mbtiles/src/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job, well done, thank you!!!

@nyurik nyurik enabled auto-merge (squash) November 13, 2023 06:00
@nyurik nyurik merged commit 0398336 into maplibre:main Nov 13, 2023
18 checks passed
@sharkAndshark sharkAndshark changed the title [WIP] Add tile statistics to mbtiles Add tile statistics to mbtiles Nov 13, 2023
@sharkAndshark
Copy link
Collaborator Author

Awesome job, well done, thank you!!!

Thx all your guidance !Hope I can do better in next : )

@sharkAndshark sharkAndshark deleted the mbstats branch November 14, 2023 01:14
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.

Add tile statistics to mbtiles, e.g. bbox, number of tiles, ...
3 participants