Fix #9685 measurements issues and added some enhancements to it #9809

MV88 · 2023-12-12T10:09:27Z

Description

Fix vector measurement layer issue
Filter out point text label as annotations
Rename features id to use measureType numbered with a counter
Transform geodesic lines before calling the process and for highlight features

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x", remove the others)

Issue

What is the current behavior?

Fix #9685

What is the new behavior?

arc length as a buffer and blue line highlight

intersection of a buffer with states, (check lower parts should be the dark one

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

Yes, and I documented them in migration notes
No

Other useful information

…nts to it

offtherailz

See comments inline
When I try to select, I have an error and a success notification at the same time.

screencast-github.aaakk.us.kg-2023.12.13-10_35_57.webm
Selection do not look to work as expected.
When I try to select the buffered geometry, I have a crash (the video shows the selection issue above and the crash).

screencast-github.aaakk.us.kg-2023.12.13-10_37_09.webm

Error in console:

StandardRouter.jsx:86 TypeError: Cannot read properties of undefined (reading 'id')
    at createFeatureId (GeoProcessingUtils.js:77:18)
    at eval (SourceLayer.jsx:119:89)
    at Array.map (<anonymous>)
    at Source (SourceLayer.jsx:118:29)
    at renderWithHooks (react-dom.development.js:16320:18)
    at updateFunctionComponent (react-dom.development.js:18284:20)
    at beginWork$1 (react-dom.development.js:20098:16)
    at HTMLUnknownElement.callCallback (react-dom.development.js:362:14)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:411:16)
    at invokeGuardedCallback (react-dom.development.js:466:31)
    at beginWork$$1 (react-dom.development.js:25730:7)
    at performUnitOfWork (react-dom.development.js:24638:12)
    at workLoopSync (react-dom.development.js:24614:22)
    at performSyncWorkOnRoot (react-dom.development.js:24182:11)
    at eval (react-dom.development.js:12238:24)
    at unstable_runWithPriority (scheduler.development.js:815:12) '\n    at ErrorBoundary (webpack://mapstore2/./node_modules/react-error-boundary/dist/commonjs/ErrorBoundary.js?:36:5)\n    at div\n    at Router (webpack://mapstore2/./node_modules/react-router/es/Router.js?:36:5)\n    at ConnectedRouter (webpack://mapstore2/./node_modules/connected-react-router/esm/ConnectedRouter.js?:60:7)\n    at ConnectedRouterWithContext (webpack://mapstore2/./node_modules/connected-react-router/esm/ConnectedRouter.js?:158:25)\n    at Connect (webpack://mapstore2/./node_modules/react-redux/es/components/connectAdvanced.js?:152:37)\n    at IntlProvider (webpack://mapstore2/./node_modules/react-intl/lib/index.es.js?:804:9)\n    at Localized (webpack://mapstore2/./web/client/components/I18N/Localized.jsx?:28:5)\n    at eval (webpack://mapstore2/./web/client/components/theme/Theme.jsx?:42:5)\n    at Connect (webpack://mapstore2/./node_modules/react-redux/es/components/connectAdvanced.js?:152:37)\n    at div\n    at StandardRouter (webpack://mapstore2/./web/client/components/app/StandardRouter.jsx?:51:5)\n    at Connect (webpack://mapstore2/./node_modules/react-redux/es/components/connectAdvanced.js?:152:37)\n    at DragDropContextContainer (webpack://mapstore2/./node_modules/react-dnd/lib/DragDropContext.js?:81:5)\n    at Provider (webpack://mapstore2/./node_modules/react-redux/es/components/Provider.js?:24:24)\n    at StandardApp (webpack://mapstore2/./web/client/components/app/StandardApp.jsx?:78:5)\n    at WithExtensions (webpack://mapstore2/./web/client/components/app/withExtensions.js?:50:7)'

offtherailz · 2023-12-13T09:03:30Z

web/client/reducers/geoProcessing.js

+                features: (state[action.source].features || []).concat(action.data.features || []).filter(f => !f?.properties?.measureId).map((ft, i)  => ({...ft,
+                    id: createFeatureId(ft, i)})),


Please explain why you are filtering out all features that do not have measureId and creating ids for all features. It is not trivial the reason and it is a special behaviour that have to be commented.
I suggest to do this changes outside the reducer. The reducer should expect well formed features.

the main problem was that the vector layer generated from feature measurements are missing ids and it makes some other part of the code to fail. i'll centralize this behaviour in the layers selector of the geoprocessing tool and also in epics

web/client/utils/GeoProcessingUtils.js

offtherailz · 2023-12-13T09:12:21Z

web/client/utils/GeoProcessingUtils.js

+ * @return {object} the transformed feature if geodesic
+ */
+export const transformLineToArc = (feature) => {
+    if (feature?.properties?.geodesic) {


If it handles only linestrings, than do this check before transformation.

what about adding a condition check for the geometry property? doing it external to the function is worse imo

offtherailz · 2023-12-13T09:18:27Z

web/client/epics/geoProcessing.js

@@ -589,12 +603,12 @@ export const runBufferProcessGPTEpic = (action$, store) => action$
                        }));
                    })
        );
-        const feature = sourceFeatureSelector(state);
+        const feature = transformLineToArc(sourceFeatureSelector(state));


The fact that we have to transform the geometry in 8 different points of the code risk to make the feature not maintainable. Try to centralize this behavior in a single place (selector).
Try to do also the normalization once (labels, ids, managemant of measureId etc...). The geodesic tool should be as much transparent as possible to the special case of measure).

sounds good, let me better see this

web/client/utils/GeoProcessingUtils.js

MV88 · 2023-12-14T09:05:17Z

@offtherailz this pr is ready for a new review.
i've tested many use cases as you can see here

measurement.vector.demo.mp4

offtherailz

I'll test it as well as the changes suggested are applied.
Thank you .

offtherailz · 2023-12-14T10:08:40Z

web/client/selectors/geoProcessing.js

@@ -73,5 +74,20 @@ export const wfsBackedLayersSelector = (state) => {
    const layers = layersSelector(state);
    return layers
        .filter(l => l.group !== "background")
-        .filter(layer => hasWFSService(layer) || layer.type === "vector");
+        .filter(layer => hasWFSService(layer) || layer.type === "vector")


rename this with "vectorLayerSelector". I suggest to separate densification where effectively needed, and cache using reselect to avoid to densify features on every state update.

but it does not fetch only vector layers

offtherailz · 2023-12-14T10:09:57Z

web/client/selectors/layers.js

+export const layersSelector = ({layers: layersProp, config} = {}) => {
+    const layers = layersProp && isArray(layersProp) ? layersProp : layersProp && layersProp.flat || config && config.layers || [];
+    // adding foreach feature an id if missing
+    return layers?.map(layer => {
+        return layer?.features?.length ? {
+            ...layer,
+            features: layer?.features
+                ?.map((ft, i) => ({
+                    ...ft,
+                    id: createFeatureId(ft, i)
+
+                }))
+        } : layer;
+    });
+};


This should not be done. It creates temporary ids that are not unique, and it does it on every state update.
Let's save instead the ids where missing on layer add or map load.

MV88 · 2023-12-14T14:32:47Z

@offtherailz review is ready

offtherailz

I'm trying to use this map to select the buffer layer but I see a warning of no feature clicked.

geoprocessing_map.json

screencast-localhost_8081-2023.12.15-10_22_56.webm

same issue if I try to select as vector layer of input natural earth 10m admin data (Can not upload it here).

web/client/epics/widgets.js

offtherailz

👍 works quite fine.
The only things I noticed here:

Circle annotation cause an error because internally is handled as a point
When using big sets of data (e.g. natural earth 10m admin boundaries) the geoprocessing tool slows down a bit. It happens also to the map, until I hide the layer anyway. Probably some optimizations can be done on this side.

Because circle annotation was not strictly a part of this issue and because the performance are anyway acceptable under stress, I think I can approve this PR, that fulfills the requirements and does not introduce any regression.

Please @MV88 and @tdipisa decide what to do with my notes.

Thank you for your development @MV88 now this tool is really powerful.

tdipisa · 2023-12-18T09:32:43Z

@offtherailz thank you

When using big sets of data (e.g. natural earth 10m admin boundaries) the geoprocessing tool slows down a bit. It happens also to the map, until I hide the layer anyway. Probably some optimizations can be done on this side.

it would be good to understand where is the performance issue, if it is on the client side when rendering complex features or on the WPS side.

@MV88 please complete the fix for the first point, please. Then, we can maybe open a dedicated issue for the other one.

offtherailz

🚀 👍 perfect, it works.
With point annotation it fails, but with the correct error from the back-end.

The performance issues are not so big, and the happen only on big vector data (22mb admin 0 from natural earth).

tdipisa · 2023-12-18T13:55:06Z

@ElenaGallo please test in DEV once deployed and let us know for the backport.

ElenaGallo · 2023-12-18T15:59:24Z

Test passed @MV88 please backport to 2023.02.xx. Thanks

…nts to it (geosolutions-it#9809) * Fix geosolutions-it#9685 measurements issues and added some enhancements to it * adding some extra tests * Fixed problems related to measurements and missing ids * refactor and optimization * final fixes for vector layers for the click on map action * fix geodesic circle * Fix tests # Conflicts: # web/client/epics/widgets.js # web/client/reducers/config.js # web/client/utils/LayersUtils.js

MV88 · 2023-12-19T09:43:39Z

@offtherailz backport is here #9835 for you to review

…hancements to it (#9809) (#9835)

MV88 added 2 commits December 12, 2023 11:03

Fix geosolutions-it#9685 measurements issues and added some enhanceme…

a014569

…nts to it

adding some extra tests

b611d55

MV88 added enhancement BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch C265-ATOLCD-2022-SUPPORT labels Dec 12, 2023

MV88 added this to the 2023.02.02 milestone Dec 12, 2023

MV88 requested a review from offtherailz December 12, 2023 10:09

MV88 self-assigned this Dec 12, 2023

offtherailz requested changes Dec 13, 2023

View reviewed changes

Fixed problems related to measurements and missing ids

7972481

MV88 requested a review from offtherailz December 14, 2023 09:05

offtherailz requested changes Dec 14, 2023

View reviewed changes

refactor and optimization

5ad4511

MV88 requested a review from offtherailz December 14, 2023 14:32

offtherailz requested changes Dec 15, 2023

View reviewed changes

web/client/epics/widgets.js Outdated Show resolved Hide resolved

final fixes for vector layers for the click on map action

48789f1

MV88 requested a review from offtherailz December 15, 2023 15:08

offtherailz approved these changes Dec 15, 2023

View reviewed changes

fix geodesic circle

ff0ce61

Fix tests

dbcf660

MV88 requested a review from offtherailz December 18, 2023 13:31

offtherailz approved these changes Dec 18, 2023

View reviewed changes

tdipisa merged commit c9cc31b into geosolutions-it:master Dec 18, 2023
6 checks passed

MV88 mentioned this pull request Dec 19, 2023

backport 2023.02.xx - Fix #9685 measurements issues and added some enhancements to it (#9809) #9835

Merged

offtherailz pushed a commit that referenced this pull request Jan 9, 2024

backport 2023.02.xx - Fix #9685 measurements issues and added some en…

ed5c8a7

…hancements to it (#9809) (#9835)

ElenaGallo removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9685 measurements issues and added some enhancements to it #9809

Fix #9685 measurements issues and added some enhancements to it #9809

MV88 commented Dec 12, 2023 •

edited

Loading

offtherailz left a comment •

edited

Loading

offtherailz Dec 13, 2023

MV88 Dec 13, 2023

offtherailz Dec 13, 2023

MV88 Dec 13, 2023

offtherailz Dec 13, 2023

MV88 Dec 13, 2023

MV88 commented Dec 14, 2023

offtherailz left a comment

offtherailz Dec 14, 2023 •

edited

Loading

MV88 Dec 14, 2023

offtherailz Dec 14, 2023

MV88 commented Dec 14, 2023

offtherailz left a comment

offtherailz left a comment

tdipisa commented Dec 18, 2023 •

edited

Loading

offtherailz left a comment •

edited

Loading

tdipisa commented Dec 18, 2023 •

edited

Loading

ElenaGallo commented Dec 18, 2023

MV88 commented Dec 19, 2023

		features: (state[action.source].features \|\| []).concat(action.data.features \|\| []).filter(f => !f?.properties?.measureId).map((ft, i) => ({...ft,
		id: createFeatureId(ft, i)})),

Fix #9685 measurements issues and added some enhancements to it #9809

Fix #9685 measurements issues and added some enhancements to it #9809

Conversation

MV88 commented Dec 12, 2023 • edited Loading

Description

Issue

Breaking change

Other useful information

offtherailz 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

MV88 commented Dec 14, 2023

offtherailz left a comment

Choose a reason for hiding this comment

offtherailz Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MV88 commented Dec 14, 2023

offtherailz left a comment

Choose a reason for hiding this comment

offtherailz left a comment

Choose a reason for hiding this comment

tdipisa commented Dec 18, 2023 • edited Loading

offtherailz left a comment • edited Loading

Choose a reason for hiding this comment

tdipisa commented Dec 18, 2023 • edited Loading

ElenaGallo commented Dec 18, 2023

MV88 commented Dec 19, 2023

MV88 commented Dec 12, 2023 •

edited

Loading

offtherailz left a comment •

edited

Loading

offtherailz Dec 14, 2023 •

edited

Loading

tdipisa commented Dec 18, 2023 •

edited

Loading

offtherailz left a comment •

edited

Loading

tdipisa commented Dec 18, 2023 •

edited

Loading