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

handle {r} in tile urls for hidpi raster tiles (fix #141) #637

Merged
merged 2 commits into from
Nov 13, 2021

Conversation

xabbu42
Copy link
Contributor

@xabbu42 xabbu42 commented Nov 12, 2021

Replace {r} in tile urls with @2x for devices with a pixelratio > 1.

This feature is compatible with leafletjs and useful to fetch hidpi tiles for
layers that support it.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Write tests for all new functionality.
  • Suggest a changelog category: "skip changelog".

This feature is compatible with leafletjs and useful to fetch hidpi tiles for
layers that support it.
@xabbu42
Copy link
Contributor Author

xabbu42 commented Nov 12, 2021

There are no tests for pixelratio specific behaviour and this patch is rather trivial, so I don't think tests are necessary, but I can add them if requested.

I'm also not sure about documentation and changelog entries. So far the documentation only mentions https://github.com/mapbox/tilejson-spec but the code already supports other undocumented variables. So I think its ok to not document it for now. Its probably also not important enough for a changelog entry.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2021

Bundle size report:

Size Change: +20 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB +20 B
maplibre-gl.css 9.49 kB 9.49 kB 0 B
ℹ️ View Details
Source file Before After Change
rollup/build/tsc/src/source/tile_id.js 1.1 kB 1.13 kB +30 B

@xabbu42
Copy link
Contributor Author

xabbu42 commented Nov 12, 2021

I did test the patch with a raster layer from https://www.thunderforest.com and it works as intended.

@xabbu42
Copy link
Contributor Author

xabbu42 commented Nov 12, 2021

fixes #141

@HarelM
Copy link
Collaborator

HarelM commented Nov 12, 2021

I think it's worth a change log entry. I also think there's a unit test that can be enhanced to covert this too.
Thanks!

@xabbu42
Copy link
Contributor Author

xabbu42 commented Nov 13, 2021

Should be ready for merging now.

@xabbu42 xabbu42 merged commit 69cf4ab into maplibre:main Nov 13, 2021
@xabbu42 xabbu42 deleted the fix-141 branch November 13, 2021 11:11
@jonasmatthias
Copy link

👍 Thanks for implementing this.

The {r} option should also be documented. Maybe Maplibre should even fork the tilejson spec and add the {r} option to the spec? See mapbox/tilejson-spec#16

@xabbu42
Copy link
Contributor Author

xabbu42 commented Nov 14, 2021

I don't think we should fork the tilejson spec just for documenting but rather document this and other differences of maplibre to the spec in our documentation. Feel free to tackle this yourself if you want as I will not have the time.

There seems to be some more documentation in maplibre-gl-native (https://github.com/maplibre/maplibre-gl-native/blob/master/platform/ios/platform/ios/docs/guides/Tile%20URL%20Templates.md) and also already the same feature just with {ratio}. Of course compatiblity to maplibre-gl-native is more important than with leafletjs so I will make a follow up pull request to use ratio instead.

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