rewrite tunnel bridge enter proc

[cirdan]

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

Yes, that is precisely what I was referring to. And I know why it is done, it is just that abusing the tile entry status function for that is the wrong thing to do. In my new map features branch I have a couple of patches that implement a better alternative, which is cleaner and improves performance (fewer random indirect function calls).

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.

So, what you propose is to do all that stuff in the first tick the vehicle is in the wormhole? That could be certainly done, but it would require extra computations in part 3. (Computing whether you are in the first wormhole tick is not trivial. Plus some bridges have no wormhole part.)

(HacklaLittleBit wonders if wormholes should not be handled first. )

At the very least, it would help reduce indentation level, making code more readable. (That's also something I do in my new map features branch.)

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.

I think we agree on this: some patch to unify tunnel and bridge behaviour, done right, would be nice. And I already solved the z==0 problem; perhaps I should try to cherry-pick the relevant patches from my branch so that they apply to trunk.

[Eddi]

how it (imho) could make sense:

• on the bridge/tunnel head: treat the vehicle like on a normal track bit.
• upon leaving the track bit on the wormhole side, enter the "wormhole" trackbit of the next tile.
• remove the "hack" of entering the opposite bridge/tunnelhead tile with a huge offset.
• this means, vehicles can enter "other" tiles while having the "wormhole" flag, but it will in the future allow having bendy bridges etc.
• on each "vehicle enter tile", check whether the next tile has a wormhole exit of the appropriate height, and enter "normal" trackbit if appropriate.

[HackaLittleBit]

I should try to cherry-pick the relevant patches from my branch so that they apply to trunk

Cirdan If you could split up you patch according to:
Track reservation, path finding, signals, map array changes and bit shuffling it would be easier to analise.

on the bridge/tunnel head: treat the vehicle like on a normal track bit.

Yep

remove the "hack" of entering the opposite bridge/tunnelhead tile with a huge offset.

Done!

Ok little update to patch.

I save some instructions by using TILE_UNIT_MASK instead of TILE_SIZE - 1
Also replaced the break; with return VETSB_ENTERED_WORMHOLE;

I opted for

Code: Select all

			if (tbframe == TILE_UNIT_MASK &&
					v->tile != tile &&

to save instructions
Train blows horn on first tick when entering tunnel.

Now option to hide or unhidden is absolute!(very important)
z == 0 problem solved by adding assert(z == 0)
I am not able to trigger that assert.

Reintroduced GVF_GOINGUP_BIT, GVF_GOINGDOWN_BIT.
Not sure but does not affect performance much.
Difference with trunk is that it is also done in tunnels.
I think frosch solved that one in line 3297 train_cmd.cpp

Code: Select all

				/* Prevent v->UpdateInclination() being called with wrong parameters.
				 * This could happen if the train was reversed inside the tunnel/bridge. */
				if (gp.old_tile == gp.new_tile) {
					gp.old_tile = GetOtherTunnelBridgeEnd(gp.old_tile);
				}

I reintroduced them however for security reasons.

EDIT:
find attached below tunnelbridge_cmd.c release 1 (Tron) z == 0 is there already

[cirdan]

I should try to cherry-pick the relevant patches from my branch so that they apply to trunk

Cirdan If you could split up you patch according to:
Track reservation, path finding, signals, map array changes and bit shuffling it would be easier to analise.

Err, my "patch" is already split into small patches. Are you suggesting a particular arrangement for those patches?

Here are the patches I referred to earlier, or some of them. In my opinion, they should be applied before attempting bigger changes in the wormhole code, because they make the code cleaner (no more abs(z)<2 checks, only the right tile is tested).

The first patch makes it so that all vehicles which are in a wormhole have the wormhole exit stored in their 'tile' field. Currently, when a vehicle is in a wormhole, its 'tile' field stores either end of the wormhole; with this patch, it is always the end that the vehicle is heading to.

The second patch is not really necessary, but it is included for completeness. It adds a consistency check in the pathfinder to ensure that everything works as intended.

The third patch takes advantage of the behaviour introduced in the first patch in the wormhole exit code. Since vehicles now know at which tile they should exit a wormhole, it is no longer necessary to call the tile entry status function for all intermediate virtual tiles, only for the actual bridgehead/tunnelhead.

The fourth patch is a one-line patch that turns the abs(z) check into an assertion, since now the vehicle entry status function should only be called for vehicles at ground height.

These patches do not deal with the z==0 problem, that comes later.

[cirdan]

Here is the fourth patch.

[HackaLittleBit]

Ok results for testing proposed procedure change.

After exhaustive testing and changing I come to the following end result:
Initial profiling showed that my new procedure was +/-3% slower than the old
I tried to shave off as much as possible unnecessary instructions. (still bit more possible but would make code ugly)
Below you can find the final result.
In zip file profiling is included in exl format + test game + tictoc code.

I found out that because of delaying the wormhole condition, the trains in tunnels were continuing to pass
section 1 (see previous discussion) for a longer time (till the end).
Ratio between visible and non visible = 7 : 3 or in old code, NOT TRACK_BIT_WORMHOLE || TRACK_BIT_WORMHOLE
This means that vehicles in tunnels were passing the last part of the train controller more often.

I included in section 1 the following code to simulate tunnel wormhole line 3078 in train_cmd.cpp.
This was done for proof of concept only.

Code: Select all

					uint32 r = VehicleEnterTile(v, gp.new_tile, gp.x, gp.y);
					if (HasBit(r, VETS_CANNOT_ENTER)) {
						goto invalid_rail;
					}
					if (v->vehstatus & VS_HIDDEN) { // in tunnel
						v->x_pos = gp.x;
						v->y_pos = gp.y;
						VehicleUpdatePosition(v);
						continue;
					}
					if (HasBit(r, VETS_ENTERED_STATION)) {

All of a sudden my code was running as fast or even a bit faster than the existing code.
(see worksheet in zip file for details).
This proves that speed of my code is ok.
Testing was done on game with only tunnels and bridges. (see test game in zip file below)
So in reality the impact of speed loss (if procedure would be introduced) will be significantly lower.
It depends on the amount of tunnels constructed.

Coming back to the z == 0 issue.
I managed to get the earliest code of ottd release 0.1
already there that line exists. (??leftover??)
I have been investigating and the only place were z is 1 is when vehicle is climbing onto ramp
since that line is only significant when vehicle is near the middle of the tile z is 'ALWAYS' 0
so I eliminated the line.

Disclaimer: all testing was done with trains.