BGDIINF_SB-2958 : add cesium component #415

vladyslav-tk · 2023-06-20T16:39:05Z

Added Cesium lib
Updated vite.config to make cesium work
Added CesiumMap component and made the switch between Ceium/OL components

Test link

gberaudo · 2023-06-21T09:38:41Z

@pakb, what is the failing "AWS CodeBuild eu-central-1 (web-mapviewer)" check?
How can we have access to it?

gberaudo

See my comments, otherwise looks like a good start to me.
I let @pakb handle the full review.

src/modules/map/components/cesium/CesiumMap.vue

src/modules/map/components/openlayers/OpenLayersMap.vue

pakb · 2023-06-21T10:01:46Z

@pakb, what is the failing "AWS CodeBuild eu-central-1 (web-mapviewer)" check? How can we have access to it?

That's our CI hosted on Amazon, I'll check if we can give you read access to it. Here's the error it gave for this PR :

[vite:load-fallback] Could not load /codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils (imported by src/modules/map/components/cesium/CesiumMap.vue): ENOENT: no such file or directory, open '/codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils'

Error: Could not load /codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils (imported by src/modules/map/components/cesium/CesiumMap.vue): ENOENT: no such file or directory, open '/codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils'

You might want to add the .js to the import, maybe it would fix it. Try to run a npm run build or even better a npm run ci before pushing

pakb

As you've clearly detected 3 steps in your work, would it be possible to have this 3 steps represented as commits?
That would make it much easier to review this PR, as it's already quite big

vladyslav-tk · 2023-06-21T10:24:02Z

@pakb, what is the failing "AWS CodeBuild eu-central-1 (web-mapviewer)" check? How can we have access to it?

That's our CI hosted on Amazon, I'll check if we can give you read access to it. Here's the error it gave for this PR :
[vite:load-fallback] Could not load /codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils (imported by src/modules/map/components/cesium/CesiumMap.vue): ENOENT: no such file or directory, open '/codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils'

Error: Could not load /codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils (imported by src/modules/map/components/cesium/CesiumMap.vue): ENOENT: no such file or directory, open '/codebuild/output/src788/src/github.com/geoadmin/web-mapviewer/src/modules/map/components/cesium/utils/navigationUtils'
You might want to add the .js to the import, maybe it would fix it. Try to run a npm run build or even better a npm run ci before pushing

I fixed the import and CI passed locally

vladyslav-tk · 2023-06-21T10:24:51Z

As you've clearly detected 3 steps in your work, would it be possible to have this 3 steps represented as commits? That would make it much easier to review this PR, as it's already quite big

I split it into 3 commits, I will try to do smaller PRs in future

pakb

Thanks a lot!
I would like to change a bit your approach to map component switching as I find your solution a bit too obfuscated.

vite.config.ts

pakb · 2023-06-21T12:38:38Z

src/modules/map/MapModule.vue

+        <KeepAlive>
+            <component :is="selectedMap">
+                <!-- So that external modules can have access to the map instance through the provided 'getMap' -->
+                <template #openlayers>
+                    <slot name="openlayers" />
+                </template>
+                <template #cesium>
+                    <slot name="cesium" />
+                </template>
+                <slot />
+                <LocationPopup v-if="!is3DActive" />
+            </component>
+        </KeepAlive>


would it not be possible to have something like

<CesiumMap v-if="is3DActive" /> <OpenLayersMap v-else> // ... </OpenLayersMap>

I find your approach way too hard to understand and the implication it has (some slot down the lines are implicitly called this way, like the DrawingModule and Footer, but that is not clear here that this will be the case)

It was my first solution but it is not really good in the case of performance when switching OL/cesium. With my solution with templates and keepAlive, OL/cesium component, map, and everything related creates and loads only one time when you switch it on the first time. With an if/else solution everything creates and loads each time when you switch between 2d/3d so it takes more time on the second switch and the same data loads again

Anyway, I reworked my solution with if/else so you can test and then we can keep it or back to a templates solution

src/modules/map/components/cesium/CesiumMap.vue

pakb · 2023-06-21T13:12:40Z

src/modules/map/components/openlayers/OpenLayersMap.vue

@@ -357,17 +362,16 @@ export default {
        this.map.on('pointerdown', this.onMapPointerDown)
        // TODO: trigger a click after pointer is down at (roughly) the same spot
        // for longer than 1sec (no need to wait for the user to stop the click)
-        this.map.on('pointerup', this.onMapPointerUp)
+        // this.map.on('pointerup', this.onMapPointerUp) todo onMapPointerUp undefined


good catch, but I would prefer we stick to the task at hand.
I'll create a ticket on our side to deal with this

src/modules/map/components/openlayers/OpenLayersMap.vue

src/modules/menu/components/toolboxRight/CompassButton.vue

src/modules/map/components/openlayers/OpenLayersMap.vue

src/views/MapView.vue

gberaudo · 2023-06-22T07:50:21Z

@vladyslav-tk, I propose we rely on https://www.npmjs.com/package/@cesium/engine

vladyslav-tk · 2023-06-22T11:37:45Z

@vladyslav-tk, I propose we rely on https://www.npmjs.com/package/@cesium/engine

engine doesn't contain a viewer so we can't use the engine only

pakb

That's much easier to understand what's going on with the component hierarchy this way, thanks!
The trade off is that it's a bit less efficient if you switch back and forth between 3D and 2D, but that's not something users should be doing too often.

Now a last little thing to add : some Cypress test that check that an intercepted (mockup) 3D tile is loaded after we've clicked on the 3D button (checking that Cesium was configured and loaded correctly)

src/modules/map/MapModule.vue

src/modules/map/components/cesium/CesiumMap.vue

pakb · 2023-06-26T15:13:00Z

tests/e2e-cypress/integration/3d/transitionTo3d.cy.js

+            cy.get('#cesium canvas').should('be.visible')
+            cy.get('#ol-map canvas').should('not.exist')


if possible, it would be better to use custom data-cy="..." CSS selector (Cypress best practices), at least to grab the OL and Cesium map element (grabbing the canvas cannot be done any other way)

pakb · 2023-06-26T15:13:36Z

tests/e2e-cypress/integration/3d/transitionTo3d.cy.js

+                expect(cameraPosition.longitude).to.eq(0.13147292983909972)
+                expect(cameraPosition.latitude).to.eq(0.819968266410843)


is that expressed in radians?

yes, when using camera.positionCartographic, camera.position will be in Cartesian3

vladyslav-tk requested a review from pakb June 20, 2023 16:39

github-actions bot added the feature label Jun 20, 2023

gberaudo reviewed Jun 21, 2023

View reviewed changes

src/modules/map/components/cesium/CesiumMap.vue Outdated Show resolved Hide resolved

src/modules/map/components/openlayers/OpenLayersMap.vue Show resolved Hide resolved

src/modules/map/components/openlayers/OpenLayersMap.vue Show resolved Hide resolved

vladyslav-tk force-pushed the feat-BGDIINF_SB-2958_add-3d-modules branch from e2cb4a7 to f2d54e1 Compare June 21, 2023 10:02

pakb reviewed Jun 21, 2023

View reviewed changes

vladyslav-tk added 3 commits June 21, 2023 13:20

BGDIINF_SB-2958 : add cesium lib

60ff92e

BGDIINF_SB-2958 : update vite.config

28401af

BGDIINF_SB-2958 : Added CesiumMap and switch between Ceium/OL

c306c47

vladyslav-tk force-pushed the feat-BGDIINF_SB-2958_add-3d-modules branch from f2d54e1 to c306c47 Compare June 21, 2023 10:23

pakb requested changes Jun 21, 2023

View reviewed changes

BGDIINF_SB-2958 : updates according to comments in PR

8bf99b4

pakb reviewed Jun 26, 2023

View reviewed changes

src/modules/map/MapModule.vue Outdated Show resolved Hide resolved

src/modules/map/components/cesium/CesiumMap.vue Outdated Show resolved Hide resolved

BGDIINF_SB-2958 : corrections

7b7aa0c

pakb reviewed Jun 26, 2023

View reviewed changes

vladyslav-tk force-pushed the feat-BGDIINF_SB-2958_add-3d-modules branch from 7cc71b0 to 195349b Compare June 26, 2023 15:26

BGDIINF_SB-2958 : add 3d load tests

4dfb76b

vladyslav-tk force-pushed the feat-BGDIINF_SB-2958_add-3d-modules branch from 195349b to 4dfb76b Compare June 27, 2023 07:11

pakb approved these changes Jun 27, 2023

View reviewed changes

pakb merged commit bedbd7c into develop Jun 27, 2023

pakb deleted the feat-BGDIINF_SB-2958_add-3d-modules branch June 27, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BGDIINF_SB-2958 : add cesium component #415

BGDIINF_SB-2958 : add cesium component #415

vladyslav-tk commented Jun 20, 2023 •

edited by github-actions bot

Loading

gberaudo commented Jun 21, 2023

gberaudo left a comment

pakb commented Jun 21, 2023 •

edited

Loading

pakb left a comment

vladyslav-tk commented Jun 21, 2023

vladyslav-tk commented Jun 21, 2023

pakb left a comment

pakb Jun 21, 2023

vladyslav-tk Jun 22, 2023

pakb Jun 21, 2023

gberaudo commented Jun 22, 2023

vladyslav-tk commented Jun 22, 2023

pakb left a comment

pakb Jun 26, 2023

vladyslav-tk Jun 26, 2023

pakb Jun 26, 2023

vladyslav-tk Jun 26, 2023

		cy.get('#cesium canvas').should('be.visible')
		cy.get('#ol-map canvas').should('not.exist')

		expect(cameraPosition.longitude).to.eq(0.13147292983909972)
		expect(cameraPosition.latitude).to.eq(0.819968266410843)

BGDIINF_SB-2958 : add cesium component #415

BGDIINF_SB-2958 : add cesium component #415

Conversation

vladyslav-tk commented Jun 20, 2023 • edited by github-actions bot Loading

gberaudo commented Jun 21, 2023

gberaudo left a comment

Choose a reason for hiding this comment

pakb commented Jun 21, 2023 • edited Loading

pakb left a comment

Choose a reason for hiding this comment

vladyslav-tk commented Jun 21, 2023

vladyslav-tk commented Jun 21, 2023

pakb left a comment

Choose a reason for hiding this comment

pakb Jun 21, 2023

Choose a reason for hiding this comment

vladyslav-tk Jun 22, 2023

Choose a reason for hiding this comment

pakb Jun 21, 2023

Choose a reason for hiding this comment

gberaudo commented Jun 22, 2023

vladyslav-tk commented Jun 22, 2023

pakb left a comment

Choose a reason for hiding this comment

pakb Jun 26, 2023

Choose a reason for hiding this comment

vladyslav-tk Jun 26, 2023

Choose a reason for hiding this comment

pakb Jun 26, 2023

Choose a reason for hiding this comment

vladyslav-tk Jun 26, 2023

Choose a reason for hiding this comment

vladyslav-tk commented Jun 20, 2023 •

edited by github-actions bot

Loading

pakb commented Jun 21, 2023 •

edited

Loading