GSLUX-623 intégration 3D #69

mki-c2c · 2023-08-18T11:58:07Z

JIRA issue

https://jira.camptocamp.com/browse/GSLUX-326

Description

do not bundle OL to allow a single instance of OL in v3 webpack
revert to legacy OL 6.5
wrap mapbox in static maplibre lib to simplify backport
add properties for 3D extents
add information for fallback raster layers xyz, xyzCustom
add 3D layers in catalog -> https://jira.camptocamp.com/browse/GSLUX-642
correctly register styles -> https://jira.camptocamp.com/browse/GSLUX-648

Screenshots

(only if the final render is different)

github-actions · 2023-08-22T06:53:19Z

GitHub Pages links: * Luxembourg-geoportail: https://geoportail-luxembourg.github.io/luxembourg-geoportail/maplibre_legacy_OL/

AlitaBernachot

Thanks for the work, did not test in v3, yet.
Needs some cleaning (eg. app.store.ts.orig is committed).

A JS code editor would have suggested you to install ol types.

Regarding the bundle, only commit what is produced by npm run build:lib:prod (after a mandatory rebase). Don't commit what has been built in dev mode.

AlitaBernachot · 2023-08-22T07:52:58Z

src/stores/app.store.ts.orig

+import { Ref, ref } from 'vue'
+import { acceptHMRUpdate, defineStore } from 'pinia'
+
+export const DEFAULT_LANG = 'fr'


To be cleaned

of course , oops.

AlitaBernachot · 2023-08-22T09:12:12Z

src/App.vue

@@ -44,6 +45,15 @@ watch(layersOpen, () =>
 onMounted(() => window.addEventListener('resize', resizeMap))
 onUnmounted(() => window.removeEventListener('resize', resizeMap))

+function traverseLayer(layer, ancestors, visitor) {


This utility function should be moved in another file as it has no link with the App component.

I'll put it into the Mapbox lib since it is just useful while the old OL is used.

AlitaBernachot · 2023-08-22T09:13:54Z

src/App.vue

@@ -52,10 +62,12 @@ function resizeMap() {
  map.updateSize()

  // And trigger update MapLibre layers' canvas size
-  map.getAllLayers().forEach(layer => {
+  // map.getAllLayers().forEach(layer => {


Clean code comment.

I will add a comment to restore the newer version when OL is updated.

AlitaBernachot · 2023-08-22T09:19:19Z

src/composables/map/ol.composable.ts

@@ -228,7 +234,8 @@ export default function useOpenLayers() {
    layersCache.set(id, layer)
  }

-  function getLayerFromCache(layer: Layer): BaseLayer {
+  function getOrCreateLayer(layer: Layer | undefined | null): BaseLayer | null {
+    if (layer === null || layer === undefined) return null


Why not using getLayerFromCache() here? This code block from l.238 to l.241 is doing the same thing as getLayerFromCache().

they used to have the same function. Now getLayerFromCache does not create a layer if it does not exist.

AlitaBernachot · 2023-08-22T09:23:18Z

src/composables/map/ol.composable.ts

+    if (layer === null || layer === undefined) return null
+    const id = layer.id
+
+    return layersCache.get(id)


Can be simplified

Suggested change

return layersCache.get(id)

function getLayerFromCache(

layer: Layer | undefined | null

): BaseLayer | null {

return layer ? layersCache.get(layer.id) : null

}

AlitaBernachot · 2023-08-22T09:32:00Z

src/composables/mvt-styles/mvt-styles.composable.ts

+    const newVectorSources: VectorSourceDict = new Map()
+    styleStore.bgVectorSources.forEach((vectorSource, key) => {
+      if (key === bgLayer.id) {
+        const newVectorSource = Object.assign({}, vectorSource, {


Spread syntax is less verbose and now mostly used.

https://stackoverflow.com/questions/32925460/object-spread-vs-object-assign

wow, that's neat, thanks.

AlitaBernachot · 2023-08-22T09:34:06Z

src/composables/mvt-styles/mvt-styles.composable.ts

@@ -222,7 +247,7 @@ export default function useMvtStyles() {
        method: 'POST',
        body: formData,
      }
-      return fetch(uploadvtstyleUrl_, options)
+      return fetch(registerUrls.get('upload') || '', options)


what's the point in fetching an empty url?

I will never be empty, but TypeScript requests a backup value

AlitaBernachot · 2023-08-22T10:05:03Z

src/stores/style.store.ts

    const appliedStyle: ShallowRef<StyleSpecification | undefined> =
      shallowRef()
+    const registerUrls: ShallowRef<Map<string, string>> = shallowRef(


Don't use stores to configure env. variables, move this into a configurable service instead if needed. By the way, @tkohr has already handled the case, see: e5d151a

AlitaBernachot · 2023-08-22T10:15:15Z

src/composables/map/ol.composable.ts

@@ -293,7 +310,9 @@ export default function useOpenLayers() {
        olMap.addLayer(bgBaseLayer)
      }
    }
-    statePersistorStyleService.restoreStyle(true)
+    if (oldBgLayerId !== bgLayer?.id) {


I don't understand how, at this point, bg layer's id cannot be different?
What use case is it?

the style shall be restored from local storage when changing bg Layers. When only the style is changed, restoreStyle shall not be calles.

https://github.com/Geoportail-Luxembourg/luxembourg-geoportail/blob/maplibre_legacy_OL/src/composables/map/ol.synchronizer.ts#L106

…roduce in OL v6.6

…d, without wanting to create it.

tkohr

Thanks for this complex PR @mki-c2c!

I did not go into all the details of the 3D and vector styles, but will try to give a rather general testing feedback:

To make the PR work within the v3 we must not forget to adapt: https://github.com/Geoportail-Luxembourg/geoportailv3/blob/integration-vue/geoportal/geoportailv3_geoportal/static-ngeo/js/resizemapDirective.js#L23

Then however I still get the error Uncaught (in promise) TypeError: Cannot read properties of null (reading 'get') at BackgroundLayerMgr.set (BackgroundLayerMgr.js:106:70) when turning off 3D mesh and 3D gets stuck in this mode no matter which of the two buttons I click.

The glitch we saw this morning that the wrong bgLayer is selected is not present anymore. However, I only manage to display the satellite imagery as bgLayer. Also, I do not see the other 3D layers like bridges and trees on the map yet.

tkohr · 2023-09-11T13:24:42Z

src/lib/ol-maplibre-layer.js

+ * @property {string|HTMLElement} container
+ */
+
+export default class MapBox extends Layer {


Shouldn't this class rather be called MapLibreLayer?

Well this is the heart of the Issue. Yes, I will rename all this lib to mapbox and import the library with an alias. That will be clearer.

tkohr · 2023-09-11T13:26:59Z

src/lib/tools.js

@@ -0,0 +1,10 @@
+import olLayerGroup from 'ol/layer/Group.js'
+
+export default function traverseLayer(layer, ancestors, visitor) {


Note that we also have a utils.ts in services. Could make sense to put this in the same place, woudn't it?

indeed, that's a good idea, I also thought about it. But this function is only needed with OL 6.5, so it will be dropped when transitioning to maplibre and OL>6.15 in v4. I think it's clearer when the function is grouped with the custom Mapbox lib

tkohr

As discussed, I've retested this branch together with https://github.com/Geoportail-Luxembourg/geoportailv3/tree/GSLUX-623_integrate3D

The overall integration of the 3D mode works on my side, thanks @mki-c2c!

I still get the error Uncaught (in promise) TypeError: Cannot read properties of null (reading 'get') at BackgroundLayerMgr.set (BackgroundLayerMgr.js:106:70) when turning off 3D mesh and 3D gets stuck in this mode no matter which of the two buttons I click.

Also, I do not see the other 3D layers like bridges and trees on the map yet.

Let's try to fix these problems within the new tickets you have created.

AlitaBernachot · 2023-09-21T09:10:58Z

src/lib/tools.js

Should be a ts file instead of a js file

both library files are copy pasted from a very old js repo, so they are js and not ts.

AlitaBernachot · 2023-09-21T09:11:18Z

src/lib/ol-maplibre-layer.js

Should be a ts file instead of a js file

both library files are copy pasted from a very old js repo, so they are js and not ts.

oh, I forgot to include the d.ts files

AlitaBernachot

There are 2 js files in src/lib, maybe switch to ts file as the entire project is in TS?

MKI: I agree with you but let's just consider these files as an external legacy js library which will be removed as soon as OL is upgraded.

mki-c2c

Thanks for your good comments, I am taking them into account, then merge the PR

mki-c2c · 2023-08-24T11:22:16Z

src/composables/map/ol.composable.ts

@@ -228,7 +234,8 @@ export default function useOpenLayers() {
    layersCache.set(id, layer)
  }

-  function getLayerFromCache(layer: Layer): BaseLayer {
+  function getOrCreateLayer(layer: Layer | undefined | null): BaseLayer | null {
+    if (layer === null || layer === undefined) return null


they used to have the same function. Now getLayerFromCache does not create a layer if it does not exist.

mki-c2c · 2023-08-24T11:27:03Z

src/composables/map/ol.composable.ts

@@ -293,7 +310,9 @@ export default function useOpenLayers() {
        olMap.addLayer(bgBaseLayer)
      }
    }
-    statePersistorStyleService.restoreStyle(true)
+    if (oldBgLayerId !== bgLayer?.id) {


the style shall be restored from local storage when changing bg Layers. When only the style is changed, restoreStyle shall not be calles.

https://github.com/Geoportail-Luxembourg/luxembourg-geoportail/blob/maplibre_legacy_OL/src/composables/map/ol.synchronizer.ts#L106

mki-c2c · 2023-08-24T11:32:28Z

src/composables/map/ol.composable.ts

+    if (layer === null || layer === undefined) return null
+    const id = layer.id
+
+    return layersCache.get(id)


mki-c2c · 2023-08-24T11:34:14Z

src/App.vue

@@ -52,10 +62,12 @@ function resizeMap() {
  map.updateSize()

  // And trigger update MapLibre layers' canvas size
-  map.getAllLayers().forEach(layer => {
+  // map.getAllLayers().forEach(layer => {


I will add a comment to restore the newer version when OL is updated.

mki-c2c · 2023-08-24T11:34:54Z

src/App.vue

@@ -44,6 +45,15 @@ watch(layersOpen, () =>
 onMounted(() => window.addEventListener('resize', resizeMap))
 onUnmounted(() => window.removeEventListener('resize', resizeMap))

+function traverseLayer(layer, ancestors, visitor) {


I'll put it into the Mapbox lib since it is just useful while the old OL is used.

mki-c2c · 2023-08-24T11:49:48Z

src/stores/app.store.ts.orig

+import { Ref, ref } from 'vue'
+import { acceptHMRUpdate, defineStore } from 'pinia'
+
+export const DEFAULT_LANG = 'fr'


of course , oops.

mki-c2c · 2023-09-21T09:48:57Z

src/lib/tools.js

@@ -0,0 +1,10 @@
+import olLayerGroup from 'ol/layer/Group.js'
+
+export default function traverseLayer(layer, ancestors, visitor) {


indeed, that's a good idea, I also thought about it. But this function is only needed with OL 6.5, so it will be dropped when transitioning to maplibre and OL>6.15 in v4. I think it's clearer when the function is grouped with the custom Mapbox lib

mki-c2c · 2023-09-21T09:50:50Z

src/lib/ol-maplibre-layer.js

+ * @property {string|HTMLElement} container
+ */
+
+export default class MapBox extends Layer {


Well this is the heart of the Issue. Yes, I will rename all this lib to mapbox and import the library with an alias. That will be clearer.

mki-c2c · 2023-09-21T09:54:41Z

src/lib/tools.js

both library files are copy pasted from a very old js repo, so they are js and not ts.

mki-c2c · 2023-09-21T09:54:46Z

src/lib/ol-maplibre-layer.js

both library files are copy pasted from a very old js repo, so they are js and not ts.

mki-c2c force-pushed the maplibre_legacy_OL branch from 88a2618 to 19cfb89 Compare August 22, 2023 06:52

mki-c2c requested a review from AlitaBernachot August 22, 2023 06:55

AlitaBernachot requested changes Aug 22, 2023

View reviewed changes

mki-c2c force-pushed the maplibre_legacy_OL branch from 6cb46d5 to bb43d72 Compare September 11, 2023 07:38

AlitaBernachot and others added 15 commits September 11, 2023 09:43

chore(lib): npm run build:lib:prod

9766834

feat(lib): build lib without ol in dev mode

2adae93

bundle maplibre library statically to revert to OL 6.5

ce1a1a6

provide MaplibreLayer to V3

89455cc

adapt maplibre layers to OL v6.5.0 renderer

2eadc41

fake custom property behaviour (propertiers going into -> values) int…

bad24f0

…roduce in OL v6.6

fix positioning and resizing of maplibre container

d7e0304

enable alternative raster tiles for vector layers (in3D)

18dce30

bundle maplibre-layers statically as a mapbox wrapper

6e056f1

register correctly custom styles and set xyz URL

312d33f

clarify naming of method to avoid side effect where getLayer is calle…

be32c6c

…d, without wanting to create it.

add compiled library

098f69b

review comments

2effb94

fix typing

7aaf9bd

add lib bundles

35597cb

mki-c2c force-pushed the maplibre_legacy_OL branch from bb43d72 to 35597cb Compare September 11, 2023 08:03

mki-c2c requested a review from tkohr September 11, 2023 08:08

tkohr reviewed Sep 11, 2023

View reviewed changes

tkohr approved these changes Sep 18, 2023

View reviewed changes

AlitaBernachot approved these changes Sep 21, 2023

View reviewed changes

AlitaBernachot reviewed Sep 21, 2023

View reviewed changes

mki-c2c commented Sep 21, 2023

View reviewed changes

rename lib

8c4f64f

fix missing imports

1f80144

mki-c2c merged commit bf76f3e into main Sep 22, 2023
2 checks passed

		@@ -0,0 +1,10 @@
		import olLayerGroup from 'ol/layer/Group.js'

		export default function traverseLayer(layer, ancestors, visitor) {

GSLUX-623 intégration 3D #69

GSLUX-623 intégration 3D #69

Conversation

mki-c2c commented Aug 18, 2023 • edited Loading

JIRA issue

Description

Screenshots

github-actions bot commented Aug 22, 2023

AlitaBernachot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot left a comment • edited by mki-c2c Loading

Choose a reason for hiding this comment

mki-c2c left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mki-c2c commented Aug 18, 2023 •

edited

Loading

AlitaBernachot left a comment •

edited

Loading

AlitaBernachot Aug 22, 2023 •

edited

Loading

AlitaBernachot left a comment •

edited by mki-c2c

Loading

mki-c2c left a comment •

edited

Loading