-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
update one more frame after canvas source paused #8130
Conversation
Calling `canvasSource.pause()` lets us know we can stop pulling in updates from it. Since it is possible that changes were made to the canvas source since the last render we should pull in one more update before stopping. This also lets you call play and pause immediately to render just one frame: ctx.fillRect(10, 10, 50, 50); canvasSource.play(); canvasSource.pause();
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 change looks great, but I'm confused about the test: there's a new updateFakeCanvas
operation but no corresponding style.json
that would invoke it, did you accidentally forget to commit it?
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.
As a developer I would expect that calling CanvasSource#pause
allows me to make updates to the underlying canvas without updating the rendered map. With this change, is it possible for unwanted updates to be rendered in the case when:
- call
pause()
- update the
canvas
element map
renders next frame
const canvasSource = map.getSource(operation[1]); | ||
canvasSource.play(); | ||
// update before pause should be rendered | ||
updateFakeCanvas(window.document, options.addFakeCanvas.id, operation[2]); |
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.
This is putting the logic to be tested in the suite implementation instead of in a specific test. It is not a good precedent, but there is probably not a better way to do this with the current render test framework. Maybe an additional note here tying it back to a specific ticket or render-test would make it easier to remember why it is done this way.
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.
Yeah, I agree that expanding the suite implementation further is not great but I'm not seeing a better way. I'm hesitant to add a note pointing to a specific test because notes tend to get outdated. I think searching for "updateFakeCanvas" within tests should be good enough.
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 would try making the render test / image smaller if possible, but otherwise 👍
* update one more frame after canvas source paused Calling `canvasSource.pause()` lets us know we can stop pulling in updates from it. Since it is possible that changes were made to the canvas source since the last render we should pull in one more update before stopping. This also lets you call play and pause immediately to render just one frame: ctx.fillRect(10, 10, 50, 50); canvasSource.play(); canvasSource.pause(); * add test * don't render changes made after canvasSource.pause() * optimize * optimize
* update one more frame after canvas source paused Calling `canvasSource.pause()` lets us know we can stop pulling in updates from it. Since it is possible that changes were made to the canvas source since the last render we should pull in one more update before stopping. This also lets you call play and pause immediately to render just one frame: ctx.fillRect(10, 10, 50, 50); canvasSource.play(); canvasSource.pause(); * add test * don't render changes made after canvasSource.pause() * optimize * optimize
* release-liquid: (84 commits) v0.54.0 (take two) (#8184) Fix disappearing controls in Safari 12+ (#8193) (#8194) [docs] token refactor in docs-page-shell (#8174) (#8181) v0.54.0-beta.2 (#8166) Bugfix - removeFeatureState fails with target.id === 0 (#8150) (#8164) update one more frame after canvas source paused (#8130) (#8163) move docs dependencies to dev (#8121) (#8129) v0.54.0 (#8115) CP removeFeatureState fix (#8090) v0.54.0-beta.1 (#8084) Allow setStyle diff by default with localIdeographFontFamily on (#8081) Upgrade various (mostly dev) deps, downgrade GL (#8078) Fix default marker positioning (#8074) Use @mapbox/gazetteer for benchmark coordinates (#8063) bump react-dom to v16.3.3 to fix minor vulnerability warning Fix and refactor the benchmark suite (#8066) Add max width for popups (#7906) Extrusions: Do not try to triangulate non-polygon type features (#7685) docs fixes after the merge limit flow max_workers and upgrade to v0.95.1 (#8061) ...
* Set default thumbnail for examples page for development (#8025) * Set default thumbnail * Add a little guidance about creating example images * creates placeholder.png * v0.54.0-beta.1 (#8084) * CP removeFeatureState fix (#8090) * Empty out features whose states have been removed instead of removing the feature object. This allows updating the buffers for features whose state has been removed, but also requires keep track of previously removed features. * Update Changelog * v0.54.0 (#8115) * Example for `fill-pattern` (#8022) * move docs dependencies to dev (#8121) (#8129) * Mapbox-gl-geocoder v4 examples (#8053) * add geocoder custom render example * language localize example * Update marker demo to use built-in marker function * remove proximity bias for gl-geocoder example because this is now default behavior * update point from geocode demo to set custom marker color * update examples to use new mapbox-gl-geocoder API options * lint new examples * Update examples to gl-geocoder 4.0.0 api * Options should be added to the geocoder, not the map * lint examples * remove templating syntax * lint fixes * @katydecorah 's code review fixes * Studio does support rtl plugin (#8135) * Update comment for setRTLTextPlugin (#8143) * update comment for setRTLTextPlugin * clarify wording in example * update one more frame after canvas source paused (#8130) (#8163) * update one more frame after canvas source paused Calling `canvasSource.pause()` lets us know we can stop pulling in updates from it. Since it is possible that changes were made to the canvas source since the last render we should pull in one more update before stopping. This also lets you call play and pause immediately to render just one frame: ctx.fillRect(10, 10, 50, 50); canvasSource.play(); canvasSource.pause(); * add test * don't render changes made after canvasSource.pause() * optimize * optimize * Bugfix - removeFeatureState fails with target.id === 0 (#8150) (#8164) * Fix an edge case where 0 target.id would fail to clear the feature state * Check for number & string explicitly * Add tests for 0 feature ID * Fix logical check * v0.54.0-beta.2 (#8166) * [docs] meta updates for search (#8142) * update docs-page-shell, update page-shell; update page meta * Update react-page-shell.js * remove contentType from examples page * Update react-page-shell.js * Update react-page-shell.js * [docs] token refactor in docs-page-shell (#8174) * Update page-shell-script.js * do not force production page shell * remove hardcoded token from example, add unit test to look for access token in example html files * [docs] token refactor in docs-page-shell (#8174) (#8181) * Update page-shell-script.js * do not force production page shell * remove hardcoded token from example, add unit test to look for access token in example html files * Fix disappearing controls in Safari 12+ (#8193) (#8194) * workaround for a Safari 12 bug with disappearing controls * fix css lint * v0.54.0 (take two) (#8184) * v0.54.0 * update changelog once more * fix imagery endpoint on WMS example (#8203) * mapbox-gl-geocoder@v4.2.0 (#8212) * [docs] add trailing slash to `pathname` so resolved URL matches the canonical (#8228) * Update documentation.yml (#8230) * update mapbox-gl-geocoder to v4.3.0 (#8231) * @mapbox/mapbox gl style spec@13.7.0 (#8264) * Prepare @mapbox/mapbox-gl-style-spec@13.7.0 * Add changelog of relevant items * Add sku token to Mapbox API tile requests (#14) (#8276) * v1.0.0 (#8277) * [docs] update docs-page-shell (#8254) * fix pathname * Update react-page-shell.js
Calling
canvasSource.pause()
lets us know we can stop pulling in updates from it. Since it is possible that changes were made to the canvas source since the last render we should pull in one more updatebefore stopping. This also lets you call play and pause immediately to render just one frame:
Launch Checklist