Skip to content

Conversation

@clarani
Copy link

@clarani clarani commented Oct 24, 2025

Description

This PR allows to handle times updates correctly when a node is collapsed.

I will add more details later.

Issues

OpenRailAssociation/osrd#13347

Checklist

  • This PR contains a description of the changes I'm making
  • I've read the Contribution Guidelines
  • I've added tests for changes or features I've introduced
  • I documented any high-level concepts I'm introducing in documentation/
  • CI is currently green and this is ready for review

);
}

static getSectionTravelTime(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static getSectionTravelTime(
static getSectionDistributedTravelTime(

trsTimeStructure.travelTime = TrainrunsectionHelper.getTravelTime(
if (prevInitialTargetArrival !== null) {
// gérer les modulo 60 et positif
stopTime = (nextPair.trainrunSection.getSourceDeparture() - prevInitialTargetArrival) % 60;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to deal with negative stopTimes (dirty way):

Suggested change
stopTime = (nextPair.trainrunSection.getSourceDeparture() - prevInitialTargetArrival) % 60;
stopTime = (((nextPair.trainrunSection.getSourceDeparture() - prevInitialTargetArrival) % 60) + 60) % 60;

@emersion emersion self-requested a review October 31, 2025 08:53
@clarani clarani changed the base branch from main to mrd/new-nodes-management-system October 31, 2025 15:25
@clarani clarani force-pushed the cni/13347-handle-times-update-on-reduced-nodes branch from 14aff1d to e5de33f Compare November 3, 2025 09:11
Copy link
Contributor

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The left (source) -> right (target) works

The left (target) <- right (source) not; I'm trying to find a solution if you don't find it first :-)

You can test with this also:
reticulaire(3).json

Also, do not forget to remove the last commit


export class NonStopTrainrunIterator extends TrainrunIterator {
/** Iterate on the trainrun sections until we find a node which is a stop of the trainrun and not collapsed */
export class NextVisibleStopIterator extends TrainrunIterator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we name it NextExpandedStopIterator? To re-use the "expanded"/"collapsed" paradigm

expect(tt).toBe(1);
it("getLastSectionTravelTime -- 004", () => {
const tt = TrainrunsectionHelper.getLastSectionTravelTime(10, 2, 0);
expect(tt).toBe(20);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the tests:

Chrome Headless 141.0.0.0 (Windows 10) TrainrunsectionHelper getLastSectionTravelTime -- 004 FAILED
        Expected 8 to be 20.
Suggested change
expect(tt).toBe(20);
expect(tt).toBe(8);

if (prevInitialTargetArrival !== null) {
stopTime = MathUtils.mod60(nextPair.trainrunSection.getSourceDeparture() - prevInitialTargetArrival);
trsTimeStructure.leftDepartureTime = trsTimeStructure.rightArrivalTime + stopTime;
trsTimeStructure.leftArrivalTime = trsTimeStructure.rightDepartureTime + stopTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trsTimeStructure.leftArrivalTime = trsTimeStructure.rightDepartureTime + stopTime;
trsTimeStructure.leftArrivalTime = trsTimeStructure.rightDepartureTime - stopTime;

Since the train goes from:

  • left -> right then
  • left <- right

You can try on a non-stop chain with more than 1 collapsed node

reticulaire(3).json

Copy link
Author

@clarani clarani Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, you're right !! :)

Copy link
Contributor

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further testing, happens this actually does not work for right times updates :/

Copy link
Contributor

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to add the stop time prop!

This makes the easy case work (rightIsTarget = true) but not the other situation still.

Also, I think you have forgotten to add the stop time stuff in onNodeLeftArrivalTimeChanged()? I quickly tried to test but I'm not sure of what I'm saying rn

ps: you have formatting issues, npm run format

);
} else if (!this.lockStructure.travelTimeLock && this.lockStructure.leftLock) {
const extraHour = this.timeStructure.travelTime - (this.timeStructure.travelTime % 60);
// pas sûre d'avoir compris comment fonctionnait l'extraHour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extraHour is the rest of the euclidian division of travelTime / 60 ; this is used for recalculating the travel time using the departure/arrival times, which are % 60

303min of travelTime = 300min of "base"
say departure is 2' and arrival 5', travelTime = 5 - 2 + 300 = 303min

Then, you are right to add the stop time value to this one also

@clarani clarani force-pushed the cni/13347-handle-times-update-on-reduced-nodes branch from fad0e73 to 48bc480 Compare November 5, 2025 12:59
`TrainrunsectionHelper.getTravelTime()` computes the new travel time of a trainrunSection, either by using the
`travelTimeFactor` and the current travel time of the section, or by using the total trainrun travel time and
the sum of the new travel times computed up to that point.

This logic can be split into 2 independent functions, which will be used depending on whether the considered section
is the last one to compute or not.

Signed-off-by: Clara Ni <[email protected]>
Until now, a time structure has represented one or several trainrun sections without any stops,
so it could be described by its departure and arrival times (right and left) and its travel time.
Since we are now going to handle several trainrun sections that include stops, we need to add a
 new property to store the total stop time in the time structure.

Signed-off-by: Clara Ni <[email protected]>
…masked

The behavior is pretty much the same as when a node is a non-stop node, except
when updating the travel time, because we need to take into account the stop times of the reduced nodes.

Signed-off-by: Clara Ni <[email protected]>
This commit needs to be droped before the merge.

Signed-off-by: Clara Ni <[email protected]>
@clarani clarani force-pushed the cni/13347-handle-times-update-on-reduced-nodes branch from 48bc480 to 4206b78 Compare November 5, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants