Resolving the Studio blocker

[sgolbabaei]

The problem which is blocking studio to upgrade the gl-js version v1.11.0 is as follow:

The map.loaded() is returning false as result of onClick() which is causing the studio not showing its popup menu.

mouse events don’t immediately trigger map changes. but they trigger render frames which then combine all the changes since the last frame and apply them once mouseup triggers, because the pan handler goes from being “active” to “inactive” which mark the sources as dirty. This change will avoid updating the map and directly will call the repaint to avoid setting the source as dirty.

[arindam1993]

This has been around for 2 years, so it seems to me its the unlikely this broke it.

Can we change it so that in handler_manager.js we trigger triggerRepaint for click's instead of always using _requestRenderFrame?

[sgolbabaei]

this specifically didn't break the Studio, but the combination of this code and also handler_manager did.. The sourceDirty was set during the mouseup (which happens just before the click).. So the problem is not actually happening in case of click..

[karimnaaji]

Yes even though this code was there for a while there may very well have been a side effect from the gesture improvements as it was a fairly large change. By directly calling triggerRepaint instead of _update, we're effectively skipping those two statements every time we request a frame render:

this._styleDirty = this._styleDirty || updateStyle;
this._sourcesDirty = true;

This could have a similar side effect as it could trigger other types of events and it's a very high-level operation change. So the question would be whether these code paths strictly requires those two operations to be done, and whether there are any event triggered whenever any of those two booleans change.

[sgolbabaei]

Another approach we can take is to actually figure out if a render frame and avoid calling for render frame. I think figuring this out might be tricky and might break some other flow.. It probably is safer to avoid marking the source as dirty. I tested ease to functionality which could be effected but didn't see any problem there.. Also ran the tests:
yarn test &&
yarn run test-expressions &&
yarn run test-query &&
yarn run test-render

@ansis

[karimnaaji]

The one that was the most concerning would have been ease to (The other code path being in handler manager), thanks for testing that! I think we should be safe here.

[arindam1993]

I think basic easeTo will continue to work, but there are some additional use cases like

• binding a map.once('idle', at right after triggering easeTo to detect when the animation ends, and map is ready with placed labels.
• easestart, easing and easeend events
• map.loaded() state check inside these various callbacks.

[karimnaaji]

Ok so it really seems like we should be going for adding triggerRepaint() + queue render task from handler_manager (basically inlining the code path that was changed here) to be free from introducing any other issue.

[karimnaaji]

After this change, one difference with main would be that we do not flag sources as dirty while running an ease to event, which I don't believe is necessary. Though, if we want to go the safe route and prevent any other side effect from this difference, the safest option would be what @arindam1993 suggested in handler_manager.js:

    _triggerRenderFrame() {
        if (this._frameId === undefined) {
            this.map.triggerRepaint();
            this.frameId = this.map._renderTaskQueue.add(timeStamp => {
                delete this._frameId;
                this.handleEvent(new RenderFrameEvent('renderFrame', {timeStamp}));
                this._applyChanges();
            });
        }
    }

[sgolbabaei]

@karimnaaji

Adding triggerRepaint to the _triggerRenderFrame() function would not skip seting sourceDirty to true for only this code path since _requestRenderFrame would set the sourceDirty as true which will cause the same issue.. I have manually tested that and it still produce the same problem..

Sorry the code I was testing was not set properly. I'll update the code 👍

[ansis]

This looks good to me after the fix! It's clearer that this iteration won't break other parts accidentally. HandlerManager also calls _update directly so that should be fine as well.