-
-
Notifications
You must be signed in to change notification settings - Fork 924
Fix: raster-fade-duration is ignored (issue #5038) #6053
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
base: main
Are you sure you want to change the base?
Conversation
|
@wagewarbler see if you can take a look. |
|
The tests were added in #4072, so I think @wagewarbler is the right person to ask. I tried looking more at the code, and it seems like it is already expected to return null when looking at one of the tests in the maplibre-gl-js/src/source/source_cache.test.ts Lines 2138 to 2154 in 672bea3
Though it is not super transparent that that is the intention. Anyways, I tried to understand the logic, and if I understand it correctly, maplibre-gl-js/src/source/source_cache.ts Lines 431 to 447 in 672bea3
Instead, as I understand it at least, the intention of /**
* Find a loaded sibling of the given tile (and make sure to only return sibling tiles)
*/
findLoadedSibling(tileID: OverscaledTileID): Tile {
const tileKey = tileID.key;
const siblingKey = tileID.wrapped().key;
// Check if tile is in _tiles
if (this._tiles[tileKey]) {
return null;
}
// Check if tile is in cache
if (this._cache.getByKey(tileKey)) {
return null;
}
// Check if sibling tile is in _tiles
const tile = this._tiles[siblingKey];
if (tile) {
return tile;
}
// Check if sibling tile is in cache
const cachedTile = this._cache.getByKey(siblingKey);
if (cachedTile) {
return cachedTile;
}
// If tile is not found, return null
return null;
}And the test would be something like: describe('SourceCache#findLoadedSibling', () => {
test('adds from previously used tiles (sourceCache._tiles)', () => {
const sourceCache = createSourceCache({});
sourceCache.onAdd(undefined);
const tr = new MercatorTransform();
tr.resize(512, 512);
sourceCache.updateCacheSize(tr);
const tile = {
tileID: new OverscaledTileID(1, 0, 1, 0, 0),
hasData() { return true; }
} as any as Tile;
sourceCache.getTiles()[tile.tileID.key] = tile;
// If the tile is neither the same nor a sibling, it should return null
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 0, 1, 1, 0))).toBeNull();
// If the tile is the same, it should return null
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 0, 1, 0, 0))).toBeNull();
// If the tile is a sibling, it should return the tile
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, -1, 1, 0, 0))).toEqual(tile);
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 1, 1, 0, 0))).toEqual(tile);
});
test('retains siblings', () => {
const sourceCache = createSourceCache({});
sourceCache.onAdd(undefined);
const tr = new MercatorTransform();
tr.resize(512, 512);
sourceCache.updateCacheSize(tr);
const tile = new Tile(new OverscaledTileID(1, 0, 1, 0, 0), 512);
sourceCache.getCache().add(tile.tileID, tile);
// If the tile is neither the same nor a sibling, it should return null
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 0, 1, 1, 0))).toBeNull();
// If the tile is the same, it should return null
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 0, 1, 0, 0))).toBeNull();
// If the tile is a sibling, it should return the tile
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, -1, 1, 0, 0))).toEqual(tile);
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 1, 1, 0, 0))).toEqual(tile);
expect(sourceCache.getCache().order).toHaveLength(1);
});I'm not entirely sure though, so I'll wait and see if @wagewarbler has time to look at it before I continue diving more into it 🙂 |
|
Wanted to add a quick comment to say I've been able to test the PR code in a local app and confirm the functionality introduced in #4072 (cross-fading between "sibling" tiles) is still supported correctly in this PR. 🎉 Need to dig a little more and regain some context to hopefully provide some answers to the questions I've been tagged on. |
|
I've had some time to work through the code a bit and debug. A few high-level observations:
I ran the code locally and observed no blinking effect but also no retrieval of sibling tiles. I think the solution to the issue is one of two options:
@HarelM curious to hear your thoughts on improvements in tile rendering that I'm seeing and possible next steps! |
|
I would go with reverting to fix the regression and if later on this logic is still needed create a new PR making sure everything works as expected. There were a lot of code changes so I don't have a good idea what might have solved the issue, sorry... |
|
Seems like maybe we should close this PR and open a different one to revert the changes and add some tests around raster-fade-duration then? I could take that task unless you'd like to @JacobJeppesen |
|
You're very welcome to take it @wagewarbler if you can. I'm away on vacation, so it'll take some time before I'd be able to look at it 🙂 |
|
As a user of historical raster data, I want a per-tile fading between old and new raster-tile source, so that the map style changes result in smoothly transitioning from one TMS date to another, in order to be less distracting. Following comment #1235 (comment), I just realized this feature was introduced in #4072 so already present in maplibre, got reverted around 4.2.0 according to this issue, and this PR is the place for adding back that functionality. My original intention was adding the ability in mapbox, and maplibre before mapbox for legal hurdles, to smoothly fade between old and new raster-tile-source, especially useful for historical tile sources like below. tms_url = `https://gibs-b.earthdata.nasa.gov/wmts/epsg3857/all/MODIS_Aqua_CorrectedReflectance_TrueColor/default/${year}-08-02/GoogleMapsCompatible_Level9/{z}/{y}/{x}.jpg`The siblings loading is the piece that would parse pre-existing tile with same xyz in order to pass the old texture to the raster fragement shader. From my point-of-view, the assumption that textures EDIT: Could it be that the source cache gets cleared before old maplibre tries to load siblings? Trying to track it down, it seems that a chain of events resets the source_cache when
EDIT 2: Just checked-out commit 700ae88 with the below example, and there is no smooth cross-fading but a sharp transition from old to new source like first screencast of version 5.6.0 - as shown in the gif of PR associated to that commit. I'm not entirely sure this feature had been completed up to the smooth-fading @wagewarbler, up to passing the sibling texture to the newly loaded tile raster fragment uniform instead of parentTile for fading. Note I had to maplibre-gl-js/src/source/source_cache.ts Lines 259 to 274 in 50da15c
For reference current implementation: Test implementation<!DOCTYPE html>
<html>
<head>
<title>Mapbox-Raster-Reprojection</title>
<meta
name="viewport"
content="initial-scale=1,maximum-scale=1,user-scalable=no"
/>
<!--
<link
href="https://api.mapbox.com/mapbox-gl-js/v3.14.0/mapbox-gl.css"
rel="stylesheet"
/>
<script src="https://api.mapbox.com/mapbox-gl-js/v3.14.0/mapbox-gl.js"></script>
<link rel='stylesheet' href='../dist/mapbox-gl.css' />
5.6.2
-->
<link href="https://unpkg.com/maplibre-gl@^3.0.0/dist/maplibre-gl.css" rel="stylesheet" />
<meta charset="UTF-8" />
<style>
body {
font-family: sans-serif;
}
html,
body,
#map {
margin: 0;
height: 100%;
}
</style>
</head>
<body>
<div id="map"></div>
<!--
<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
-->
<script src='https://unpkg.com/maplibre-gl@^3.0.0/dist/maplibre-gl.js'></script>
<script>
let year = 2015;
// mapboxgl.accessToken =
// "pk.eyJ1IjoiYnJhbnpoYW5nIiwiYSI6ImNqM3FycmVldjAxZTUzM2xqMmllNnBjMHkifQ.Wv3ekbtia0BuUHGWVUGoFg";
// const map = new mapboxgl.Map({
const map = new maplibregl.Map({
container: "map",
// style: "mapbox://styles/mapbox/streets-v11",
style: 'https://demotiles.maplibre.org/style.json', // style URL
center: [0, 55],
zoom: 3,
hash: true,
});
map.showTileBoundaries = true;
map.on("load", () => {
map.addSource("custom-source", {
scheme: "xyz",
type: "raster",
tiles: [
`https://gibs-b.earthdata.nasa.gov/wmts/epsg3857/all/MODIS_Aqua_CorrectedReflectance_TrueColor/default/${year}-08-02/GoogleMapsCompatible_Level9/{z}/{y}/{x}.jpg`,
],
tileSize: 256,
});
map.addLayer({
id: "custom-source-layer",
type: "raster",
source: "custom-source",
paint: {
"raster-opacity": 1,
"raster-fade-duration": 500,
"raster-opacity-transition": { duration: 500 },
},
});
launchInterval();
});
function callback() {
year += 1;
if (year > 2024) {
year = 2015;
}
console.log(year)
let source = map.getSource("custom-source");
let new_url = `https://gibs-b.earthdata.nasa.gov/wmts/epsg3857/all/MODIS_Aqua_CorrectedReflectance_TrueColor/default/${year}-08-02/GoogleMapsCompatible_Level9/{z}/{y}/{x}.jpg`;
//source?.setUrl(new_url); // not working
source?.setTiles([new_url]);
}
let intervalId;
function launchInterval() {
intervalId = setInterval(callback, 2000);
}
</script>
</body>
</html>maplibre 5.6.0There is no fading between old and new tile sources even with 2025-08-18.-.12-04-38.mp4maplibre ^3.0.0raster-fade-duration respected, but no smooth cross-fade between old and new raster tile source 2025-08-18.-.12-40-19.mp4mapbox 3.14.0Note, in current mapbox implementation: there is a smooth fade-in, but the source-cache is cleared before loading the new raster tile source. Therefore, the entire screen/map viewport gets cleared before new tiles load smoothly, which is unwanted as well. 2025-08-12.-.15-24-06.mp4 |
|
@wagewarbler @JacobJeppesen what's the status of this PR? Any chance you can push this forward? |
|
@HarelM I have some time carved out next week to tackle this. |
|
Thanks @wagewarbler! I truly appreciate it. |
|
😕 😕 😕 findLoadedSibling(tileID: OverscaledTileID): Tile {
return null;
// If a tile with this ID already exists, return it
// return this._getLoadedTile(tileID);
}I know, but the 1 line fixed the fade for me. Not sure if this helps as a clue for anything... so what I'm saying I guess is that it might not be in the findLoadedSibling function. |
|
Not sure it's related, but there is some ongoing progress tracking another bug here, missing children for overzoomed tiles. Just wanted to mention it since my understanding (afk atm) is roughly the same code path is used for overzoomed and siblings for new tileset called via setTiles, where the siblingTile is passed to the raster shader parentTile slot. |
|
I don't think it is related because I'm using the same example to test both of these issues and the other issue clearly throws an error and it does not in this case... |
|
Are you sure that Source is the proper place for data cross-fading? The fading logic within a source is highly complex and is meant for fading within that source data. Seems to me that maybe a better way to do this is to cross-fade between layers using styles as abstraction. I think a new source may be warranted versus hot-swapping the data in a source as I don't believe sources are built for data cross-fading. The wanted behavior can possibly be achieved by a timeout across newer and incoming source/layers by using an opacity timer? Screen.Recording.2025-09-23.at.23.25.27.movIt seems counter-intuitive that a source would need to be responsible for cross-fading using different sets of data within itself provided said data as a new source would likely be a better option as cross-fading is already extremely complex with pitched maps as is. See: let opacity = 1;
function fadeOut() {
map.setPaintProperty('simple-tiles', 'raster-opacity', opacity);
map.setPaintProperty('simple-tiles-2', 'raster-opacity', 1 - opacity);
setTimeout(() => {
opacity -= .01;
if (opacity <= 0) return;
fadeOut();
}, 50);
}
map.once('load', () => {
setTimeout(() => fadeOut(), 3000);
}); |
|
@wayofthefuture thanks for looking into this! |
|
I think the reason why this cross-fading was applied at the source level rather than layer is because the logic was already existing to cross-fade between lower/overzoomed and current-level tile while higher-zoom tiles would load. Extending cross-fading, and swapping/replacing that lower-zoom tile image with the new source tile would allow applying per-tile cross-fade, which could be very beneficial in the cases shown in earlier comment. |
|
I am currently re-writing fading and the existing logic had issues and appeared incomplete, evidenced by the fact that getLoadedSibling returned itself, and the bulk of the logic was built to work around that assumption. Your request seems to be data-crossfading due to existing logic you had, but when you get into the weeds on ideal tiles you can start to see why fading is so hard... and why separate sources could be a better choice than adding complex logic to already complex logic. This seems the case especially if you want to support fading on pitched maps where the front tilts up while the back tilts down. |
|
@wayofthefuture With the release of 5.9.0 and your rework of fading I had hoped Did I have false expectations or is there a phase 2 or 3 where that property will be "re-enabled"? I have a project where some layers/sources look better with the "default" 300 ms fade but a lot of stuff that needs instant visibility changes. Is there anything that can be done to help out? |
|
Could you be more specific? Raster fading currently applies to close ancestors, descendants, or unloaded edge tiles. |
|
Yeah absolutely. So (and maybe this is the issue itself) I have a setup wrapped by The fade-out of the opacity has been locked at 300 ms regardless of the value passed to the style's Paint property for face duration. My hope was that 5.9.0 or the fix for this particular PR would allow the fade-out to occur over a duration specified by that StyleSpec property. I'm likely exposing a huge skill issue here but I'm happy to be educated. Use-case is here: https://beta.prairiewx.ca/map Implementation is here: https://github.com/plymer/wx/blob/dev/src/client/components/map/layers/base/RasterData.tsx |
|
You know, sometimes I have to say things out loud to really, really drive home my skill issues. Please ignore me. |
|
Nah, we each have our own unique skillsets, i.e. yours being weather. I think you may want to try setting raster fade duration at 0 and dynamically adjust raster opacity on the layer yourself. Think of fading as between similar tiles of different Z or level of detail, versus your need to fade between different sources of data. You have a special case see my example above. If you load your next frame behind the current layer ahead of time you could smoothly fade between them dynamically using raster opacity. Your fade duration could then be controlled by a setTimeout. |
|
I appreciate your kind words! It turns out, the property I really needed to work with was just Thirteen months of not knowing how to fix my issue and hacking around it... RTFM strikes again (lol) |
This PR fixes #5038.
The code provided in the first commit is a very early version, mostly written by Github Copilot. It does however identify the issue with
raster-fade-duration, which was checked with:I have added a test at what I believe is the right place, which does fail/succeed depending on the fix, though the fix seems to break some of the existing
findLoadedSiblingtests. The test I added is also written by Github Copilot, so it should be taken with a grain of salt. It'll take me some time to get up to speed with the codebase, but I submitted the PR with these implementations if someone can quickly take a look, and check if it's on the right track. Or if anyone who is already well versed in the codebase and have time, they'd be very welcome to propose a better (/proper) implementation of the function and the test 🙂Launch Checklist
CHANGELOG.mdunder the## mainsection.