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

Fix event.isSourceLoaded to reflect the state of source loading for sourcedata event #2543

Merged
merged 11 commits into from
May 22, 2023

Conversation

ambientlight
Copy link
Sponsor Contributor

@ambientlight ambientlight commented May 17, 2023

Fixes #2539

  • use sourceCache.loaded() for event.isSourceLoaded
  • sourceCache.loaded() is true, if used or usedForTerrain in set to false (after source loaded)
  • do not consider source cache as loaded before update is called (because we don't know if we need to fetch tiles yet).
  • added new idle sourcedatatype used in event emissions to propagate the source_cache.loaded() change to true for cases when aren't any tile loads pending: on sourceCache.update() called when viewport is outside of source bounds, on image/video/canvas source.prepare()

Context

The goal of this change is to make sure we have a single sourcedata invocation with event.isSourceLoaded true for each source at the end after initial load and subsequent interactions that result in tile fetches. Current behavior has used style.loaded() for the value of event.isSourceLoaded and this would result in whatever source tile load that comes last to have this set. Flipping the

isSourceLoaded: this.loaded(),
to sourceCache.loaded() is not enough because we will have event.isSourceLoaded set to true after the source is ready (tilejson fetched), but prior to any tiles actually being fetched.

We may only know whether the source will have tiles to fetch once the source receives transform (on update on render), and thus source_cache.loaded() would now require update to be called at least once on the source to be considered loaded.

Additionally, because the sourcedata event emissions are not tied to source_cache.loaded(), we are not guaranteed we will actually receive the emission with isSourceLoaded set to true. This applies to cases, when:

  • we don't have any tiles to fetch when our source has bounds outside of the viewport (and all sourcedata invocations may happen prior to the first source_cache.update())
  • we use image_source, canvas_source, video_source that will not set the tile state to loaded on its load, and will only set the tile state later on prepare() on map.painter.render().

To compensate for this, additional sourcedata event invocations with sourcedatatype idle have been added.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Handle the source with bounds outside of the viewport, where on initial load there may be no events with isSourceLoaded true until the camera is moved to tileBounds.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Add an entry to CHANGELOG.md under the ## main section.

@ambientlight ambientlight changed the title use sourceCache.loaded() for event.isSourceLoaded, do not consider so… Fix event.isSourceLoaded to reflect the state of source loading May 17, 2023
@ambientlight ambientlight changed the title Fix event.isSourceLoaded to reflect the state of source loading Fix event.isSourceLoaded to reflect the state of source loading for sourcedata event May 17, 2023
@ambientlight ambientlight force-pushed the fix/sourcedata-isSourceLoaded branch from 5ae1713 to 544a239 Compare May 19, 2023 16:25
@HarelM
Copy link
Collaborator

HarelM commented May 19, 2023

Note that we will release 3.0 soon. If you would like to have this in please finish the changes and request for review.
Thanks!!

@ambientlight
Copy link
Sponsor Contributor Author

@HarelM: yes, I would like to have this in, sure, need to add the tests now, probably need a few hours, thanks.

@ambientlight ambientlight marked this pull request as ready for review May 20, 2023 09:59
@ambientlight ambientlight force-pushed the fix/sourcedata-isSourceLoaded branch from 8a503a5 to 98f6c2b Compare May 20, 2023 10:29
@birkskyum
Copy link
Member

birkskyum commented May 20, 2023

just fyi - the breaking render test ( /debug/collision , pitched-rotated-debug, and ~3 other ) are known to be a bit flaky since the migration from node mocks to puppeteer, because of what I think is a race-condition, so don't mind it.

@birkskyum birkskyum added this to the 3.0.0 milestone May 20, 2023
@birkskyum
Copy link
Member

birkskyum commented May 20, 2023

@ambientlight , is this ready for review?

@ambientlight
Copy link
Sponsor Contributor Author

@birkskyum: yes, it is ready for review.

@birkskyum birkskyum requested a review from HarelM May 20, 2023 12:32
@HarelM
Copy link
Collaborator

HarelM commented May 20, 2023

It's a bit more complicated than I expected, so I'll need a bit more time to review this, hopefully tonight.

src/ui/map.test.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented May 21, 2023

Can you also check the code coverage for the new code in the report here in the CI to make sure the new code is covered?

@ambientlight
Copy link
Sponsor Contributor Author

ambientlight commented May 22, 2023

@HarelM, @birkskyum: I've addressed comments and added remaining tests for coverage of idle emissions in canvas / image / video sources. Let me know if there are more questions here, thanks.

Copy link
Member

@birkskyum birkskyum left a comment

Choose a reason for hiding this comment

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

Thanks @ambientlight !

@birkskyum birkskyum merged commit ded439d into maplibre:main May 22, 2023
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.

sourcedata's event.isSourceLoaded is only true for last emission when the tiles for all sources are loaded
3 participants