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

Design proposal: Support for refreshTiles per tile source #61

Open
pramilk opened this issue Mar 17, 2023 · 11 comments
Open

Design proposal: Support for refreshTiles per tile source #61

pramilk opened this issue Mar 17, 2023 · 11 comments
Labels
enhancement New feature or request

Comments

@pramilk
Copy link
Contributor

pramilk commented Mar 17, 2023

#Updated Proposal

Motivation

Currently map.options provide a property refreshExpiredTiles (default true). This applies to all tile sources.

Not all tile sources need to be refreshed and currently there is no way to override the map.options.refreshExpiredTiles per tileSource. In our use case on Bing maps only Traffic tiles need to be refreshed and no other tiles irrespective of the caching header needs to be refreshed.

Proposed Change

  • Option 1

    Proposal is the have a property refreshExpiredTiles (default: true) which can be set for each tile source. This will allow SDK user more granular control over which tile source need to be refreshed. Tiles which does not require refresh can be downloaded using HTMLImageElement.

    This property can be made available for Raster, Raster_dem and Vector source Type. Support for Vector source type is just for consistency.

    If user has not provided refreshExpiredTiles override then code will continue to use fetch as done currently.

    • Alternative property name: 'refreshTiles'
  • Option 2

    Instead of having a boolean refreshExpiredTiles (Option 1) on tile sources, we can have a property like expiryDuration in seconds. With this all raster tiles can also be downloaded using HTMLImageElement (Both refreshable and non-refreshable raster tiles).

    • Issue with this approach is decoupling of cache data between tiles and style endpoints. This might not be desirable in some cases.

    If user has not provided expiryDuration override then code will continue to use fetch as done currently.

API Modifications

  • Design option 1 with refreshExpiredTiles

    {sources:
    	{
    		"source1": {
    			"type": "raster",
    			"tiles": ["<Tile url>"],
    			"minzoom": 5,
    			"maxzoom": 13,
    			"tileSize": 256,
    			"refreshExpiredTiles": false
    		},
    		// Default current production behavior
    		"traffic": {
    			"type": "raster",
    			"tiles": ["<Tile url>"],
    			"minzoom": 5,
    			"maxzoom": 13,
    			"tileSize": 256
    		}
    	}
    }
  • Design option 2 with expiryDuration

    {sources:
    	{
    	    // Default current production behavior
    	    "source1": {
    			"type": "raster",
    			"tiles": ["<Tile url>"],
    			"minzoom": 5,
    			"maxzoom": 13,
    			"tileSize": 256 
    		},
    		"source2": {
    			"type": "raster",
    			"tiles": ["<Tile url>"],
    			"minzoom": 5,
    			"maxzoom": 13,
    			"tileSize": 256,
    			"expiryDuration": 0 // <=0 sec to not refresh
    		},
    		"traffic": {
    			"type": "raster",
    			"tiles": ["<Tile url>"],
    			"minzoom": 5,
    			"maxzoom": 13,
    			"tileSize": 256,
    			"expiryDuration": 600 // refresh in 600 sec
    		}
    	}
    }

Migration Plan and Compatibility

Both design options described above adds an optional property and thus are backward compatible.

@HarelM
Copy link
Member

HarelM commented Mar 19, 2023

Thanks for taking the time to write this!
I think there are two different aspects here that are mixed a bit:

  1. Allow more granularity for refreshing tiles
  2. Improve performance in web.

While the second point is interesting I think we should focus on the first point.
We can also argue in favor of the first point that allowing it will allow improving performance in web implementation (for some cases).

So I would like to try and focus the discussion around the spec, use cases, alternatives etc without getting into implementation details ATM.

@pramilk
Copy link
Contributor Author

pramilk commented Mar 20, 2023

My primary motivation for this change is to improve performance of raster tiles. #1(granularity for refreshing tiles) is the only way I see it can be achieved and it additionally helps with having better control on which set of tiles require refresh.

Basically, if a tile source does not need a refresh then we don't care about its cache headers and thus HTMLImageElement can be used.

#1 in itself will help

  • Having granular control for refreshing tiles
  • Help a little bit with performance/memory, as we don't have to set a timer for individual tiles to refresh after x ms.

What is the additional input/data required to move forward from here? I have put in 2 options in design proposal. My preference is Option 2, but Option 1 also fine for our scenarios.

@zhangyiatmicrosoft
Copy link

option 1 seems easier. @HarelM what's your suggestion?

@wipfli
Copy link
Member

wipfli commented Mar 21, 2023

@pramilk since the style specification is independent of MapLibre GL JS in the sense that we also use it in MapLibre GL Native, I think it would be good if you could rewrite the design proposal in such a way that it does not reference javascript or web specific things like HTMLImageElement or the fetch API.

@wipfli
Copy link
Member

wipfli commented Mar 21, 2023

The idea is cool, we just have to make the motivation a bit more generic such that we know what to do in Native...

@HarelM
Copy link
Member

HarelM commented Mar 21, 2023

In theory, raster tiles performance improvements can probably be achieved using other "tricks", so I agree with @wipfli, the discussion should be about the spec and be platform independent.

@pramilk
Copy link
Contributor Author

pramilk commented Mar 22, 2023

Sure will update the proposal.

@pramilk
Copy link
Contributor Author

pramilk commented Apr 3, 2023

Sorry for the delay. I have updated proposed design above and have removed any implementation details.

Given that this change is also in TileJSON, if everyone is OK with this change, shall I go ahead and propose the changes to TileJON spec?

@HarelM
Copy link
Member

HarelM commented Apr 8, 2023

We believe the spec to be abandoned in terms of maintenance, we need to understand what are our next steps related to this spec (and probably the mbtiles as well).
This proposal and the other one will be discussed in the technical steering committee I believe.

@ovivoda
Copy link
Contributor

ovivoda commented Apr 15, 2023

As discussed in the TSC meeting on 12'th of April, the proposal is generally approved

Action points:

  • need to think if this should be part of the spec or not or there should be an API to override this per platform/use case

@ovivoda
Copy link
Contributor

ovivoda commented Apr 15, 2023

Also from TSC a note from Minh Nguyễn:

An API setting could be complementary to something in TileJSON or style JSON. It would allow the MapLibre Navigation SDK to synchronize refreshing of the route data (which includes traffic) and basemap traffic, avoiding user confusion.

@HarelM HarelM added the enhancement New feature or request label Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants