Continue rendering tile despite errors #10011
Conversation
Since 9a9408e, we marked tiles as non-renderable if an error occured. This lead to situations where a tile was loaded + parsed successfully, then a revalidation attempt occured (e.g. because the resource was stale) which failed. In this case, we used to mark the tile as non-renderable although we could've used the perfectly parsed (stale) resource.
…perations completed Previously, when we started a worker operation that eventually throws an exception (e.g. due to the tile not being parseable), and then enqueue another worker operation while the first one is processing, we treated the worker as idle once the first operation's error callback fired, even though the second operation was still in progress. Due to our use of coalescing, I was unable to come up with a reliable test since we'd need to reproduce the behavior described above, which is timing dependent.
There was a problem hiding this comment.
The change makes sense, and the diff looks fine. My hesitation is that this part of the code seems to have a history of a fix for one issue introducing another issue.
- We first changed the logic for raster tiles to fall back to a parent tile when receiving a 404 (Don't treat empty raster tiles as valid #8164).
- This change didn't work for "still" mode, so we adjusted the parent loading logic again (Render parent raster tiles when tiles can't be loaded #8769).
- But that adjustment caused at least two new issues:
- Situations arose where the same area might have two loaded raster tiles, leading to overdrawing artifacts. We fixed that by clipping children out of parents (Clip raster tiles #9468).
- An error on an attempt to revalidate a tile triggers the same parent tile loading logic, which is undesired if the error is caused by the device's being offline (this issue).
How confident are you that this will be the final issue in this chain? Is there anything we can change about how we're approaching the problem that will increase your confidence?
One concrete thing that comes to mind is the edge case where a raster tileset update lowers the maximum zoom level in a particular area from zN to zN-1. Will devices that have zN tiles cached notice that and at some point start showing zN-1 tiles as "parent tiles" at zN? Or will this change cause them to show permanently stale zN tiles?
I'm not 100% confident that's going to be the last one, but I've spent a significant amount of time testing this fix, and thinking through how tiles are supposed to be loaded. Not having an overall view of how tiles are loaded, and what constitutes completeness was the reason I haven't been able to provide a complete fix initially. The underlying problem is that it's hard to test this, since our testing code is based on still image rendering, but tile completeness works differently in this rendering mode compared to the continuous rendering mode. Ultimately, I think that we need to write tests that use continuous rendering mode as well. I attempted to do this as part of this PR, but backtracked the changes since I couldn't make the test reliable enough since our
I believe this issue is slightly different. It fixes situations where the same tile was moved from a renderable to a non-renderable state (which should never happen). Changing the maximum zoom level affects which tiles are loaded to begin with. |
Since 9a9408e, we marked tiles as non-renderable if an error occured. This lead to situations where a tile was loaded + parsed successfully, then a revalidation attempt occured (e.g. because the resource was stale) which failed. In this case, we used to mark the tile as non-renderable although we could've used the perfectly parsed (stale) resource.
Fixes #9563 and #9705
This needs porting to our release branch.