Skip to content

#3304 - Initial resolution for local pmtiles asset, likely better resolution would be upstream in providers. #3305

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

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

Conversation

zach-snell
Copy link

No description provided.

@louwers louwers requested a review from tdcosta100 March 13, 2025 09:05
@louwers
Copy link
Collaborator

louwers commented Mar 13, 2025

Not sure if this is something that should be implemented in the PMTiles library itself? (cc @bdon)

@zach-snell Can you provide some more details in what cases the current implementation is problematic?

@zach-snell
Copy link
Author

This was an issue when loading PMTiles directly as an asset on Android.

i.e.

pmtiles://asset://world.pmtiles

In this use case the Android Asset Provider would return the full file without respecting the dataRange concept and break the current implementation.

@tdcosta100
Copy link
Collaborator

This was an issue when loading PMTiles directly as an asset on Android.

i.e.

pmtiles://asset://world.pmtiles

In this use case the Android Asset Provider would return the full file without respecting the dataRange concept and break the current implementation.

Shouldn't it be handled in the asset URL handler instead of directly doing it in the PMTiles handler? Asking it because PMTiles puts a range data within the file request, but it's up to each handler read this data. As far as I know, there isn't handling of ranges in asset yet. I did it only for file:// and http(s):// protocols.

@louwers
Copy link
Collaborator

louwers commented Mar 13, 2025

Yes I think that's the way to go. 👍

@zach-snell
Copy link
Author

This was an issue when loading PMTiles directly as an asset on Android.
i.e.
pmtiles://asset://world.pmtiles
In this use case the Android Asset Provider would return the full file without respecting the dataRange concept and break the current implementation.

Shouldn't it be handled in the asset URL handler instead of directly doing it in the PMTiles handler? Asking it because PMTiles puts a range data within the file request, but it's up to each handler read this data. As far as I know, there isn't handling of ranges in asset yet. I did it only for file:// and http(s):// protocols.

I agree with you, which is what I was noting before. Though I have less knowledge of the overall code base as to if changing that would impact other areas.

I was mostly working on resolving this for a game for my kid who likes geography ha.
That does feel like the correct solution - if we are using dataRange, every provider should support it.

@tdcosta100
Copy link
Collaborator

If it works for you, it's a solution, despite not an optimal one. I will dive in the asset handler. The PMTiles component relies in forwarding the original request into a general request so other handlers can provide the data. I initially thought only local files and network files, but not asset URLs, but this can be provided, of course. It's a work in progress component, so as people use and ask for fixed and features, we will improve it.

@zach-snell
Copy link
Author

Absolutely, if my day job ever gives me some free time I will consider addressing it in the asset manager.
This may apply to other sources on other platforms - so while the fix I have created is not the proper solution - it would likely work as a fix across all providers while each provider is updated / reviewed.

@tdcosta100
Copy link
Collaborator

Hi, @zach-snell. Could you please tell if #3404 fullfills your needs? If so, we can close this PR.

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.

3 participants