EOX-A / EOxElements

A Web Component collection of geospatial UI elements, crafted by EOX.
https://eox-a.github.io/EOxElements/
MIT License
12 stars 2 forks source link

smart layer update for changes not working for grouped layers #975

Closed santilland closed 2 months ago

santilland commented 4 months ago

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 commented 3 months ago

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

santilland commented 3 months ago

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 commented 3 months ago

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

santilland commented 2 months ago

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: https://github.com/EOX-A/EOxElements/blob/974aa84ee81439b5e5fdf8dca9caf03742bf88ec/elements/map/src/generate.ts#L400

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

RobertOrthofer commented 2 months ago

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

RobertOrthofer commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

this seems to be fixed via https://github.com/EOX-A/EOxElements/pull/1141. I will add a test for faster debugging in the future.

santilland commented 2 months ago

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 commented 2 months ago

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: image 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 commented 2 months ago

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 commented 2 months ago

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: image

Console in Storybook: image

Console in Cypress: image

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 commented 2 months ago

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 commented 2 months ago

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.