BGDIINF_SB-2890: better handling of zoom conversion by pakb · Pull Request #471 · geoadmin/web-mapviewer

pakb · 2023-10-19T05:42:13Z

between Swiss and non-Swiss projections, fixing the Zoom to extent functionality in the process
Most, if not all, zoom related utils function and constants have been moved to the zoomLevelUtils.js file

Test link

ltshb

Ok after a call with @pakb here what we decided to make the code easier to understand and to enhanced with other coordinate systems:

Add a mandatory abstract method in CoordinateSystem class that would translate the zoom level. The method could be then named for example: getZoomFromResolution(resolution, coordinate). I personally would provide the whole coordinate even if for the moment only the latitude is needed for mercator.
Use class inheritance for the coordinate system specialization class Lv95 extends CoordinateSystem. This specialization would implement the getZoomFromResolution
Add also a getResolutionFromZoom in CoordinateSystem this would remove the getters resolution in position.store.js

It may be a good idea to first merge this PR with just some minor changes before this big refactoring, and do the big refactoring in separate PR.

src/router/legacyPermalinkManagement.routerPlugin.js

src/utils/coordinates/CoordinateSystem.class.js

src/store/modules/search.store.js

src/store/plugins/reproject-everything-on-projection-change.plugin.js

We still have an issue with the zoom level, but this might be solved by #471

ltshb · 2023-10-24T11:51:28Z

@pakb Passing to 3d view crash the application

ltshb

You can (either by scrolling, clicking on zoom or entering in the url) go up to zoom 13, however you got only tiles up to zoom 9.4 above you have either empty tiles or failures from backend.

pakb · 2023-10-24T15:07:05Z

There was a missing resolution, I forgot one in the process of moving them from config.js to the dedicated projection file somehow 🙈

3D should be fixed too

ltshb

Great works, looks much cleaner. 🎉

I have only a few minors comments/suggestions.

src/utils/coordinates/LV03CoordinateSystem.class.js

src/utils/coordinates/WebMercatorCoordinateSystem.class.js

ltshb · 2023-10-25T08:11:46Z

src/utils/coordinates/WGS84CoordinateSystem.class.js

+                -90.0,
+                90.0,
+                // center of LV95's extent transformed with epsg.io website
+                [8.239436, 46.832259]


Might be better to use proj4 to dynamically re-project the LV95 extent here ?

Or even better make this configurable in the config.js, this way if someone want to use our soft and want to center on france for example he only needs to change its config.js file and not the implementation.

I think this needs some kind of startup rework, as proj4 is not yet configured when we instantiate these coordinate systems.
Plus it would not be super robust, as if I start the app with .../?sr=3857 without specifying a center, it would not reproject the default center automatically (so it needs some more thoughts)
But I like this idea!

src/utils/coordinates/WebMercatorCoordinateSystem.class.js

ltshb · 2023-10-25T08:21:51Z

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

+        this.viewer.scene.postRender.addEventListener(
+            limitCameraCenter(LV95.getBoundsAs(WGS84).flatten)
+        )


Similar comment as down below, would it make sense to have config value for the default bound/center ? To avoid hardcoding it here, so our default bounds and center is CH but for other it might something else. So could we have something like this

// CesiumMap.vue limitCameraCenter(DEFAULT_PROJECTION.getBoundsAs(WGS84).flatten)

Maybe then also add a comment in the config.js that the default projection is also used as default bound and center.

maybe we should do that only when the DEFAULT_PROJECTION is a CustomCoordinateSystem, as it would normally be the case that such a system will shrink its size to fit some part of the World.

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

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

ltshb · 2023-10-25T08:23:33Z

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

-            const tileGridLV95 = new TileGrid({
-                resolutions: TILEGRID_RESOLUTIONS,
-                extent: TILEGRID_EXTENT,
-                origin: TILEGRID_ORIGIN,
-                tileSize: WMS_TILE_SIZE,
-            })
-            // If we are using LV95, we can constrain the WMS to only request tiles over Switzerland
-            if (this.projection.epsg === LV95.epsg) {
-                source.tileGrid = tileGridLV95
-            } else if (this.gutter !== -1) {
-                // in WebMercator, the tile grid is reprojected analog to WMTS.
-                // This is to prevent that the layer appears twice, once in CH and once near New Zealand.
-                // see: https://github.com/geoadmin/web-mapviewer/commit/c689f9a650c546c6e52a91fc2086d7cbbf48faa2
-                source.setTileGridForProjection(
-                    WEBMERCATOR.epsg,
-                    new TileGrid({
-                        resolutions: TILEGRID_RESOLUTIONS,
-                        origin: proj4(LV95.epsg, WEBMERCATOR.epsg, TILEGRID_ORIGIN),
-                        extent: transformExtent(
-                            tileGridLV95.getExtent(),
-                            LV95.epsg,
-                            WEBMERCATOR.epsg
-                        ),
-                    })
-                )
+            if (this.projection instanceof CustomCoordinateSystem) {
+                source.tileGrid = new TileGrid({
+                    resolutions: this.projection.getResolutions(),
+                    extent: this.projection.bounds.flatten,
+                    origin: this.projection.getTileOrigin(),
+                    tileSize: WMS_TILE_SIZE,
+                })


Very nice code simplification 👍🏼

src/router/legacyPermalinkManagement.routerPlugin.js

between Swiss and non-Swiss projections, fixing the Zoom to extent functionality in the process Most, if not all, zoom related utils function and constants have been moved to the zoomLevelUtils.js file

to better represents the different flavor of projection we use in our app. And also to gather all the transformation operations necessary to switch from one another directly within them (thus deleting the zoomUtils.js file)

as they are not "config" in a sense that they should not be changed, but are linked to how our application will function when having LV95 as its primary coordinate system

And simplify how we declare tile grids in our OpenLayers components Also fix an issue of number order in the LV95 bounds

There was one step missing while close to the ground, making all tiles past zoom level 10 failing to be correctly requested

and update proj4 definition (they were updated on the EPSG website)

use the default projection to constrain Cesium (instead of hard-coding LV95) fix some comments

github-actions bot added the bug label Oct 19, 2023

pakb mentioned this pull request Oct 19, 2023

Default projection to lv95 #470

Merged

17 tasks

pakb force-pushed the bug_BGDIINF_SB-2890_fix_zoom_to_extent branch from 2c9bd56 to c78d5f1 Compare October 19, 2023 07:27

pakb changed the title ~~BGDIINF_SB-2890: add utils function to calculate zoom~~ BGDIINF_SB-2890: better handling of zoom conversion Oct 19, 2023

pakb requested a review from ltshb October 19, 2023 07:27

pakb marked this pull request as ready for review October 19, 2023 07:28

ltshb reviewed Oct 19, 2023

View reviewed changes

ltshb added a commit that referenced this pull request Oct 23, 2023

BGDIINF_SB-2890: Fixed 3d to 2d center of map

1ebc912

We still have an issue with the zoom level, but this might be solved by #471

ltshb mentioned this pull request Oct 23, 2023

BGDIINF_SB-2890: Fixed 3d to 2d center of map #482

Merged

pakb force-pushed the bug_BGDIINF_SB-2890_fix_zoom_to_extent branch 2 times, most recently from 868522f to 34d2793 Compare October 24, 2023 10:13

pakb requested a review from ltshb October 24, 2023 10:54

pakb force-pushed the bug_BGDIINF_SB-2890_fix_zoom_to_extent branch 3 times, most recently from 6b04f29 to a056148 Compare October 24, 2023 11:49

ltshb reviewed Oct 24, 2023

View reviewed changes

pakb requested a review from ltshb October 24, 2023 15:07

ltshb approved these changes Oct 25, 2023

View reviewed changes

pakb added 10 commits October 25, 2023 10:56

BGDIINF_SB-2890: better handling of zoom conversion

cd459fd

between Swiss and non-Swiss projections, fixing the Zoom to extent functionality in the process Most, if not all, zoom related utils function and constants have been moved to the zoomLevelUtils.js file

BGDIINF_SB-2890: Add a coordinate system class hierarchy

af8c0e0

to better represents the different flavor of projection we use in our app. And also to gather all the transformation operations necessary to switch from one another directly within them (thus deleting the zoomUtils.js file)

BGDIINF_SB-2890: moving all LV95 constant to coordinate system file

7a96fd0

as they are not "config" in a sense that they should not be changed, but are linked to how our application will function when having LV95 as its primary coordinate system

BGDIINF_SB-2890: use the round function linked to projections

a267566

BGDIINF_SB-2890: use the new zoom/resolution functions from projections

a832b3d

BGDIINF_SB-2890 : move resolution definition in coordinate system

7b8dd88

And simplify how we declare tile grids in our OpenLayers components Also fix an issue of number order in the LV95 bounds

BGDIINF_SB-2890 : fix refactoring miss

ecc357c

BGDIINF_SB-2890 : fix missing resolution

2d49e36

There was one step missing while close to the ground, making all tiles past zoom level 10 failing to be correctly requested

BGDIINF_SB-2890 : fix missing import

b4c7810

BGDIINF_SB-2890 : explain magic numbers

8906261

and update proj4 definition (they were updated on the EPSG website)

pakb force-pushed the bug_BGDIINF_SB-2890_fix_zoom_to_extent branch from f813240 to 8906261 Compare October 25, 2023 09:54

BGDIINF_SB-2890 : PR review

9bbbabb

use the default projection to constrain Cesium (instead of hard-coding LV95) fix some comments

pakb merged commit 75d2d46 into develop-lv95 Oct 25, 2023

pakb deleted the bug_BGDIINF_SB-2890_fix_zoom_to_extent branch October 25, 2023 10:07

Conversation

pakb commented Oct 19, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltshb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ltshb commented Oct 24, 2023

Uh oh!

ltshb left a comment

Choose a reason for hiding this comment

Uh oh!

pakb commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltshb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ltshb Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

ltshb Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

pakb Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ltshb Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

pakb Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ltshb Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pakb commented Oct 19, 2023 •

edited by github-actions bot

Loading

pakb commented Oct 24, 2023 •

edited

Loading

ltshb left a comment •

edited

Loading