Skip to content

Conversation

@JacobJeppesen
Copy link

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:

<!DOCTYPE html>
<html>
<head>
    <style>
        #map { height: 100vh; }
    </style>
</head>
<body>
    <div id="map"></div>
<!--    Using v5.6.0 will make tiles immediately appear (i.e., raster-fade-duration is not properly applied) -->
<!--    <link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/maplibre-gl.css" rel="stylesheet" />-->
<!--    <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/maplibre-gl.js"></script>-->
<!--    Using a locally built version with the fix applied makes tiles slowly fade into view -->
    <link href="maplibre-gl.css" rel="stylesheet" />
    <script src="maplibre-gl.js"></script>
    <script>
        new maplibregl.Map({
            container: "map",
            style: {
                version: 8,
                sources: {
                    "osm-tiles": {
                        type: "raster",
                        tiles: ["https://a.tile.openstreetmap.org/{z}/{x}/{y}.png"],
                        tileSize: 256,
                    },
                },
                layers: [{
                    id: "osm-layer",
                    type: "raster",
                    source: "osm-tiles",
                    paint: {
                        "raster-fade-duration": 5000,
                    },
                }],
            },
            center: [8.0745, 56.5789],
            zoom: 11,
        });
    </script>
</body>
</html>

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 findLoadedSibling tests. 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

  • 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.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@HarelM
Copy link
Collaborator

HarelM commented Jun 24, 2025

@wagewarbler see if you can take a look.
@JacobJeppesen maybe you could tag the people who added the failing tests (using blame assuming it us after december 2020) and see they can understand why these tests are now failing.
Generally speaking though, adding a return value of null to a method that previously didn't return null seems risky...

@JacobJeppesen
Copy link
Author

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 main branch (line 2152):

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;
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 0, 1, 1, 0))).toBeNull();
expect(sourceCache.findLoadedSibling(new OverscaledTileID(1, 0, 1, 0, 0))).toEqual(tile);
});

Though it is not super transparent that that is the intention. Anyways, I tried to understand the logic, and if I understand it correctly, findLoadedSibling() is only supposed to return sibling tiles. The current implementation returns the requested tile if it is already in _tiles or in the cache:

/**
* Find a loaded sibling of the given tile
*/
findLoadedSibling(tileID: OverscaledTileID): Tile {
// If a tile with this ID already exists, return it
return this._getLoadedTile(tileID);
}
_getLoadedTile(tileID: OverscaledTileID): Tile {
const tile = this._tiles[tileID.key];
if (tile && tile.hasData()) {
return tile;
}
// TileCache ignores wrap in lookup.
const cachedTile = this._cache.getByKey(tileID.wrapped().key);
return cachedTile;
}

Instead, as I understand it at least, the intention of findLoadedSibling() would rather be something like:

    /**
     * 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 🙂

@wagewarbler
Copy link
Contributor

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.

@wagewarbler
Copy link
Contributor

I've had some time to work through the code a bit and debug. A few high-level observations:

  • Contrary to my previous comment, sibling tiles do not appear to be correctly loading in the current PR.
  • Having said that, I'm also not seeing the behavior I originally saw in the maplibre prior to Support cross-fading between old & new tiles of the same zoom level #4072 where there was a blinking effect when updating tile sources. @HarelM there have been a number of changes in the past 14 months since that PR, can you think of any that might account for smoother tile imagery transitions?
  • The changes in this PR operate under the assumption that sibling tiles have different wrap values. This is a reasonable assumption as I wrote it in the comments further down in source_cache.ts. With my current understanding of the code, I'm not sure I agree with my previous self that wrap values will always be different. In fact, they seem to frequently be the same! This was part of the reason for a number of test failures but it's a detail that may not matter depending on our next steps.

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:

  • Remove the sibling tile logic thereby reverting back to the lib state where raster fade duration worked correctly
  • Do another pass on sibling tile logic accounting for the observations of Co-pilot. If we preserve that logic, unfortunately I think it's a little more involved than a 1-for-1 swap out of findLoadedSibling. Happy to elaborate further if needed, but am leaning towards the previous option pending some feedback.

@HarelM curious to hear your thoughts on improvements in tile rendering that I'm seeing and possible next steps!

@HarelM
Copy link
Collaborator

HarelM commented Jul 9, 2025

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.
It's also worth making sure all the issues that were encountered as part of this feature/bug are covered with tests so that changing the logic will be tested by these tests.

There were a lot of code changes so I don't have a good idea what might have solved the issue, sorry...

@wagewarbler
Copy link
Contributor

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

@JacobJeppesen
Copy link
Author

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 🙂

@jo-chemla
Copy link

jo-chemla commented Aug 18, 2025

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 wrap are different might not be the right move but I'm still trying to fiugre out what this represents. Can one assume simply that if a sourceCache.findLoadedSibling returns a tile, it will necessary be the older one since the new tile is not yet in cache? Or that the first tile returned is the older one?

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 sourceDataChanged=true:

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 npm install -g node-gyp and switch to "canvas": "^3.0", for npm i to work on node 24.

reload(sourceDataChanged?: boolean) {
if (this._paused) {
this._shouldReloadOnResume = true;
return;
}
this._cache.reset();
for (const i in this._tiles) {
if (sourceDataChanged) {
this._reloadTile(i, 'expired');
} else if (this._tiles[i].state !== 'errored') {
this._reloadTile(i, 'reloading');
}
}
}

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.0

There is no fading between old and new tile sources even with paint: { "raster-fade-duration": 500, },

2025-08-18.-.12-04-38.mp4

maplibre ^3.0.0

raster-fade-duration respected, but no smooth cross-fade between old and new raster tile source

2025-08-18.-.12-40-19.mp4

mapbox 3.14.0

Note, 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

@HarelM
Copy link
Collaborator

HarelM commented Sep 5, 2025

@wagewarbler @JacobJeppesen what's the status of this PR? Any chance you can push this forward?

@wagewarbler
Copy link
Contributor

@HarelM I have some time carved out next week to tackle this.

@HarelM
Copy link
Collaborator

HarelM commented Sep 7, 2025

Thanks @wagewarbler! I truly appreciate it.

@wayofthefuture
Copy link
Collaborator

wayofthefuture commented Sep 8, 2025

😕 😕 😕
I know this sounds totally nuts, because I have been playing with the implementation in this PR for the past 2 hours, and when I went to just remark it out to test something else, it had the same effect. In other words, I thought it was the solution in this PR that fixed things, but then I accidentally realized raster-fade-duration worked after I did this:

    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.

@jo-chemla
Copy link

jo-chemla commented Sep 8, 2025

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.
#5630 (comment)

@wayofthefuture
Copy link
Collaborator

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...

@wayofthefuture
Copy link
Collaborator

wayofthefuture commented Sep 24, 2025

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.mov

It 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);
        });

https://codepen.io/wayofthefuture/pen/NPxqZdW?editors=1000

@HarelM
Copy link
Collaborator

HarelM commented Sep 24, 2025

@wayofthefuture thanks for looking into this!
You did a lot of changes in the relevant code, so that makes you the most knowledgeable person I believe.
If you think the required functionality can be done outside the code base in order to reduce complexity, maybe using a plugin or something, I think it's a better approach.

@jo-chemla
Copy link

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.

@wayofthefuture
Copy link
Collaborator

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.

@plymer
Copy link

plymer commented Oct 11, 2025

@wayofthefuture With the release of 5.9.0 and your rework of fading I had hoped raster-fade-duration would be respected but testing today didn't seem like that was the case.

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?

@wayofthefuture
Copy link
Collaborator

Could you be more specific? Raster fading currently applies to close ancestors, descendants, or unloaded edge tiles.

@plymer
Copy link

plymer commented Oct 11, 2025

Yeah absolutely.

So (and maybe this is the issue itself) I have a setup wrapped by react-map-gl where I am adding a whole whack of Source+Layer combos that I am dynamically changing opacities on to simulate animation (since updating the tiles URLs caused massive UX and perf degradation).

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

@plymer
Copy link

plymer commented Oct 11, 2025

You know, sometimes I have to say things out loud to really, really drive home my skill issues.

Please ignore me.

@wayofthefuture
Copy link
Collaborator

wayofthefuture commented Oct 11, 2025

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.

@plymer
Copy link

plymer commented Oct 11, 2025

I appreciate your kind words! It turns out, the property I really needed to work with was just raster-opacity-transition with a duration: 0

Thirteen months of not knowing how to fix my issue and hacking around it...

RTFM strikes again (lol)

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.

raster-fade-duration is ignored after v4.2.0

6 participants