Fix terrain cache not updating on `setFeatureState` #11209

SnailBones · 2021-11-03T03:30:50Z

Closes https://github.com/mapbox/gl-js-team/issues/312

Currently, feature-state doesn't update on terrain until the map is zoomed a certain amount (possibly an integer zoom level).Demonstration here.

This is because the terrain render cache doesn't update. This P.R. fixes that.

This issue is a regression introduced in 2.4, probably from #10701. #10302 may be a related issue, but predates this regression.

Launch Checklist

briefly describe the changes in this PR
include before/after visuals or gifs if this PR includes visual changes
write tests for all new functionality
manually test the debug page
tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fixes feature-state not updating when terrain is enabled.</changelog>

karimnaaji

Nice catch, I think this fix is valid, but I left a comment to investigate whether we could be more granular in the way we invalidate the cache.

src/terrain/terrain.js

asheemmamoowala · 2021-11-03T18:49:33Z

src/style/style.js

@@ -1120,6 +1120,7 @@ class Style extends Evented {
        for (const sourceCache of sourceCaches) {
            sourceCache.setFeatureState(sourceLayer, target.id, state);
        }
+        this.fire(new Event('setfeaturestate'));


The fix should also apply when removeFeatureState is called to clear a value. When you do that, maybe a different name is appropriate

Good catch @asheemmamoowala!

@karimnaaji Thoughts on how to get this behavior working now that this is no longer using events? Should removeFeatureState call a new function tile.removeFeatureState to trigger this update?

I haven't stepped through the code in debug, but from reading it removeFeatureState should eventually result in a bucket update. I'd verify that with a step through to confirm.

If we tie a render cache invalidation to a buffer update, we ensure that when something actually changes visually (like you do now in this PR) we invalidate the draping cache entry. bucket.update should be the end result of any mutation of feature state changes, which is the end result of going through source_state:coalesceChanges.

But to confirm that some unit test using these cases would be necessary (to check that we invalidate the cache correctly).

Debugging shows that source_state:coalesceChanges (and thus the code in this fix) is indeed being called after removeFeatureState. I'm not totally sure how that works, but I can confirm that it works as expected in browser testing.

src/style/style.js

src/terrain/terrain.js

karimnaaji

@SnailBones LGTM!

Is it possible to add some unit tests covering this to prevent further regression?

src/source/tile.js

arindam1993 · 2021-11-04T17:15:43Z

src/source/tile.js

@@ -527,6 +531,12 @@ class Tile {
            if (!sourceLayer || !sourceLayerStates || Object.keys(sourceLayerStates).length === 0) continue;

            bucket.update(sourceLayerStates, sourceLayer, availableImages, this.imageAtlas && this.imageAtlas.patternPositions || {});
+            if (bucket instanceof LineBucket || bucket instanceof FillBucket || bucket instanceof FillExtrusionBucket || bucket instanceof CircleBucket) {


Could you move this as a function on a bucket, instead of all of these instanceof checks?

also is FillExtrusion right here? because thats not draped? same with circle?

The instance checks are necessary for Flow. Really we're just checking that it's not a SymbolBucket, since SymbolBucket doesn't have the programConfigurations.

Good point about draping, should this just be LineBucket and FillBucket?

well symbols are draped in certain situations 😅 (map aligned)?

😓 Hmmm... If this bug exists for symbols then this fix won't work as is. Though we could bypass the check for bucket.programConfigurations.needsUpload and clear the cache for all tiles.

Do we have an examples of feature-state on a symbol layer?

Checked that symbols are not yet draped in any situation on gl-js, pending #10365. The draped layer list is https://github.com/mapbox/mapbox-gl-js/blob/main/src/style/style.js#L116.

So this check should be good now?

Yes!
Tested that symbol layers don't have this bug and that the fix works with line layers as well as fill layers.

* Invalidating terrain render cache on style.setFeatureState * Changed approach to directly clear render cache from Tile * Only on LineBucket and FillBucket Tests will be in a new PR

Invalidating terrain render cache on style.setFeatureState

432578b

SnailBones added the bug 🐞 label Nov 3, 2021

Trimming whitespace

bcf8ceb

asheemmamoowala requested review from astojilj and karimnaaji November 3, 2021 16:01

karimnaaji reviewed Nov 3, 2021

View reviewed changes

src/terrain/terrain.js Outdated Show resolved Hide resolved

asheemmamoowala reviewed Nov 3, 2021

View reviewed changes

arindam1993 suggested changes Nov 3, 2021

View reviewed changes

src/style/style.js Outdated Show resolved Hide resolved

src/terrain/terrain.js Outdated Show resolved Hide resolved

SnailBones added 2 commits November 3, 2021 17:06

Changed approach to directly clear render cache from Tile

b4cdedf

Fix flow errors

a47ab06

karimnaaji reviewed Nov 4, 2021

View reviewed changes

src/source/tile.js Outdated Show resolved Hide resolved

Removing double check for terrain

1e1c077

SnailBones changed the title ~~Fix terrain cache not updating on style.setFeatureState~~ Fix terrain cache not updating on setFeatureState Nov 4, 2021

arindam1993 reviewed Nov 4, 2021

View reviewed changes

Only on LineBucket and FillBucket

295cb5c

SnailBones requested a review from arindam1993 November 4, 2021 17:21

ansis added the release blocker ⛔ label Nov 4, 2021

arindam1993 approved these changes Nov 4, 2021

View reviewed changes

SnailBones merged commit 2843074 into main Nov 4, 2021

SnailBones deleted the aidan/feature-state-terrain branch November 4, 2021 18:26

SnailBones restored the aidan/feature-state-terrain branch November 4, 2021 23:17

SnailBones mentioned this pull request Nov 8, 2021

Adding terrain to feature state debug #11240

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix terrain cache not updating on `setFeatureState` #11209

Fix terrain cache not updating on `setFeatureState` #11209

SnailBones commented Nov 3, 2021 •

edited

karimnaaji left a comment

asheemmamoowala Nov 3, 2021

SnailBones Nov 3, 2021

SnailBones Nov 3, 2021

karimnaaji Nov 4, 2021 •

edited

SnailBones Nov 4, 2021 •

edited

karimnaaji left a comment

arindam1993 Nov 4, 2021

arindam1993 Nov 4, 2021 •

edited

SnailBones Nov 4, 2021

arindam1993 Nov 4, 2021

SnailBones Nov 4, 2021

karimnaaji Nov 4, 2021

SnailBones Nov 4, 2021

Fix terrain cache not updating on setFeatureState #11209

Fix terrain cache not updating on setFeatureState #11209

Conversation

SnailBones commented Nov 3, 2021 • edited

Launch Checklist

karimnaaji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karimnaaji Nov 4, 2021 • edited

Choose a reason for hiding this comment

SnailBones Nov 4, 2021 • edited

Choose a reason for hiding this comment

karimnaaji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arindam1993 Nov 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix terrain cache not updating on `setFeatureState` #11209

Fix terrain cache not updating on `setFeatureState` #11209

SnailBones commented Nov 3, 2021 •

edited

karimnaaji Nov 4, 2021 •

edited

SnailBones Nov 4, 2021 •

edited

arindam1993 Nov 4, 2021 •

edited