The International Simutrans Forum

Simutrans Extended => Simutrans-Extended development => Simutrans-Extended bug reports => Topic started by: Ranran(retired) on September 17, 2018, 03:38:51 AM

Title: Convoy length calculation bug
Post by: Ranran(retired) 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

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
(https://i.imgur.com/K5qOckD.png)

3. Change the order as follows

B&LR "Lancashire Witch" 0-4-0
Four wheel brake carriage (first class)
Four wheel mail brake carriage (+ mail locker)

(https://i.imgur.com/HCkw8rp.png)

4. Now Station tiles has changed from 1 to 2 ::(
(https://i.imgur.com/0DFZsIF.gif)


This is thought to occur when "length = 0 vehicle" such as railway-mail-locker comes to the convoy end.
Title: Re: Convoy length calculation bug
Post by: ACarlotti 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.

// 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.
Title: Re: Convoy length calculation bug
Post by: DrSuperGood 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?
Title: Re: Convoy length calculation bug
Post by: jamespetts 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.
Title: Re: Convoy length calculation bug
Post by: ACarlotti on September 18, 2018, 09:36:58 PM
Quote from: jamespetts on September 18, 2018, 09:03:39 PMI 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.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) 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.

(https://i.imgur.com/K5qOckD.png)
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 <a href="https://drive.google.com/open?id=1FSIa03XY29lVO_hpHixp_8MJNgWkv1C5">saved game</a> 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.
(https://i.imgur.com/33nwXxR.png)

There is a possibility that deadlock may be caused if convoy including short vehicle is reversed like this.
(´・ω・`)
Title: Re: Convoy length calculation bug
Post by: ACarlotti on September 19, 2018, 10:42:41 AM
Quote from: Ranran on September 19, 2018, 10:16:27 AMn 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.

Quote from: Ranran on September 19, 2018, 10:16:27 AMThis 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.
Title: Re: Convoy length calculation bug
Post by: jamespetts 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:


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.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on September 19, 2018, 04:47:54 PM
I suppose I fixed this bug.
Please check my Github branch (https://github.com/Ranran-the-JuicyPork/simutrans-extended/tree/fix-convoy-length-calculation).

It seems to be working properly.

(https://i.imgur.com/uGPoTy2.png)
2 + 2 + 3 + 3 + 3 + 3 + 0 = 16 ... 16/16 = 1 tile    (OK)

(https://i.imgur.com/lmbWVh4.png)
2 + 2 + 3 + 3 + 3 + 3 + 0 + 3 = 19 ... 19/16 = 2 tiles   (OK)


(https://i.imgur.com/IPVfoJz.png)
2 * 8 = 16 ... 16/16 = 1 tile    (OK)


(https://i.imgur.com/9aRS2hP.png)
2 * 7 + 3 = 17 ... 17/16 = 2 tiles    (OK)




(https://i.imgur.com/2wqk8oo.gif)
Also it does not fluctuate the convoy length when reversing.


Is there any problem with the code? (´・ω・`)
Title: Re: Convoy length calculation bug
Post by: ACarlotti on September 19, 2018, 09:52:35 PM
Quote from: Ranran on September 19, 2018, 04:47:54 PMIs 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.
Title: Re: Convoy length calculation bug
Post by: jamespetts on September 19, 2018, 10:34:27 PM
Thank you both very much for your contributions on this: they are much appreciated.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) 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 (https://github.com/Ranran-the-JuicyPork/simutrans-extended/commits/fix-convoy-length-calculation) 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().

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.(´・ω・`)
Title: Re: Convoy length calculation bug
Post by: prissi 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.
Title: Re: Convoy length calculation bug
Post by: prissi on September 22, 2018, 02:28:19 PM
Here is the thread in question:
https://forum.simutrans.com/index.php/topic,3376.0.html
Title: Re: Convoy length calculation bug
Post by: jamespetts on September 22, 2018, 02:47:32 PM
Gosh, 2009 - that is a long time ago.
Title: Re: Convoy length calculation bug
Post by: jamespetts 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?
Title: Re: Convoy length calculation bug
Post by: prissi 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.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) 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.
Title: Re: Convoy length calculation bug
Post by: jamespetts on December 27, 2018, 01:51:42 PM
Thank you for confirming - that is most helpful.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on January 09, 2020, 03:46:54 PM
I remembered that Ranran which translated Japanese help text pointed out inconsistencies in the content of the English help text.
depot.txt says:
The length is important because, if a convoy is too long for a stop, the vehicles at the back will be left out of the stop and will not be able to load/unload there.
Standard has such specifications, but Extended does not have such specifications and convoy is stuck if convoy exceeds station tile length . Therefore, this description seems to be incorrect.
By the way, point is, why does extended not allow exceeding station length?  ???
At worst, this bug should avoid getting stuck if the length of the station platform can be exceeded. (Note that the vehicle is still considered unexpectedly exceed the station. But think better than the stuck.)
And there are many examples in Japan and Asia that the train exceed the length of the station.
Actually, passengers can get off by moving to the previous vehicle. Or sometimes exceeding vehicles are mail and cargo vehicle that will not be load/unloaded at the station on the way.
Title: Re: Convoy length calculation bug
Post by: Vladki on January 09, 2020, 04:31:58 PM
It happens occasionally on czech railways too. Passengers are told in advance that if they want to get off at some station, they have to go to the first car. But there is no such mechanism in extended. In standard, you at least know which cars are exceeding the platform, and arrange the train accordingly, but in extended it won't work due to reversal mechanics.
Title: Re: Convoy length calculation bug
Post by: Mariculous on January 09, 2020, 05:06:48 PM
Same for Germany, at least these days, at some stations where platforms were not upgraded in time. This also happened in Britain with class 373 trains in GNER service from London to York and Leeds.

However, stucking because of being too long for a platform is still better than standards behavior, where people to a short station will stack up at cars that can't unload at the station and will never get out of it, effectively making that car useless ballast.

When allowing trains to be longer than the platform, we should in my opinion, at least warn players in lines status about "some vehicle is too long for some station in the line" and let passengers leave the train at increased loading times if a car does not fit in the station.
Also keep choose signals in mind. These should never route a train to a platform that is shorther than the scheduled one.

In addition, I don't think this special length calculation for the last car is a good idea. I already had some trains stucking after they reversed because of this.
If you think this is a good idea, we should really warn the player in the depot window if the train will be longer after reversing.
Title: Re: Convoy length calculation bug
Post by: jamespetts on January 09, 2020, 06:48:19 PM
Thank you for noting this. The correct behaviour where a train is too long for a station should be that it takes longer to load/unload as passengers may have to move through the train to get off. I will have to check the code when I get home (eventually - there is an extremely long queue of things for me to do) to see whether this is what happens and, if so, amend the help text accordingly.
Title: Re: Convoy length calculation bug
Post by: Vladki on April 25, 2020, 12:00:55 AM
Sorry to revive an old topic, but I was just wondering why modern british trams cannot fit in one tile, even if the lenghts add up to 16.

Quote from: prissi 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.

So if I understand correctly - any vehicle that starts in second half of tile will require one more tile for loading?. Lets say If I have a tram with lengths 9+7, it will need two tiles, but 8+8 will need only one? Is this logic used for anything else then loading? E.g. track reservations?
Title: Re: Convoy length calculation bug
Post by: Mariculous on April 25, 2020, 11:22:52 AM
Quote from: Vladki on April 25, 2020, 12:00:55 AMbut 8+8 will need only one?
Exacly. 8 is the default car length if you don't specify anything btw.

Quote from: Vladki on April 25, 2020, 12:00:55 AMIs this logic used for anything else then loading? E.g. track reservations?

Not sure from the codebase, but doesn't seem so from ingame observations as 9+7 trams seem to fit on a single tile e.g. when waiting at a signal.
Title: Re: Convoy length calculation bug
Post by: prissi on April 25, 2020, 12:52:24 PM
Before car position (inside and outside of station) was used to determine if it can be loaded (because on old Simutrans one tile was enough to unload any train, like TTD).

Then it was noted that trains could only unload in one direction. and it changed when the map was rotated. Hence the unloading length calculation was seperated from the position of the car.

Putting a zero length car inside the train at the previous to last position is indeed asking for troubles.
Title: Re: Convoy length calculation bug
Post by: Vladki on April 25, 2020, 01:46:32 PM
The problem is also with articulated trams, that are composed of many small parts, e.g. lengths 3+4+2+4+3 = 16, should fit on one tile.
Title: Re: Convoy length calculation bug
Post by: Mariculous on April 25, 2020, 01:53:45 PM
We don't have zero length cars here.
Many trams are built up from many quite small sections of less than 15m each being slightly less than 30m (1 tile) length in total.

Such vehicles are an isue in simutrans as they will rquire longer platforms than they should.
There are very many examples for such trams in the real world. Especially, but not limited to trams from 2000 and later.
Title: Re: Convoy length calculation bug
Post by: prissi on April 26, 2020, 02:55:48 PM
The calculation is consistent with the positioning of the cars that the first tile is only 8 long, all following tiles are 16 long.

effective length of station=(tile*16-8)

If you change this all vehicles need realignment and some special code for cornering and diagonal movement.
Title: Re: Convoy length calculation bug
Post by: Mariculous on April 26, 2020, 04:08:29 PM
Quote from: prissi on April 26, 2020, 02:55:48 PMeffective length of station=(tile*16-8)
That doesn't seem to be true.
A single car of length 16 fits on one tile stop.
Two cars of length 8+8 also fit on a single tile stop.
Two cars of length 9+7, however, don't fit on a single tile stop.
Title: Re: Convoy length calculation bug
Post by: prissi on April 27, 2020, 01:14:41 PM
It is the start position of the car, as mentioned before. A single car will always on position 0. But if the length is longer than 8, the second car will be starting on the bend outside the stations. This is why it matters where a zero length car is. Because if it is inside the convoi, all cars are loaded. At the end, this car starts with the endposition, which could be outside of the stations.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on April 29, 2020, 07:38:26 AM
Drawing the vehicle in the correct position and calculating the correct convoy length are important to the convoy recombination system.
Currently, the system has many problems. When you mix reversed vehicles, the drawing position of convoy is chaos.

For example in Britain-ex, you may see a weird large space between the tender and the next coach on the return trip of the SL train. This is impossible to solve with current systems. Because standard took no account of reversing the vehicle at all, but extended inherited that system. Therefore, extended needs its own underlying solution.

Breaking true convoy length calculations due to small vehicles will now be visible graphically in the tile occupancy bar which is unincorporated patch I made. I tried to faithfully reproduce the formula.
It is sometimes seen when connecting small vehicles. Then players will find that strange behavior. But that is just a correct representation of the internal calculations. (´・ω・`)
Title: Re: Convoy length calculation bug
Post by: jamespetts on May 13, 2020, 03:37:41 PM
Quote from: Ranran on April 29, 2020, 07:38:26 AM
Drawing the vehicle in the correct position and calculating the correct convoy length are important to the convoy recombination system.
Currently, the system has many problems. When you mix reversed vehicles, the drawing position of convoy is chaos.

This may well need a considerable amount of work.

Does anyone have any ideas as to an efficient implementation of a better system that will not require re-rendering the graphics in the pakset, which is not realistically feasible for Pak128.Britain-Ex owing to the number of vehicles?
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on May 13, 2020, 10:19:04 PM
Quote from: jamespetts on May 13, 2020, 03:37:41 PMThis may well need a considerable amount of work.

Does anyone have any ideas as to an efficient implementation of a better system that will not require re-rendering the graphics in the pakset, which is not realistically feasible for Pak128.Britain-Ex owing to the number of vehicles?
This is a very important issue, so first I explain the mechanism by which it occurs to come up with a solution.

Look at the image.
(https://i.imgur.com/K7Quz3c.png)
This is an example of a vehicle drawn to current specifications. SW and NE, that is, one is the reverse one. I arranged them one above the other. Pay attention to the drawing position.
(https://i.imgur.com/hSC1iAY.png)

Reversed vehicles should be drawn in the same position if they may be mixed. Because the program just draws the same position for every vehicle.


Simutrans is not drawn in 3D technology. It simply puts a 2D picture at the calculated coordinates and overlays.

When the vehicles in the same position are made continuous, it is drawn like this.
(https://i.imgur.com/96sKOxH.png)


Then connect the reversed vehicle in the same way
(https://i.imgur.com/OY5LK13.png)


Next, connect the same vehicle as the first and second ones
(https://i.imgur.com/x8HPXll.png)

Currently, this symptom can be seen only in a few combinations such as un-bidirectional locomotives and bidirectional coaches and wagons.
It is noticeable when they are a certain length.


So we have to take one of the measures;
- We rewrite the image to draw in the same position (Redraw the drawing position code after drawing at the center position.)
- Corresponding this in code
- Correspond to some extent with code and rewrite the image only in the part that can not be complemented


Issues to consider:
1) Difference in drawing position in length.
Larger vehicles appear to be drawn in the center, but are actually only fitted to the front or rear reference position. Small vehicles are skewed to either side.

2a) Difference in drawing position for each pakset.
The reference position for pakset may be different.

2b) We should also consider pak size.

3) Differences depending on the waytype.
Road vehicles are not the center of the road. May need different rules than railroad.


In any case, I think we can't avoid the confusion of drawing positions without a lot of work. (´・ω・`)
Title: Re: Convoy length calculation bug
Post by: jamespetts on May 13, 2020, 10:48:22 PM
Thank you for this summary. I wonder whether there might be any role for offsets, which can be specified in .dat files, and possibly for new systems applying offsets dynamically? More thought will definitely be needed on this.
Title: Re: Convoy length calculation bug
Post by: TurfIt on May 13, 2020, 11:31:52 PM
Did the fixes in Understanding the graphics code: vehicle positioning (https://forum.simutrans.com/index.php/topic,17217.0.html) get undone?
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on May 14, 2020, 12:21:08 AM
Thank you for making me aware of that thread.
I think I have seen the symptoms several times since then.
For example is this a problem with the drawing position of this coach?
(https://i.imgur.com/emS1gOo.png)

I can't remember why I was aware of the symptom...

EDIT: The example seems to have a problem with the coach

EDIT2: I may have seen it with a deliberately reversed EMU/DMU or a Jalapagos customized pakset during patch testing. In that case, it may not be a problem. I think the former was simply caused by the fact that it didn't go through that process. I don't understand if that code does the right thing if the cars are reversed on each car.
Title: Re: Convoy length calculation bug
Post by: jamespetts on May 14, 2020, 04:02:04 PM
From what I can see of that thread, reversed vehicle graphics were not implemented at the time. I cannot remember enough of the relevant detail to work out how much work that it would be to implement this, but this is likely to need to be done before convoy recombination.
Title: Re: Convoy length calculation bug
Post by: jamespetts on May 30, 2020, 12:46:39 AM
I am taking some early steps to look into this. First of all, it seems that it will not be possible simply to re-use the existing offset code used when defining the vehicle graphics. This is because makeobj combines the offsets with the image dimensions and writes the product of the two into the pak file, effectively pre-calculating and combining the offset with the image itself such that there are no offset calculations performed in Simutrans itself.

Any offset functions will have to be copied from elsewhere or written from scratch. Ranran - are you able to set up a saved game where this can be tested effectively?

I am wondering whether the function used for showing the graphics of vehicles moving along tiles (get_screen_offset()) might be modified or co-opted for the purpose of imparting a new reversing offset. This is particularly relevant since the misalignment that you show in your illustration images shows that that the vehicles are only misaligned along the same axis as they travel, rather than the vertical axis and therefore it should be possible in principle to use the same basic algorithm here to impart a reversing offset of some sort which might be defined at pakset level (and which might programmatically depend on the vehicle length).

Thank you for your help with this.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on May 30, 2020, 06:03:11 AM
I made a saved game in which the vehicle was forcibly reversed. Please note that this will be restored when it arrives at the next station.
This is because the vehicle can currently be reversed only when the convoy is reversed. But once recombination is possible, such restrictions should disappear.

In the debug build, the reversed vehicle is marked like this.
(https://i.imgur.com/c5daZYE.png)

The britain-pakset does not show much image shift. Certain vehicles show a position mismatch.


However, I think that the vertical direction may not have the previous correction applied.


(https://i.imgur.com/B60mxmq.png)(https://i.imgur.com/CJEQlMJ.png)


Next, I think that the previous position correction processing is pak-britain or 128pixel size specific.
Reversing a pak256 vehicle alternatively makes it so terrible.  :::)
(https://i.imgur.com/vRxHusF.png)


saved game for testing is here:
Vehicles are alternately reversed:
https://drive.google.com/open?id=1d5gnRdQ11-WQQjikeAT7Gj8pVB26O-Ht

The vehicle is reversed once every three cars:
https://drive.google.com/open?id=1lDx0FLga7F-eQoVEfMC9kTjaTQbDc6t1

Pak256:
https://drive.google.com/open?id=18-GpZJNX67zLlvviqyYSRXNyhb7qjP0F
Title: Re: Convoy length calculation bug
Post by: Phystam on May 31, 2020, 11:08:54 PM
Pak256 doesn't use the offset values for vehicles, but they are aligned only in the image files.
I've never seen such a strange alignment issue.
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on June 01, 2020, 03:09:08 AM
Quote from: Phystam on May 31, 2020, 11:08:54 PMI've never seen such a strange alignment issue.
Please read past posts in the thread.
This is possible if the recombination system is implemented with the current specifications. The experiment.
Title: Re: Convoy length calculation bug
Post by: jamespetts on June 08, 2020, 09:42:43 PM
I have been looking into this and have pushed some incomplete testing code to the reversed-vehicle-graphics branch on my Github repository. I have sought to address the problem by providing for reverencing specific offsets for each vehicle direction. I have so far completed only the cardinal directions (some of the ordinal directions did not need any adjusting). For testing purposes, I have hard-coded these numbers calibrated for Pak128.Britain-Ex, but, if this proves to be a workable approach, I will set these up to be user configurable in .dat files or (preferably) simuconf.tab (if I can get these to be universal - see below) or possibly both.

Here are some screenshots of results so far using Ranran's alternate reverse saved game:

(http://bridgewater-brunel.me.uk/screenshots/reversed-vehicle-graphics-4.png)

(http://bridgewater-brunel.me.uk/screenshots/reversed-vehicle-graphics-3.png)

However, this approach has not worked well so far for vehicles with greatly differing lengths:

(http://bridgewater-brunel.me.uk/screenshots/reversed-vehicle-graphics-1.png)

(http://bridgewater-brunel.me.uk/screenshots/reversed-vehicle-graphics-2.png)

I have found that there was already code to deal with reversed vehicle graphcs:


if (veh  &&  veh->is_reversed())
{
adjusted_steps += (VEHICLE_STEPS_PER_TILE / 2 - veh->get_desc()->get_length_in_steps());
}


Here is the relevant part of the code after my adjustments:


void vehicle_base_t::get_screen_offset( int &xoff, int &yoff, const sint16 raster_width ) const
{
sint32 adjusted_steps = steps;
const vehicle_t* veh = obj_cast<vehicle_t>(this);
const int dir = ribi_t::get_dir(get_direction());
if (veh  &&  veh->is_reversed())
{
adjusted_steps += (VEHICLE_STEPS_PER_TILE / 2 - veh->get_desc()->get_length_in_steps());
adjusted_steps += reverse_base_offsets[dir][2];
}

// vehicles needs finer steps to appear smoother
sint32 display_steps = (uint32)adjusted_steps*(uint16)raster_width;

if(dx && dy) {
display_steps &= 0xFFFFFC00;
}
else {
display_steps = (display_steps*diagonal_multiplier)>>10;
}

xoff += (display_steps*dx) >> 10;
yoff += ((display_steps*dy) >> 10) + (get_hoff(raster_width)) / (4 * 16);

if (veh && veh->is_reversed())
{
xoff += tile_raster_scale_x(reverse_base_offsets[dir][0], raster_width);
yoff += tile_raster_scale_y(reverse_base_offsets[dir][1], raster_width);
}

if(  drives_on_left  ) {
xoff += tile_raster_scale_x( driveleft_base_offsets[dir][0], raster_width );
yoff += tile_raster_scale_y( driveleft_base_offsets[dir][1], raster_width );
}
}



static sint8 reverse_base_offsets[8][3] =
{
{ 4, -1, 34 },
{ -1, 0, 2 },
{ 0, 0, 0 },
{ 0, 0, 0 },
{ -1, 0, 14 },
{ -3, -2, 32 },
{ 0, 0, 0 },
{ 0, 0, 0 }
};


I have borrowed the general scheme of the code for the system for driving on the left offsets (initial attempts not using this system resulted in inconsistent results depending on the zoom level). However, I had to add a third parameter to deal with position along the length of the vehicle, as this seems to be adjusted separately.

I wonder whether this last parameter is responsible for the inconsistent spacing of much shorter vehicles, although it is hard to tell without more detailed testing.

If anyone has any insight into this graphics code that might assist with this process, this would be much appreciated.
Title: Re: Convoy length calculation bug
Post by: jamespetts on June 10, 2020, 01:14:04 AM
I have been doing some more experimenting with this, and it seems that my conclusion in the previous post about vehicle length affecting the correctness of the spacing was incorrect. Having tried a number of algorithms in which the adjustment spacing was scaled with the length of the vehicle, I found that these all produced thoroughly inconsistent results, whereas the original algorithm (above) in which spacing did not scale with the length of the vehicle produced mostly consistent results, even with vehicles of differing lengths, but with a few anomalies.

I therefore investigated some of the individual anomalies. One of the most noticeable was in the container flat wagons in the freight train:

(http://bridgewater-brunel.me.uk/screenshots/faa-before.png)

As can be seen, alternating wagons are noticeably inconsistent with one another. However, I noticed that this wagon was using older graphics and had not been re-rendered with the new transparent system. After re-rendering, the spacings are now consistent:

(http://bridgewater-brunel.me.uk/screenshots/faa-after.png)

(I have pushed the re-rendered wagons to the master branch of the pakset as these are graphically preferable to the previous wagons in any event).

I have also looked into the LNER EM1 (the black and green electric locomotives). I found that these were significantly different in their position depending on their livery. I have re-rendered a number of these to be in a correct position, and, then finding that the length was too great, shortened this, and I now get a much more consistent appearance with these locomotives, without the large gap that previously existed.

The locomotive in the train pictured above, the BR Class 21, is unfortunately a locomotive for which .blend files are not available. I believe that this was produced by Junna some years ago; I wonder whether Junna might be able to find the graphics of this and other locomotives which were produced some time ago so that I can re-render these with the new transparent workflow?

Ranran - with the proviso that I have not yet coded the ordinal directions and that this is currently hardcoded for Pak128.Britain-Ex and not yet customisable, can you test this branch to confirm whether there is a significant improvement in the appearance of reversed graphics? If few anomalies remain, this offset system may be the solution. It would require configuring once per pakset, which might take some work to deduce the correct values, but once that has been done, that should be all the work required of a pakset author.

(You could possibly test this with Pak.256 by editing the values in reverse_base_offsets[][]).
Title: Re: Convoy length calculation bug
Post by: Ranran(retired) on June 16, 2020, 09:41:13 AM
I have tested this.

First, it seems that the offset can be specified only with the pakset size specific width, not px.
Probably 64px pakset is 1, 128px pakset is 2, 256px pakset is 4. I think the specification of 4px for 256px pakset is too wide.

James didn't seem to make any adjustments for SE and NW for the Britain-Ex vehicle, but as far as I can see it is off by 9px.
{ 4, -1, 34 },
{ -1, 0, 2 },
{ 0, 0, 0 },
{ 0, 4.5, 0 },
{ -1, 0, 14 },
{ -3, -2, 32 },
{ 0, 0, 0 },
{ 0, -4.5, 0 }

So it looks like this, but of course the fractional part is ignored.
We cannot specify 9px, and specifying 4 or 5 will correct the shift, but a shift of 1px will occur.


And with 256size pakset, we can only adjust in 4px increments, so the subtle distortion that results from this will appear as greater distortion.

Pak256 seems to work with this kind of offset (step is not adjusted), but it does not completely eliminate the distortion mentioned above.
{ -12, 6, 0 },
{ -12, -6, 0 },
{ -17, 0, 0 },
{ 0, 8, 0 },
{ 12, -6, 0 },
{ 12, 6, 0 },
{ 17, 0, 0 },
{ 0, -8, 0 }

(https://i.imgur.com/lFMxQTp.png)
Title: Re: Convoy length calculation bug
Post by: jamespetts on August 26, 2020, 05:17:11 PM
Thank you very much for calculating the correct numbers for the ordinal directions: that is very helpful. After some months' break, I have been able to return to this, and have now fully implemented this feature, configurable in simuconf.tab. This will be in to-morrow's nightly build.

Here is an example of the simuconf.tab syntax from Pak128.Britain-Ex:

# The below settings give an offset in pixels per direction for each vehicle
# when it is in a reversed state. This allows paksets whose vehicle alignments are
# not fully centered to have vehicles reversing algorithmically and allowing them
# to be properly aligned in game when they are reversed.
#
# By default, all of these values are 0.
#
reverse_base_offset_south = 4, -1, 34
reverse_base_offset_west = -1, 0, 2
reverse_base_offset_southwest = 0, 0, 0
reverse_base_offset_southeast = 0, 4, 0
reverse_base_offset_north = -1, 0, 14
reverse_base_offset_east = -3, -2, 32
reverse_base_offset_northeast = 0, 0, 0
reverse_base_offset_northwest = 0, -4, 0


I should be grateful if people could let me know how this works out, especially for other paksets (e.g. Pak.256 or Pak192.Comic-Ex).