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

Initial PMTiles support #2882

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

tdcosta100
Copy link
Collaborator

This PR introduces the PMTiles support, which was first asked in #1978. It's experimental for now, and needs more testing, so I need everyone interested in this feature to test it, so we can mitigate problems that can arise from my modifications.

@louwers louwers marked this pull request as ready for review September 30, 2024 09:38
@louwers louwers added the enhancement New feature or request label Sep 30, 2024
Copy link

github-actions bot commented Sep 30, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +1.1% +1.65Mi  +1.2%  +379Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2882-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +31% +36.3Mi  +439% +26.2Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2882-compared-to-legacy.txt

Copy link

github-actions bot commented Sep 30, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0046         -0.0044             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2882-compared-to-main.txt

Copy link

github-actions bot commented Sep 30, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +1.6%  +210Ki  +1.5%  +192Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2882-compared-to-main.txt

@JesseCrocker
Copy link
Collaborator

I could probably figure this out from reading the code, but does this add support for reading local pmtiles files, reading over http, or both?

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Great stuff!

Needs some tests. Due to the binary size increase we also need to make sure there is an opt-out with a preprocessor macro

@tdcosta100
Copy link
Collaborator Author

I could probably figure this out from reading the code, but does this add support for reading local pmtiles files, reading over http, or both?

It is agnostic about it, it just makes requests to the Resource Loader, then it use the appropriate source, that is, you can use http://, https:// or file://, which one is better for your scenario.

@tdcosta100
Copy link
Collaborator Author

Great stuff!

Needs some tests. Due to the binary size increase we also need to make sure there is an opt-out with a preprocessor macro

Thank you! It was a great effort to achieve this implementation, but I'm still learning how to make such a component integration properly. Some adjustements will be made. Since the pmtiles_file_source.cpp is added in the Bazel/CMake files, maybe I can add a new switch in CMake to opt-out the PMTiles support, something like MLN_WITH_PMTILES=OFF if you don't want it to be included by default.

@tdcosta100 tdcosta100 force-pushed the pmtiles-support branch 3 times, most recently from 94f448c to e778def Compare October 2, 2024 04:29
@tdcosta100 tdcosta100 requested a review from louwers October 2, 2024 04:35
@tdcosta100 tdcosta100 force-pushed the pmtiles-support branch 3 times, most recently from 03b9ef4 to d3c191c Compare October 3, 2024 05:00
@tdcosta100
Copy link
Collaborator Author

Great stuff!

Needs some tests. Due to the binary size increase we also need to make sure there is an opt-out with a preprocessor macro

Hi, @louwers. Could you check also the last commit, please? I'm not sure if I took the best way to control the PMTiles code using CMake.

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Looks OK!

I would prefer as little #defines in the codebase as possible. Maybe zero, with a dummy implementation available?

Some tests are also still needed.

@tdcosta100
Copy link
Collaborator Author

Looks OK!

I would prefer as little #defines in the codebase as possible. Maybe zero, with a dummy implementation available?

Some tests are also still needed.

Interesting. My implementation was in direction of vanishing alway everything if MLN_WITH_PMTILES=OFF. What a dummy implementation would do? Implement the required methods as stubs doing nothing? Display an error telling PMTiles support is not available? Because if so, I'm afraid the binary footprint reduction will be no that great. Anyway, I will adjust to either scenario you will tell me to do. I'm open to suggestions.

@louwers
Copy link
Collaborator

louwers commented Oct 7, 2024

I want to avoid #ifdefs sprinkled across the codebase, because it makes code hard to read and reason about.

The dummy implementation would be another file that does not pull in the PMTiles library as a dependency, which is where the bulk of the binary size increase is coming from. The dummy implementation would just always return false from canRequest().

@tdcosta100
Copy link
Collaborator Author

Okay, I will think about it and implement these changes. Without #ifdefs everywhere, of course hahaha.

@tdcosta100
Copy link
Collaborator Author

tdcosta100 commented Dec 1, 2024

Since now we can test PMTiles with Windows-CI (and other workflows, of course) and tests are passing, I think we are ready for the review.

@tdcosta100 tdcosta100 marked this pull request as ready for review December 1, 2024 05:09
@tdcosta100 tdcosta100 requested review from acalcutt and bdon December 1, 2024 05:09
@navisinghnz9
Copy link

This PR introduces the PMTiles support, which was first asked in #1978. It's experimental for now, and needs more testing, so I need everyone interested in this feature to test it, so we can mitigate problems that can arise from my modifications.

Hi,

I am working on an cross-platorm mobile app using maplibre-native-qt.
I had been struggling to support pmtiles in the app.

I was wondering what would be the best way to experiment with the code in the pull request?

I can see that there is a sub-directory called maplibre-native, inside source tree of maplibre-native-qt.
(I mean here - maplibre-native-qt->vendor->maplibre-native )

Not sure if it will be a good practice to put folder maplibre-native with your PR changes/branch and recompile maplibre-native-qt SDK.
Or I need to write some Qt binding to be able to use pmtiles features in the app.

Thanks!

@tdcosta100
Copy link
Collaborator Author

This PR introduces the PMTiles support, which was first asked in #1978. It's experimental for now, and needs more testing, so I need everyone interested in this feature to test it, so we can mitigate problems that can arise from my modifications.

Hi,

I am working on an cross-platorm mobile app using maplibre-native-qt. I had been struggling to support pmtiles in the app.

I was wondering what would be the best way to experiment with the code in the pull request?

I can see that there is a sub-directory called maplibre-native, inside source tree of maplibre-native-qt. (I mean here - maplibre-native-qt->vendor->maplibre-native )

Not sure if it will be a good practice to put folder maplibre-native with your PR changes/branch and recompile maplibre-native-qt SDK. Or I need to write some Qt binding to be able to use pmtiles features in the app.

Thanks!

Hi. If you have all the changes of this PR in your maplibre-native directory, after compiling you just need to use a pmtiles:// prefix in the source URL, pretty much the same way you would do with MBTiles (mbtiles://). This is the only thing you should care about, you can use everything as usual, nothing is changed in the API with this PR.

@navisinghnz9
Copy link

This PR introduces the PMTiles support, which was first asked in #1978. It's experimental for now, and needs more testing, so I need everyone interested in this feature to test it, so we can mitigate problems that can arise from my modifications.

Hi,
I am working on an cross-platorm mobile app using maplibre-native-qt. I had been struggling to support pmtiles in the app.
I was wondering what would be the best way to experiment with the code in the pull request?
I can see that there is a sub-directory called maplibre-native, inside source tree of maplibre-native-qt. (I mean here - maplibre-native-qt->vendor->maplibre-native )
Not sure if it will be a good practice to put folder maplibre-native with your PR changes/branch and recompile maplibre-native-qt SDK. Or I need to write some Qt binding to be able to use pmtiles features in the app.
Thanks!

Hi. If you have all the changes of this PR in your maplibre-native directory, after compiling you just need to use a pmtiles:// prefix in the source URL, pretty much the same way you would do with MBTiles (mbtiles://). This is the only thing you should care about, you can use everything as usual, nothing is changed in the API with this PR.

Thanks @tdcosta100, I have compared the two dirs, there is some difference in directory structure, looks like I will need to manually follow all commits and make changes in files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PMTiles support
7 participants