-
-
Notifications
You must be signed in to change notification settings - Fork 333
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
base: main
Are you sure you want to change the base?
Initial PMTiles support #2882
Conversation
Bloaty Results 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2882-compared-to-main.txtCompared to d387090 (legacy)
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2882-compared-to-legacy.txt |
Benchmark Results ⚡
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2882-compared-to-main.txt |
Bloaty Results (iOS) 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2882-compared-to-main.txt |
6888722
to
e0bed86
Compare
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? |
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.
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
It is agnostic about it, it just makes requests to the Resource Loader, then it use the appropriate source, that is, you can use |
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 |
94f448c
to
e778def
Compare
03b9ef4
to
d3c191c
Compare
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. |
d3c191c
to
5e9e6cc
Compare
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.
Looks OK!
I would prefer as little #define
s 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 |
I want to avoid 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 |
Okay, I will think about it and implement these changes. Without |
dd8a01f
to
28c4b01
Compare
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. |
Hi, I am working on an cross-platorm mobile app using maplibre-native-qt. 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. 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. Thanks! |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
2e20f53
to
171c0c9
Compare
Hi. If you have all the changes of this PR in your |
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. |
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.