[Csantucci]

Thank you all for support!
I'm not in favour of providing to the user means to enable/disable this option. It is a very technical one, and if the user makes the wrong decision he gets faulty semaphores. I'm neither in favour to apply the option only to signal files residing in the route's main folder, as this again makes only things more difficult for players; moreover for some routes it is useful to simply copy the signal files into the OpenRails subfolder, to have a better behaviour of SignalNumClearAhead; the player should therefore know that when copying the file also semaphores would work differently.

I'm instead in favour of writing a single warning line in OpenRailsLog.dat when the patch gets active and operates the changes to the semaphore indexes.

[jonas]

 Csantucci, on 30 December 2016 - 03:42 AM, said:

Thank you all for support! ...

Thanks for your support too!

 Csantucci, on 30 December 2016 - 03:42 AM, said:

...I'm instead in favour of writing a single warning line in OpenRailsLog.dat when the patch gets active and operates the changes to the semaphore indexes.

I agree, because I am sure that the patch affects only the semaphore signals in question.

But in a way I can also understand that someone do not feel really good when such exceptions are allowed. Therefore, I also would not mind, if only an option under "Experimental" activates the patch, default setting "off". A hint to use it "on" may given in OpenRailsLog.dat (or as a popup box while loading the route?). Finally options such as: "Correct questionable braking parameters" are also quite technical.

[Csantucci]

Here are the modified executables writing into the logfile the information string "Reindexing semaphore entries for compatibility with MSTS"

Here the modified patch
1/1/17: Files deleted as new release of the patch available

[James Ross]

 jonas, on 28 December 2016 - 10:17 AM, said:

SemaphorePos (0) - SemaphorePos (126) - SemaphorePos (287)

This is the correct one. The first value in "tcb_key" (and the other items that appear at that level under "controllers") is the frame number of that key frame, and everything else to do with animation uses that. The number of frames, for example, is the last of these values.

 Csantucci, on 29 December 2016 - 03:05 AM, said:

As you like. I won't basically change it and if you don't like, I won't upload it. But it works accordingly to Jonas' first tests on 5 routes. It's not myself that has invented a relationship between semaphore indexes and animations as defined in the .s file, it's MSTS. Try Jonas' test route, look at its code excerpts near the signals and you'll see. And there are also in general important relationships between sigcfg.dat and .s file. And the existing code within viewer3D\signals.cs also looks at both sigcfg.dat and .s file.

So, I assume that it's your turn now to provide a working solution.

I am not doubting that it solves this particular problem, I am saying that I don't like where in the code it is. I have seen nothing to show that the shape is required to identify the problematic cases and I want to try the simple solution first (rather than go straight for the complex one as here).

 eugenR, on 29 December 2016 - 04:37 AM, said:

I doesn't understand why confusing sigcfg.dat?

 jonas, on 29 December 2016 - 04:40 AM, said:

What confusion is to be afraid of? The relation between SignalType (sigcfg.dat) and the related signal (.s-file) is one-to-one. Other signals are not concerned. What do I overlook?

The code is not changing the data loaded from sigcfg.dat, it is changing something else, so if any other code looks at the values in sigcfg.dat it will see the original values. This may mean two different parts of the program see different values for the same signal.

 eugenR, on 29 December 2016 - 04:37 AM, said:

To have only two entry's in a semaphore-animation is a not correct shape-design!

I'm not sure where this comes from, actually. A basic semaphore signal only has two states, so why does it need 3 key frames?

 eugenR, on 29 December 2016 - 04:37 AM, said:

But it was taken in many of german routes, we don't know why, and as Jonas an I have tested, ONLY in this case MSTS calculate the SemaphorePos down. in MSTS such animations are not working, with the correct SemaphorePosNumber's 0,1 the signals stay "hanging" in the Position 0 (stop) instead to show Position 1 (Clear),to activate the Signal you can move back and forward the train,see Testroute "SemaTestMSTSvsOR.zip" in Post#33 and Results in Post #39.

 Jovet, on 29 December 2016 - 10:36 PM, said:

Values should only be shifted by one for specific signal shapes which only contain 2 animation keyframes. Shapes with more than two animation keyframes should be okay already in MSTS and OR.

Do we have examples of this? Does MSTS actually treat 1,2 as 1,2 when there are 3 key-frames and 0,1 when there are 2 key-frames?

 Jovet, on 29 December 2016 - 10:36 PM, said:

The patch Carlo made sounds like a good deal to me as it seems to solve the animation problems currently experienced. I don't see why it can't be implemented as an experimental option and be able to turned off. It should also be automatically disabled for any OpenRails\sigcfg.dat signal configuration.

I don't think it should be an option, but I am also not sure whether it should be active for OpenRails\sigcfg.dat or not.

[Csantucci]

 James Ross, on 31 December 2016 - 04:43 AM, said:

I am not doubting that it solves this particular problem, I am saying that I don't like where in the code it is. I have seen nothing to show that the shape is required to identify the problematic cases and I want to try the simple solution first (rather than go straight for the complex one as here).

See below about dependencies in MSTS between SemaphorePos and animation steps.

 James Ross, on 31 December 2016 - 04:43 AM, said:

The code is not changing the data loaded from sigcfg.dat, it is changing something else, so if any other code looks at the values in sigcfg.dat it will see the original values. This may mean two different parts of the program see different values for the same signal.

I had the impression that as policy OR always leaves unchanged data read from the various MSTS files, and changes only derived data. By the way as of now within Viewer3D.Signals.cs, class SignalAspectData you have this line

                SemaphorePos = drawStateData.SemaphorePos;

and the code then always uses the first variable (the second one is the one derived from sigcfg.dat).
However, if you prefer, I don't have problems in changing also drawStateData.SemaphorePos.

 James Ross, on 31 December 2016 - 04:43 AM, said:

Do we have examples of this? Does MSTS actually treat 1,2 as 1,2 when there are 3 key-frames and 0,1 when there are 2 key-frames?

Yes James,
as already said, if you run with MSTS Jonas' test route you will have first hand evidence. Here a picture of MSTS operation with such route:

Near the signals Jonas wisely laid down a sheet showing how the semaphores are indexed in sigcfg.dat and what animation steps are present in the .s file. The first signal and the third one in the picture are defined the same in sigcfg.dat and as .s file; only difference, the first signal has two animation steps and the third one has three animation steps in the .s file. As you can see, semaphores are in the opposite position, even if both signals have green light and are in fact in a CLEAR_2 state.
Same applies for second and fourth signal.
I had the same tests on the test route Eugen sent to me, and added and removed the third animation step in the .s file, arriving to the same conclusion. By the way, if you want to know it fully, if there are two animation steps MSTS behaves following way:
SemaphorePos (2) is interpreted as SemaphorePos (1)
SemaphorePos (1) is interpreted as SemaphorePos (0)
SemaphorePos (0) is interpreted as SemaphorePos (0) !!!
I had already noticed this in my tests, and hoped that no signals had this last case (with SemaphorePos (0)). Well, just this afternoon Eugen wrote me an email: he found a signal in a - you can guess - German route (Biggetalbahn 2) where there are two animation steps ( 0 and 1) and the SemaphorePos used are 0 and 2, which MSTS interprets as 0 and 1 as per above rule. So my patch should be a bit modified if also this case were to be taken into account.
If there are three animation steps in the .s file MSTS behaves as a good boy.
So, yes there is a strict relationship between .s file animation step number and MSTS behaviour.

This means that you have to do the adjusting of the SemaphorePos indexes in a place where you have also access to the .s file. Such a place is Viewer3D.Signals.cs. If you look e.g. at the SignalShapeHead class, where I inserted the patch, you will see that there are already code parts that access both sigcfg.dat and the .s file, e.g. (it is not the only example)

                for (int mindex = 0; mindex <= signalShape.SharedShape.MatrixNames.Count - 1; mindex++)
                {
                    string MatrixName = signalShape.SharedShape.MatrixNames[mindex];
                    if (String.Equals(MatrixName, mstsSignalSubObj.MatrixName))
                        MatrixIndices.Add(mindex);
                }

So to me the chosen one is the best place to insert the patch.

 James Ross, on 31 December 2016 - 04:43 AM, said:

I don't think it should be an option, but I am also not sure whether it should be active for OpenRails\sigcfg.dat or not.

I'd prefer to KISS, that is same rule everywhere. With the management of SignalNumClearAhead there is already quite a complicated situation that players have problems in managing.

Should this be considered as a bug correction or as a blueprint implementation?

[NF1-800]

Hello!
I made this time a test route and added each signal the exact way the signal pack creator described in the mini tutorial. This time the signals work properly in MSTS (by default) and also in Open Rails after i changed the SemaphorePos ( 1 ) and ( 2 ) to SemaphorePos ( 0 ) and ( 1 ). This leads me to the conclusion that the route creator might've damaged the sigscr.dat file in which he combined both mechanical signals and color light signals. Both packages were made by the same creator and explained how to combine them in order to make things work clockwork, so probably the route creator might've missed something. Just for curiosity i checked the shape file that i accused for not working properly and it has only 2 animation keys for each arm. So far so good and i am happy that my semaphores work correctly :). I have to try someday to combine them with the color light ones to check if everything is alright. Till then, i wish you all a Happy New Year! Cheers! Next station is 2017!

[eugenR]

 James Ross, on 31 December 2016 - 04:43 AM, said:

The code is not changing the data loaded from sigcfg.dat, it is changing something else, so if any other code looks at the values in sigcfg.dat it will see the original values. This may mean two different parts of the program see different values for the same signal.

Now I understand: never store a the same Data at two places!

 James Ross, on 31 December 2016 - 04:43 AM, said:

I'm not sure where this comes from, actually. A basic semaphore signal only has two states, so why does it need 3 key frames?

If there is no End-Key (same as Key 0) in Exlorer-mode in MSTS the semaphorearm is hanging. After the train has passed the Signal and has moved backward, the signal will work correctly. I think this MSTS-error was the reason why MSTS-Routedesignes have searched for a solution to avoid this Problem and have font the tricks with Semaphorepos 1,2 or 0,2

But in MSTS-Standardroute Europe1 is given a better solution:
This Route Europe1 has three semaphores OESignal01.s, ...02.s and ...03.s all with two Animationkey's+endkey

 James Ross, on 31 December 2016 - 04:43 AM, said:

Do we have examples of this? Does MSTS actually treat 1,2 as 1,2 when there are 3 key-frames and 0,1 when there are 2 key-frames?

I have attached a copy of an excelsheet (Wy I can't attach excel?) where I show the testresults from all my tests.
It show the SemaphorePos at the signal depending from different number of Key's, with and without end-key's and with different SemaphorePos() in the sigcfg.dat

Attached thumbnail(s)

• 

[Csantucci]

Here a further version of the patch, managing also the "Biggetal" case (columns G, H, I in the above Excel sheet), and correcting also the original values of the sigcfg.dat file as preferred by James.

01/01/17: Attachments deleted because patch uploaded in partly different form.

[James Ross]

 Csantucci, on 31 December 2016 - 12:50 AM, said:

Here are the modified executables writing into the logfile the information string "Reindexing semaphore entries for compatibility with MSTS"

Here the modified patch
1/1/17: Files deleted as new release of the patch available

This version is seeming like the best so far, although I think the warning needs to include details on which signal type it is.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

I had the impression that as policy OR always leaves unchanged data read from the various MSTS files, and changes only derived data.

I don't remember that (but I do forget things) - though there are a lot of warnings printed about things being corrected during loading, mostly index numbers for things.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

By the way as of now within Viewer3D.Signals.cs, class SignalAspectData you have this line

                SemaphorePos = drawStateData.SemaphorePos;

and the code then always uses the first variable (the second one is the one derived from sigcfg.dat).
However, if you prefer, I don't have problems in changing also drawStateData.SemaphorePos.

The point was that if SemaphorePos is wrong and we can fix it during loading, we should do that for everyone which uses it; having a local copy is the correct way to load data though, because it keeps the data you need close in memory and lets you discard the data from the file that's not needed.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

Yes James,
as already said, if you run with MSTS Jonas' test route you will have first hand evidence. Here a picture of MSTS operation with such route:
Semaphores_MSTS.jpg

:sign_thanks: Thank you Jonas for making the test route, it's very useful, and thank you Csantucci for explaining below. This picture is slightly misleading as it is showing an MSTS bug with the 1st signal's position, but I have now seen the actual positions in MSTS.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

I had the same tests on the test route Eugen sent to me, and added and removed the third animation step in the .s file, arriving to the same conclusion. By the way, if you want to know it fully, if there are two animation steps MSTS behaves following way:
SemaphorePos (2) is interpreted as SemaphorePos (1)
SemaphorePos (1) is interpreted as SemaphorePos (0)
SemaphorePos (0) is interpreted as SemaphorePos (0) !!!

So it turns out from my testing that I am not convinced MSTS is changing these values at all, but is instead doing the animation differently to OR. If you change the "SetFrameCycle" in Signals.cs to "SetFrameWrap" then all the signals in OR animate in the same direction (up or down) as MSTS - they just jump positions at the start or end of the movement. I consider this pretty strong evidence that we're not doing the animations quite right and that changing SemaphorePos is a hack rather than a real fix, but at this point I am okay with that.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

If there are three animation steps in the .s file MSTS behaves as a good boy.
So, yes there is a strict relationship between .s file animation step number and MSTS behaviour.

Yes, thank you for explaining that.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

This means that you have to do the adjusting of the SemaphorePos indexes in a place where you have also access to the .s file. Such a place is Viewer3D.Signals.cs. If you look e.g. at the SignalShapeHead class, where I inserted the patch, you will see that there are already code parts that access both sigcfg.dat and the .s file, e.g. (it is not the only example)

Yes, the issue was the lack of evidence that the number of frames was involved. I'm sorry if anyone explained before; I have read everything but if it's not completely clear to me I'll probably not have the time to figure it out.

 Csantucci, on 31 December 2016 - 08:33 AM, said:

Should this be considered as a bug correction or as a blueprint implementation?

Bug.

 Csantucci, on 01 January 2017 - 08:41 AM, said:

Here a further version of the patch, managing also the "Biggetal" case (columns G, H, I in the above Excel sheet), and correcting also the original values of the sigcfg.dat file as preferred by James.
Runactivities.zip
Semaphores3a.zip

This is worse because then the value other parts of the program see depends on whether a viewer signal has been created for it. The super-elevation code was doing something similar, only initialising simulation values when the viewer viewed that area. Instead, keep the code in the Viewer's signal code but don't let it leak out, and make sure to include a comment listing all the specific cases we're correcting.

Sorry about going around in a circle here. :sign_sorry:

[Csantucci]

Nice that we are arriving to a shared solution.
So the patch is as follows (removing the modification of the raw data read from the sigcfg.dat and leaving the "Biggetal" part).
 Signals_semaphores5.cs.patch.zip (1.02K)
Number of downloads: 211
I will upload it this way. It doesn't indicate in the logfile the signal affected as of now, because it writes only a line for every OR run; writing a line every time the patch modifies the semaphore indexes would mean that a line is written for every instance of the affected signal shapes, and that would in my opinion fill up the logfile with information messages. However if there is a better way to report the patch action in the logfile, I can improve the bug fix subsequently.

P.S. the above patch has been uploaded as x.3735. Then a further patch adding explanatory comments has been uploaded in x.3736, see next post.