Skip to content
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

feat(deck adapter) wait for async layers to load #161

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Sep 17, 2021

Currently the render handler on a hubble/deck adapter (adapter.onAfterRender) is expected to be called after every deck.gl render update (deck.onAfterRender), or if using mapbox/deck integration, on every map render event [0]. Our testing suggests a render update occurs every time a layer's data updates (including any internal asynchronous data update, such as tile loading). So, hubble can wait until the render update call where all deck layers are loaded, and then capture the frame.

Changelog
The current Hubble implementation can already wait for a number of internal flags before progressing the animation manager to the next frame, so we can add one more.

After this PR, the "wait" flags will be:

  1. Is hubble currently supposed to be recording?

  2. [New flag] Is all deck data loaded?

  3. Is hubble already capturing a frame?

Demo
Before: Notice the black artifacts where data isn't loaded, and the black frames around second 7.

before_deck_loaded.mp4

After: No artifacts or skipped frames. A frame is only captured when all deck layers are loaded. 🎉

after_deck_loaded.mp4

Note: These videos are exported with the webm encoder and locally recompressed with ffmpeg.
ffmpeg -i after_deck_loaded.webm -vcodec libx264 -crf 23 -pix_fmt yuv420p -tune animation after_deck_loaded.mp4

[0] Look for: map.on('render', () => {adapter.onAfterRender(...)}). In this PR only the deck render update will be addressed, and mapbox will need further investigation.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1243641288

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 26.563%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modules/core/src/adapters/deck-adapter.js 0 2 0.0%
Totals Coverage Status
Change from base Build 1239713806: -0.08%
Covered Lines: 217
Relevant Lines: 709

💛 - Coveralls

@@ -127,7 +127,8 @@ export default class DeckAdapter {
* @param {(nextTimeMs: number) => void} proceedToNextFrame
*/
onAfterRender(proceedToNextFrame) {
if (this.videoCapture.isRecording()) {
const areAllLayersLoaded = this.deck.props.layers.every(layer => layer.isLoaded);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! By defining customRender you should be able to prevent rendering from happing until you are ready: https://github.com/visgl/deck.gl/blob/master/modules/core/src/lib/deck.js#L347

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mainly be an optimization to prevent wasted work....

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would I call the private this._drawLayers(redrawReason); when I want to eventually render?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an issue to continue that work. Merging as-is for now since it's an improvement to the current renderer.

@chrisgervang chrisgervang merged commit c900704 into master Sep 27, 2021
@chrisgervang chrisgervang deleted the chr/deck-frame-sync branch September 30, 2021 20:02
@chrisgervang chrisgervang added this to the 1.3 milestone Oct 15, 2021
@coveralls
Copy link

coveralls commented Nov 5, 2024

Pull Request Test Coverage Report for Build 1243641288

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 26.563%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modules/core/src/adapters/deck-adapter.js 0 2 0.0%
Totals Coverage Status
Change from base Build 1239713806: -0.08%
Covered Lines: 217
Relevant Lines: 709

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants