Automatically batch the work done by GeoJSON#setData()

[lucaswoj]

If is relatively common for users to animate geometries on the map using GeoJSONSource#setData

It can be hard to do so performantly and without causing the queue of data update operations to become seriously clogged.

We could make this use of GL JS easier via a newMap#requestSetDataFrame(sourceId: string, callback: Function) method (analogous to requestAnimationFrame).

    function requestSetDataFrame(map, sourceId, callback) {

      var countdown = 2;

      function dataCallback() {
          if (map.style.sourceCaches[sourceId].loaded()) {
              if (--countdown === 0) callback(performance.now());
          } else {
              map.once('data', dataCallback);
          }
      }

      dataCallback();

      requestAnimationFrame(function(now) {
          if (--countdown === 0) callback(now);
      });

   }

[lucaswoj]

cc @kronick

[kronick]

This sounds great and addresses the issue I ran into pretty quickly once I started trying out some relatively complicated animations.

Just thinking about the API for this, ideally I would be able to do:

function animate() {
    // update myData
    source.setData(myData, function() {
        // "Data is updated" callback
        requestAnimationFrame(animate);
    });
}

Would it be possible to integrate the requestDataUpdateFrame() code like this? Otherwise, given @lucaswoj's code above, I picture:

    function animate() {
         // update myData

         map.requestDataUpdateFrame("sourcename", function() {
             requestAnimationFrame(animate);
         });
    
         source.setData(myData);
    }

Which is awkward 1) because for correctness you should call requestDataUpdateFrame() before setData() (to avoid a race condition that I think is possible if you call it the other way around) and 2) I can't think of a use case where you would want to call requestDataUpdateFrame() without also calling setData()

[lucaswoj]

Would it be possible to integrate the requestDataUpdateFrame() code like this? Otherwise, given @lucaswoj's code above, I picture:

This would be possible syntactically if GeoJSONSource#setData accepted a callback. That is a separate discussion (one which would be valuable to have!)

Conceptually, this nesting would not result in the optimal animation loop. In the worst case, the time between calls to GeoJSONSource#setData would be frame time + data update time whereas we want the worst case to be max(frame time, data update time).

1. because for correctness you should call requestDataUpdateFrame() before setData() (to avoid a race condition that I think is possible if you call it the other way around)

We should debug this race condition. GeoJSONSource#setData is an asynchronous operation so it shouldn't matter whether you call it before or after calling Map#requestDataUpdateFrame.

2. I can't think of a use case where you would want to call requestDataUpdateFrame() without also calling setData()

Agreed 👍

[lucaswoj]

All you should need to do with this API is the following:

    function animate() {
        source.setData(myData);
        map.requestDataUpdateFrame("sourcename", animate);
    }

(note that we call requestAnimationFrame within requestDataUpdateFrame)

[lucaswoj]

In conjunction with this method it may be wise to add a warning when the map's event queue becomes backlogged, suggesting the use of requestDataUpdateFrame.

[ryanbaumann]

@lucaswoj

In conjunction with this method it may be wise to add a warning when the map's event queue becomes backlogged, suggesting the use of requestDataUpdateFrame

Agreed on the requestDataUpdateFrame syntax. For large managing a backlog of data mutation requests, a debounce function could be useful:

_.debounce(requestSetDataFrame(), delayTime, function reportError(e) { console.log(e) } )

@kronick

Is it possibly to abstract this function to work well with setData(), or setFilter(), or setPaintProperty()?

While performance is best with setData() for animations, vector tile filters or style mutations are often appropriate.

[kronick]

@ryanbaumann @lucaswoj

Is it possibly to abstract this function to work well with setData(), or setFilter(), or setPaintProperty()?

Good question. This would make sense to me as a callback to each of those functions. Otherwise we'll have requestDataUpdateFrame(), requestFilterUpdateFrame(), requestPaintPropertyUpdateFrame(), etc. Besides it being a change to longstanding functions in the API, is there an argument against setData(), setFilter(), and setPaintProperty() accepting optional completion callbacks?

[lucaswoj]

For large managing a backlog of data mutation requests, a debounce function could be useful.

I'm not sure I understand. requestSetDataFrame is a debounce function. What's motivating your use of an additional debounce function?

---

Is it possibly to abstract this function to work well with setData(), or setFilter(), or setPaintProperty()?

Good question. This would make sense to me as a callback to each of those functions. Otherwise we'll have requestDataUpdateFrame(), requestFilterUpdateFrame(), requestPaintPropertyUpdateFrame(), etc.

The proposed implementation of requestSetDataFrame works generically with source mutation operations. It would work with setData, setFilter, and setPaintProperty. (With the caveat that setPaintProperty is synchronous and doesn't require this debouncing unless used with a data-driven function).

Can you think of a name for this method that makes it more apparent it can be used with all setData, or setFilter, and setPaintProperty?

---

Besides it being a change to longstanding functions in the API, is there an argument against setData(), setFilter(), and setPaintProperty() accepting optional completion callbacks?

I'm not aware of any strong arguments against completion callbacks.

Completion callbacks are a different feature and deserve a separate discussion in a separate issue. They do not address the same need as requestSetDataFrame. Mutating a source within a completion callback would not come with some of the assurances that requestSetDataFrame does:

• the source's event queue doesn't become backlogged
• the source is not updated faster than the map is being drawn

[ansis]

I think this can be done without adding something to the external api:

From a user's perspective:

function update() {
    map.setData(data);
    requestAnimationFrame(update)
})
update();

implementation:

GeoJSONSource.prototype._queuedData = null;
GeoJSONSource.prototype._currentlySettingData = false;

GeoJSONSource.prototype.setData = function(data) {
    if (!this._currentlySettingData) {
        this._currentlySettingData = true;
        this.actualSetData(data, done.bind(this));
    } else {
        this._queuedData = data;
    }

    function done() {
        this._currentlySettingData = false;
        var data = this._queuedData;
        if (data) {
            delete this._queuedData;
            this.setData(data);
        }
    }
};  

This has the advantage of working automatically without any extra effort from the user. They just call setData like they normally do. The external interface doesn't need to change. Another small advantage is that the queued data will be slightly less stale with this approach, since it'll be the most recent instead of the first.

The downside is that the work the user does before calling setData isn't throttled, but I think this doesn't matter too much. It's usually cheap. And when it's more expensive than setData you wouldn't need to throttle.

requestAnimationFrame could be the browser's api, or we could add a map.requestAnimationFrame that would be guaranteed to get called right before the map gets rendered.

[kronick]

@ansis I really like this option-- especially how it automatically uses data from the most recent call. This behavior is what I would almost always want, and I think you're right that asking the user to throttle their own calls is not a major burden (which requestAnimationFrame() helps with here). It keeps setData() asynchronous (as I understand it needs to be for performance), but without the user having to think too much about what that means under the hood.

[lucaswoj]

Good points @ansis. I'm in favor of automatic throttling as you propose.

The downside is that the work the user does before calling setData isn't throttled, but I think this doesn't matter too much. It's usually cheap. And when it's more expensive than setData you wouldn't need to throttle.

Can you think of any cases where the data update wouldn't be cheap and access to this primitive would be necessary?

[ansis]

Can you think of any cases where the data update wouldn't be cheap and access to this primitive would be necessary?

I can't think of anything right now. And if we add a callback to setData the user could always implement it themselves.