From 92393c2e7dc63f8f47465229faa7d865dbab581b Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Wed, 8 Jan 2025 16:31:10 +0100 Subject: [PATCH 1/3] front: set SuggestedOP.pathStepId in formatSuggestedOperationalPoints() Various spots need to check whether a SuggestedOP has a matching PathStep. Ensure the pathStepId field is always set when appropriate. Signed-off-by: Simon Ser --- .../hooks/useSetupItineraryForTrainUpdate.ts | 1 + .../stdcm/hooks/useStdcmResults.ts | 1 + .../pathfinding/hooks/usePathfinding.ts | 1 + front/src/modules/pathfinding/utils.ts | 57 +++++++++++-------- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts b/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts index 5cae2213a28..5fffcbacf51 100644 --- a/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts +++ b/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts @@ -173,6 +173,7 @@ const useSetupItineraryForTrainUpdate = (trainIdToEdit: number) => { ); const suggestedOperationalPoints: SuggestedOP[] = formatSuggestedOperationalPoints( operational_points, + trainSchedule.path, geometry, pathfindingResult.length ); diff --git a/front/src/applications/stdcm/hooks/useStdcmResults.ts b/front/src/applications/stdcm/hooks/useStdcmResults.ts index 0efca49e639..9e1c4318c96 100644 --- a/front/src/applications/stdcm/hooks/useStdcmResults.ts +++ b/front/src/applications/stdcm/hooks/useStdcmResults.ts @@ -106,6 +106,7 @@ const useStdcmResults = ( const suggestedOperationalPoints: SuggestedOP[] = formatSuggestedOperationalPoints( operationalPointsWithMetadata, // Pass the operational points with metadata + [], geometry, path.length ); diff --git a/front/src/modules/pathfinding/hooks/usePathfinding.ts b/front/src/modules/pathfinding/hooks/usePathfinding.ts index 54e3b6b187e..82f517404d8 100644 --- a/front/src/modules/pathfinding/hooks/usePathfinding.ts +++ b/front/src/modules/pathfinding/hooks/usePathfinding.ts @@ -118,6 +118,7 @@ const usePathfinding = ( const suggestedOperationalPoints: SuggestedOP[] = formatSuggestedOperationalPoints( operational_points, + pathStepsInput, geometry, pathResult.length ); diff --git a/front/src/modules/pathfinding/utils.ts b/front/src/modules/pathfinding/utils.ts index 0ede09223d5..cfc9d2a5a02 100644 --- a/front/src/modules/pathfinding/utils.ts +++ b/front/src/modules/pathfinding/utils.ts @@ -16,29 +16,6 @@ import { getPointCoordinates } from 'utils/geometry'; import getStepLocation from './helpers/getStepLocation'; -export const formatSuggestedOperationalPoints = ( - operationalPoints: Array< - NonNullable>[number] & { - metadata?: NonNullable; - } - >, - geometry: GeoJsonLineString, - pathLength: number -): SuggestedOP[] => - operationalPoints.map((op) => ({ - opId: op.id, - name: op.extensions?.identifier?.name, - uic: op.extensions?.identifier?.uic, - ch: op.extensions?.sncf?.ch, - kp: op.part.extensions?.sncf?.kp, - trigram: op.extensions?.sncf?.trigram, - offsetOnTrack: op.part.position, - track: op.part.track, - positionOnPath: op.position, - coordinates: getPointCoordinates(geometry, pathLength, op.position), - metadata: op?.metadata, - })); - export const matchPathStepAndOp = ( step: PathItemLocation, op: Pick @@ -55,6 +32,37 @@ export const matchPathStepAndOp = ( return step.track === op.track && step.offset === op.offsetOnTrack; }; +export const formatSuggestedOperationalPoints = ( + operationalPoints: Array< + NonNullable>[number] & { + metadata?: NonNullable; + } + >, + pathSteps: PathStep[], + geometry: GeoJsonLineString, + pathLength: number +): SuggestedOP[] => + operationalPoints + .map((op) => ({ + opId: op.id, + name: op.extensions?.identifier?.name, + uic: op.extensions?.identifier?.uic, + ch: op.extensions?.sncf?.ch, + kp: op.part.extensions?.sncf?.kp, + trigram: op.extensions?.sncf?.trigram, + offsetOnTrack: op.part.position, + track: op.part.track, + positionOnPath: op.position, + coordinates: getPointCoordinates(geometry, pathLength, op.position), + metadata: op?.metadata, + })) + .map((op) => ({ + ...op, + pathStepId: pathSteps.find( + (pathStep) => matchPathStepAndOp(pathStep, op) && op.kp === pathStep.kp + )?.id, + })); + export const getPathfindingQuery = ({ infraId, rollingStock, @@ -124,10 +132,9 @@ export const upsertPathStepsInOPs = (ops: SuggestedOP[], pathSteps: PathStep[]): } } else { updatedOPs = updatedOPs.map((op) => { - if (matchPathStepAndOp(step, op) && op.kp === step.kp) { + if (step.id === op.pathStepId) { return { ...op, - pathStepId: step.id, stopFor, arrival, receptionSignal, From 67d74ab38a7c8162bf77eb7c4c75a20cd927ffde Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Tue, 10 Dec 2024 12:41:40 +0100 Subject: [PATCH 2/3] front: match PathStep and SuggestedOP with pathStepId Matching via uic/trigram/ch/etc is unreliable because: - Two OPs can have the same trigram/ch. - The same OP could appear multiple times at different spots in a path. Moreover, we had multiple variants of the matching with slight differences. Some of the matching logic got adjusted to account for specific bugs, while others were left alone. Each path step can be identified with a unique ID stored in PathStep.id. Use that instead. Signed-off-by: Simon Ser --- .../hooks/useSetupItineraryForTrainUpdate.ts | 6 ++-- .../Itinerary/ModalSuggestedVias.tsx | 7 ++-- .../pathfinding/hooks/usePathfinding.ts | 10 ++---- front/src/modules/pathfinding/utils.ts | 34 ------------------- .../modules/timesStops/TimesStopsInput.tsx | 7 ++-- front/src/modules/timesStops/helpers/utils.ts | 21 ++---------- front/src/reducers/osrdconf/helpers.ts | 3 +- 7 files changed, 15 insertions(+), 73 deletions(-) diff --git a/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts b/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts index 5fffcbacf51..4e10d01d7e9 100644 --- a/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts +++ b/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts @@ -13,7 +13,7 @@ import { } from 'common/api/osrdEditoastApi'; import { useOsrdConfActions } from 'common/osrdContext'; import buildOpSearchQuery from 'modules/operationalPoint/helpers/buildOpSearchQuery'; -import { formatSuggestedOperationalPoints, matchPathStepAndOp } from 'modules/pathfinding/utils'; +import { formatSuggestedOperationalPoints } from 'modules/pathfinding/utils'; import { getSupportedElectrification, isThermal } from 'modules/rollingStock/helpers/electric'; import type { SuggestedOP } from 'modules/trainschedule/components/ManageTrainSchedule/types'; import computeBasePathStep from 'modules/trainschedule/helpers/computeBasePathStep'; @@ -39,8 +39,8 @@ export function updatePathStepsFromOperationalPoints( stepsCoordinates: Position[] ) { const updatedPathSteps: PathStep[] = pathSteps.map((step, i) => { - const correspondingOp = suggestedOperationalPoints.find((suggestedOp) => - matchPathStepAndOp(step, suggestedOp) + const correspondingOp = suggestedOperationalPoints.find( + (suggestedOp) => step.id === suggestedOp.pathStepId ); const { kp, name } = correspondingOp || step; diff --git a/front/src/modules/pathfinding/components/Itinerary/ModalSuggestedVias.tsx b/front/src/modules/pathfinding/components/Itinerary/ModalSuggestedVias.tsx index b4f8d8422a6..8ce30fae5e2 100644 --- a/front/src/modules/pathfinding/components/Itinerary/ModalSuggestedVias.tsx +++ b/front/src/modules/pathfinding/components/Itinerary/ModalSuggestedVias.tsx @@ -9,7 +9,6 @@ import ModalBodySNCF from 'common/BootstrapSNCF/ModalSNCF/ModalBodySNCF'; import ModalFooterSNCF from 'common/BootstrapSNCF/ModalSNCF/ModalFooterSNCF'; import ModalHeaderSNCF from 'common/BootstrapSNCF/ModalSNCF/ModalHeaderSNCF'; import { useOsrdConfActions, useOsrdConfSelectors } from 'common/osrdContext'; -import { isVia, matchPathStepAndOp } from 'modules/pathfinding/utils'; import type { SuggestedOP } from 'modules/trainschedule/components/ManageTrainSchedule/types'; import type { OperationalStudiesConfSliceActions } from 'reducers/osrdconf/operationalStudiesConf'; import type { PathStep } from 'reducers/osrdconf/types'; @@ -37,12 +36,12 @@ const ModalSuggestedVias = ({ suggestedVias, launchPathfinding }: ModalSuggested ); const removeViaFromPath = (op: SuggestedOP) => { - const newPathSteps = pathSteps.filter((step) => !matchPathStepAndOp(step!, op)); + const newPathSteps = pathSteps.filter((step) => step!.id !== op.pathStepId); launchPathfinding(newPathSteps); }; const formatOP = (op: SuggestedOP, idx: number, idxTrueVia: number) => { - const isInVias = isVia(vias, op); + const isInVias = vias.find((step) => step.id === op.pathStepId); return (
{ if (!isOriginOrDestination(via)) { // If name is undefined, we know the op/via has been added by clicking on map - if (isVia(vias, via)) idxTrueVia += 1; + if (vias.find((step) => step.id === via.pathStepId)) idxTrueVia += 1; return formatOP(via, idx, idxTrueVia); } return null; diff --git a/front/src/modules/pathfinding/hooks/usePathfinding.ts b/front/src/modules/pathfinding/hooks/usePathfinding.ts index 82f517404d8..f21ccb23d5f 100644 --- a/front/src/modules/pathfinding/hooks/usePathfinding.ts +++ b/front/src/modules/pathfinding/hooks/usePathfinding.ts @@ -14,11 +14,7 @@ import type { } from 'common/api/osrdEditoastApi'; import { osrdEditoastApi } from 'common/api/osrdEditoastApi'; import { useOsrdConfActions, useOsrdConfSelectors } from 'common/osrdContext'; -import { - formatSuggestedOperationalPoints, - getPathfindingQuery, - matchPathStepAndOp, -} from 'modules/pathfinding/utils'; +import { formatSuggestedOperationalPoints, getPathfindingQuery } from 'modules/pathfinding/utils'; import { useStoreDataForRollingStockSelector } from 'modules/rollingStock/components/RollingStockSelector/useStoreDataForRollingStockSelector'; import type { SuggestedOP } from 'modules/trainschedule/components/ManageTrainSchedule/types'; import { setFailure, setWarning } from 'reducers/main'; @@ -126,8 +122,8 @@ const usePathfinding = ( // We update existing pathsteps with coordinates, positionOnPath and kp corresponding to the new pathfinding result const updatedPathSteps: (PathStep | null)[] = pathStepsInput.map((step, i) => { if (!step) return step; - const correspondingOp = suggestedOperationalPoints.find((suggestedOp) => - matchPathStepAndOp(step, suggestedOp) + const correspondingOp = suggestedOperationalPoints.find( + (suggestedOp) => step.id === suggestedOp.pathStepId ); const theoreticalMargin = i === 0 ? step.theoreticalMargin || '0%' : step.theoreticalMargin; diff --git a/front/src/modules/pathfinding/utils.ts b/front/src/modules/pathfinding/utils.ts index cfc9d2a5a02..5964fcce03a 100644 --- a/front/src/modules/pathfinding/utils.ts +++ b/front/src/modules/pathfinding/utils.ts @@ -148,40 +148,6 @@ export const upsertPathStepsInOPs = (ops: SuggestedOP[], pathSteps: PathStep[]): return updatedOPs; }; -export const pathStepMatchesOp = ( - pathStep: PathStep, - op: Pick< - SuggestedOP, - 'pathStepId' | 'opId' | 'uic' | 'ch' | 'trigram' | 'track' | 'offsetOnTrack' | 'name' | 'kp' - >, - withKP = false -) => { - if (!matchPathStepAndOp(pathStep, op)) { - return pathStep.id === op.pathStepId; - } - if ('uic' in pathStep) { - return withKP ? pathStep.kp === op.kp : pathStep.name === op.name; - } - return true; -}; - -/** - * Check if a suggested operational point is a via. - * Some OPs have same uic so we need to check also the ch (can be still not enough - * probably because of imports problem). - * If the vias has no uic, it has been added via map click and we know it has an id. - * @param withKP - If true, we check the kp compatibility instead of the name. - * It is used in the times and stops table to check if an operational point is a via. - */ -export const isVia = ( - vias: PathStep[], - op: Pick< - SuggestedOP, - 'pathStepId' | 'opId' | 'uic' | 'ch' | 'trigram' | 'track' | 'offsetOnTrack' | 'name' | 'kp' - >, - { withKP = false } = {} -) => vias.some((via) => pathStepMatchesOp(via, op, withKP)); - export const isStation = (chCode: string): boolean => chCode === 'BV' || chCode === '00' || chCode === ''; diff --git a/front/src/modules/timesStops/TimesStopsInput.tsx b/front/src/modules/timesStops/TimesStopsInput.tsx index f384f8b908f..e720f3e018b 100644 --- a/front/src/modules/timesStops/TimesStopsInput.tsx +++ b/front/src/modules/timesStops/TimesStopsInput.tsx @@ -7,7 +7,6 @@ import { useTranslation } from 'react-i18next'; import { useScenarioContext } from 'applications/operationalStudies/hooks/useScenarioContext'; import { useOsrdConfActions } from 'common/osrdContext'; -import { isVia, matchPathStepAndOp } from 'modules/pathfinding/utils'; import type { SuggestedOP } from 'modules/trainschedule/components/ManageTrainSchedule/types'; import type { OperationalStudiesConfSliceActions } from 'reducers/osrdconf/operationalStudiesConf'; import type { PathStep } from 'reducers/osrdconf/types'; @@ -43,7 +42,7 @@ const createClearViaButton = ({ pathStepsAndSuggestedOPs && rowIndex > 0 && rowIndex < pathStepsAndSuggestedOPs.length - 1 && - isVia(pathSteps || [], rowData, { withKP: true }) && + pathSteps.find((step) => step.id === rowData.pathStepId) && (!isNil(rowData.stopFor) || rowData.theoreticalMargin !== undefined || rowData.arrival !== undefined || @@ -78,9 +77,7 @@ const TimesStopsInput = ({ const { getTrackSectionsByIds, trackSectionsLoading } = useScenarioContext(); const clearPathStep = (rowData: TimesStopsInputRow) => { - const index = pathSteps.findIndex( - (step) => matchPathStepAndOp(step, rowData) && step.positionOnPath === rowData.positionOnPath - ); + const index = pathSteps.findIndex((step) => step.id === rowData.pathStepId); const updatedPathSteps = pathSteps.map((step, i) => { if (i === index) { diff --git a/front/src/modules/timesStops/helpers/utils.ts b/front/src/modules/timesStops/helpers/utils.ts index 7435a702463..3f0216c4bbb 100644 --- a/front/src/modules/timesStops/helpers/utils.ts +++ b/front/src/modules/timesStops/helpers/utils.ts @@ -6,7 +6,6 @@ import { keyColumn, createTextColumn } from 'react-datasheet-grid'; import type { ReceptionSignal } from 'common/api/osrdEditoastApi'; import type { IsoDurationString, TimeString } from 'common/types'; -import { matchPathStepAndOp } from 'modules/pathfinding/utils'; import type { SuggestedOP } from 'modules/trainschedule/components/ManageTrainSchedule/types'; import type { PathStep } from 'reducers/osrdconf/types'; import { NO_BREAK_SPACE } from 'utils/strings'; @@ -24,18 +23,6 @@ import { import { marginRegExValidation, MarginUnit } from '../consts'; import { TableType, type TimeExtraDays, type TimesStopsInputRow } from '../types'; -const matchPathStepAndOpWithKP = (step: PathStep, op: SuggestedOP) => { - if (!matchPathStepAndOp(step, op)) { - return step.id === op.pathStepId; - } - // We match the kp in case two OPs have the same uic+ch (can happen when the - // infra is imported) - if ('uic' in step || 'trigram' in step) { - return step.kp === op.kp; - } - return true; -}; - export const formatSuggestedViasToRowVias = ( operationalPoints: SuggestedOP[], pathSteps: PathStep[], @@ -49,7 +36,7 @@ export const formatSuggestedViasToRowVias = ( // to move it to the first position const origin = pathSteps[0]; const originIndexInOps = origin - ? operationalPoints.findIndex((op) => matchPathStepAndOpWithKP(origin, op)) + ? operationalPoints.findIndex((op) => origin.id === op.pathStepId) : -1; if (originIndexInOps !== -1) { [formattedOps[0], formattedOps[originIndexInOps]] = [ @@ -60,9 +47,7 @@ export const formatSuggestedViasToRowVias = ( // Ditto: destination should be last const dest = pathSteps[pathSteps.length - 1]; - const destIndexInOps = dest - ? operationalPoints.findIndex((op) => matchPathStepAndOpWithKP(dest, op)) - : -1; + const destIndexInOps = dest ? operationalPoints.findIndex((op) => dest.id === op.pathStepId) : -1; if (destIndexInOps !== -1) { const lastOpIndex = formattedOps.length - 1; [formattedOps[lastOpIndex], formattedOps[destIndexInOps]] = [ @@ -72,7 +57,7 @@ export const formatSuggestedViasToRowVias = ( } return formattedOps.map((op, i) => { - const pathStep = pathSteps.find((step) => matchPathStepAndOpWithKP(step, op)); + const pathStep = pathSteps.find((step) => step.id === op.pathStepId); const { name } = pathStep || op; const objectToUse = tableType === TableType.Input ? pathStep : op; diff --git a/front/src/reducers/osrdconf/helpers.ts b/front/src/reducers/osrdconf/helpers.ts index 0af704eec02..63340c503cf 100644 --- a/front/src/reducers/osrdconf/helpers.ts +++ b/front/src/reducers/osrdconf/helpers.ts @@ -4,7 +4,6 @@ import nextId from 'react-id-generator'; import { calculateDistanceAlongTrack } from 'applications/editor/tools/utils'; import type { ManageTrainSchedulePathProperties } from 'applications/operationalStudies/types'; -import { pathStepMatchesOp } from 'modules/pathfinding/utils'; import type { SuggestedOP } from 'modules/trainschedule/components/ManageTrainSchedule/types'; import { addElementAtIndex } from 'utils/array'; @@ -76,7 +75,7 @@ export function upsertPathStep(statePathSteps: (PathStep | null)[], op: Suggeste }), }; - const stepIndex = cleanPathSteps.findIndex((step) => pathStepMatchesOp(step, op)); + const stepIndex = cleanPathSteps.findIndex((step) => step.id === op.pathStepId); if (stepIndex >= 0) { // Because of import issues, there can be multiple ops with same position on path // To avoid updating the wrong one, we need to find the one that matches the payload From a0d75cf85c1030da81d14a23ebbff888c028f926 Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Wed, 8 Jan 2025 17:08:51 +0100 Subject: [PATCH 3/3] wip --- .../useSetupItineraryForTrainUpdate.spec.ts | 5 +- front/src/modules/pathfinding/utils.ts | 48 +++++++++++-------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts b/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts index 785044bb175..692af2a5dfc 100644 --- a/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts +++ b/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest'; import type { PathfindingResult } from 'common/api/osrdEditoastApi'; +import { populatePathStepIdInSuggestedOPs } from 'modules/pathfinding/utils'; import { updatePathStepsFromOperationalPoints } from '../useSetupItineraryForTrainUpdate'; @@ -177,7 +178,7 @@ describe('updatePathStepsFrom', () => { ]; const result = updatePathStepsFromOperationalPoints( pathSteps, - suggestedOpPoints, + populatePathStepIdInSuggestedOPs(suggestedOpPoints, pathSteps), pathFindingResult as Extract, stepsCoordinates ); @@ -253,7 +254,7 @@ describe('updatePathStepsFrom', () => { ]; const result = updatePathStepsFromOperationalPoints( pathSteps, - suggestedOpPoints, + populatePathStepIdInSuggestedOPs(suggestedOpPoints, pathSteps), pathFindingResult as Extract, stepsCoordinates ); diff --git a/front/src/modules/pathfinding/utils.ts b/front/src/modules/pathfinding/utils.ts index 5964fcce03a..e338d138403 100644 --- a/front/src/modules/pathfinding/utils.ts +++ b/front/src/modules/pathfinding/utils.ts @@ -32,6 +32,17 @@ export const matchPathStepAndOp = ( return step.track === op.track && step.offset === op.offsetOnTrack; }; +export const populatePathStepIdInSuggestedOPs = ( + suggestedOPs: SuggestedOP[], + pathSteps: PathStep[] +): SuggestedOP[] => + suggestedOPs.map((op) => ({ + ...op, + pathStepId: pathSteps.find( + (pathStep) => matchPathStepAndOp(pathStep, op) // TODO: && op.kp === pathStep.kp + )?.id, + })); + export const formatSuggestedOperationalPoints = ( operationalPoints: Array< NonNullable>[number] & { @@ -41,27 +52,22 @@ export const formatSuggestedOperationalPoints = ( pathSteps: PathStep[], geometry: GeoJsonLineString, pathLength: number -): SuggestedOP[] => - operationalPoints - .map((op) => ({ - opId: op.id, - name: op.extensions?.identifier?.name, - uic: op.extensions?.identifier?.uic, - ch: op.extensions?.sncf?.ch, - kp: op.part.extensions?.sncf?.kp, - trigram: op.extensions?.sncf?.trigram, - offsetOnTrack: op.part.position, - track: op.part.track, - positionOnPath: op.position, - coordinates: getPointCoordinates(geometry, pathLength, op.position), - metadata: op?.metadata, - })) - .map((op) => ({ - ...op, - pathStepId: pathSteps.find( - (pathStep) => matchPathStepAndOp(pathStep, op) && op.kp === pathStep.kp - )?.id, - })); +): SuggestedOP[] => { + const suggestedOPs = operationalPoints.map((op) => ({ + opId: op.id, + name: op.extensions?.identifier?.name, + uic: op.extensions?.identifier?.uic, + ch: op.extensions?.sncf?.ch, + kp: op.part.extensions?.sncf?.kp, + trigram: op.extensions?.sncf?.trigram, + offsetOnTrack: op.part.position, + track: op.part.track, + positionOnPath: op.position, + coordinates: getPointCoordinates(geometry, pathLength, op.position), + metadata: op?.metadata, + })); + return populatePathStepIdInSuggestedOPs(suggestedOPs, pathSteps); +}; export const getPathfindingQuery = ({ infraId,