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

Update base_tile_provider.dart to use retinaMode option #1666

Closed
wants to merge 1 commit into from
Closed

Update base_tile_provider.dart to use retinaMode option #1666

wants to merge 1 commit into from

Conversation

bramp
Copy link
Contributor

@bramp bramp commented Sep 27, 2023

It seems if a urlTemplate contained a {r} it was always populated with a '@2x', instead of optionally setting that if the TileLayer retina argument was set to true.

It seems if a urlTemplate contained a {r} it was always populated with a '@2x', instead of optionally setting that if the `TileLayer` `retina` argument was set to true.
@JaffaKetchup
Copy link
Member

JaffaKetchup commented Sep 27, 2023

This is actually a common misconception, although the docs could be cleared up a bit (did I do this on the online docs?).

retinaMode is used to simulate retina mode. '{r}' and '@2x' are recommended if the tile server actually supports this.

However, I do think this is non-standard behaviour, and potentially needs changing - what's the point of '{r}' if '@2x' can be used instead. But as it stands, this is actually the correct behaviour. More discussion is needed to find whether this should be changed, and if it should, documentation also needs changing, amongst potentially other things.

See this extract of the somewhat self-contradictory with the rest of the message and unclear documentation (I would imagine caused by this exact confusion) on retinaMode:

If geoserver supports retina @2 tiles then it it advised to use them instead of simulating it (use {r} in the [urlTemplate])

@bramp
Copy link
Contributor Author

bramp commented Sep 27, 2023

Thanks for the detailed response. Confusing for sure.

I just sent over #1668 to add an Example page with a retina toggle (before reading your response).

To help me understand (and I'm happy to propose doc fixes), here are some examples

So Flutter Map needs to fill a 256x256 square on the screen, and it'll fetch either of these, and whatever the returned image size, it will put it into the 256x256 square.

  1. If {r} is in the urlTemplate, the @2x will always be added, regardless of the devices dpi (e.g even if the device only supports @1x, details from the image will be lost when displayed).

  2. If retinaMode is set to true, then flutter_maps will fetch 4 squares (at one zoom lower) to make a 512x512 image for this space, and fill that in on the screen. This only makes sense to do if the device supports dpi > @1x.

  3. If the url contains {r} and retinaMode is set to true, we end up fetching 4 squares at 512x512 each (so total of 1024x1024), and jamming that into the 256x256 space?

Next, I don't know how to think about the fact mapBox let's you ask for 512 tiles (e.g this), but to use them correctly, TileLayer's tileSize should be set to 512?

so #3 seems wasteful. Should the correct behaviour be, if the TileProvider supports Retina, then @2x is added, if it does not it should be simulated, but in either case retina should not be used unless retinaMode is set to true? The recommendation could continue to be to set retinaMode true for devices dpi > 1.

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Sep 27, 2023

Also see #1668 (comment).

Your point 3 is correct from my understanding.

My suggestion would be that TileLayer.retinaMode becomes a nullable boolean (maybe this PR was already on the way to this):

  • false: don't do anything about retina mode
  • true: if '{r}' is provided, fill it with '@2x', else simulate it
  • null (default): choose between false and true dependent on dpi automatically
  • Add documentation/logging that using '@2x' manually is not recommended, and if used, retinaMode should always be set false
  • Note that this would mean it would be impossible to force simulation - if this is necessary, we need an enumerable instead

I have no idea how this should interact with different tile sizes, I can only think of tiles as 256px :D

@bramp
Copy link
Contributor Author

bramp commented Sep 27, 2023

Ok, so your proposal seems good. I'm happy to send the PR your way (unless you are half way to finishing it).

I can also file a bug, with all these details and proposal, instead of designing this on a closed PR :)?

As for tileSize, from your testing in #1668 it seems clearly broken. Perhaps deprecate/big red warning? Either way I'm happy to file a bug to address that.

@JaffaKetchup
Copy link
Member

I haven't started the PR (in fact, it's gone 10pm here), so I'm happy for you to do that if you're up for it.
I guess we should use an enum just to allow for some strange situation where simulation must be forced: maybe off (false), preferReal (true), automatic (null), forceSimulation.
We should file a feature request for all this.

Then a bug report is needed for the tile size issue.
Unfortunately, we can't just deprecate or remove it, we actually need to fix whatever is underlying - I don't know what this would be, given that just changing the URL seems to work, might need some digging back through commit history, but I'm happy to attempt that - or, at the very least, leave it hanging uselessly, except for the new '{d}' placeholder effect (as plugins need to use it).

(You seem to be quite involved suddenly, and we're actually looking for more maintainers! If you have the spare time for a while, please consider applying (see the v6 docs > Contributing), but no worries at all if not, your contributions are hugely appreciated!)

@bramp
Copy link
Contributor Author

bramp commented Sep 27, 2023

Bug for the tileSize issue: #1669

Bug/FR for resolving retina issues: #1670

As for being a maintainer, I'll respectfully decline for now, as I honestly don't think I'll be active enough. But I'm using flutter_maps in a project, and as I progress I'd be happy to continue to contribute to the project.

@JaffaKetchup
Copy link
Member

Thanks :)

No worries at all about not wanting to be a maintainer, your contributions are always greatly appreciated!

@bramp bramp deleted the patch-1 branch October 2, 2023 22:07
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.

2 participants