core: handle consist changes by Sh099078 · Pull Request #15992 · OpenRailAssociation/osrd

Sh099078 · 2026-03-30T08:45:16Z

No description provided.

Erashin

Huge PR, thanks for the work.
Was kinda hard to review ngl, this is dense, I'll need to re-review it later on when you've fixed the conflicts/e2e tests/integration tests. Reviewing it with you both in a call would probably help :)
Noting it right now just so we don't forget later on: we'll need to add a description to the PR and modify the commit body/header.

Erashin · 2026-04-01T09:48:50Z

core/src/main/java/fr/sncf/osrd/cli/BenchSTDCM.kt

+                            it.copy(
+                                supportedSignalingSystems =
+                                    it.supportedSignalingSystems.filter {
+                                        // Ignoring ETCS as it is not (yet) supported for


for STDCM

Erashin · 2026-04-01T12:41:45Z

core/src/main/kotlin/fr/sncf/osrd/api/CommonSchemas.kt

        }
        return distanceRangeMapOf(rangeMapEntries)
    }
+


nit: A method description to specify this fails if the distance shifts the boundaries to negative values could be nice.

Erashin · 2026-04-01T12:44:43Z

core/src/main/kotlin/fr/sncf/osrd/api/stdcm/STDCMEndpoint.kt

+                    it.copy(
+                        supportedSignalingSystems =
+                            it.supportedSignalingSystems.filter {
+                                // Ignoring ETCS as it is not (yet) supported for


for STDCM

Erashin · 2026-04-01T12:47:19Z

core/src/main/kotlin/fr/sncf/osrd/api/stdcm/STDCMEndpoint.kt

-                generateDebugData(infra, path, simulationResponse, departureTime, requirements)
-            )
-        }
+    companion object {


I might not understand the intricacies of a companion object, but what's the point of moving those private methods into a companion object exactly? Especially since we don't really use them elsewhere it seems?

of a companion object, but what's the point of moving those private methods into a companion object exactly

They aren't private anymore, and being in the companion object, you can think of them as like static methods in java here. They could have been free functions to tbh.

The main use is they are useful for testing/debugging, but not easily usable when you need to build the whole endpoint. We do have an @disabled test that showcases this.

Erashin · 2026-04-01T13:03:19Z

core/src/main/kotlin/fr/sncf/osrd/conflicts/IncrementalRequirementEnvelopeAdapter.kt

+        }

-        val criticalPoint = end + rollingStock.length
+        val criticalPoint = end + (rollingStock.length)


Think you forgot to remove the parentheses.

Erashin · 2026-04-01T13:24:14Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt

-                        endAtStop(),
-                    )
-                )
+        val spacingRequirementAutomaton = spacingRequirementAutomaton.clone().updateCallbacks(null)


Ok I feel like this should be a bug since behavior changes. I know this method is used in PostProcessingSimulation.findConflictOffsets. Are callbacks not being used at all there? If so, what's actually the point of the method updateCallbacks, since its only use is in this method?

Turns out, in every use of the callbacks (aka. spacingRequirementAutomaton.processUpdate()) in the infra explorer, we update the callbacks just before. So we don't need to have callbacks ready here.

tbh, the callbacks should probably just be a parameter to processUpdate()

Erashin · 2026-04-01T13:28:50Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt

+        val previousStepPos = 0.meters
+        return distanceRangeMapOf(
+            getStepTracker()
+                .iterateSeenStepsBackwards()


It seems we're iterating backwards on the seen steps only to reverse them further along. We should probably use getSeenSteps directly.

Erashin · 2026-04-01T13:31:39Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt

+                .plus(
+                    run {
+                        val lastStep =
+                            getStepTracker().iterateSeenStepsBackwards().lastOrNull { it.isPlanned }


This isn't the last step since you're using iterateSeenStepsBackwards, this is the first planned step. That might be what you want, I don't quite understand this part of the code, but it doesn't match the name of the variable.

Erashin · 2026-04-01T13:32:06Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt

+                    val stepPos = step.travelledPathOffset.distance
+                    DistanceRangeMap.RangeMapEntry(previousStepPos, stepPos, rollingStock)
+                }
+                .plus(


I need some more context here: what are we actually doing with the plus() here? Or aiming to do?

Erashin · 2026-04-01T13:41:27Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/StepTracker.kt

    }

+    /** Get the index of the current reached step */
+    fun getCurrentReachedStep(): Int {


Ok so, for your case, I'm guessing you want the reached step excluding overtakes because you want it to determine which rolling stock to use. However, that means the name of the method and its implementation are not on phase. getCurrentReachedStep (which should probably be renamed to getCurrentReachedStepIndex) should just be returning nSeenStepsExcludingLookahead - 1.
If we want to keep the method implementation as is, we should probably rename it to something lik getCurrentReachedPlannedStepIndex (which is long yeah, but does fit the implementation better).

Co-authored-by: Younes Khoudli <younes.khoudli@epita.fr> Signed-off-by: Loup Federico <16464925+Sh099078@users.noreply.github.com>

eckter

Impressive, good job.

It adds so much complexity though (as expected). I feel like with these changes we need a lot more precise unit tests, and some classes could be split up or refactored.

Hopefully someday we'll have to time to work on #15685 and restructure all this.

Note: I haven't carefully checked all the places where there could be off-by-ones.

core/src/main/kotlin/fr/sncf/osrd/api/stdcm/STDCMEndpoint.kt

eckter · 2026-04-01T13:10:08Z

core/src/main/kotlin/fr/sncf/osrd/api/RollingStockParser.kt

+                val constraint =
+                    CachedBlockConstraintCombiner(
+                        initConstraints(infra, rollingStock, allowedTrackSections)
+                    )


Using getCachedConstraintCombiner would mean we could share identical constraint caches, across requests but also within one request with consist changes

eckter · 2026-04-01T13:13:53Z

core/src/main/kotlin/fr/sncf/osrd/api/RollingStockParser.kt

+/**
+ * Associates a list of rolling stocks with their related pathfinding constraints. This class
+ * provides two ways to be built:
+ * - From a list of STDCM query inputs.
+ * - From a list of rolling stocks and their pathfinding constraints. This approach is mostly useful
+ *   for testing purposes.
+ */
+data class ConsistSchedule(
+    val rollingStocks: List<PhysicsRollingStock>,
+    val constraints: List<PathfindingConstraint>?,
+) {


The docstring could explain that it has one entry per path step (even when it doesn't change). I didn't understand that immediately. Especially since we should actually need n-1 consist entries and not n, though I guess it can make the code more straightforward.

The class could have its own file.

core/src/main/kotlin/fr/sncf/osrd/api/stdcm/STDCMEndpoint.kt

eckter · 2026-04-01T13:49:55Z

core/src/main/kotlin/fr/sncf/osrd/pathfinding/Pathfinding.kt

                    location,
                    steps = steps,
-                    constraints = constraints,
+                    constraints = constraints?.let { MutableList(waypoints.size) { _ -> it } },


For non-STDCM pathfinding, this can result in very long lists. Imported trains can have several hundred waypoints.

I'll keep doing the review with this in mind to see if it would cause issues

core/src/main/kotlin/fr/sncf/osrd/stdcm/graph/PostProcessingSimulation.kt

eckter · 2026-04-01T14:06:24Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/graph/PostProcessingSimulation.kt

+                .map { it.copy(offset = it.offset - currentOffset, time = it.time - currentTime) }
+                .filter { it.offset.meters <= envelope.endPos && it.offset.meters > 0 }
+        val ranges = makeAllowanceRanges(envelope, shiftedFixedPoints)
+        if (ranges.isEmpty()) finalEnvelopes.add(envelope)


This condition should also skip the rest of the loop.

Which also means we lack unit tests here, this part of the code is really hard to debug on real-life scenarios, when the bugs aren't caught by precise unit tests.

(Yeah we already lack tests there, it would just be worse with the added complexity)

So, I wrote a test where we have no fixed point, hoping it would trigger the bug... And it didn't.

Looking at makeAllowanceRanges():

/** Create the list of `AllowanceRange`, with the given fixed points */ private fun makeAllowanceRanges( envelope: Envelope, fixedPoints: Collection<FixedTimePoint>, ): List<AllowanceRange> { var transition = 0.0 var transitionTime = 0.0 var prevAddedTime = 0.0 val res = ArrayList<AllowanceRange>() for (point in fixedPoints) { val baseTime = envelope.interpolateArrivalAtClamp(point.offset.meters) - envelope.interpolateDepartureFromClamp(transition) val pointArrivalTime = transitionTime + baseTime val neededDelay = max(0.0, point.time - pointArrivalTime - prevAddedTime) res.add( AllowanceRange(transition, point.offset.meters, AllowanceValue.FixedTime(neededDelay)) ) prevAddedTime += neededDelay transitionTime += baseTime + (point.stopTime ?: 0.0) transition = point.offset.meters } if (transition < envelope.endPos) res.add(AllowanceRange(transition, envelope.endPos, AllowanceValue.FixedTime(0.0))) return res }

Either we enter the loop, and we add a range. Or we don't, but then transition == 0.0 and we add a range unless we are dealing with a zero-sized envelope.

Trying to add a zero-sized enveloppe to the test, this breaks again... Because we try to create zero-sized SubPhysicsPath.

Anyways, I now believe the proper fix is to just assert that we do have at least one AllowanceRange, even if it's one where we don't loose any time.

Fix and test here: a1d2f45

eckter · 2026-04-01T14:06:56Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/graph/PostProcessingSimulation.kt

@@ -428,29 +444,55 @@ private fun getUpdatedExplorer(
 * set to true, the allowances follow the mareco distribution (more accurate but less reliable).
 */
 private fun runSimulationWithFixedPoints(


The function could maybe have been simpler if we kept the old one and wrapped it in a loop

eckter · 2026-04-01T14:08:36Z

core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt

+    override fun getFullRollingStockRangeMap(): DistanceRangeMap<PhysicsRollingStock> {
+        val cache = rollingStockRangeMapCache?.get()
+        if (cache != null) return cache
+        val previousStepPos = 0.meters


The fact that this is read-only feels very sus

Ooops. Fixed in 590b93a

They should all have a zero begin speed.

Signed-off-by: Younes Khoudli <younes.khoudli@epita.fr>

github-actions bot added area:core Work on Core Service area:integration-tests Work on Integration test, by nature related to different services labels Mar 30, 2026

Sh099078 force-pushed the lf/changement-convoi branch 3 times, most recently from e3597fb to a2f9c6b Compare March 30, 2026 09:50

Khoyo assigned Khoyo and Sh099078 Mar 30, 2026

Khoyo added this to Board PI 19 Mar 30, 2026

Khoyo moved this to In Progress in Board PI 19 Mar 30, 2026

Sh099078 force-pushed the lf/changement-convoi branch 4 times, most recently from d452fdd to a033171 Compare March 31, 2026 09:20

Khoyo requested a review from Erashin March 31, 2026 13:05

Khoyo assigned Erashin Mar 31, 2026

Sh099078 force-pushed the lf/changement-convoi branch 3 times, most recently from b039557 to d9c0ee3 Compare March 31, 2026 15:03

github-actions bot removed the area:integration-tests Work on Integration test, by nature related to different services label Mar 31, 2026

Erashin reviewed Apr 1, 2026

View reviewed changes

handle multiple consists per train schedule

1b0ee8f

Co-authored-by: Younes Khoudli <younes.khoudli@epita.fr> Signed-off-by: Loup Federico <16464925+Sh099078@users.noreply.github.com>

Sh099078 force-pushed the lf/changement-convoi branch from d9c0ee3 to 98a39e6 Compare April 1, 2026 13:46

github-actions bot added the area:front Work on Standard OSRD Interface modules label Apr 1, 2026

eckter reviewed Apr 1, 2026

View reviewed changes

fixup: bugfixes related to failing e2e and integration tests

9adf44b

Sh099078 force-pushed the lf/changement-convoi branch from 98a39e6 to 9adf44b Compare April 1, 2026 16:00

github-actions bot removed the area:front Work on Standard OSRD Interface modules label Apr 1, 2026

Sh099078 added 2 commits April 1, 2026 18:05

fixup: calculate correct begin indexes in getFullRollingStockRangeMap

590b93a

fixup: runSimulationWithFixedPoints: add assert on envelopes begin speed

3210643

They should all have a zero begin speed.

Sh099078 force-pushed the lf/changement-convoi branch from ab27251 to 3210643 Compare April 1, 2026 16:15

Sh099078 added 4 commits April 1, 2026 18:16

fixup: repair truncated comment

73a7b89

fixup: parseSteps: rename param to rollingStockLenghts (plural)

7adefd7

fixup: linting on map

3340a12

fixup: STDCMEndpoint: remove deadcode

b29ec8f

Sh099078 marked this pull request as ready for review April 1, 2026 16:28

Sh099078 requested a review from a team as a code owner April 1, 2026 16:28

Sh099078 requested a review from Khoyo April 1, 2026 16:28

add a test for runSimulationWithFixedPoints when given no fixed points

a1d2f45

Signed-off-by: Younes Khoudli <younes.khoudli@epita.fr>

Conversation

Sh099078 commented Mar 30, 2026

Uh oh!

Erashin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Khoyo Apr 1, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eckter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eckter Apr 1, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Khoyo Apr 1, 2026 •

edited

Loading

eckter Apr 1, 2026 •

edited

Loading