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

smooth zooming #13

Merged
merged 25 commits into from
Feb 20, 2023
Merged

smooth zooming #13

merged 25 commits into from
Feb 20, 2023

Conversation

rafaqz
Copy link
Collaborator

@rafaqz rafaqz commented Jan 17, 2023

This PR plays with smooth zooming by plotting multiple layers at once.

Edits for changed syntax

This will plot 20 layers above (so zooming all the way out is pretty smooth):

tyler = Tyler.Map(Rect2f(-0.0921, 51.5, 0.04, 0.025); depth=20)

But the defaults are best:

tyler = Tyler.Map(Rect2f(-0.0921, 51.5, 0.04, 0.025); depth=8, halo=0.2)

You can get close to the original behavior with this:

tyler = Tyler.Map(Rect2f(-0.0921, 51.5, 0.04, 0.025); depth=0, halo=0.0)

With higher_zoom > 0 the scale of the tiles in the plot will be too small. It would be good to load theses high res layers but not actually show the plots of them until we need to. But having the next layer already downloaded is nice for zooming in. I removed this now

Closes #17

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 20, 2023

The default now is much more like using google maps than what we have on master. Someone should try it ;)

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 21, 2023

Updates:

  • Using an OrderedSet from OrderedCollections.jl improves things a lot as we can control what loads first, like large lower res tiles
  • Adding a halo around the viewed region fixes the updates around the edges while zooming out. With the OrderedSet this loads after the main tiles load. The halo keyword controls how big the halo is with a Float64

There is still some kind of bug when you zoom in or out very fast. The screen will blank to white and start loading tiles again - it shouldn't be possible for this to happen but somehow it does. Fixed

  • calculating the z index directly for the extent and a resolution

@lazarusA
Copy link
Collaborator

from where this ERROR: LoadError: UndefVarError: GeoFormatTypes not defined is coming from? is also in #16

@visr
Copy link
Collaborator

visr commented Jan 21, 2023

Is from MapTiles changes, see #18.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 22, 2023

It should be working now

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 22, 2023

I think this is good to merge, just have to update the test for however many tiles there are now 😅

@SimonDanisch
Copy link
Member

Sorry, I haven't really gotten to review this 😓
162 tiles for a small map seems quite a lot though :-O I feel like the idea I had with just not deleting so many tiles when zooming should combine the best of both approaches, but I still haven't had time to look into the specifics..

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 25, 2023

You should use it first ;)

It's both higher res and just up-front cost of adding layers.

After that much less are downloaded, because they're zoomed out and the ring around what you are currently looking at, which you would download next anyway.

Removing the halo will cut the numbers a lot too. But Google totally downloads around the edges.

Also, play with those two parameters before you go rewriting things... you can cut the downloads heaps by reducing them.

I put heaps of work into this and the feel is really good compared to before.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 25, 2023

Looking at the test failure (after tweaking the 2 parameters) It's 81 tiles without anything here besides using the figure resolution to calculate z:
https://github.com/MakieOrg/Tyler.jl/actions/runs/4010427694/jobs/6886926050#step:7:338

We can change the default resolution maybe ?

So using 8 layers and the halo (which is the point you rarely see white flashes) only doubles the first download to 162.

Using 4 layers and no halo adds half to it (121):
https://github.com/MakieOrg/Tyler.jl/actions/runs/4010338840/jobs/6886736048#step:7:338

But with four layers you zoom out and hit white pretty easily.

Edit: ok numbers are way down!! the problem was halo=0 was not zero, and halo=0.5 was a lot of tiles. Now you should get for 1000 * 1000 resolution

  • no depth or halo : 16 - 40 tiles
  • default depth and halo: ~ 25 - 70 tiles (48 for the test area/zoom)

The 16 - 40 range is because how I've used the z_index method, the resolution will never be lower than the plot resolution, but may be higher at certain zooms. Also just panning will alternate from 16 to 25 tiles for the same square as the window crosses tiles.

The 25-70 numbers are just the same dynamic but with occasional extra halo tiles when near edges, and with the other zoomed out layers loading first. The immediate next layer will be 8 - 20 tiles, above that 4-10 tiles. Etc. Pretty quickly adding more layers has very little cost.

@rafaqz rafaqz requested review from visr and lazarusA January 31, 2023 16:54
@lazarusA
Copy link
Collaborator

I will check this later. But, did you test that this example works with this branch? It's the only one failing in the docs.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 31, 2023

Ah ok I wasn't sure what was happening there, will fix

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jan 31, 2023

Also note: the zoom argument and the min_tiles and max_tiles keywords are gone now - the figure resolution and extent is used instead to set zoom.

We were getting different resolution on zoom in and zoom out with the old method.

src/Tyler.jl Show resolved Hide resolved
@lazarusA
Copy link
Collaborator

lazarusA commented Feb 1, 2023

@SimonDanisch merge?

@visr
Copy link
Collaborator

visr commented Feb 1, 2023

This is definitely smooth. Looked at openstreetmap.org and this PR side-by-side, first Tyler and then openstreetmap.org:

image
image

the resolution will never be lower than the plot resolution, but may be higher at certain zooms

At almost every zoom level I find the street names are too small to read, do others see the same?

From the openstreetmap.org url you can also see that one scroll always directly jumps to the next zoom level, so you are never in between zoom levels. It looks like https://leafletjs.com/ is doing the same. Should we perhaps do the same by default, or as an option? For raster tiles with pre-rendered images at fixed zoom levels this makes sense, for vector tiles not anymore, hence for instance Google Maps zooming is much more continuous.

@SimonDanisch
Copy link
Member

I still think we should simply remove "less" tiles and be less greedy loading from further away zoom levels... But haven't had time to try it out compared to what's being done in this PR...

@rafaqz
Copy link
Collaborator Author

rafaqz commented Feb 1, 2023

@visr you can set the resolution keyword to whatever you want it to be. The default is (1000 , 1000), and yeah maybe that's too fine. What do you think the default should be?

We could also target the mean resolution instead of minimum? so it is sometimes better sometimes worse than the figure resolution?

My comparison is with google maps satellite view, which behaves pretty similar to this and has similar tile resolution, except it loads center out, which I'll add too when this is merged. Leaflet is different to google maps in that it loads one total layer then displays it at once, leaving the grey edges while you wait. You could add a leaflet mode that does that?

@SimonDanisch what do you mean by remove less tiles?

And we can totally change the default depth and halo too (to be less greedy). What do you think the defaults should be?

@visr
Copy link
Collaborator

visr commented Feb 1, 2023

The default is (1000 , 1000), and yeah maybe that's too fine.

With 1000x1000 pixels I get a good size window on my monitor, that is fine. It is just the zoom level that it is showing that seems to be too high, since it is impossible to read the smaller letters. If I set resolution to 500x500, I do indeed see a lower zoom level, but still too high to comfortably read the letters.

image
image

It seems sensible to try to show the pixels 1:1. Perhaps mean is already better? Looking at the original tiles, like the one below, it only starts to be legible when I zoom in to 125%.

jl_MrO9S3ARIH

And putting the tile zoomed in to 125%, such that it is 320x320 pixels, it actually seems to match what openstreetmap.org does exactly
image

@visr
Copy link
Collaborator

visr commented Feb 1, 2023

Though of course I'm only looking at OpenStreetMap examples. If I'm looking at sattellite images it might be nicer to see every pixel once on average. So a scaling option might be nice, keeping your current defaults.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Feb 1, 2023

The letter size may be an artefact OSM server loads needing to serve less tiles. Google maps uses a lot more. But of course their text is separate from the tiles so they don't have that problem. I mostly made this PR using google satellite so the defaults mostly reflect that. (Try using the Google() provider too)

But yes I agree the mean is better and makes more sense, I will change it.

Edit: we may need a scaling factor besides figure resolution to deal with tiles being meant for different resolutions?

@visr
Copy link
Collaborator

visr commented Feb 1, 2023

Ha the 125% is actually my OS setting. And my browser listens to it, but Tyler doesn't. That's the difference.

image

@rafaqz
Copy link
Collaborator Author

rafaqz commented Feb 1, 2023

Haha posted the same idea before I read yours. Let's do that (scaling)

@lazarusA
Copy link
Collaborator

lazarusA commented Feb 9, 2023

it fail because of:

osm_base Dispatcher_Client::request_read_and_idx::timeout. The server is probably too busy to handle your request.

@SimonDanisch
Copy link
Member

@SimonDanisch what do you mean by remove less tiles?

So on master we're doing this in pseudo code:

newtiles= Set(get_tiles(visible_extent, zoom))
set_diff!(map.displayed_tiles, newtiles) # remove any tile that's not in newtiles

Tiles are hashed by their x, y indices and zoom level, so if zooming in/out changes the zoom level, all tiles from the previous zoom level will get removed and we'll start displaying the new tiles from zero.
What we should do instead is this:

newtiles = Set(get_tiles(visible_extent, zoom))
union!(map.displayed_tiles, newtiles) # add the newtiles that are perfect for visible_extent + zoom
filter!(map.displayed_tiles) do tile  # filter out any tile outside the bounds
     tile_extent = project(tile, TileIndexSpace, map.target_space)
     return !(tile_extent in visible_extent) # or whatever method we have to figure out overlap
end

This will leave any displayed tile from another zoom level that's still inside the bounds of the area we look at.
So, zooming in will continue showing the bigger tiles from the previous zoom level until completely covered by the tiles from the new zoom level...

@rafaqz
Copy link
Collaborator Author

rafaqz commented Feb 20, 2023

Does anyone understand that GLFW error in the docs?

@lazarusA
Copy link
Collaborator

than's a new one for me. Also, there is an strange error before
ERROR: LoadError: The repo path https://github.com/MakieOrg/Tyler.jl.git should not contain the protocol.

And recently I learned that we should be using here

steps:
      - uses: actions/checkout@v3

instead of @v2 as the one we are using now.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Feb 20, 2023

Alright all green thanks @SimonDanisch !

Lets merge this before I have to rebase again, we can make changes in new PRs.

The default tile number is now based on the mean resolution, there is a scale keyword, and the tests load only 24 tiles :)

@SimonDanisch SimonDanisch merged commit d8d5002 into master Feb 20, 2023
@SimonDanisch SimonDanisch deleted the smoooth branch February 20, 2023 10:24
@SimonDanisch
Copy link
Member

Thanks :)

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.

Use figure resulution to choose zoom level
4 participants