rewrite tunnel bridge enter proc

Forum for technical discussions regarding development. If you have a general suggestion, problem or comment, please use one of the other forums.

Moderator: OpenTTD Developers

User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Hello all


Intro:

I would like to present you with a piece of code to facilitate future development for
custom bridge heads, tunnels bridges with signals and underground railways.


Reason:

During development of my signals in wormhole patch the most difficult thing was that bridge and
tunnel ramps act differently.
This made it very hard to determine if a vehicle is actually on the ramp or not.
Writing fast and simple code for that situation is difficult and many courageous attempts have failed.
Even my patch is not good enough for trunk but it shows a direction.
By showing this patch I am trying to show a possible beginning.
Another reason for this patch is to make it easier to change the exact point where visibility
of vehicle changes when entering or leaving tunnel.
If you really study the table you can see that by readjusting those points even faster code is
achievable.
This however is not possible without some graphical adjustments in the sprites.:(
And last but not least my time is getting more and more limited so I am trying to pass on my
knowledge.


Story:

In ttd in ottd bridges work completely different.
In ttd bridges are actually drawn on the map and vehicles simply step from the ramp onto the bridge and
bridges are simply another 'rail type'.
So vehicles can do all necessary stuff right on the moment when they step from the ramp onto the bridge.
In ottd vehicles are send into the wormhole on the incorrect location. The correct location should be
the moment that they are about to enter the wormhole, eg leave ramp (wormhole = space between ramps).
Only like that it is easy to determine if vehicle is on ramp or in wormhole.(TRACK_BIT_WORMHOLE)
Like this you can make the ramps a sort of 'safe tile'.
If you look at the VehicleEnter_TunnelBridge code you can see that this a very fast piece of code and
to improve on that code was only possible by doing a complete rewrite.
When you have a look at the code you can see that only on the very last moment the vehicle enters the
wormhole.
This is critical for trains because of signals and in this patch is only applied for trains.
To do it for road vehicles or even ships would only require minor changes.

/* First handle trains entering wormhole */
if (v->vehstatus & VS_AIRCRAFT_BROKEN){
v->vehstatus &= ~VS_AIRCRAFT_BROKEN;
v->track = TRACK_BIT_WORMHOLE;
goto wormhole_state;
}


I used the flag VS_AIRCRAFT_BROKEN that is only used for aircraft's.
I did not ad other flag name (VS_ENTER_WORMHOLE) in order to reduce compile time.:)
I am aware of the "if it ain't broke don't fix it" slogan but still I would like some people
to have a look and test it in order to get some feedback.

You can see in FS3304 that I have been struggling with this problem already some time.

The patch:
The patch is broken up in two and are supplied in FS3304.
One patch to rectify old safe games
and one that is a complete rewrite of the VehicleEnter_TunnelBridge proc with a few extra
lines in train_cmd.cpp to adjust train reversing and cleanup of redundant code.


Additional:

proposed addition in vehicle_base.h
enum VehStatus {
VS_HIDDEN = 0x01,
VS_STOPPED = 0x02,
VS_UNCLICKABLE = 0x04,
VS_DEFPAL = 0x08,
VS_TRAIN_SLOWING = 0x10,
VS_SHADOW = 0x20,
VS_AIRCRAFT_BROKEN = 0x40,
VS_ENTER_WORMHOLE = 0x40, <----
VS_CRASHED = 0x80,
};
and change VS_AIRCRAFT_BROKEN for VS_ENTER_WORMHOLE in train_cmd.cpp and tunnelbridge_cmd.cpp
to ditinguish between the two.


Future:

Try to get this patch in trunk after extensive testing.

Another big job awaits before bridge heads and signals etc. could become reality and that is
making a 'tunnel bridge pool'.( I agree devellopers)
Properly done I think it could reduce overhead.
I can somehow visualise it but I am not able to do it.:(
just a small example to give some ideas:
arr.PNG
arr.PNG (4.69 KiB) Viewed 10690 times
1: index equal to 16 bits m2 in map array
2: wormhole entrance tile 32 bits
3: wormhole exit tile 32 bits
4: wormhole length 32 bits
5: axis or direction 8 bits
6: flags 16 bits
7: pointer 32 bits to 8 array structue or whatever.
8: underground stations or signals or something else.

Entrance could be defined as NE and NW ramp, and exit as SW and SE ramp axis.
so the moment you know the axis you will be able to find entrance and exit tile.
this data could be used to eliminate the GetOtherTunnelEnd and GetOtherBridgeEnd procs.

Hoping to have been of any help

HackaLittleBit
Hirundo
Transport Coordinator
Transport Coordinator
Posts: 298
Joined: 27 Jan 2008 13:02

Re: rewrite tunnel bridge enter proc

Post by Hirundo »

hackalittlebit wrote: I used the flag VS_AIRCRAFT_BROKEN that is only used for aircraft's.
I did not ad other flag name (VS_ENTER_WORMHOLE) in order to reduce compile time.:)
This is nothing short of a hack, I'd say. If it's really needed to store the bridge state in a different way (isn't TRACK_BIT_WORMHOLE good enough?) there should be a bit free in the vehicle flags.

Also, a table with 256 magic numbers, which are then compared with other magic numbers, isn't exactly pretty either.


On my train trip today I took a look into the tunnel/bridge code as-is, and I agree that it's quite messy. There are quite a lot of (undocumented) constants and such that determine when a vehicle enters a tunnel. There are some things in it that I don't quite understand. For example, for trains the 'hide' and 'show' positions are exactly one unit apart, as is to be expected. For RVs these positions are sometimes the same, and sometimes they are two units apart.

Attached is an attempt to sanitize and document the code, while maintaining all the existing quirks. As the code has worked pretty much since the beginning, there is not much reason to change this. Since nothing actually changes there should be no compatibility issues of any kind, though a bit more rigorous testing than mine wouldn't hurt. You may consider it to be the first patch in your patch stack.
Attachments
tunnelbridge.diff
(5.47 KiB) Downloaded 266 times
Create your own NewGRF? Check out this tutorial!
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Thanks Hirundo for having a look at the code.

I used part of your code in afterload.cpp to adjust frames.
I hope you don't mind. :wink:
I keep the main code as it is because of speed.
Hirundo wrote:a table with 256 magic numbers
numbers 0,1,2,3 are diagonal directions
number 9 is chosen because it 0..8 are direction values maybe for future use.
number 11 was chosen for readability. :)
This table represents all possible x,y positions of a tile.

VS_AIRCRAFT_BROKEN is a non used flag for ground vehicles and is a hack yes but it will do for now.

The VehicleEnter_TunnelBridge procedure is a bottle neck so fast code is required.
When you take a good look at the existing code you will see that no effort has been spared to make it as fast as possible.
It is however not good enough and needs to be improved for future development.
That is what I have been trying to do.

I managed without speed loss to change the vehicle TrackBits to TRACK_BIT_WORMHOLE at exactly the right moment. (only for trains at this moment)
That is when the vehicle is about to enter new tile.
That means that vehicle with TrackBits TRACK_BIT_WORMHOLE is not standing on tunnel bridge ramp.
Anybody who has been struggling with wormholes knows what I am talking about.

I need testing games with lots of tunnels and bridges.
Please send me some links for downloading or pm.

Updated patches you can find in FS3304 see first post.
Some bugs were eliminated.

No known bugs at this moment.
EDIT: still bug when loading game with lots of tunnels.

Regards
Last edited by HackaLittleBit on 17 Oct 2010 22:58, edited 1 time in total.
Eddi
Tycoon
Tycoon
Posts: 8289
Joined: 17 Jan 2007 00:14

Re: rewrite tunnel bridge enter proc

Post by Eddi »

hackalittlebit wrote:This table represents all possible x,y positions of a tile.
what happened to RemapCoords?
Hirundo
Transport Coordinator
Transport Coordinator
Posts: 298
Joined: 27 Jan 2008 13:02

Re: rewrite tunnel bridge enter proc

Post by Hirundo »

hackalittlebit wrote:The VehicleEnter_TunnelBridge procedure is a bottle neck so fast code is required.
When you take a good look at the existing code you will see that no effort has been spared to make it as fast as possible.
It is however not good enough and needs to be improved for future development.
That is what I have been trying to do.
Do you have any profiling results that show that this is actually a bottle neck?

How much time is spent in this function, compared to e.g. CallVehicleTicks (which moves all vehicles on the map) ?
Does your or mine code have a significant effect on this?

Remember that memory reads cost time to read as well. A single cache miss costs far more time than some arithmetic operations.
Create your own NewGRF? Check out this tutorial!
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Hirundo wrote:If it's really needed to store the bridge state in a different way (isn't TRACK_BIT_WORMHOLE good enough?) there should be a bit free in the vehicle flags.
Yes it should.
The patch that I am giving here is far from complete.
It does however give a possibility to test.
When you look at VehicleEnter_TunnelBridge proc you can see that vehicles are assigned TRACK_BIT_WORMHOLE flag
when they are still on the ramp.
On bridges this is done on the very last tile coordinate.
But it is still on the ramp.
Only the next loop in TrainController the vehicle will actually enters the wormhole.
(Think of all the other vehicle movements between those two moments)
On tunnel tiles the situation is even worst.
The whole purpose of this exercise is trying to get a more precise handling of vehicles in wormholes.
Ask frosch about the zillion times he reversed vehicle to solve bugs. (and me too) :wink:
Just reverse a train on the custom bridgehead patch and you will see the headache.

If you look at my code you can see that I create four distinct point on the tunnel tile.(instead of three)
The first and last tile fraction coordinates and the two moments of visibility.
I use the last moment on the tile to give indicator to vehicle so that I can catch it when it actually is about to enter
next tile.
Only like that you can securely reserve track, or flip signals or whatever you intend to do when entering or leaving wormhole.
When you look for at existing code you can see that steam engines are blowing whistle only on second frac. cood.
The reason is because you are not able to work with directions on the first fraction coordinate.
In my code however you can get it by way of deduction.
You will need those instances for more advanced features in the future.
Eddi wrote:RemapCoords?
Eddi I will look at that.
Hirundo wrote:Do you have any profiling results that show that this is actually a bottle neck?
Here are some results.
I tested on tracks like foto.
testtrack.png
testtrack.png (392.08 KiB) Viewed 9964 times
It gives you only the speed differences between the two procedures.
I managed however to do de testing including TrainController because of the many returns inside the procedure itself.
On bridges mine is faster on tunnels mine is slower.
Most probably I am paying a price for TrainCheckIfLineEnds proc because TRACK_BIT_WORMHOLE is switched on only on the last moment.
eg I pass this proc more times.

TB_test = my patch
TB_orig = trunk

tunnel
dbg: [misc] [TB_test] 583294280 [avg: 58329.4]
dbg: [misc] [TB_test] 568724410 [avg: 56872.4]
dbg: [misc] [TB_test] 578754380 [avg: 57875.4]
dbg: [misc] [TB_test] 580705440 [avg: 58070.5]
dbg: [misc] [TB_test] 575307770 [avg: 57530.8]
dbg: [misc] [TB_test] 568515920 [avg: 56851.6]
dbg: [misc] [TB_test] 581377600 [avg: 58137.8]
dbg: [misc] [TB_test] 570644650 [avg: 57064.5]

dbg: [misc] [TB_orig] 549644040 [avg: 54964.4]
dbg: [misc] [TB_orig] 525324860 [avg: 52532.5]
dbg: [misc] [TB_orig] 538110200 [avg: 53811.0]
dbg: [misc] [TB_orig] 536313920 [avg: 53631.4]
dbg: [misc] [TB_orig] 536006580 [avg: 53600.7]
dbg: [misc] [TB_orig] 561250780 [avg: 56125.1]
dbg: [misc] [TB_orig] 529028950 [avg: 52902.9]
dbg: [misc] [TB_orig] 539574510 [avg: 53957.5]

dbg: [misc] [TB_test] 586364480 [avg: 58636.4]
dbg: [misc] [TB_test] 563409880 [avg: 56341.0]
dbg: [misc] [TB_test] 569127080 [avg: 56912.7]
dbg: [misc] [TB_test] 589278630 [avg: 58927.9]
dbg: [misc] [TB_test] 562655750 [avg: 56265.6]
dbg: [misc] [TB_test] 565795680 [avg: 56579.6]
dbg: [misc] [TB_test] 566968040 [avg: 56696.8]
dbg: [misc] [TB_test] 583396220 [avg: 58339.6]

dbg: [misc] [TB_orig] 539806470 [avg: 53980.6]
dbg: [misc] [TB_orig] 535581740 [avg: 53558.2]
dbg: [misc] [TB_orig] 527087590 [avg: 52708.8]
dbg: [misc] [TB_orig] 530160160 [avg: 53016.0]
dbg: [misc] [TB_orig] 533677430 [avg: 53367.7]
dbg: [misc] [TB_orig] 543330480 [avg: 54333.0]
dbg: [misc] [TB_orig] 526903510 [avg: 52690.4]
dbg: [misc] [TB_orig] 537070270 [avg: 53707.0]

bidge

dbg: [misc] [TB_test] 533829410 [avg: 53382.9]
dbg: [misc] [TB_test] 507856410 [avg: 50785.6]
dbg: [misc] [TB_test] 509173360 [avg: 50917.3]
dbg: [misc] [TB_test] 515581980 [avg: 51558.2]
dbg: [misc] [TB_test] 518404090 [avg: 51840.4]
dbg: [misc] [TB_test] 507193040 [avg: 50719.3]
dbg: [misc] [TB_test] 514213900 [avg: 51421.4]
dbg: [misc] [TB_test] 514731420 [avg: 51473.1]

dbg: [misc] [TB_orig] 561974510 [avg: 56197.5]
dbg: [misc] [TB_orig] 548729470 [avg: 54872.9]
dbg: [misc] [TB_orig] 550520710 [avg: 55052.1]
dbg: [misc] [TB_orig] 553441900 [avg: 55344.2]
dbg: [misc] [TB_orig] 544391920 [avg: 54439.2]
dbg: [misc] [TB_orig] 543611860 [avg: 54361.2]
dbg: [misc] [TB_orig] 546532280 [avg: 54653.2]
dbg: [misc] [TB_orig] 548905690 [avg: 54890.6]

dbg: [misc] [TB_test] 516398390 [avg: 51639.8]
dbg: [misc] [TB_test] 515594940 [avg: 51559.5]
dbg: [misc] [TB_test] 519455820 [avg: 51945.6]
dbg: [misc] [TB_test] 517136120 [avg: 51713.6]
dbg: [misc] [TB_test] 513875980 [avg: 51387.6]
dbg: [misc] [TB_test] 503095400 [avg: 50309.5]
dbg: [misc] [TB_test] 515485890 [avg: 51548.6]
dbg: [misc] [TB_test] 516295640 [avg: 51629.6]

dbg: [misc] [TB_orig] 545188490 [avg: 54518.8]
dbg: [misc] [TB_orig] 543523890 [avg: 54352.4]
dbg: [misc] [TB_orig] 538955420 [avg: 53895.5]
dbg: [misc] [TB_orig] 542537800 [avg: 54253.8]
dbg: [misc] [TB_orig] 540327190 [avg: 54032.7]
dbg: [misc] [TB_orig] 538969600 [avg: 53897.0]
dbg: [misc] [TB_orig] 586944890 [avg: 58694.5]
dbg: [misc] [TB_orig] 558849190 [avg: 55884.9]

dbg: [misc] [TB_test] 499267260 [avg: 49926.7]
dbg: [misc] [TB_test] 497927550 [avg: 49792.8]
dbg: [misc] [TB_test] 522505170 [avg: 52250.5]
dbg: [misc] [TB_test] 498711670 [avg: 49871.2]
dbg: [misc] [TB_test] 492595250 [avg: 49259.5]
dbg: [misc] [TB_test] 494736450 [avg: 49473.6]
dbg: [misc] [TB_test] 501420370 [avg: 50142.0]
dbg: [misc] [TB_test] 498797320 [avg: 49879.7]

dbg: [misc] [TB_orig] 553609510 [avg: 55361.0]
dbg: [misc] [TB_orig] 530404040 [avg: 53040.4]
dbg: [misc] [TB_orig] 536165510 [avg: 53616.6]
dbg: [misc] [TB_orig] 534498380 [avg: 53449.8]
dbg: [misc] [TB_orig] 535899770 [avg: 53590.0]
dbg: [misc] [TB_orig] 535770660 [avg: 53577.1]
dbg: [misc] [TB_orig] 528173570 [avg: 52817.4]
dbg: [misc] [TB_orig] 535572430 [avg: 53557.2]

I will give you more answers Hirundo.
Thanks again for looking at my code.:)

Frosch:

I think the code on line 3239 is doing nothing there.
if (!HasBit(r, VETS_ENTERED_WORMHOLE)) {
VETS_ENTERED_WORMHOLE is never returned on first tile fraction coord.

Regards HackaLittleBit
Hirundo
Transport Coordinator
Transport Coordinator
Posts: 298
Joined: 27 Jan 2008 13:02

Re: rewrite tunnel bridge enter proc

Post by Hirundo »

hackalittlebit wrote:When you look at VehicleEnter_TunnelBridge proc you can see that vehicles are assigned TRACK_BIT_WORMHOLE flag
when they are still on the ramp.
On bridges this is done on the very last tile coordinate.
But it is still on the ramp.
Only the next loop in TrainController the vehicle will actually enters the wormhole.
I agree with you that this is a problem. De-coupling the hidden/visible status and the actual wormhole entry seems most sensible to me.
However, I think that introducing a new flag for this purpose doesn't solve much, if any. You still set the flag while it is on the ramp, and clear it when it enters the bridge.
Therefore, the wormhole entry process is still not 'atomic' and may break if something (such as reversing) happens in the meantime. A more clean solution, I think, would be along these lines:

Disclaimer: Untested code written without compiler, purely to illustrate the concept.

Code: Select all

if (IsTileType(gp.old_tile, MP_TUNNELBRIDGE)  //leaving wormhole entry
    && t->track != TRACK_BIT_WORMHOLE //not already in wormhole
    && DiagDirBetweenTiles(gp.old_tile, gp.new_tile) == GetTunnelBridgeDirection(gp.old_tile) //leaving in the direction of wormhole
    ) {
  /* enter wormhole */
  t->track = TRACK_BIT_WORMHOLE;
  //add other magic here
Thanks for your profiling efforts. Could you attach the diffs used for the profiling?
Create your own NewGRF? Check out this tutorial!
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Her you go
Patch is not cleaned but you can see point of investigation.

I experimented with your solution already assuming that you want to put that code at the beginning of traincontroller.
and it also works.
I am looking at this moment at the reason of so much speed loss on tunnels.
I suspect TrainCheckIfLineEnds proc

A solution may also be to give vehicles wormhole status the moment they enter tunnel bridge complex.
and give extra flag when not on ramp.
I experimented with that also and it is also possible.
Attachments
21033.patch
(14.64 KiB) Downloaded 230 times
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Hello all

DISCLAIMER: experimental code in development.

Update of patch.

In my posts you can see that the train controller had a speed loss +/- 5% in tunnels and a speed gain +/- 5%
on bridges when I was applying my patch.
The speed loss in tunnels is because the wormhole became shorter (same size as bridges).
That means that the code at the end of the controllers was passed more times on tunnel tiles.
This forced me to have a look at the code of the controllers.

I have been experimenting with various solutions.
The solution that I came up with will make handling of trains in wormholes more precise.
As an extra I get a speed improvement in the train controller of more than 25% depending on game :shock: .
(I hope I am not missing out on something)

Major change explanations:

VS_AIRCRAFT_BROKEN flag is only used for airplanes so new flag VS_ENTERING_WORMHOLE with
same value was introduce besides it, and should only be used for ground vehicles.
This flag is used to indicate the precise moment that vehicle enters wormhole
The moment of entering wormhole is when vehicle is about to leave valid track tile.
At the beginning of all controllers a small piece of code is used to indicate if vehicle is about to enter wormhole.
The trackbit TRACK_BIT_WORMHOLE is now aplied properly and is actualy true.

VETSB_ENTERED_WORMHOLE was change into VETSB_ENTERED_T_B_TILE
Reason this flag was being used for purposes that were not true.
Now this flag is exclusively used on tunnels and bridges to indicate the entering a new T_B (Tunnel Bridge) tile
at beginning and end of wormhole.
The flag was being used to indicate vehicles entering depots. I used the track flag TRACK_BIT_DEPOT instead.
A depot is not an wormhole.

Hmm makes me think should vehicle_enter_tile_proc not be called vehicle_on_tile_proc. ;)

The procedure to load old save games was changed in order to enable changes made in my patch.

By examining the train controller I discovered that it allways goes to the bottom regardless of where train is on
the tile(except wormholes).
I think that there are three distinct locatios in the train controller.
location 1 newtile == oldtile
location 2 newtile != oldtile
location 3 wormholes

On location 1 not many changes should take place and train should keep rolling.

On location 2 however involes many things like reservation, pathfinding etc.

On location 3 just keep rolling

EDIT: changed location 2 into location 1.
I started experimenting with location 1 imitating wormholes.
I wanted to catch all vehicles that stayed on the same tile and jump out of the procedure asap.
Like that I discovered that UpdateInclination took quite some processing power.
Knowing that many times the z level does not change on level tiles I applied a conditon.
if (HasBit(v->gv_flags, GVF_GOINGUP_BIT) || HasBit(v->gv_flags, GVF_GOINGDOWN_BIT)) {
I saw a big speed improvement.

The patch includes TIC TOC code to do speed measurements and some aditional
comments to try to explain how that lookup table works in tunnelbridge_cmd.cpp
any suggestions, comments or improvements are welcome :)


I hope you are willing to experiment with my patch and report glitches and errors.


Regards HackaLittleBit

P.S The patch was tested with various save games from openttdcoop
The test results are with Public Server Game 190
Thanks guys :)
Attachments
T_B_rewrite_21071.patch
(26.01 KiB) Downloaded 304 times
test.txt
test results with
PublicServerGame_190_Final
(12.02 KiB) Downloaded 241 times
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Ok here is a new attempt to sanitize the VehicleEnter_TunnelBridge code.
The code is prepared for custom bridge heads, custom tunnel heads, and signals
in tunnels and on bridges and more.
The only way to get it working was relocating part of the code back to the various
controllers (finding track).
This meant I had to fiddle around with the individual controllers.

I can't find bugs anymore so please check if you can find some and let me know.;)

Changes:

1.
Now the flag VETSB_ENTERED_WORMHOLE (new name VETSB_LEAVING_WORMHOLE) is exclusively used to tell
the controller that a vehicle is returning to a normal state.
Vehicles leave the ramp on the very last moment when entering wormhole. (entering new tile)
On entering a legal ramp vehicles are brought back to a normal track state. (entering new tile)
In the train controller this is even done more precise.
I closed the loop so that the vehicle return back in the controller just after the
place were it left to enter wormhole state. (entering new tile)
Allways when entering new tile. Also when train is reversing.
When returning from wormhole the vehicle continues with TRACK_BIT_WORMHOLE
and rolls trough the controller to find the correct track and make proper reservations etc. just like any
normal tile entering occasion.

2.
I used the frame concept from Hirundo only a bit different.
the frame count increases towards the wormhole side of ramp.
eg frame 0 is first fraction coord on ramp and frame 15 is last just before wormhole.
this is reversed when leaving wormhole.

3.
Procedure is ready for tile size 32.

4.
Cleaned up train controller.
Only on exeption the program flow will leave one of the three distinct phases
a new tile == old tile, Vehicle stays on same tile
b new tile != old tile, Vehicle enters new tile
c wormhole , Vehicle is in wormhole


No more clutter in the end.

I get good overall game speed improvement.


Prove of concept is included patch for custom bridge heads.
I glued peters patch over mine with some improvements.
Basic train movement works.

Regards HackaLittleBit
Attachments
T_B_rewrite_21476.patch
(27.85 KiB) Downloaded 248 times
T_B_rewrite_21476_plus_CBH.patch
(57.58 KiB) Downloaded 242 times
Rubidium
OpenTTD Developer
OpenTTD Developer
Posts: 3815
Joined: 09 Feb 2006 19:15

Re: rewrite tunnel bridge enter proc

Post by Rubidium »

A quick glance over the diff makes me notice a few things:
  • You're accessing the map directly in non-saveload (conversion) code. This is definitely a no-go. Use the appropriate map accessor.
  • It's lacking coding style. E.g. we never put && at the begin of a line and always add a space after //.
  • We generally begin comments with a capital and end them with a period. I know it's not consistent all around in the code, but by enforcing this on change we will eventually get there.
  • Line 60/183 of the diff says "leaving", but a few lines later you're talking about entering.
  • Line 208 say enter wormhole, but next there's an if where you're leaving the wormhole.
  • Given you totally redo the "which state is the vehicle in on bridge head/tunnel entrance", shouldn't that just override the old conversion code? It generates the stuff from scratch, doesn't it?
  • Line 228/237 renames the variable from entered to leaving, but the comment still talks about entering.
  • (Goto) Labels are usually placed at the begin of the line to make them more visible.
  • Comments about custom bridge heads isn't very useful in the non CBH patch.
  • Line 562 adds the include of debug.h, why?
  • Line 587 doesn't make much sense to me. TILE_SIZE / 2 == TILE_SIZE - (TILE_SIZE / 2), so all values in the array are the same which is definitely wrong.
What this patch does it basically decouple wormhole state change in tunnels from the moment they change their visibility, right? Well, and subtly change a lot of other things it seems. So, is it possible that this patch gets split up in smaller steps? I've got kinda lost in the diff.
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Thanks for looking Rubidium.

I will review the whole patch again and somewhere during the next two weeks I will commit to FS in the correct manner.

You are right about the decoupling.
Basically it is 2 patches.

1 handling vehicles in wormholes correctly.
2 house keeping in train controller.

For now this is just for testing.

Most changes are made in VehicleEnter_TunnelBridge proc and train controller.

see attached .txt file with code of both of them.

line 587 should be
extern const byte _tunnel_visibility_frame[DIAGDIR_END] = {TILE_SIZE - (TILE_SIZE / 4), TILE_SIZE / 2, TILE_SIZE / 2, TILE_SIZE - (TILE_SIZE / 4)};


Regards HackaLittleBit
Attachments
TrainController.txt
(12.9 KiB) Downloaded 230 times
VehicleEnter_TunnelBridge.txt
(2.81 KiB) Downloaded 235 times
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Rubidium wrote:Given you totally redo the "which state is the vehicle in on bridge head/tunnel entrance", shouldn't that just override the old conversion code? It generates the stuff from scratch, doesn't it?
I have to take care of vehicles that are still on the tunnel bridge tile but have already wormhole flag.
eg. give normal track bits.

Changes:

All requested changes are done.
See below patch

Extra
Rubidium wrote:Line 208 say enter wormhole, but next there's an if where you're leaving the wormhole.
move entering a bit lower.
On tunnels and bridges length 2 trains never enter wormhole state.
See code for entering wormhole.
Attachments
T_B_rewrite_21492.patch
(27.72 KiB) Downloaded 236 times
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Hello new try.

Sorry for bumping old subject but I couldn't resist giving it a last try.

Various difficulties encountered with existing code on tunnels and bridges will be solved.
This time a much less radical approach.(and reckless)

What it does:

Unify behavior of vehicles on bridges and tunnels.
Wormhole flags RVSB_WORMHOLE and TRACK_BIT_WORMHOLE in tunnels will be set at same moment as on bridges.
That means very last tick before entering wormhole and first tick when leaving.
It prepares the procedure for future use like custom bridgeheads etc. (If bridge heads are going to be used for that.)
Vehicles that cross the tile but don't enter wormhole will always return VETSB_CONTINUE (not yet possible).
Be aware though that vehicle is still on the tile when receiving RVSB_WORMHOLE or TRACK_BIT_WORMHOLE
and will enter wormhole only on next loop.
When leaving wormhole vehicle is entering new tile.
Reversal of vehicles is easier to predict.
Bridges behavior is exactly the same as before.
Got rid of all diag_directions.

The direction of the vehicle is now determined using its wormhole state and tunnel direction and vehicle position on the tile.
Using direction from vehicle is only used for blowing horn while entering tunnel.
if you substitute this code

Code: Select all

			if (tbframe == TUNNEL_SOUND_FRAME &&
					DirToDiagDir(v->direction) == dir &&
for this

Code: Select all

			if (tbframe == TILE_SIZE - 1 &&
					v->tile != tile &&
you can cut out TUNNEL_SOUND_FRAME and don't need direction of vehicle anymore.
The difference is that horn will be blown on first tick when train enters tile instead of second tick (recommended for future use.)

Advantages:
Procedure gives more flexibility for future use.
Faster code.
Always same point where wormhole state is changed, regardless of direction.
Code uses less lines (33%).

Disadvantages not known.

Doubts: I did not include those flags because I did not see any negative effect but can easily be included.

Code: Select all

					ClrBit(t->gv_flags, GVF_GOINGUP_BIT);
					ClrBit(t->gv_flags, GVF_GOINGDOWN_BIT);
More doubts
not 100% sure about this line in afterload.cpp

Code: Select all

					rv->frame = (vdir == DIAGDIR_NE || vdir == DIAGDIR_NW) ? TILE_SIZE - 1 - pos : pos;
Todo: Extensively tested but needs testing by more people.

Procedures affected:

I could only find one procedure that is affected:

GetAirDragArea in train.h
Pressure does not build up instantaneous when entering tunnel so I do not see a problem with that one. ;)

below find diff, plus text file that shows procedure old and new.

The picture tries to explain how the counting works from entrance till wormhole.
Image
Attachments
TB_rewrite_24262.patch
(9.93 KiB) Downloaded 237 times
proc.txt
(8.78 KiB) Downloaded 220 times
WHp.PNG
WHp.PNG (205.17 KiB) Viewed 1531 times
User avatar
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Re: rewrite tunnel bridge enter proc

Post by cirdan »

Hi HackaLittleBit,

It's nice to know that I'm not the only one that thinks that the current handling of wormholes is extremely hackish. Having bridges and tunnels behave the same would certainly improve the situation.

I see, however, a problem with your approach. You have decided to make bridges behave like tunnels (setting wormhole status on the last bridgehead tick); in a sense, this is a reasonable decision, since bridges are less hackish than tunnels (for instance, the frame where a vehicle acquires wormhole status does not depend on the bridge direction), and you mention that this could help with custom bridgeheads. Well, it doesn't, or, at least, it is not enough. The problem is that, as soon as a train has its track set to TRACK_BIT_WORMHOLE, the train controller blindly updates its position on every tick by heading straight on, so you cannot put the train into wormhole status until it is already heading in the right direction. With normal bridges, this always happens at all points on the bridgehead tile; with custom bridgeheads, it does not, not even on the last tick on the tile, because the train still has its natural direction, which may be non-diagonal.

The only sensible solution I found in my custom bridgeheads patch was to put a vehicle into wormhole status when it leaves the bridgehead/tunnelhead into the wormhole, so it is in the wormhole if and only if it is not on the bridgehead (logical, isn't it?). This also removes some silly contortions required when reversing a train that is on a bridgehead/tunnelhead. If you are interested in what I did, it starts at about patch 190 of my new map features branch.
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Cirdan I first would like to see this patch in tunk(If good enough). After that next step.
cirdan wrote:You have decided to make bridges behave like tunnels
Reverse that :) Tunnels behave like bridges. Bridge behavior is not changed.

The actual 'wormhole enter moment' you can catch (for trains) next loop before moving.
gp.old_tile == TileVirtXY(v->x_pos, v->y_pos)
Only one tick vehicle appears above (tunnel or bridge) tile when in 'v->track == TRACK_BIT_WORMHOLE' section of Controller.

I have a question though about line 1690 tunnelbridge_cmd.cpp

Code: Select all

			if (dir == ReverseDiagDir(vdir) && frame == TILE_SIZE - _tunnel_visibility_frame[dir] && z == 0) {
I was not able to figure out why z == 0 is there. Can it have a different value than 0?.

Code: Select all

	if (abs(z) > 2) return VETSB_CANNOT_ENTER;
filters out anything with improper height value.
User avatar
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Re: rewrite tunnel bridge enter proc

Post by cirdan »

HackaLittleBit wrote:Cirdan I first would like to see this patch in tunk(If good enough). After that next step.
So would I. In fact, if you look at my new map features branch, fixing the handling of wormholes comes much earlier than implementing custom bridgeheads proper. But, if we are going to fix wormholes, let us do it right from the start.
HackaLittleBit wrote:
cirdan wrote:You have decided to make bridges behave like tunnels
Reverse that :) Tunnels behave like bridges. Bridge behavior is not changed.
My bad. I actually meant "tunnels like bridges", but wrote it the other way around.
HackaLittleBit wrote:The actual 'wormhole enter moment' you can catch (for trains) next loop before moving.
gp.old_tile == TileVirtXY(v->x_pos, v->y_pos)
Only one tick vehicle appears above (tunnel or bridge) tile when in 'v->track == TRACK_BIT_WORMHOLE' section of Controller.
I'm not sure I understand what you mean here...
HackaLittleBit wrote:I have a question though about line 1690 tunnelbridge_cmd.cpp

Code: Select all

			if (dir == ReverseDiagDir(vdir) && frame == TILE_SIZE - _tunnel_visibility_frame[dir] && z == 0) {
I was not able to figure out why z == 0 is there. Can it have a different value than 0?.

Code: Select all

	if (abs(z) > 2) return VETSB_CANNOT_ENTER;
filters out anything with improper height value.
This is the worst part of the tile entry status function for tunnelbridges. If you read the code as it is, you may get the impression that it is theoretically possible for z to be 1 or 2, in which case the early check will not trigger but z is not 0 either. However, line 1690 lines in the tunnel conditional branch, and tunnelheads cannot be inclined, so I guess 0 is the only value that z can have here if it passed the early check for abs(z).

The root cause of this mess is the way the current code handles wormhole exits. As it is, when a vehicle is driving in a wormhole, every time its virtual position changes to a new tile, that tile is checked to see if it is the wormhole end, comparing the current vehicle vertical position with the tile height. But this is terribly inefficient and simply wrong: vehicles should just know where their current wormhole ends, and should stop trying to get off at random tiles in between. This is one of the things I changed in my new map features branch, with little effort and no adverse effects so far, and it is something that should be applied before any attempt to unify bridge and tunnel behaviour.
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

Cirdan check this answer from rubidium:
Thursday, 12 November 2009, 12:27 GMT
http://bugs.openttd.org/task/3304?project=1&opened=1129
more answers later
User avatar
HackaLittleBit
Director
Director
Posts: 550
Joined: 10 Dec 2008 16:08
Location: tile 0x0000

Re: rewrite tunnel bridge enter proc

Post by HackaLittleBit »

I'll try to explain.
All arguing is done with my patch in mind and is valid for train controller. Other vehicles controllers have similar behavior.

Code: Select all

		if (v->track != TRACK_BIT_WORMHOLE) {
			if (gp.old_tile == gp.new_tile) {
				((PART1))
			} else {
				/* A new tile is about to be entered. */
				((PART2))
			}

		} else {
			/* In a tunnel or on a bridge
				((PART3))			
		}
Above you see current situation in train controller.
I call them part1, part2 and part3 from here on.

Trains enter wormhole in part1 (get flag TRACK_BIT_WORMHOLE) on last tick.
From that moment on (next loop) they are handled in part3.
After the next loop vehicle is actually still above the tile before being moved (gp.old_tile == TileVirtXY(v->x_pos, v->y_pos with some security checks))
That is the moment you can adjust direction.(before moving vehicle)
The nice part of my patch is that it only returns on the first tick (when encountering valid exit) to part3
(next line after: if (IsTileType(gp.new_tile, MP_TUNNELBRIDGE) && HasBit(VehicleEnterTile(v, gp.new_tile, gp.x, gp.y), VETS_ENTERED_WORMHOLE)) {
)
after that it continues in part1.
So that first tick in part3 is where you can do your path finding, reservation stuff, signals etc. before you move.
(HacklaLittleBit wonders if wormholes should not be handled first. :roll: )

But I suggest to go slow and introducing my patch would already be a big step.
(z == o) is one of the issues that needs to be addressed.
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: Amazon [Bot] and 11 guests