[roeter]

James Ross, on 27 October 2015 - 11:31 AM, said:

I'd much rather we kept the code clean and simple than made every single thing "optional". Additionally, it's premature to do anything like this until a profiler has shown it to be a performance problem. (It's fine for something to be optional while it is experimental, but after that only if it has been truly shown to affect performance adversely.)

That could well make the program behave like a 'boiling frog'.
In case you are not familiar with this : if you put a frog in boiling water, it will jump out as it senses the change of temperature.
But, if you put a frog in cold water and then slowly heat it, the frog will never sense the change in temperature and will slowly be boiled to death without ever 'realising' what happened.

That is the risk here as well : by adding small items bit by bit there will never be a clear detoration in performance, but one day we will wake up and find the program has really become much slower. There is no way back from that once that state is reached, because there isn't a single particular change which caused it, it's just the culmination of many small steps.

Furthermore, making these additions optional need not be bad for the structure of the program at all: it may well entice the programmer to concentrate these additions in a limited number of clear methods, rather than to spread these out in small blocks of statements all over the program.
That's what has been done with much of the code for Train.cs and AITrain.cs : rather than having hundreds of 'if' statements checking for activity or timetable mode, a clear cut has been made using child classes and override methods where necessary.
The same could be done here: if these calculations are part of, say, the wagon class, then if this option is set a 'child' class is used which holds the methods for these calculations. Such a method of 'splitting' according to options also would make it easier to implement different calculations as required for different types of couplers.

Regards,
Rob Roeterdink

[Genma Saotome]

I've uploaded an Alpha version of my testing route -- alpha as-in Does this thing even work? The download is here.

I'm mentioning this route in this thread because it includes the ability for testing curves -- 13 different radii are present, each having its own path. The tightest is 60m, the widest is 3000m. Excepting the big, 3000m curve, each path takes a 180d turn, flips to the opposite direction for 60d, flips back for 60d and runs out in tangent for 2000m. You should be able to have a fairly long train complete the full sequence. I have not placed speed limits so these curves are suitable only for testing the physical dimensions of the track.

What is not (yet) provided is a standard consist; Having one would allow two persons to conduct identical tests and observe the same data. It would also enable any two persons to benchmark their PC and compare frame rates.

[James Ross]

roeter, on 27 October 2015 - 12:08 PM, said:

That could well make the program behave like a 'boiling frog'.

I feel avoiding adding unnecessary features entirely is a better approach to this problem - we obviously need to be restrained about what features we add at some level.

roeter, on 27 October 2015 - 12:08 PM, said:

Furthermore, making these additions optional need not be bad for the structure of the program at all: it may well entice the programmer to concentrate these additions in a limited number of clear methods, rather than to spread these out in small blocks of statements all over the program.

It doesn't need to be bad, but there's a fair amount of code in OR already which has a whole range of conditions and is hard to follow, so we need to be careful. :)

[steamer_ctn]

Patch #3291 has been committed with the changes indicated in this post.

As indicated exceeding the curve speed limit will cause a brake hose failure. The point at which the brake hose fails can be increased above the speed warning by increasing the durability factor in the CONF file. Values greater then 1.0 will lift the failure point by a corresponding factor.

If the train exceeds the "overturn" speed, then a coupler will be broken.

The train can be restarted from both of this events by restarting the train after reconnecting the air hose or re-coupling the train.

It should be noted that these changes will require correctly set ENG and WAG files, and the train to be driven within appropriate protypical limits.

If it is desired to exceed the curve speed limits, and ignore them, then this feature should be turned off by unchecking the "Curve dependent speed limit" box in the Simulator tab of the options menu.

[James Ross]

steamer_ctn, on 02 November 2015 - 02:03 AM, said:

Patch #3291 has been committed with the changes indicated in this post.

The blueprint did not have "Direction" set to "Approved", so this did not actually meet the criteria for committing. Please ensure that blueprints have a "Direction" of "Approved" before committing in the future. If you're ready to commit and it's not set yet, send me a PM and I'll look at quickly. Setting direction is mostly a formality but it's indented to avoid things going in that we don't want so we need to stick to it.

Thanks for the contribution, though, penalties are a key part of the realism side. :)

[Mike B]

Regarding the "boiling frog" ... I sympathize having done a little software and script maintenance over the years. In at least one case, it was easier to make a list of what one critical piece of software was supposed to do (the specs that it didn't have in the beginning - a lot of things "just happened"), then start from scratch. ORTS is far to big and complicated (and successful!) for that, but there may be times you're tempted ...

Some other notable software packages have struggled mightily to recover from the situation. Open Office and LibreOffice have come a long way - adding features AND cleaning up the code for speed and efficiency since the early days of StarOffice - but still have a way to go and have to keep the faith as more features are added; in the LibreOffice case, making a 64-bit version seemed to cause a lot of effective cruft removal.

[roeter]

steamer_ctn, on 02 November 2015 - 02:03 AM, said:

If it is desired to exceed the curve speed limits, and ignore them, then this feature should be turned off by unchecking the "Curve dependent speed limit" box in the Simulator tab of the options menu.

Checking through the code I have found various calculations relating to this feature which are unconditional. Apart from the waste of CPU time, it could also lead to problems elsewhere.

Furthermore, the logic uses at least one variable, but perhaps more, which are derived from the player train information but which are derived in activity mode only. Obviously this causes problems, see this thread. Has this feature been tested in timetable mode at all?

Finally, it seems that debug print statements have not been removed, e.g. at line 1484 in Train.cs.

Edit : there's another very serious issue here.
The coupler break and the crash happened during pre-run. Clearly, there is no player train during pre-run, which means these checks also seem to be active for AI trains. That's weird.
AI trains are run according to the speed limits as set by the route builders. It is a known fact that many curves, and in particular curves within switches, do not always have the correct speed limit. For curves within switches this is almost impossible to set due to the way MSTS speedpost apply speed limits. To test AI trains for excess speed has nothing to do with the player keeping his train under control, and these checks should be firmly limited to the player train only.

Regards,
Rob Roeterdink

[steamer_ctn]

Hi Rob,

roeter, on 03 November 2015 - 11:44 AM, said:

Checking through the code I have found various calculations relating to this feature which are unconditional. Apart from the waste of CPU time, it could also lead to problems elsewhere.

I am not 100% sure which calculations you are referring to, but I am happy to try and address any problems relating to the curve speed limit code.

roeter, on 03 November 2015 - 11:44 AM, said:

Furthermore, the logic uses at least one variable, but perhaps more, which are derived from the player train information but which are derived in activity mode only. Obviously this causes problems, see this thread. Has this feature been tested in timetable mode at all?

Thus it would be nice to see a consistent set of variables that are passed between different classes, and under what circumstances they are valid.

roeter, on 03 November 2015 - 11:44 AM, said:

Finally, it seems that debug print statements have not been removed, e.g. at line 1484 in Train.cs.

I can't comment on this statement, as it was contributed by another developer, and I am uncertain as to whether it is a debug statement or not.

Perhaps we can start a PM conversation to work through any issues related to the curve speed limit code, and thus the code can be improved upon.

Thanks

[roeter]

steamer_ctn, on 03 November 2015 - 12:59 PM, said:

Quote

Furthermore, the logic uses at least one variable, but perhaps more, which are derived from the player train information but which are derived in activity mode only. Obviously this causes problems, see this thread. Has this feature been tested in timetable mode at all?

Thus it would be nice to see a consistent set of variables that are passed between different classes, and under what circumstances they are valid.

I do not understand your comment.
The variable I am refering to is "public float CurveDurability" in Simulator.cs, this variable is introduced in update 3291 and is not properly set in timetable mode. So it has nothing to do with consistency of existing variables in any classes.

Quote

Perhaps we can start a PM conversation to work through any issues related to the curve speed limit code, and thus the code can be improved upon.

With pleasure, but in my view some serious issues need to be addressed first :

• The log-file reports a broken coupler even with curve speed check and broken coupler settings disabled.
  
• The broken coupler happens during pre-run and is therefor on an AI train, this should never happen even if both options mentioned above were activated.

These two issues lead to the crash as reported.

Regards,
Rob Roeterdink

[steamer_ctn]

Hi Rob,

roeter, on 03 November 2015 - 04:24 PM, said:

With pleasure, but in my view some serious issues need to be addressed first :

• The log-file reports a broken coupler even with curve speed check and broken coupler settings disabled.
• The broken coupler happens during pre-run and is therefor on an AI train, this should never happen even if both options mentioned above were activated.

Thanks for that.

I will send a PM so that we can start working to sort it out. If necessary we can revert the changes until all the issues are solved.

Thanks