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

Bypass the DOM rendering queue if map is idle #10567

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

zarov
Copy link
Contributor

@zarov zarov commented Apr 13, 2021

In #10530 I added a new queue dedicated to the rendering of DOM elements. Unfortunately I made the mistake to run this new queue only in map._render(), which won't be called if the map is idle. You can test this problem by trying to move a marker once the map is fully loaded and rendered in markers.html.

A mechanism like map._requestRenderFrame() could be added, but I feel there is no need to call the whole _render() loop all over again for a simple rendering of a DOM element. As far as I understand, this problem happens only when very few elements are moving/rendering. So I'm proposing to call directly the dom task, like it was the case before, only if the map is idle. The condition I choose is the same in ui/map, right before setting the map idle.

Note that preventing the map to go idle if there are still some tasks in domRenderTaskQueue won't work, as the queue could be filled while the map is already idle.

Tagging @mourner as he followed the previous PR.

Sorry about this mistake, I though I checked correctly in the other PR.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Since this code has to be repeated in multiple places, and it's easy to make this mistake again, let's add a Map method that contains this logic so that you do e.g. this._map._addDomTask(() => { ... } in either marker.js, popup.js and elsewhere.

@zarov
Copy link
Contributor Author

zarov commented Apr 14, 2021

Good call, I added a _requestDomTask to Map. I also updated the affected components from the last time, except for scale_control where I am not sure how to proceed with the parameter to pass to setScale (see here): replace domQueue by map._requestDomTask and call it in setScale, return the transformation callback from setScale and call it in updateScale or even something else?

@mourner
Copy link
Member

mourner commented Apr 14, 2021

except for scale_control where I am not sure how to proceed with the parameter to pass to setScale

In this case I think it's fine to go back to passing the map instance and using its method in setScale.

@zarov
Copy link
Contributor Author

zarov commented Apr 14, 2021

Ok, I updated it!

@karimnaaji
Copy link
Contributor

Merging to have this in the rebased version of #10564.

@karimnaaji karimnaaji merged commit 364ca37 into mapbox:main Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants