Sync continuously-updating interaction handlers with render loop #6005

anandthakker · 2018-01-17T03:17:01Z

Quoting from that issue:

the DragPan handler updates the camera directly and fires a move event. Updates to the DOM that happen in move listeners happen synchronously, but the map renderer doesn't update until the next animation frame.

This PR aims to solve this issue by having the DragPan handler (and the other continuously-updating handlers) cede control to the camera, which in turn is driven by the render loop.

I think it's also a step towards further refactoring of camera of Camera that eliminates some of the complicated internal state, simplifies/clarifies move-related event firing, etc.

Launch Checklist

briefly describe the changes in this PR
~~write tests for all new functionality~~ (blocked by Test interaction handlers, UI controls, and browser utilities #1550)
post benchmark scores
manually test the debug page

anandthakker · 2018-01-17T03:18:38Z

@mourner and others on @mapbox/gl-core, could you give this a quick look to validate the approach, before I propagate it to the other handlers (DragRotate, etc.)?

anandthakker · 2018-01-17T13:45:02Z

src/ui/camera.js

+     *
+     * @private
+     */
+    _startAnimation(onFrame: (Transform) => void,


Marked with _ because, since Camera gets mixed directly into Map, I didn't want to make it "public". Eventually, this should be a public method on an internal Camera class that's separate from Map. Similarly, a refactor like that would allow for a better naming of _updateCamera

anandthakker · 2018-01-17T13:50:57Z

src/ui/camera.js

+     */
+    _updateCamera() {
+        if (this._onFrame) {
+            this._onFrame(this.transform);


Later, we could also consider a design where, rather than mutating the Transform directly, onFrame just returns CameraOptions. Consolidating the responsibility for actual Transform manipulation into one place would slightly simplify the code in easeTo, flyTo, handlers, etc.

mollymerp

This approach seems solid to me (albeit I have little to no experience working with the camera animations / render loop codebase) – makes sense to me that the camera would be the single source of truth for animations and animation-related events. It would be nice if any new state tracking could be contained within the camera and/or eliminated, but that may not be possible right now.

mollymerp · 2018-01-17T21:55:31Z

src/ui/handler/drag_pan.js

-        }, { originalEvent: e });
+    _onUp(e: MouseEvent | TouchEvent | FocusEvent) {
+        if (!this.isActive()) return;
+        this._map.stop();


I think you need to delete this._previousPos here to avoid the map stuttering back to the end of the last dragpan on a subsequent dragpan.

mourner

This looks like a good approach, with a few exceptions I'm worried about:

startAnimation term feels confusing in context of updating transform once in a single frame (since there no animation happening, and it's not clear when the stop handler happens too in this context). Maybe we could rename/refactor this a bit with clearer terms? E.g. naming this as _setCameraUpdateHandler, and finding a different place for the stop logic?
You're introducing a lot of closures that get created on every frame. Eliminating them would ensure the code is as fast as it was, and it would also improve code clarity.

BTW there's something similar going on in Leaflet — it schedules camera updates throw requestAnimFrame rather than syncrhonously updating DOM.

anandthakker · 2018-01-18T21:58:54Z

@mourner removed closures and attempted to clarify the code a bit more. I still lean towards calling it _startAnimation, even if it's slightly stretching the idea of 'animation'... but I don't feel strongly about it / happy to change it.

This is ready for a real review now @mollymerp @mourner

mourner

Looks good to me, pending a green build. Also, would be nice to move down/up/move methods to their former positions so that there's less noise in the diff.

* Since we `_update` on move, and that already triggers a `_rerender()`, we don't also need to set a separate 'move' listener that calls `_rerender()` * Since `_updateEasing()` triggers a `move` event, which in turn schedules a subsequent frame, we don't _also_ need to check `isEasing()` at the end of `render`.

This refactors `Camera#ease()`, which assumed an animation with a fixed end time, in terms of `Camera#_startAnimation()` (and `Camera#stop()`), which doesn't. The advantage of this slight generalization is that `_startAnimation()` can then also be used by continuously-updating interaction handlers (`DragPan`, etc.)

Instead of updating the transform directly within the mousemove handler, we cede control to the render loop by doing our transform updates in the callback we pass to `Camera#_startAnimation`. This way, we synchronously update the transform, render the map, and fire the `move` event (and thus trigger any listeners that might perform DOM updates).

Closes #6063 Caused by #6005 because, after that change, the drag handlers relied on the `move` event to trigger a map rerender to pick up the next batch of mouse movements. This worked fine while dragging was in progress, but after the user paused and the render frame triggered by the last `move` event was complete, there was nothing to re-initiate the render loop once the mouse moved again.

…box#6073) Closes mapbox#6063 Caused by mapbox#6005 because, after that change, the drag handlers relied on the `move` event to trigger a map rerender to pick up the next batch of mouse movements. This worked fine while dragging was in progress, but after the user paused and the render frame triggered by the last `move` event was complete, there was nothing to re-initiate the render loop once the mouse moved again.

jfirebaugh · 2018-02-13T23:50:47Z

src/ui/map.js

@@ -1528,7 +1525,7 @@ class Map extends Camera {
        // Even though `_styleDirty` and `_sourcesDirty` are reset in this
        // method, synchronous events fired during Style#update or
        // Style#_updateSources could have caused them to be set again.
-        if (this._sourcesDirty || this._repaint || this._styleDirty || this._placementDirty || this.isEasing()) {
+        if (this._sourcesDirty || this._repaint || this._styleDirty || this._placementDirty) {


If we're planning to fully convert the handlers to use render-loop-driven camera, then we should:

Have handlers call map._update explicitly when they need to trigger a render, as in Ensure drag handlers request render frames on move #6068

Remove the move/zoom => this._update bindings

Restore this this.isEasing() trigger

anandthakker requested a review from mourner January 17, 2018 03:18

anandthakker commented Jan 17, 2018

View reviewed changes

mollymerp reviewed Jan 18, 2018

View reviewed changes

mourner reviewed Jan 18, 2018

View reviewed changes

anandthakker force-pushed the event-render-sync branch from b1be8aa to 8d60c4c Compare January 18, 2018 21:39

mourner approved these changes Jan 18, 2018

View reviewed changes

anandthakker force-pushed the event-render-sync branch 2 times, most recently from 9c46380 to e4bb33b Compare January 19, 2018 03:06

Anand Thakker added 2 commits January 18, 2018 22:20

anandthakker force-pushed the event-render-sync branch from e4bb33b to 0f4441b Compare January 19, 2018 03:20

anandthakker merged commit 0f4441b into master Jan 19, 2018

This was referenced Jan 19, 2018

SVG overlay using on("move") and project() out of sync in IE/Edge/Safari #5390

Closed

Provide access mouse/touch events in continuously-updating interaction handlers #6047

Open

anandthakker mentioned this pull request Jan 26, 2018

Ensure drag handlers request render frames on move #6068

Merged

3 tasks

anandthakker mentioned this pull request Jan 28, 2018

Backport ensure drag handlers request render frames on move (#6068) #6073

Merged

renovate bot mentioned this pull request Feb 2, 2018

Update dependency mapbox-gl to ^0.44.0 - autoclosed oSoc17/rideaway-frontend#72

Closed

jfirebaugh deleted the event-render-sync branch February 13, 2018 21:19

jfirebaugh reviewed Feb 13, 2018

View reviewed changes

jfirebaugh mentioned this pull request Feb 22, 2018

Fix output of Map#project during drag pan inertia #2901

Closed

anandthakker mentioned this pull request Feb 26, 2018

Refactor handlers to use a requestRenderFrame callback rather than Camera#_startAnimation #6233

Closed

anandthakker mentioned this pull request May 18, 2018

Expose event for camera change #3746

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync continuously-updating interaction handlers with render loop #6005

Sync continuously-updating interaction handlers with render loop #6005

anandthakker commented Jan 17, 2018 •

edited

Loading

anandthakker commented Jan 17, 2018

anandthakker Jan 17, 2018

anandthakker Jan 17, 2018

mollymerp left a comment •

edited

Loading

mollymerp Jan 17, 2018

mourner left a comment

anandthakker commented Jan 18, 2018

mourner left a comment

jfirebaugh Feb 13, 2018

Sync continuously-updating interaction handlers with render loop #6005

Sync continuously-updating interaction handlers with render loop #6005

Conversation

anandthakker commented Jan 17, 2018 • edited Loading

Launch Checklist

anandthakker commented Jan 17, 2018

anandthakker Jan 17, 2018

Choose a reason for hiding this comment

anandthakker Jan 17, 2018

Choose a reason for hiding this comment

mollymerp left a comment • edited Loading

Choose a reason for hiding this comment

mollymerp Jan 17, 2018

Choose a reason for hiding this comment

mourner left a comment

Choose a reason for hiding this comment

anandthakker commented Jan 18, 2018

mourner left a comment

Choose a reason for hiding this comment

jfirebaugh Feb 13, 2018

Choose a reason for hiding this comment

anandthakker commented Jan 17, 2018 •

edited

Loading

mollymerp left a comment •

edited

Loading