fix: missing children in retained tiles

[jessekrubin]

Fix for #5616

• 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]

Can you check which version was the first version that has this issue?
I'll be surprised if this isn't a relatively new bug. There was a PR that changed the logic of retained tiles in recent versions...

[jessekrubin]

Had to have been 5 as I only started getting these errors with v5+.

[HarelM]

@wagewarbler can you check if this issue might have been introduced by your change somehow?
I'm wondering if there's a better way to solve this besides adding a if statement check, since if this wasn't happening before it means that the content of the cache had changed and it is filled differently than before...

[HarelM]

Seems like there's no response from @wagewarbler.
@jessekrubin any change you can try and see if there's a better approach to solving this besides adding this if?

[wagewarbler]

@HarelM I've been catching up on the issue. It's not immediately clear to me how my PR would have affected this but I understand your point about the nature of the underlying cache potentially being an issue. At a high level, do you have a sense of how this tilt/angle differently utilizes the cache vs. the standard perpendicular view? Or does this require some deeper digging?

[HarelM]

I don't have a lot of insights, also I didn't fully investigate to find the possibly offending PR, so it's only a hunch at this point... Deeper investigation is needed...

[wagewarbler]

Ok, I can do some looking in parallel. Entirely your call here, but there is the option to spin off a separate issue to re-evaluate the cache model and flow and link related PRs (this one and my previous one for example). Then merge this one as a tightly scoped PR that prevents array element access that exceed the array's bounds.

[jessekrubin]

Hi yall. I say merge it. Sorry I have not been more involved -- I broke my leg a week ago :(

[HarelM]

I would prefer to have a better understanding of this issue before merging this so that we don't just use "defensive" coding approach for no reason.
If we find out this is the only solution, then sure, we can merge it.
Let's wait for @wagewarbler analysis.

[jessekrubin]

I would prefer to have a better understanding of this issue before merging this so that we don't just use "defensive" coding approach for no reason.

I feel ya but it is currently causing crashes. My guess is it has to do with at high pitches the child tiles get reduced/collapsed to single parent tiles.

Is it possible this collapsing happens multiple times? Or for tiles outside of the range of webmercator bounds (which might make sense at extreme tilts)?

[HarelM]

I don't have a clear understand at the moment.
If you need a quick fix, you can use patch-package with this fix while we decide how to proceed.

[jessekrubin]

I think what happens is when at a high pitch, certain tiles are "collapsed" to a single over zoomed tile if they are super far out.

The tiles that get collapsed are always far off in the distance.

Either way checking that the array is length 1 or 4 seems like a simple sanity check and I would be very happy if this were merged.

For now I have just limited my usage to not allow a pitch greater than 60. Using patch package seems like a huge pita.

@HarelM What do you need to merge this?

[HarelM]

I prefer to have a better root cause analysis of the original issue or what might cause this, sorry for delaying this, but it's very easy to add this type of code, but extremely hard to remove it, so I'm trying to get a more concrete answer here.

[MrBlenny]

I'm seeing the same issue with the ancient version - 1.14.0

Uncaught TypeError: Cannot read properties of undefined (reading 'key')
    at P._updateRetainedTiles (App-B4Dgqog9.js:831:478681)
    at P.update (App-B4Dgqog9.js:831:477132)

          var rn = Rt.children(this._source.maxzoom);
                                if (X[rn[0].key] && X[rn[1].key] && X[rn[2].key] && X[rn[3].key])
                                    continue

It is intermittent, it seems to happen about 1 in 5 times. I am seeing it when loading a raster layer on top of a vector map. Using react-map-gl, the offending layer is something like this.

      <Source
        id="radar"
        type="image"
        url={imageUrlForSomeBlob}
        coordinates={coordinates}
      >
        <Layer
          beforeId="some-other-layer"
          id="radar"
          type="raster"
          source="radar"
          paint={{
            'raster-fade-duration': 0,
            'raster-saturation': 1.0,
            'raster-opacity': 0.6,
            'raster-brightness-min': 1.0,
          }}
        />
      </Source>

My guess was it was something to do with the raster layer, blob or the beforeId. There is also an occasion where the coordinates for the raster may be [[0, 0],[0, 0],[0, 0],[0, 0]]

Hopefully another useful datapoint for your investigation.

[HarelM]

@MrBlenny can you reproduce this issue without react-map-gl? If so, it means that this is not a new issue and the fix here is good enough.

[jessekrubin]

@MrBlenny can you reproduce this issue without react-map-gl? If so, it means that this is not a new issue and the fix here is good enough.

I was able to point my repro at an older version and still see the issue.

[HarelM]

Is it possible to add a unit test to cover this issue?
After adding the test I will merge this.
THANKS!