The International Simutrans Forum

 

Author Topic: Convoy length calculation bug  (Read 963 times)

0 Members and 1 Guest are viewing this topic.

Offline Ranran jp

  • *
  • Posts: 293
  • Languages: ja
Convoy length calculation bug
« on: September 17, 2018, 03:38:51 AM »
Hi. (´・ω・`)
If you change the order of vehicles, the convoy length (required total station tiles) may change.

Steps to reproduce:
1. Start the game from 1830

2. Construct tracks and train depot and make convoy in the following order
Code: [Select]
B&LR "Lancashire Witch" 0-4-0
Four wheel mail brake carriage (+ mail locker)
Four wheel brake carriage (first class)
NOTE: This order is very important


3. Change the order as follows
Code: [Select]
B&LR "Lancashire Witch" 0-4-0
Four wheel brake carriage (first class)
Four wheel mail brake carriage (+ mail locker)


4. Now Station tiles has changed from 1 to 2 ::(



This is thought to occur when "length = 0 vehicle" such as railway-mail-locker comes to the convoy end.

Offline ACarlotti

  • *
  • Posts: 316
Re: Convoy length calculation bug
« Reply #1 on: September 17, 2018, 05:35:19 AM »
This is the result of a deliberate fudge. The point at which it is applied (in this context) is in vehicle_summary_t::update_summary. The same fudge appears to be intended in convoi_t::get_tile_length(). However, I think the duplication of functionality may have allowed a bug to creep in, since these two methods use different constants for what I think is intended to be the same fudge.

convoi_t::get_tile_length() gives some hint as to what is going on.
Code: [Select]
// the last vehicle counts differently in stations and for reserving track
// (1) add 8 = 127/256 tile to account for the driving in stations in north/west direction
//     see at the end of vehicle_t::hop()
// (2) for length of convoi for loading in stations the length of the last vehicle matters
//     see convoi_t::hat_gehalten

So as far as I can tell, convoys wouldn't automatically end up correctly aligned with a station (or other stopping point) when travelling north or west. (Why? I don't know. I would love to hear an explanation.) So to fix this graphics issue, the convoy is halted (just under) halfway through the tile when arriving at a station travelling in those directions. But then there is a risk that the last vehicle is shown (and oriented) as if it were on the tile before the station. This wouldn't (usually) have been a trouble when all vehicles were exactly half a tile long. But now that some vehicles are shorted it is necessary to add a bit of padding to ensure that the last vehicle always reaches the last tile the train is on (or rather, to ensure that the tile the last vehicles is shown on is considered to be part of the train).
In practice this means the last vehicle is always assumed to be at least half a tile long.

Offline DrSuperGood

  • Dev Team
  • Devotee
  • *
  • Posts: 2581
  • Languages: EN
Re: Convoy length calculation bug
« Reply #2 on: September 17, 2018, 02:18:46 PM »
In the case of the mail locker and such it does not matter if it is on the tile or not. I guess the solution would be to disable the "fudge" for 0 length?

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18210
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Convoy length calculation bug
« Reply #3 on: September 18, 2018, 09:03:39 PM »
This is a curious one - I did not write either versions of this code, and I cannot see that they are other than superficially different. I have tired to make them more consistent with one another, but I am not sure that this will have had any effect.

Offline ACarlotti

  • *
  • Posts: 316
Re: Convoy length calculation bug
« Reply #4 on: September 18, 2018, 09:36:58 PM »
I have tired to make them more consistent with one another,

I don't expect that to be causing any issues at present, although it's possible that there are (or, at least, were) assumptions being made that certain hard-coded or configurable settings are the same. I've not investigated where those parameters come from.

Offline Ranran jp

  • *
  • Posts: 293
  • Languages: ja
Re: Convoy length calculation bug
« Reply #5 on: September 19, 2018, 10:16:27 AM »
@ACarlotti - thank you for detail information.

I noticed length calculation is unnatural regardless of mail-locker.
It seems that "padding" as ACarlotti said is inserted also in the depot.


The length of this image should be
2 + 2 + 3 + 0 + 3 = 10
But connecting more than this will exceed 16 (1 tile). ???

In my hypothesis, an invisible padding is inserted so that the last vehicle is 8 if length is less than 8
That is, 1 is inserted if the length of the last vehicle is 7 and 8 is inserted if the length of the last vehicle is 0.

Therefore, when the length of the last vehicle becomes small, the padding increases and the total convoy length increases.
This includes the case of reversing at the station.

I made a saved game to reproduce the problem it caused. Please check.

.
.
.

It is just after departure from the depot when open the saved game.
You can see that this convoy's required station tiles is 1 in the convoy details window.
Please keep convoy detail window open.
Next, the convoy arrives at Bams station and reversing. At this time you can see that station tiles in the convoy detail window have changed to 2.
The convoy will arrive again at Fields station and reversing. Since the home length is 2 at this time, this convoy becomes "no route" status and can not reach Bam station despite being the station that it arrived last time.


There is a possibility that deadlock may be caused if convoy including short vehicle is reversed like this.
(´・ω・`)

Offline ACarlotti

  • *
  • Posts: 316
Re: Convoy length calculation bug
« Reply #6 on: September 19, 2018, 10:42:41 AM »
n my hypothesis, an invisible padding is inserted so that the last vehicle is 8 if length is less than 8
That is, 1 is inserted if the length of the last vehicle is 7 and 8 is inserted if the length of the last vehicle is 0.
That is correct.

This includes the case of reversing at the station.
Bother. The easiest 'fix' in the short term will be to add more padding according the maximum required either before or after reversing. Since I'm unfamiliar with the precise rules for reversing I don't know how easy that will be, but in the worst case you could just add half a tile of padding to all trains. Except that will probably break existing games, so I think we need to make the padding as efficient as possible.

In the long run (particular with convoy recombination being developed) I would like to be able to remove the need for padding - it makes the code more complicated and seems like it shouldn't fundamentally be necessary. I also suspect it is related to funny graphics alignment when turning corners, However, the fact that this hasn't previously been implented in Standard suggests that it could be infeasible to actually change.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18210
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Convoy length calculation bug
« Reply #7 on: September 19, 2018, 10:54:48 AM »
If it is of any assistance, here is the algorithm for reversing: essentially, the actual order of the vehicles in the train is simply swapped into a reversed order one by one:

Code: [Select]
void convoi_t::reverse_order(bool rev)
{
// Code snippet obtained and adapted from:
// http://www.cprogramming.com/snippets/show.php?tip=15&count=30&page=0
// by John Shao (public domain work)

uint8 a = 0;
    vehicle_t* reverse;
uint8 b  = vehicle_count;

working_method_t wm = drive_by_sight;
if(front()->get_waytype() == track_wt || front()->get_waytype() == tram_wt || front()->get_waytype() == maglev_wt || front()->get_waytype() == monorail_wt)
{
rail_vehicle_t* w = (rail_vehicle_t*)front();
wm = w->get_working_method();
}

if(rev)
{
front()->set_leading(false);
}
else
{
if(!back()->get_desc()->is_bidirectional())
{
// Do not change the order at all if the last vehicle is not bidirectional
return;
}

a++;
if(front()->get_desc()->get_power() > 0)
{
// If this is a locomotive, check for tenders/pair units.
if (front()->get_desc()->get_trailer_count() > 0 && vehicle[1]->get_desc()->get_leader_count() > 0)
{
a ++;
}

// Check for double-headed (and triple headed, etc.) tender locomotives
uint8 first = a;
uint8 second = a + 1;
while(vehicle_count > second && (vehicle[first]->get_desc()->get_power() > 0 || vehicle[second]->get_desc()->get_power() > 0))
{
if(vehicle[first]->get_desc()->get_trailer_count() > 0 && vehicle[second]->get_desc()->get_leader_count() > 0)
{
a ++;
}
first++;
second++;
}
if (vehicle_count > 1 && vehicle[1]->get_desc()->get_power() == 0 && vehicle[1]->get_desc()->get_trailer_count() == 1 && vehicle[1]->get_desc()->get_trailer(0) && vehicle[1]->get_desc()->get_trailer(0)->get_power() == 0 && vehicle[1]->get_desc()->get_trailer(0)->get_value() == 0)
{
// Multiple tenders or Garretts with powered front units.
a ++;
}
}

// Check whether this is a Garrett type vehicle (with unpowered front units).
if(vehicle[0]->get_desc()->get_power() == 0 && vehicle[0]->get_desc()->get_total_capacity() == 0)
{
// Possible Garrett
const uint8 count = front()->get_desc()->get_trailer_count();
if(count > 0 && vehicle[1]->get_desc()->get_power() > 0 && vehicle[1]->get_desc()->get_trailer_count() > 0)
{
// Garrett detected
a ++;
}
}

// Check for a goods train with a brake van
if((vehicle[vehicle_count - 2]->get_desc()->get_freight_type()->get_catg_index() > 1)
&& vehicle[vehicle_count - 2]->get_desc()->get_can_be_at_rear() == false)
{
b--;
}

for(uint8 i = 1; i < vehicle_count; i++)
{
if(vehicle[i]->get_desc()->get_power() > 0)
{
a++;
}
}
}

back()->set_last(false);

for( ; a<--b; a++) //increment a and decrement b until they meet each other
{
reverse = vehicle[a]; //put what's in a into swap spacekarte_t::load(
vehicle[a] = vehicle[b]; //put what's in b into a
vehicle[b] = reverse; //put what's in the swap (a) into b
}

if(!rev)
{
front()->set_leading(true);
}

back()->set_last(true);

reversed = !reversed;
if (rev)
{
re_ordered = !re_ordered;
}

for(const_iterator i = begin(); i != end(); ++i)
{
(*i)->set_reversed(reversed);
}

if(front()->get_waytype() == track_wt || front()->get_waytype()  == tram_wt || front()->get_waytype() == maglev_wt || front()->get_waytype() == monorail_wt)
{
rail_vehicle_t* w = (rail_vehicle_t*)front();
w->set_working_method(wm);
}

welt->set_dirty();
}

It would be splendid if padding could be eliminated - but working out how to do that would involve diving into the endless rabbit-hole that is the graphics code and I should not even know where to begin with that.

Offline Ranran jp

  • *
  • Posts: 293
  • Languages: ja
Re: Convoy length calculation bug
« Reply #8 on: September 19, 2018, 04:47:54 PM »
I suppose I fixed this bug.
Please check my Github branch.

It seems to be working properly.


2 + 2 + 3 + 3 + 3 + 3 + 0 = 16 ... 16/16 = 1 tile    (OK)


2 + 2 + 3 + 3 + 3 + 3 + 0 + 3 = 19 ... 19/16 = 2 tiles   (OK)



2 * 8 = 16 ... 16/16 = 1 tile    (OK)



2 * 7 + 3 = 17 ... 17/16 = 2 tiles    (OK)




Also it does not fluctuate the convoy length when reversing.


Is there any problem with the code? (´・ω・`)
« Last Edit: September 19, 2018, 05:09:20 PM by Ranran »

Offline ACarlotti

  • *
  • Posts: 316
Re: Convoy length calculation bug
« Reply #9 on: September 19, 2018, 09:52:35 PM »
Is there any problem with the code?
I think so. Try creating a train with vehicles whose lengths sum to 16 and whose last vehicle has length less than 8, and run it north or west into a station of length 2, where the previous tile of track is a 90° bend. When the train arrives, I think it will still be occupying the previous tile of track (i.e. the bend), and the last vehicle (or vehicles) will be orientated incorrectly for a station stop.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18210
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Convoy length calculation bug
« Reply #10 on: September 19, 2018, 10:34:27 PM »
Thank you both very much for your contributions on this: they are much appreciated.

Offline Ranran jp

  • *
  • Posts: 293
  • Languages: ja
Re: Convoy length calculation bug
« Reply #11 on: September 20, 2018, 05:12:09 PM »
@ACarlotti - Thank you very much for checking. That is the information you pointed out in the previous post, now I understand - my apologies. :-[



Apparently it still seems necessary to fix other codes. I tried some fixing on my Github branch again.

Quote
When the train arrives, I think it will still be occupying the previous tile of track (i.e. the bend), and the last vehicle (or vehicles) will be orientated incorrectly for a station stop.

I think that this issue was solved by deleting the following part in vehicle_t::hop().
Code: [Select]
if(  check_for_finish  &  leading && (direction==ribi_t::north  || direction==ribi_t::west))
{
steps_next = (steps_next/2)+1;
}

Then it came to hopping out from the station when reversing, so I commented out the movement process when reversing in convoi_t::vorfahren(), but I think that adjustment is necessary.

Although adjustment may be necessary, I suppose that it becomes somewhat decent behavior.
I am not confident that I can solve this problem altogether, so I hope if anyone could give your help for solve this.
Thank you.(´・ω・`)

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9376
  • Languages: De,EN,JP
Re: Convoy length calculation bug
« Reply #12 on: September 21, 2018, 11:28:49 AM »
To alleviate the issue, the loading of vehicles is based on the reported convoi length, not if those are within the station. However, choosing singals assume the convoi fits into the station when it length is shorter than the tile count. Occupying the tile in front of the station may block following trains to enter. So it is adviseable to rather report a too short than a too long length, i.e. begin a new station tile every 16+8 train length, i.e. (tiles in srtation=(train length+8)/16). At rail signals (at least a few version ago) and at road crossings the vehicle always go to the end of a tile, at the border to the next tile. This is why some buses are happily inside crossings at read lights.

If one is ok with the vehicles jumping further in curves, one could align them differently and thus let them drive towards then end of a station. But this requires realignment of all vehicles in four of the eight directions and different offsets for four directional vehicles, i.e. eight images instead of four. Thus this was never done, although there was once a version where the special code for stations in those two directions was removed.
« Last Edit: September 23, 2018, 11:40:55 AM by prissi »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9376
  • Languages: De,EN,JP
Re: Convoy length calculation bug
« Reply #13 on: September 22, 2018, 02:28:19 PM »

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18210
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Convoy length calculation bug
« Reply #14 on: September 22, 2018, 02:47:32 PM »
Gosh, 2009 - that is a long time ago.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18210
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Convoy length calculation bug
« Reply #15 on: December 24, 2018, 10:53:54 PM »
Can I check - Ranran, do you believe that your changes on the fix-convoy-length branch are ready for integration, or does this need more work? If it does need more work, is there any worthwhile subset of changes/fixes that can be incorporated forthwith without further work?

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9376
  • Languages: De,EN,JP
Re: Convoy length calculation bug
« Reply #16 on: December 25, 2018, 03:32:51 AM »
The bug comes from a simple reason. With the zero length car not at the back, all cars are before tile 16*n+8 and thus the train needs n+1 tiles to fully load.

However, with the zero length car trailing, this car will be stopping at a position larger than 16*n+8. Hence this train will need n+2 tiles for loading, since the starting position of a car determines if it can load or not at a station. One must add extra logic which applies only to trailing cars of length zero for that. It may missing from standard as well, but is not used so extensively there.

Offline Ranran jp

  • *
  • Posts: 293
  • Languages: ja
Re: Convoy length calculation bug
« Reply #17 on: December 27, 2018, 09:26:32 AM »
I tried testing that build again, but I think it would be better to redo the fix from scratch. (There's nothing more I can do.) (´・ω・`)
The issue of the reference position of the convoy has not been solved and found some bugs.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18210
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Convoy length calculation bug
« Reply #18 on: December 27, 2018, 01:51:42 PM »
Thank you for confirming - that is most helpful.