Handle empty 20x raster tile response as empty tile.

[cns-solutions-admin]

getImage (ajax.ts) now returns a null image, if there is no content (ArrayBuffer with length 0).
Raster source loadTile will store such an image as a tile with the new status 'empty' (to avoid loading it again, if there are cache headers).

See also #1579.

[github-actions]

Bundle size report:

Size Change: +60 B
Total Size Before: 204 kB
Total Size After: 204 kB

Output file	Before	After	Change
maplibre-gl.js	195 kB	195 kB	+60 B
maplibre-gl.css	9.03 kB	9.03 kB	0 B

ℹ️ View Details

No major changes

[HarelM]

Can you also add tests to raster source too?
Also, I can't get a grasp of what exactly this might cause in terms of risk.
Basically, I need you to clam me down and explain why this is not "risky" :-)

[HarelM]

Also please add a changelog item as part of this PR

[cns-solutions-admin]

And I hoped you could review it and tell me if it was "risky" (and why) ;-)

[HarelM]

:-o :-)

[xabbu42]

As already mentioned in #161 (comment) there was some discussion to use 204 just to silence the browser errors, but still fall back to lower zoom-level and not interpret the result as an empty tile. I personally think that compatibility with other map tools (especially current mapbox-gl-js) is the most important aspect here. That is depending on what other tools do, we should merge either #161 or this pull request to handle 204 (at least from my understanding of the two pull requests, I did not dive into the code).

Is my understanding correct that currently maplibre-gl-js falls back on scaling lower zoom levels for 204 tiles, but with this pull request does not? (I may have some time to test this later but perhaps you already know the answer).

[cns-solutions-admin]

Without the change in raster_tile_source it just suppresses the error message: the raster_tile_source ignores null results.

With the change in raster_tile_source the only change would be, that these empty tiles would not be requested again each time, if they had a cache header. But this should be checked by somebody who knows the source_cache better.

Generally I think it is not compatibility to client libraries but to tile servers, that we should strive for.
And I think that if a tile server returns 204

• there is no data and there should not be an error in the log
• if it sends cache/expire headers with 204, we should not make more requests during that period

Whether maplibre should try to use tiles from another tile or not, is another question. Currently maplibre seems handle 404 differently than other results.

[systemed]

Whenever I've written a tileserver I've had to patch in some code to send empty tiles, because some clients don't like 204s. So this would be a great improvement.

[HarelM]

@systemed can you review this as well then?

[systemed]

I don't have a Maplibre build setup (yet ;) ) so can't test the code, but from briefly grokking the patch and the test, it looks good to me.

[xabbu42]

I think the following accepted pull request mapbox/mapbox-gl-native#8164 to mapbox-gl-native is relevant to the discussion. According to that pull request it is explicit designed behavior for mapbox-gl-native (and so also for maplibre-gl-native as it is currently) to fall back on lower zoom levels for empty raster tiles. Either maplibre-gl-js should follow that design or we will have to revert the changes in maplibre-gl-native as the two repos should be as close as possible. I will try to add some integration tests for the current behavior so the difference is clearly exposed by the tests.

There is also a similar open issue for mapbox-gl-js with relevant discussion: mapbox/mapbox-gl-js#9304

[HarelM]

In terms of feature parity with native we open an issue on their repo and link the relevant PR here for reference.
I'm honesty not sure what should be the right behavior. I tend to say that 204 and 404 are different and the library should be aware of this difference i.e. accept the PR.
@xabbu42 tests are always super welcome! 😁

[HarelM]

@cns-solutions-admin can you rebase the branch with latest changes in main? @xabbu42 added tests so we can see if there's a change in behavior when introducing this PR.

[xabbu42]

I expect this branch to pass the tests as they are by the way, whereas #160 and maplibre–gl-native would fail rasterfallback204 as it is, as they would actually fallback on zoom 0. According mapbox/mapbox-gl-native#8164 the latter is intentional.

I personally follow the reasoning given in that pull request and think we should in the end adapt maplibre-gl-js to maplibre-gl-native (after checking that it actually still has a different behavior). Of course we can also add an issue in maplibre-gl-native, if the current behavior of maplibre-gl-js is considered correct.

[cns-solutions-admin]

The test passes, the behavior is like before with the exception that no error message is logged.
The empty tile is stored in the tile cache with status empty and the expiration time from the cache header.

[HarelM]

Instead of adding this case to the decorated callback I would consider simply calling the callback with null.
The decorated callback is used in the below function to facilitate less code, but this is not the case in this flow.
i.e. adding an else to the callback and then writing an if in the below code is not needed I believe.

[HarelM]

@cns-solutions-admin can you please take care of this so I can merge this PR and carve out a release?

[HarelM]

I've added some code review comments.
When fixed I think this can be merged.

[HarelM]

I'm closing this PR now.
I hope someone would pick it up so MapLibre can better support 204/empty 200.