LayerGroup.addLayer(layer)

muhamedkarajic commented 1 month ago

Checklist

[X] I've looked at the documentation to make sure the behavior isn't documented and expected.
[X] I'm sure this is an issue with Leaflet, not with my app or other dependencies (Angular, Cordova, React, etc.).
[X] I've searched through the current issues to make sure this hasn't been reported yet.
[X] I agree to follow the Code of Conduct that this project adheres to.

Steps to reproduce

I have a setup where I have to render 2.5K elements on the map.

console.log('VMapRenderLayerService', 'add: ', this.addCompArr.length, ' , remove: ', this.removeCompArr.length)

for (let i = 0; i < this.addCompArr.length; i++) {
    const element = this.addCompArr[i];
    this.layerGroup.addLayer(element);
}

for (let i = 0; i < this.removeCompArr.length; i++) {
    const element = this.removeCompArr[i];
    this.layerGroup.removeLayer(element);
}

console.log('VMapRenderLayerService finished')

My VMapRenderLayerService takes the elemnts which need to shown and puts them into a que. Anyways its not the issue that it is needing time anymore but rather that the 2 loops take 1.2s to finish.

Expected behavior

I would expect it to finish within some milliseconds.

Current behavior

Currently it takes 1.2s.

Minimal example reproducing the issue

No response

Environment

Leaflet version: "leaflet": "~1.9.4", "leaflet-ant-path": "~1.3.0", "leaflet-draw": "^1.0.4", "leaflet-editable": "^1.2.0", "leaflet-geometryutil": "~0.10.2", "leaflet-lasso": "2.2.12", "leaflet-polylinedecorator": "~1.6.0", "leaflet-snap": "^0.0.4", "leaflet.heat": "~0.2.0", "leaflet.locatecontrol": "~0.79.0", "leaflet.markercluster": "~1.5.3",
Browser (with version): Google Chrome 127.0.6533.120 (Official Build) (arm64)
OS/Platform (with version): MacOS Monterey 12.6.6 (21G646)

muhamedkarajic commented 3 weeks ago

Any updates?

N1ve5h commented 3 weeks ago

@muhamedkarajic You can avoid doing the first for-loop by adding the entire array into the LayerGroup, so it would like this:

L.layerGroup(this.addCompArr)

N1ve5h commented 3 weeks ago

For the second for-loop I would replace it with concurrency.

muhamedkarajic commented 8 hours ago

For the second for-loop I would replace it with concurrency.

Hi, sorry but I'm doing development for 5 years and have read lots of articles how the Even-loop works, even wrote some papers for University where I spent countless hours and then i had so many issues in the project where I'm working on.

Promises will block the rerendering of the DOM, so this article is "wrong". That I am saying without really reading the hole article but I read the conclusion, and looked a bit the code. Promises are not non-blocking, macro tasks are - which is settimeout. He in the source code wrote a promise and then inside of it he has a settimeout.

Anyways, it would not make anything 'faster' to make it concurrent. It would actually make it slower cause its gonna put it into the que multiple times and the browser would rerender elements on the map by taking one macro task rerender, then would repeat until macro tasks are empty.

I'm not complaining that my UI is not smooth, in that case this would solve it. I'm complaining its too slow to add it. Sorry if thats bold for me to correct you, I understand you try to help.

muhamedkarajic commented 7 hours ago

@muhamedkarajic You can avoid doing the first for-loop by adding the entire array into the LayerGroup, so it would like this:

L.layerGroup(this.addCompArr)

We will try this, although I tried it I think.

Falke-Design commented 6 hours ago

@muhamedkarajic if you have ideas how we could improve the performance, a PR would be welcomed.

Leaflet / Leaflet