smart layer update for changes not working for grouped layers

[santilland]

The smart update functionality for layers assumes a flat structure, but it is possible to work with groups.
We either need to rethink how we do the smart layer update approach, or implement recursive checks for potentially any depth level that is possible with groups

[RobertOrthofer]

What exactly is not working? I tried changing the opacity and style 2 levels deep, that seems to work just fine

[santilland]

Hello @RobertOrthofer , as far as i can see, if the group identifier stays the same, but the nested group layers change, with new ids and everything the nested layers will not update.
So for example i created 3 groups, overlays, baselayers and data layers, with "constant" ids.
If i load a new layer config, with these groups, but different nested content, the content of the group will not be updated correctly.
I solved it for now by generating a pseudo uid for the layer groups each time i make a change in the nested layer content.

[RobertOrthofer]

Thanks for the detailed test case, i will look into it

[santilland]

I have continued testing and as you said, it seems to work as expected, i can't reproduce the issue again right now.
I have found another issue testing where changing layer config after a webGL layer or vectortile layers was added produces an error trying.
Somehow there is an undefined when doing the forEach here:

EOxElements/elements/map/src/generate.ts

Line 400 in 974aa84

if (!newLayerIds.includes(l.get("id"))) {

Not sure if i should open a new issue, or we can leave it as part of this one.

Here are the configs:

[{"type":"Group","properties":{"id":"OVERLAYS","title":"Overlay Layers"},"layers":[{"type":"Tile","properties":{"id":"overlay_bright","title":"Binary S2 Segmentation results","roles":["overlay","visible"],"group":"overlay","visible":true},"source":{"type":"XYZ","url":"//s2maps-tiles.eu/wmts/1.0.0/overlay_base_bright_3857/default/g/{z}/{y}/{x}.png"}}]},{"type":"Group","properties":{"id":"ANALYSIS","title":"Analysis Layers","layerControlExpand":true},"layers":[{"type":"WebGLTile","source":{"type":"GeoTIFF","normalize":true,"sources":[{"url":"https://eox-gtif-public.s3.eu-central-1.amazonaws.com/test_data_polartep/input-cog.tif"}]},"properties":{"id":"lyyhke6d54dueocvmxi","title":"Binary S2 Segmentation results - source image","layerControlExpand":true,"layerControlToolsExpand":true}},{"type":"WebGLTile","source":{"type":"GeoTIFF","normalize":true,"sources":[{"url":"https://eox-gtif-public.s3.eu-central-1.amazonaws.com/test_data_polartep/inference-cog.tif"}]},"properties":{"id":"lyyhkecxrjl8gop1mc","title":"Binary S2 Segmentation results - inference","layerControlExpand":true,"layerControlToolsExpand":true},"style":{"color":["case",["==",["band",1],1],["color",255,255,255,1],["color",0,0,0,0]]}}]},{"type":"Group","properties":{"id":"BASELAYERS","title":"Base Layers"},"layers":[{"type":"Tile","properties":{"id":"cloudless-2022","title":"Binary S2 Segmentation results","roles":["baselayer"],"group":"baselayer","layerControlExclusive":true},"source":{"type":"XYZ","url":"//s2maps-tiles.eu/wmts/1.0.0/s2cloudless-2022_3857/default/g/{z}/{y}/{x}.jpeg"}}]}]

[{"type":"Group","properties":{"id":"OVERLAYS","title":"Overlay Layers"},"layers":[{"type":"Tile","properties":{"id":"overlay_bright","title":"Global temperature","roles":["overlay","visible"],"group":"overlay","visible":true},"source":{"type":"XYZ","url":"//s2maps-tiles.eu/wmts/1.0.0/overlay_base_bright_3857/default/g/{z}/{y}/{x}.png"}}]},{"type":"Group","properties":{"id":"ANALYSIS","title":"Analysis Layers","layerControlExpand":true},"layers":[{"type":"Tile","properties":{"id":"lyyhol0sb2vogkmqe8a","title":"Global temperature","layerControlExpand":true,"layerControlToolsExpand":true},"source":{"type":"TileWMS","url":"https://services.sentinel-hub.com/ogc/wms/0635c213-17a1-48ee-aef7-9d1731695a54","params":{"LAYERS":["AWS_VIS_2MTEMPERATURE"],"TILED":true,"TIME":"2024-03-01T00:00:00Z"}}}]},{"type":"Group","properties":{"id":"BASELAYERS","title":"Base Layers"},"layers":[{"type":"Tile","properties":{"id":"cloudless-2022","title":"Global temperature","roles":["baselayer"],"group":"baselayer","layerControlExclusive":true},"source":{"type":"XYZ","url":"//s2maps-tiles.eu/wmts/1.0.0/s2cloudless-2022_3857/default/g/{z}/{y}/{x}.jpeg"}}]}]

[RobertOrthofer]

Unfortunately i can't reproduce this problem either, but since you have a very specific part of the code where the error is thrown, maybe there is a layer that changes it's type from Group to something else, while the id stays the same?

[santilland]

That is strange, i can reproduce the error also in the storybook, e.g. here:
https://eox-a.github.io/EOxElements/?path=/story/elements-eox-map--geo-tiff-layer
i copy the first config into the layers field (click outside the textbox to make it load)
i copy the second config (click outside the textbox to make it load) and i get the error
in chrome and firefox.
Could you try it out there @RobertOrthofer ?

[silvester-pari]

I also get Cannot read properties of undefined (reading 'get')

[RobertOrthofer]

Interesting, i'm looking into this, and also why the error doesn't occur in the testing environment

[RobertOrthofer]

This was a bit tricky, but it seems to be the same problem that we already had some time ago. In the function getFlatLayersArray we are looking for group layers in a clean fashion: layer instanceof Group, but it seems, because of a faulty installation, the ol used here is a different ol used for creating the layers, therefore this fails. This also explains why the same tests succeeded for me yesterday, yet today they fail. I have noticed that there are 2 instances of ol installed, one in node_modules, and one in elements/map/node_modules.

I am yet to find the real culprit here. As I am not an npm expert, @silvester-pari do you have any suspicion in this regard? It seems that in the current main after a clean install the build fails because ol-stac does not find ol, even when switching back to older version in all elements

[silvester-pari]

Since the merging of ol version 9.2.5-dev, everything fails, since the peer dependencies of ol-stac and ol-pmtiles don't seem to be met. I hope this gets resolved once we have ol 10 (or we revert back to ol 9.2.4).

[RobertOrthofer]

this seems to be fixed via #1141. I will add a test for faster debugging in the future.

[santilland]

Silvester updated the package lock and now in theory in the latest storybook deployment there should not be an older OL version? Could you check again @RobertOrthofer if you can see the version conflict you found last time?
I am still getting the same error, i think potentially we might need to re-release some of the packages if it is still the same issue.

[santilland]

So, i have been experimenting around and trying locally to use all packages build at the latest state, and i don't think this is a version issue.
In the current framework i am integrating the elements i realized while testing that the order in which i change the eox-map config matters. For some reason if i load one of the configurations first changing to the others will prompt this error. If i start loading another configuration the problem no longer appears and i can use any configuration without errors...
I have not managed to reproduce this in the storybook though, there it seems that the order does not matter, setting the configurations i posted one after the other seems to always give an error.
Maybe the error appears in the same place but are unrelated? Here is the error in the dashboard where i am integrating the components:

So it seems to be the same part as happens in the storybook
https://github.com/EOX-A/EOxElements/blob/map/test/grouplayer-instanceof/elements/map/src/generate.ts#L400

This is quite a stubborn issue, i hope you have more luck finding the underlying cause @RobertOrthofer with these inputs

[RobertOrthofer]

yes, there is another issue that was hidden underneath the package issue, you are completely right about the "same place but unrelated". It's a very basic problem, but was hard to find, I will update the PR asap. I'm still not sure why the error occurs in storybook but not in my test case though

[silvester-pari]

There is something very weird going on with Cypress; the error obviously lies in the layerCollection.forEach loop, since a layer is removed inside the loop and thus the next iteration is undefined; this can be fixed by e.g. using getArray() on top of layerCollection:

    layerCollection.getArray().forEach((l: Layer) => {
      if (!newLayerIds.includes(l.get("id"))) {
        layerCollection.remove(l);
      }
    });

But the really strange thing is that Cypress doesn't break on this error:

Added a break if l is unefined:

Console in Storybook:

Console in Cypress:

So Cypress doesn't seem to care that l is undefined, and doing l.get("id") doesn't bother, so the code just continues and the layers are updated anyways!

[RobertOrthofer]

yes, that's exactly that. IMO the error is that the collection.forEach does a normal for-loop internally, which is definitely not what i expected. Maybe this is just some cypress thing the first error is caught, and in cypress fashion it is retried, but then the array is already corrected and it works.

[silvester-pari]

I tried setting the retries to 0, didn't change anything. We need to investigate further here, but it's not connected anymore to the issue which has been fixed.