-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Automatically batch style updates #2355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4bafb00
to
1f2b317
Compare
* @private | ||
*/ | ||
batch: function(work) { | ||
styleBatch(this, work); | ||
update: function(classes, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The terminology here is starting to feel overwhelming: mutate
, update
, change
, cascade
, recalculate
...
How do you feel about picking a consistent verb (i.e. update
, change
) and differentiating these properties and methods with direct objects?
cascade
->updateTransitions
recalculate
->updateZoom
mutations
->updates
broadcastLayers
->updateWorkerLayers
reloadSource
->updateSource
change
event ->update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great point! I like all the suggested renames except the change
event, which we use throughout the code for other classes, and it's generally more common to have events like this called change
rather than update
in JS APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also leave reloadSource
-> updateSource
rename for another PR because it's tricky, potentially involving a lot of changes that are out of scope for this PR (e.g. it calls source.reload
, which in turn calls pyramid.reload
, but pyramid
also has update
method which is different, so you also have to rethink pyramid API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed mutations
, broadcastLayers
and did some other minor cleanups/renames, but leaving the rest for future PRs (especially cascade
and recalculate
) so that it doesn't interfere with current DDS PRs, otherwise we would have a lot of conflicts to resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I openend #2359
return this; | ||
}, | ||
|
||
_resetMutations: function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_resetMutations
--> _resetUpdates
This is ready to 🚢 once you address the two minor comments above. Great work! I love a negative line PR. |
Closes #2343. Removes
map.batch
and implements automatic batching of style changes, applying them once per frame. The style keeps track of all mutations done throughStyle
setLayoutProperty
,addLayer
, etc., and then all mutations are applied with thestyle.update
method which is called inmap._render
and happens once per frame.Remaining tasks:
fix API accessor methods to immediately reflect style changes before they're batch-updated, as perthey do already removemap.batch
and automatically batch style changes #2343 (comment)sprites-pattern
render tests failed