IguannaB wrote:

Actually, it's 0.6.0 beta 4 - I forgot to change it :)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3518

peter1138 wrote:

This is probably too much work for too little benefit...

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3519

SmatZ wrote:

Caused by rounding at train_cmd.cpp : 2704 for trains with non-diagonal direction

if (!(v->direction & 1)) spd = spd * 3 >> 2;

It depends on order in which the trains enter curves, too.

Most likely won't change.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3520

IguannaB wrote:

I added some code using a double to save the remainder and remove the rounding error, and found there never was one in the test: spd 640 * 3 >> 2 = 480 - no remainder. Trains get to 640 before they reach any diagonal track where the rounding would occur.

Taking out the 'if (!(v->direction & 1)) spd = spd * 3 >> 2;' does fix it, but trains look a bit funny going so fast diagonally. I'm going to try and modify the current code to work as it does now, but without the problem I spoke of and see if that make a difference.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3521

SmatZ wrote:

I was wrong:

UpdateTrainSpeed is called before it enters the curve, and it returns number of "pixels" it has to move.
If it enters/leaves curve in these two or more "moves", the speed is not updated.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3522

IguannaB wrote:

Well, it did make a difference - it fixed the problem (draft patch attached). I've tried to change as little as possible, but there were some things that were difficult to avoid.

The way it was before, it would work out the spd it had to use that tick, then scale it if the train was pointing diagonally, and this spd divided by 256 was the number of moves it would make - regardless of what direction the moves actually ended up being in.

In the patch, I put the move loop from TrainLocoHandler() inside of UpdateTrainSpeed() after the bit where it has worked out the speed it has to use. At the start of the loop it subtracts from spd an amount based on direction (256 for straight track, and 341 for diagonal). It loops as many times as it has spd to move with, then stores the remaining spd in progress similar to how it did before. Unfortunately, since spd is not scaled at this point the remainder stored in progress can be up to 340, and this is now too large to fit in a byte. I've changed the datatype to uint16 for testing. Is it possible to easily change that, or will it break savegame compatibility etc?

Other differences are:
TrainCheckIfLineEnds(v); is called for each move, not just the 1st one.
Previously, if (j == 0) and if (v->cur_speed != 0) it would return without calling SetLastSpeed(v, v->cur_speed). It is always called now. I could add code so it exactly as before, but I'm not sure if it's needed. If so, let me know and I'll change it.
Besides that, as far as I'm aware it will work the same. Diagonal movement will be a tiny bit slower since it really needs 341.33333 spd to move diagonally, and not it uses only 341, but I don't expect that will matter. It may have been rounded before anyway - I'm not sure.

Anyhow, if you could please let me know if this idea is acceptable, and/or what changes are needed that would be great. I haven't checked the style yet (I think the comments are wrong) or cleaned it up, but I can do that. This was just to try the idea to see if it worked any better and find out if it would be an acceptable change. Cheers.

Attachments

• Inconsistant travel time fix v0.2 - 0.6.0 beta 4.patch (4.83 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3523

IguannaB wrote:

I found a way to do this without having to change the datatype for 'progress' - always scale the speed, have going diagonally use 256 spd and have going straight use 192. This is probably better as 256 * 3 >> 2 = 192, so there is no rounding introduced there.

I'll run some timing tests to check the speed is unchanged for slow trains etc.

Attachments

• Inconsistant travel time fix v0.3 - 0.6.0 beta 4.patch (4.27 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3524

Belugas wrote:

Don't add stuff that is beyond the scope of your proposal

Code style:
- /* if the vehicle has speed 0, update the last_speed field. */
+ // if the vehicle has speed 0, update the last_speed field.
The comment style is right.
Thus your change is not right

end of line comment : "// bla"
comment on its own line : "/* bla */"
tabs are used from beginning of line(indentation and all), spaces are used for aligment.
read back your patch (nor your code) and you'll see a lot of rogue spaces

spd_needed_to_change_position = !(v->direction & 1) ? 256 : 192;
or, of course
spd_needed_to_change_position = (v->direction & 1) ? 192 : 256;
and those values could have an enum, maybe...

position_changed = spd >= spd_needed_to_change_position);
if (position_change) {
bla
}

Don't keep the old code. it's easier to see what has been removed this way

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3525

IguannaB wrote:

Thanks, and sorry for the delay - I meant to get look at this earlier but was caught up doing other things.

I'm not sure if you meant me to fix this up and re-submit it or if it was no good, so I've made the changes and attached it:

- changed some spaces to tabs in the code (Not sure what you meant about reading patch back. I tried opening the .patch in a text editor and removing the spaces at the start, but it just gave an error when I went to apply it (TortoseSVN).)
- change oneway signal wait code/values to be the same as twoway (traincmd.cpp:3099). This corrects a bug in v0.6.0 beta 4 where fast trains don't wait long enough at one way signals.
- format change: if -> spd_needed_to_change_position = (v->direction & 1) ? 192 : 256; (I looked for an enum for 256, 192 and there isn't one. These values are only used in UpdateTrainSpeed(), and I didn't know where to add the enum so didn't.
- format change: position_changed = spd >= spd_needed_to_change_position);
- fix: to allow 'position' to be set in TrainController() as it is when waiting at signals. The way I had it to begin with, it was overwritten after TrainController() returned.
- fix: the speeds was not always shown as 0 when stopped waiting at a signal.

I checked with fast and slow trains and as far as I can tell this works exactly how it did before, but without the error. When previewing the patch, there are a lot of lines that showed changed which aren't - just have comments added at the end.

The save game attached is a test which jams after 1-2 loops on beta4 without this patch due to the small error. This doesn't occur with the patch. It's meant to simulate a branch & merge for a 6 track mainline, like you might have at a T junction. Having the clock sync the train starts so the relative position of the trains is such that they can switch tracks and fit into gaps seems to work well.

It would be great to get this fix included with v0.6 so if I try setting up a passenger network using this idea, it will run. Like I said, I'm not sure if this was rejected or what. Logically, I think it's sound now as there were no further problems I noticed doing that test, or in any others I ran with slow trains. If there's anything that needs changing, I'm happy to do it, or if there is some reason stopping it from being used, like too big a change or what ever, it would be nice to know what that is too.

Attachments

• Inconsistant travel time fix v0.5 - 0.6.0 beta 4.patch (3.95 KiB)
• Clock Merger Test.sav (310.87 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3603

matthijs wrote:

I've been reading the patch and have some comments.

- v->progress = 255 - 100;
- if (++v->load_unload_time_rem < _patches.wait_oneway_signal * 20) return;
+ v->progress = 255 - 10;
+ if (++v->load_unload_time_rem < _patches.wait_oneway_signal * 73) return;
This part looks completely unrelated to me. If so, it should probably be put into a seperate patch. In any case, it could use some documentation, and since you've figured out what it does and why, I suggest you add it :-)

Furthermore, I can't really see why the loop has to be moved from TrainLocoHandler() to UpdateTrainSpeed(). As far as I can see, UpdateTrainSpeed can just return spd (so not spd / 256) and let TrainLocoHandler do the loop (but instead of spd-- as originally let it do spd -= 256 or 192 as in your patch). That way, there is less code movement and things stay at more logical places. Essentially you then modify the meaning of the return value of UpdateTrainSpeed to be "256 * Number of movements on a diagonal track" whereas it used to be "Number of movements on the current track type". If you do this, TrainLocoHandler should be able to handle the loop as it does now (but by decrementing by 256 or 192 instead of always 1).

Furthermore, you might want to spend a few more words of comment on the spd = spd * 3 / 4 part in UpdateTrainSpeed. I've been under the impression for a while that it was double work to do that and also use 256 and 192 for different tile types. Yet it turns it it actually does make sense. The essential thing is that spd, as calculated by UpdateTrainSpeed is "256 * The number of movements on a straight track". In this light, a straight track costs 256 spd while a diagonal track (being longer) costs 256 * 4/3 = 341. Yet, as stated above, we don't want UpdateTrainSpeed to return that value, in particular because we don't want v->progress to have the same meaning (It would allow a remaining spd of 256 < spd < 341, which we can't store in progress, which is only a byte).

Because we don't want to completely rewrite all of the speed related code so that spd will mean "256 * The number of movements on a diagonal track", you settle for scaling spd halfway UpdateTrainSpeed such that spd (and, in my proposal, the return value of UpdateTrainSpeed) now means "256 * The number of movements on a diagonal track", whereas a diagonal track now costs 256 (instead of 341) and a straight track costs 192 (instead of 256). This requires only limited changes while still allowing any leftover spd to be stored in a byte (since the largest amount of speed required for a single step is now 256).

And of course you probably knew all of this already (the last 2 paragraphs anyway), but I intended my explaination both for other developers and perhaps you could base the extra comments I proposed on them. Perhaps you could also give spd another name after the scaling, to emphasize the change in meaning (so, spd_scaled = spd * 3 / 4 or something like that).

Lastly, I'd like to note that you could look at the values above as fixed point numbers, where you can imagine a point being before the eight bit (so 192 is reall y 0.75, or 0.1100000 in binary).

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment3735

fonsinchen wrote:

I reduced the patch to its minimum and addressed the critique by matthijs. Note that the following lines inside the loop had no effect before, so I removed them:

if (v->cur_speed <= 0x100)
break;

I didn't test with the latest save game by IguannaB because I don't get how it is supposed to work. However the trains in the first save game here (http://bugs.openttd.org/task/1793/getfile/2510/Inconsistant%20Travel%20Time%20-%20start.sav) are perfectly in sync with this patch.

Attachments

• traveltime_r14405.diff (2.07 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment4786

matthijs wrote:

The patch looks fine to me now. I do have a small extra comment, to better document the return value of UpdateTrainSpeed()

/**
* This function looks at the vehicle and updates it's speed (cur_speed
* and subspeed) variables. Furthermore, it returns the distance that
* the train can drive this tick. This distance is expressed as 256 * n,
* where n is the number of straight (long) tracks the train can
* traverse. This means that moving along a straight track costs 256
* "speed" and a diagonal track costs 192 "speed".
*/
static int UpdateTrainSpeed(Vehicle *v)

With this addition, the patch looks ok to commit (but I don't have the time to properly test and commit it right now).

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793#comment4799

Rubidium closed the ticket.

Reason for closing: Fixed

In r14436.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1793