-
-
Notifications
You must be signed in to change notification settings - Fork 335
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 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 |
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 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Just FYI I let a PR review AI bot loose on this. I'm curious. 🙈 |
Hi @navisinghnz9, Qt bindings for now fully support only the OpenGL 2 branch. While |
This comment was marked as off-topic.
This comment was marked as off-topic.
@navisinghnz9 Please open a Q&A Discussion thread so we can keep this thread focussed on the PR. Thanks! |
@louwers @tdcosta100 @ntadej FYI, I have created a discussion topic at this link, #3118. Thank you so much to you guys for your time and guidance so far! |
@tdcosta100 Oops, I think you force-pushed my commit so Android is broken. |
@louwers I'm sorry, didn't notice that. Thank you for correcting this again! |
No worries. Too bad I couldn't get the server tests to work for Android. I will try again. Thanks again for implementing this! |
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.