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

Wait for icons to be loaded before firing rendercomplete event #13626

Merged
merged 7 commits into from
May 25, 2022

Conversation

MoonE
Copy link
Contributor

@MoonE MoonE commented May 1, 2022

The rendercomplete event should wait for icons to load before it is being fired.
As there is already similar behavior for the webgl renderers with the ready property, this uses the same property.

This also fixes an error when an icon is loaded after the renderer has been disposed.

@github-actions
Copy link

github-actions bot commented May 1, 2022

📦 Preview the examples and docs from this branch here: https://deploy-preview-13626--ol-site.netlify.app/.

@MoonE MoonE marked this pull request as draft May 1, 2022 22:28
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks, @MoonE! Did you intentionally leave this in Draft state?

@MoonE
Copy link
Contributor Author

MoonE commented May 3, 2022

Yes, I believe the VectorTileLayer's ready is currently set to the last tile's state, I think I fixed it in my local branch, but I'd like to write a test to confirm it.

@ahocevar
Copy link
Member

ahocevar commented May 3, 2022

Excellent, thanks!

@MoonE MoonE force-pushed the rendercomplete-wait-for-icons branch from 33233dd to 2d82233 Compare May 24, 2022 20:25
@MoonE MoonE force-pushed the rendercomplete-wait-for-icons branch from 2d82233 to e8925a9 Compare May 24, 2022 20:26
@@ -255,7 +250,6 @@ class CanvasVectorTileLayerRenderer extends CanvasTileLayerRenderer {
const bufferedExtent = equals(sourceTileExtent, sharedExtent)
? null
: builderExtent;
builderState.dirty = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this did only take the last source tile's dirty state into account.

@@ -289,7 +283,6 @@ class CanvasVectorTileLayerRenderer extends CanvasTileLayerRenderer {
builderGroup,
declutterBuilderGroup
);
this.dirty_ = this.dirty_ || dirty;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.dirty_ was never reset to false , though it was also unused.

for (let i = tiles.length - 1; i >= 0; --i) {
const tile = /** @type {import("../../VectorRenderTile.js").default} */ (
tiles[i]
);
ready = ready && !tile.getReplayState(layer).dirty;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ready is true when the currently drawn tiles have no loading icons. Tile state is tracked separately.

@MoonE MoonE marked this pull request as ready for review May 24, 2022 20:45
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thank for the explanations and the thorough work on this!

@MoonE MoonE merged commit bed1e51 into openlayers:main May 25, 2022
@MoonE MoonE deleted the rendercomplete-wait-for-icons branch May 25, 2022 20:32
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.

2 participants