[Serana]

Then, that just means that the scripts must be adapted.
I have just modified my pull request so that the modification of the script is easier than before (2 lines to change).

For example, I attached the AWS script modified from the latest version on Carlo's website (mep_AWS_demo_pack.zip Rev. 10).

Attached File(s)

•  TCS_AWS_UK.zip (5.36K)
  Number of downloads: 193

[Csantucci]

I appreciate Cédric's effort to simplify things, however I am asking here for a pronouncement by someone the ORMT. If this PR will be committed, the AWS TCS script that will be compatible with this PR will be different from the actual one, that is compatible with the present testing versions and with the 1.4 release version, because this PR breaks backwards compatibility. The script has been downloaded 1014 times since I have introduced the download counter.
So I ask whether the ORMT agrees with this or not, and, at least for the official OR releases, I will comply with the ORMT decision.

[cjakeman]

 Csantucci, on 23 December 2021 - 02:00 AM, said:

So I ask whether the ORMT agrees with this or not, and, at least for the official OR releases, I will comply with the ORMT decision.

Hi Carlo,
Thanks for raising this. We're looking at the issue and will respond as soon as possible.

[gpz]

I am actually wondering, what is the point in extending the scripting interface with new UpdateXyz() functions. Why don't the single Update() call can handle all kind of updates? I see that with this particular function the purpose is that it could overwrite a default implementation. But it might not be the most efficient programmming structure, especially if we consider future new systems and additions. Actually this particular system may be a good candidate to be hadled by the event system instead. I mean, when some specific events happen, e.g. the train speed exceeds a specific speed, an event can be generated, that is to be handled by the event handler.

When I considered extending the scripting interface in a general way some years ago, my basic concept was to make it possible to disable any built-in event with a script call, like re-directing the event handling to the script, thus making possible to write an own implementation inside the script. Of course a symmetrical function had also been needed, to possibly re-call the original function, in case someone wanted to write only a partial re-implementation. (I gave up on this since, I got out of mood for different reasons, and in the meantime other methods got implemented in this direction.)

So my "ideal" implementation for this would be:

1. The above overspeeding in question generates an event.
2. There is a default event handler written inside OpenRails.
3. The OpenRails event handler calls the default method unless the script "re-directed" this particular event to itself.
4. The script does its own stuff in its own HandleEvent() call, then (or before) possibly is able to call back the original event handler function.

For this to work there must be a dictionary of "events" paired to their event handlers. Beside others all keyboard commands could generate such a re-directable event, but this is highly out of the scope of this PR. But I still think the event system is underutilized in favor of different update-overrides. I think one Initialize(), one Update() and one HandleEvent() had been more than enough for the scripting interface, how I implemented that originally.

[cjakeman]

 Csantucci, on 23 December 2021 - 02:00 AM, said:

because this PR breaks backwards compatibility.

It occurs to me that the project has no mechanism for communicating with users who are not visiting the Open Rails website or involved with the forums. All they have is a convenient link on the Menu form which updates their installation to the newest version of Unstable, Testing or Stable. And also another link to find out what's new.

In the event of a breaking change (which amazingly we have so far managed to avoid but I reckon is inevitable at some point) or some other major event, I think Open Rails should have a means in place to communicate that.

I'll propose a solution to this shortly which I hope we can discuss.

[cjakeman]

 cjakeman, on 24 December 2021 - 07:33 AM, said:

Hi Carlo,
Thanks for raising this. We're looking at the issue and will respond as soon as possible.

Please see the new thread Scripting with backward compatibility.

[James Ross]

 Serana, on 01 December 2021 - 06:07 AM, said:

This functionnality is now part of the train control system. If you have a TCS script, you have to implement this functionnality in the TCS script.

Unfortunately, this does not meet our desired compatibility goals. We need to not break existing scripts when providing new functionality.

 Csantucci, on 01 December 2021 - 12:03 PM, said:

I am not happy with what has occurred here. As I have written also elsewhere, I am not against making changes in the core OR code that could affect existing TCS scripts, but this should be clearly announced, discussed and a balance should be publicly be drawn between changing the core OR code and changing the scrips.
In this case AFAIK nothing of the above has occurred, and the problem has been raised by an attentive OR simmer starting from the consequences of the issue.

Agreed - we need to avoid any requirements for TCS scripts or other content to be updated to work in a new release.

 Serana, on 22 December 2021 - 08:25 AM, said:

But anyway, this functionnality should have never been implemented outside of the TCS interface. The DoesBrakeCutsPower functionnality was implemented in the TCS interface long before pull request 103 implemented it for vacuum brakes seperatedly.

Not really relevant to the problem, but this is a good example of why everyone should have a forum thread for their own new features - it'll help spot such conflicting functionality.

 darwins, on 22 December 2021 - 10:49 AM, said:

As a content creator I thought that OR would stick to the matter of backwards compatibility. So often I have asked for changes to features to be told that it would break backwards compatibility with MSTS or with previous versions of OR. Right or wrong, this seems to me a case of something that was once working, that is no longer working.

Yup, this is a problem that needs fixing - we cannot release a new stable version without the TCS compatibility fixed.

 Serana, on 22 December 2021 - 05:24 PM, said:

Then, that just means that the scripts must be adapted.

 Csantucci, on 23 December 2021 - 02:00 AM, said:

I appreciate Cédric's effort to simplify things, however I am asking here for a pronouncement by someone the ORMT. If this PR will be committed, the AWS TCS script that will be compatible with this PR will be different from the actual one, that is compatible with the present testing versions and with the 1.4 release version, because this PR breaks backwards compatibility. The script has been downloaded 1014 times since I have introduced the download counter.
So I ask whether the ORMT agrees with this or not, and, at least for the official OR releases, I will comply with the ORMT decision.

The ruling from ORMT is that we need to keep this compatible. We must change the TCS scripting interface so that the original AWS TCS script works.

 gpz, on 24 December 2021 - 09:30 AM, said:

I am actually wondering, what is the point in extending the scripting interface with new UpdateXyz() functions. Why don't the single Update() call can handle all kind of updates? I see that with this particular function the purpose is that it could overwrite a default implementation. But it might not be the most efficient programmming structure, especially if we consider future new systems and additions. Actually this particular system may be a good candidate to be hadled by the event system instead. I mean, when some specific events happen, e.g. the train speed exceeds a specific speed, an event can be generated, that is to be handled by the event handler.

The current method is to use a single "Update" method, but that has caused a problem here because it provides no way for a script to only partially implement the TCS interface, something which is needed for backwards compatibility.

Using virtual "UpdateXyz" methods would solve that, because the base class would have the default/built-in implementation of a feature and the TCS script can override it (if it is new enough) or not (if it is older) and it would work both ways.

I feel like the events are just a different way of writing the virtual "UpdateXyz" methods, but perhaps I am missing something you'd thought of?

However, I do not mind what we use going forwards, but we must maintain compatibility with existing scripts that work with Open Rails 1.4 (and earlier versions).

[Serana]

I think I will look like the French guy who likes to complain... but anyway...
I will first put the private message I sent to Chris and James when asked about the change... there was no real discussion with me about this later on... so I am putting this in public.

Serana, on 28 December 2021 - 04:01 PM, said:

Hi,
I don't agree with the application of the "not-for-unstable" tag since the pull request doesn't break compilation and the run of the software. It doesn't provoke any crash or warnings.

To give you some backstory:
- The "DoesBrakeCutsPower" was implemented in the TCS interface in 2016 (https://github.com/o...5956d782da886bd). This allows the customization of how the traction/braking interlocking works depending on the rules of the corresponding country. Also, the traction/braking interlock intervenes on the traction authorization which is also used by the signalling part of the TCS system so it makes sense including it in the TCS subsystem.
- The "ORTSDoesVacuumBrakeCutsPower" and other associated parameters were implemented in the MSTSLocomotive in 2020 by Peter Newell (https://github.com/o.../pull/103/files). I did not participate to the review process of this pull request (I may not have been available at that time), but I think that the implementation made back then is wrong. Peter implemented the traction/braking interlock for vacuum brakes in the MSTSLocomotive class whereas it should have been implemented in the TCS interface.

On the principle behind this refactoring, to me, the traction/brake interlocking is part of the TCS system as much as the vigilance subsystem and the signalling onboard subsystem.
The existing code for air brakes only was able to check the brake cylinder pressure.
I first moved the code from the vacuum brakes traction/braking interlock (which is able to check the vacuum brake pipe pressure (with hysteresis).
I then added the code needed for air brake pipe pressure check (with a single limit or with hysteresis).
And finally, I added new parameters needed for the implementation of the traction/braking interlock specified in the european TSI LOC&PAS.

I also documented the parameters since they were not included in the manual modification of the pull request of Peter. By the way, if we made a big change in a part of the code for a functionnality that was not even documented in the manual, is it really a problem ? Since it was not documented, It was not really made available to content creators.

About Darwins' TCS script, it's not technically his script: it's the AWS script written by Carlo. My change did not really break any code on his side (I mean that there was no compilation error for this particular script after my change) but the traction/braking interlock for vacuum brakes only doesn't have an effect anymore since it was not implemented in the AWS script and is no longer inside the locomotive code. I provided a new version which adds the traction/braking interlocking (this is a two line change) and it may need to be added to the italian script if needed.

To go back to the origin of the occurence of this problem, I think this bad design happened also because of how we manage the review process. Currently, any developer can say that the code is right, but the coherency of the code in the different parts of the simulator may not have been checked. It needs to be checked by maintainers which are specialists of the part of the simulator which is modified. The review by only one (random) developer only provides code quality verification, not coherence with the design.
I already discussed a bit about the need for maintainers with Chris some months ago but we didn't start a topic about this on the forum yet. I think it's time we start a topic about this.
Also, i currently have a feeling that I want to share. Most of the members of the team only knows each other from the discussions on the forums. This impacts the team dynamics since each developer works in their own corner.

I think we should organize team talks in vocal (for example, on Discord every month) in order to know what other developers want to do, to discuss in vocal some designs, some feature requests or some pull requests... and to know the other developers beyond the discussions we have on the forums.

We really need to have a talk about this... this bad design also originates from how we manage the project.

I would really like for us to talk about it because I am starting getting fed up with how the project is managed and I am really starting thinking about creating an official fork and that's not something I would like to do.

It's not necessarily related to the current problem but :
- Blueprints are not validated for months. At least, tell us what is wrong.
- We wait for code reviews for months.
- Most code reviews do not look beyond the correctness of the code. Design must also be checked.
- Most developers don't really know each other since we only talk through the forums, there's no real team dynamics.

The corresponding solutions could be:
- We need people who are specialist of some main parts of the code to be maintainers, who can manage their part (blueprint validation, code review with emphasis on the design check, perhaps also with the maintaining of an UML model to represent how must be the class hierarchy)
- Yes, I know, we are all volunteers but if someone can't validate all of a pull request, at least, validate the parts that you can understand. That's by the way what I try to do... with varying results.
- We need to be able to talk in an easier way than a forum. When you want to debate, to discuss an idea, a topic in a forum is a very slow way and sometimes difficult way to talk about it. You sometimes need dozens of lines to convey the idea you have. A solution could be to create meetings on a voice server (for example, monthly) with developers and content creators.

About the previous answers:

 James Ross, on 20 January 2022 - 01:57 PM, said:

Not really relevant to the problem, but this is a good example of why everyone should have a forum thread for their own new features - it'll help spot such conflicting functionality.

This is relevant because if there was a bad design, it makes no sense maintaining it. It's like saying "There was some (bad) design in MSTS, I will follow it blindly." You will never improve the simulator with that thinking.

 James Ross, on 20 January 2022 - 01:57 PM, said:

Yup, this is a problem that needs fixing - we cannot release a new stable version without the TCS compatibility fixed.
The ruling from ORMT is that we need to keep this compatible. We must change the TCS scripting interface so that the original AWS TCS script works.

Public APIs are not everlasting. Even .Net or Qt have breaking changes once in a while. Then packages who are depending on them are upgraded to support the new version of the API and people then update their software.
As I said, updates are not really a problem when the community follows your project (just look at download stats on Carlo's website). And if it really is, the solution is simple: content updaters (we can, for example, distribute scripts directly with the simulator updater) or package managers (if we want each developer to manage the script distribution by themselves).

 James Ross, on 20 January 2022 - 01:57 PM, said:

Yup, this is a problem that needs fixing - we cannot release a new stable version without the TCS compatibility fixed.
The ruling from ORMT is that we need to keep this compatible. We must change the TCS scripting interface so that the original AWS TCS script works.

The current method is to use a single "Update" method, but that has caused a problem here because it provides no way for a script to only partially implement the TCS interface, something which is needed for backwards compatibility.

Using virtual "UpdateXyz" methods would solve that, because the base class would have the default/built-in implementation of a feature and the TCS script can override it (if it is new enough) or not (if it is older) and it would work both ways.

I feel like the events are just a different way of writing the virtual "UpdateXyz" methods, but perhaps I am missing something you'd thought of?

However, I do not mind what we use going forwards, but we must maintain compatibility with existing scripts that work with Open Rails 1.4 (and earlier versions).

The problem is that we are talking about systems that are not managed the same for each country. Sometimes the traction braking interlock will disable traction, sometimes it will open the circuit breaker.
This functionnality must be able to work correctly with other subsystems, such as national TCS or ETCS. It is interdependent with the onboard signalling system.
For example, when the traction requires the circuit breaker to be open when the TCS wants it to be closed, how do you say which one has priority?
The current answer: you decide in the main Update function if the action of closing or the action of opening must be triggered.

So that's why you must have everything managed in the same place: to manage when different subsystems have antagonist commands.

So two solutions are possible:

• You modularize everything: each subsystem will have it own class/interface and there is a unifying layer which goal is to merge what each subsystems wants (a bit like what is done with the PowerSupply classes). This unifying layer needs to be customizable to correspond to how it works in each country or even in each locomotive. That would be great design-wise, but, at first, it will completely break the compatibility since we will have to move code from the TCS interface to new interfaces.

• You put everything in the same place: that's what is proposed currently for the traction braking interlock with reduced compatibility issue (the traction braking interlock will not work temporarily for trains with vacuum brake and TCS script at the same time) but that can be re-added when the script is updated. As I said, it's not a blocking problem.

[cjakeman]

Hi Cedric,

 Serana, on 20 January 2022 - 07:12 PM, said:

I think I will look like the French guy who likes to complain... but anyway...

Don't worry about it. We're all friends on Elvas Tower and trying to do our best.

 Serana, on 20 January 2022 - 07:12 PM, said:

We really need to have a talk about this... this bad design also originates from how we manage the project.
It's not necessarily related to the current problem but :
- Blueprints are not validated for months. At least, tell us what is wrong.
- We wait for code reviews for months.
- Most code reviews do not look beyond the correctness of the code. Design must also be checked.

There was a dry period in 2020, but we changed the way we do things a bit during last year. I hope you'll agree that things are moving along a bit quicker now as a result.

 Serana, on 20 January 2022 - 07:12 PM, said:

- We need to be able to talk in an easier way than a forum. When you want to debate, to discuss an idea, a topic in a forum is a very slow way and sometimes difficult way to talk about it. You sometimes need dozens of lines to convey the idea you have. A solution could be to create meetings on a voice server (for example, monthly) with developers and content creators.

If you would like to try this, I'm willing to join in. My hours are pretty flexible so just tell me where and when.

 Serana, on 20 January 2022 - 07:12 PM, said:

About the previous answers:
This is relevant because if there was a bad design, it makes no sense maintaining it. It's like saying "There was some (bad) design in MSTS, I will follow it blindly." You will never improve the simulator with that thinking.
Public APIs are not everlasting. Even .Net or Qt have breaking changes once in a while.

Agreed, but we don't need a breaking change just now.

Perhaps there's a misunderstanding. James has offered a way forward which maintains compatibility but doesn't stop you from extending the API.

[James Ross]

 Serana, on 20 January 2022 - 07:12 PM, said:

- We wait for code reviews for months.

As far as I can tell, we're doing okay on pull request times - see this histogram of the last 483 PRs merged (~three years):

https://james-ross.co.uk/temp/orts_166.png

That's 350 of 483 PRs (72%) being merged within two weeks, and this includes any time a PR spends in draft status.

Perhaps you've been unlucky and had multiple PRs that took a long time? There will always be a "long tail" of PRs that take ages, that is impossible to avoid, but please have a look at the histogram to see how we're doing overall.

 Serana, on 20 January 2022 - 07:12 PM, said:

- Most code reviews do not look beyond the correctness of the code. Design must also be checked.

We do ask reviewers to check architecture but unfortunately the architecture is currently blank.

If you'd like to suggest particular areas of the Open Rails architecture we should document here first - or even contribute some documentation yourself - you're more than welcome.