teksi / wastewater

[DEV] Future TEKSI wastewater module, adapted data model to fit VSA-DSS 2020 new standard
https://teksi.github.io/wastewater
GNU General Public License v3.0
0 stars 5 forks source link

reachpoint level recalculated when reachpoint moves #312

Open urskaufmann opened 1 month ago

urskaufmann commented 1 month ago

Describe the bug Reach with correct levels (I1 = 2736.51). I move the reachpoint with snapping, then the level is recalculated to 0.00 if there is snapping to a detail geometry or recalculated to the point-level if it snaps to a point (example: the cover).

To Reproduce Exact steps to reproduce the behavior:

  1. Use the Vertex tool
  2. Move the end-point of a reach (reach-point) to a place, where it snaps
  3. Control the reach-point level

Expected behavior automatic levels if there exists already a level for a reachpoint will be often not wanted. If the new level is 0, then there shoul be never a change. If there is already a level and the new level changes, then the user should confirm the new level.

Screenshots / data Start with I1=2736.51 image

after moving on the detail geometry: level is set to 0.00! image

after moving to symbol = node=cover: level is equal to cover level (without confirm!) image

If I move without snapping, the level does not change (as expected).

Desktop (please complete the following information):

urskaufmann commented 1 month ago

It also recalculates, if I move another vertex of the reach!

But if I change also snaping on the from reachpoint side, also the from level is reset.

If I move to to-reachpoint away from the detail geometry and let let it snap on his own reach (shorten the line), the there is a very strange now reachpoint level (could be interpolated from vertex level = 0 and old to-reachpoint-level = 2736.51). HELP - do not interpolated or calculate if there is a 0 value!!

ponceta commented 1 month ago

There's something broken in the synchronisation between reach points altitudes and reach geometries.

image

If you edit the altitude of the reach point the label is correctly recalculated.

cymed commented 1 month ago

The problem is that the progression3d_geometry snaps in 3d, which in turn is used in in tww_app.ft_vw_tww_reach_update(). We update the reach's progression3d_geometry as follows

      -- Synchronize geometry with level
      IF NEW.rp_from_level <> OLD.rp_from_level OR (NEW.rp_from_level IS NULL AND OLD.rp_from_level IS NOT NULL) OR (NEW.rp_from_level IS NOT NULL AND OLD.rp_from_level IS NULL) THEN
        NEW.progression3d_geometry = ST_ForceCurve(ST_SetPoint(ST_CurveToLine(NEW.progression3d_geometry),0,
        ST_MakePoint(ST_X(ST_StartPoint(NEW.progression3d_geometry)),ST_Y(ST_StartPoint(NEW.progression3d_geometry)),COALESCE(NEW.rp_from_level,'NaN'))));
      ELSE
        IF ST_Z(ST_StartPoint(NEW.progression3d_geometry)) <> ST_Z(ST_StartPoint(OLD.progression3d_geometry)) THEN
          NEW.rp_from_level = NULLIF(ST_Z(ST_StartPoint(NEW.progression3d_geometry)),'NaN');
        END IF;
      END IF;

    [same procedure with rp_to_level]

    UPDATE tww_od.reach_point SET
          [...]
          , situation3d_geometry = ST_StartPoint(NEW.progression3d_geometry)
        WHERE obj_id = OLD.rp_from_obj_id;

   [same procedure with rp_to_obj_id]

So if the rp_level did not change and the progession3d_geometry snapped to a wrong 3d Z coordinate, that Z coordinate is transferred to rp_level and rp_situation3d_geometry. The question is in which cases we want that z matching

  1. If the rp_level is zero
  2. If the rp_level is NULL
  3. If the rp_level is distinct from the reach's Z value in general

I suggest either 1 and 2

      -------------------------------------
      -- Synchronize geometry with level --
      -------------------------------------

      -- Start point
      IF NEW.rp_from_level IS NULL OR NEW.rp_from_level=0 THEN
        NEW.rp_from_level = NULLIF(ST_Z(ST_StartPoint(NEW.progression3d_geometry)),NEW.rp_from_level);
      ELSE NULL;
      END IF;
      IF NEW.rp_from_level IS DISTINCT FROM ST_Z(ST_StartPoint(NEW.progression3d_geometry)) THEN
        NEW.progression3d_geometry = ST_ForceCurve(ST_SetPoint(ST_CurveToLine(NEW.progression3d_geometry),0,
        ST_MakePoint(ST_X(ST_StartPoint(NEW.progression3d_geometry)),ST_Y(ST_StartPoint(NEW.progression3d_geometry)),COALESCE(NEW.rp_from_level,'NaN'))));
      ELSE NULL;
      END IF;

      -- End point
     IF NEW.rp_to_level IS NULL OR NEW.rp_to_level=0 THEN
        NEW.rp_to_level = NULLIF(ST_Z(ST_EndPoint(NEW.progression3d_geometry)),NEW.rp_to_level);
     ELSE NULL;
     END IF;
     IF NEW.rp_to_level IS DISTINCT FROM ST_Z(ST_EndPoint(NEW.progression3d_geometry)) THEN
        NEW.progression3d_geometry = ST_ForceCurve(ST_SetPoint(ST_CurveToLine(NEW.progression3d_geometry),0,
        ST_MakePoint(ST_X(ST_EndPoint(NEW.progression3d_geometry)),ST_Y(ST_EndPoint(NEW.progression3d_geometry)),COALESCE(NEW.rp_from_level,'NaN'))));
      ELSE NULL;
      END IF;

or not at all

      -------------------------------------
      -- Synchronize geometry with level --
      -------------------------------------

      -- Start point
      IF NEW.rp_from_level IS DISTINCT FROM ST_Z(ST_StartPoint(NEW.progression3d_geometry)) THEN
        NEW.progression3d_geometry = ST_ForceCurve(ST_SetPoint(ST_CurveToLine(NEW.progression3d_geometry),0,
        ST_MakePoint(ST_X(ST_StartPoint(NEW.progression3d_geometry)),ST_Y(ST_StartPoint(NEW.progression3d_geometry)),COALESCE(NEW.rp_from_level,'NaN'))));
      ELSE NULL;
      END IF;

      -- End point
     IF NEW.rp_to_level IS DISTINCT FROM ST_Z(ST_EndPoint(NEW.progression3d_geometry)) THEN
        NEW.progression3d_geometry = ST_ForceCurve(ST_SetPoint(ST_CurveToLine(NEW.progression3d_geometry),0,
        ST_MakePoint(ST_X(ST_EndPoint(NEW.progression3d_geometry)),ST_Y(ST_EndPoint(NEW.progression3d_geometry)),COALESCE(NEW.rp_from_level,'NaN'))));
      ELSE NULL;
      END IF;

Does anyone use snapping on 3d nodes to set the z coordinate of reach points?

ponceta commented 4 weeks ago

In Pully we snap on nodes and construction points to get the 3D information.

cymed commented 4 weeks ago

Ok, so we need to switch it (overriding 0 and NaN elevations on start and End points).

Suggestion:

      -- Synchronize geometry with level
      IF 
        NEW.rp_from_level IS DISTINCT FROM OLD.rp_from_level  OR -- update when changing manually
        NULLIF(ST_Z(ST_StartPoint(NEW.progression3d_geometry)),'NaN') IS NULL OR
        ST_Z(ST_StartPoint(NEW.progression3d_geometry)) = 0  
      THEN
        NEW.progression3d_geometry = ST_ForceCurve(ST_SetPoint(ST_CurveToLine(NEW.progression3d_geometry),0,
       ST_MakePoint(ST_X(ST_StartPoint(NEW.progression3d_geometry)),ST_Y(ST_StartPoint(NEW.progression3d_geometry)),COALESCE(NEW.rp_from_level,'NaN'))));
      ELSE
        NULL;
      END IF;

      IF ST_Z(ST_StartPoint(NEW.progression3d_geometry)) <> ST_Z(ST_StartPoint(OLD.progression3d_geometry)) THEN
          NEW.rp_from_level = NULLIF(ST_Z(ST_StartPoint(NEW.progression3d_geometry)),'NaN');
      ELSE
        NULL;
      END IF;

    [same procedure with rp_to_level]

    UPDATE tww_od.reach_point SET
          [...]
          , situation3d_geometry = ST_StartPoint(NEW.progression3d_geometry)
        WHERE obj_id = OLD.rp_from_obj_id;

   [same procedure with rp_to_obj_id]

In @urskaufmann 's example, the rp_level would not be set to 0 after snapping to the detail_geometry3d_geometry, but it would take the cover elevation (we have to leave it to the user to know which layer he is snapping to - snapping to the wastewater node instead can make sense)

@urskaufmann @ponceta are you ok with that?

urskaufmann commented 4 weeks ago

Means this: if I want to correct my data, which is "Pickellochmodell" (the reachpoint.level is 492.00) and I draw a detail-geometry, and I now snap the reach to the detail-geometry (no more Pickelloch, the reach ends at the wall of the structure), then the reachpoint.level will be set to the cover.level (496.2)? Without remark or question to the user? I do not agree. I think if a value other than 0 or NaN will be recalculated, then the user should decide, if the level changes or not.

cymed commented 4 weeks ago

Means this: if I want to correct my data, which is "Pickellochmodell" (the reachpoint.level is 492.00) and I draw a detail-geometry, and I now snap the reach to the detail-geometry (no more Pickelloch, the reach ends at the wall of the structure), then the reachpoint.level will be set to the cover.level (496.2)? Without remark or question to the user? I do not agree. I think if a value other than 0 or NaN will be recalculated, then the user should decide, if the level changes or not.

No.

Reachpoint.level was set to cover.level in your test because you snapped an existing reach to to a cover, which is a PointZ that had a Z value. If you snap to a detail geometry segment, QGIS snaps to it in 2d and sets z=0. (although that might change upstream in the future as it is an undesired snapping behaviour) If you snap to a detail geometry vertex, QGIS snaps to it in 3d and sets z to the node's Z.

With my change, Z=0 and Z=NaN from the reach geometry are ignored on the new trigger.

@ponceta do you snap on PointZ on creation only or also on update? The simplest solution would be to use the reach Z values on insert only and once the reach is created, we control the start and end point elevation from the rp_level fields