Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rewrite tunnel bridge enter proc #3304

Closed
DorpsGek opened this issue Nov 8, 2009 · 19 comments
Closed

rewrite tunnel bridge enter proc #3304

DorpsGek opened this issue Nov 8, 2009 · 19 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Nov 8, 2009

hackalittlebit opened the ticket and wrote:

please be so kind and implement this patch to trunk.
Reasons for this patch:
Readability of code
Change state of vehicle to "in wormhole" as close as possible to the actual wormhole.
reduce overhead.
Prepare trunk for future custom bridge heads.
Prepare trunk for future tunnel signal simulation, not bridges.
No know bugs!

Regards HackaLittleBit
Please note

if (abs(z) > 2) return VETSB_CANNOT_ENTER;

was changed to

if (abs(z) > 2) return VETSB_CONTINUE;

I think this is correct because when train not on same level other bridge or tunnel should continue rolling.

I tried to write as efficient as possible.
I do think some small speed improvement may still be possible.

Can be implemented without changing savegame version.

Regards HackaLittleBit

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/3304
@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 8, 2009

hackalittlebit wrote:

minor code style changes (thanks planet maker) :)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment6917

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Some more code changes.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment6928

@DorpsGek
Copy link
Member Author

Rubidium wrote:

I think changing VETSB_CANNOT_ENTER to VETSB_CONTINUE is not correct; see the attached savegame.
If you would've done some digging as to why it was added you would've found the following commit:

r4879 | celestar | 2006-05-16 12:52:18 +0200 (Tue, 16 May 2006) | 2 lines
[bridge] -Fix: Vehicles can no longer "jump" onto a bridge ramp from below the bridge

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment6940

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Confirmed, checked it but trains do not have this behaviour, I did not check for ships yet.
I am not happy with the wormhole enter tile procedure and I will keep on working on it till I found a acceptable solution.
I do think some good speed improvements are possible here.

What is needed is a clear distinction between state of vehicle when it enters tunnel/bridge head and when it enters wormhole.
this has to be done on the very first moment vehicle enters tunnel/bridge tile and on the very last moment it leaves tunnel/bridge tile.
I know it is easier said than done ;)
I am only busy with this part of the code now and I will come back about it soon with some suggestions.

P.S. do not implement this patch because the way it is is a half solution.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment6943

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Rubidium patch to fix Bug in and to make VehicleEnter_TunnelBridge proc aprrox. 5% faster
It has a few lines with TIC TOC

Times measured:

patch
dbg: [misc] [WH] 92707100 [avg: 927.1]
dbg: [misc] [WH] 95152900 [avg: 951.5]
dbg: [misc] [WH] 93952530 [avg: 939.5]
dbg: [misc] [WH] 93531810 [avg: 935.3]

trunk
dbg: [misc] [WH] 98861250 [avg: 988.6]
dbg: [misc] [WH] 99501710 [avg: 995.0]
dbg: [misc] [WH] 98428680 [avg: 984.3]
dbg: [misc] [WH] 98166870 [avg: 981.7]

When you see the patch it will speak for it self I guess.
Trains gan still be on the orignal tile when entering wormhole, reversing can make them skip the x,y point on which the should come out of the wormhole.
It took me quite a while to discover it but the bug has been haunting me for a long time.
EDIT: I verified this and I can't reproduce this so called bug. :(

I do not think it is necessary to change the save game version because of the rare occurrence.

Regards

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment7750

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Hereby I send you an completely revised VehicleEnter_TunnelBridge procedure.

Vehicles in tunnels and on bridges are treated equally.
That means on both the vehicle enter on last fraction coordinate of tunnel bridge in worm hole and comes out on first fraction coordinate.
Hiding and showing vehicle are done on the usual place.
The procedure is using a look-up table to speed things up.

Reasons for changing this procedure:

  1. Speed procedure up considerable.(+/- 12%)

  2. Disconnect the relation between entering wormhole and becoming visible and invisible.

  3. Having in mind the discussion in viewtopic :http://www.tt-forums.net/viewtopic.php?p=861654# p861654 making it easier to implement a more flexible point of becoming visible/invisible. (Don't know this for sure but it is a hunch).

  4. When vehicle enters/leaves wormhole it should be done in the proper place.
    Unfortunately the train is still on the last tile fraction coordinate when it is send into the wormhole.´
    This means that many vehicles move before the next move that will put the vehicle actually inside the wormhole and outside the tunnel/bridge ramp.
    but still it is much better like this.

  5. For future use with signals on tunnel and bridge on entrance tile and custom bridge head.

    a simple enumeration like this one could be used to find trains on entrance / exit tile

    static Vehicle *FindTrainOnWormholeTile(Vehicle *v, void *data)
    {
    if (v->type != VEH_TRAIN || Train::From(v)->track == TRACK_BIT_WORMHOLE) return NULL;

    return v;
    }

  6. Make life easier for next tunnel/bridge signal or custom bridge head developer :)

  7. Vehicles on tunnels and on bridges are treated equal.

If fraction coordinates to make vehicle (in)visible would be chosen on different points the procedure could be faster even.
I know that that is almost impossible but could be a point of consideration.

In my previous post I spoke about a possible bug, I verified it and I can not reproduce nor detect any bug in the code so
therefor my apologies.
Regards HackaLittleBit

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment7789

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 3, 2010

hackalittlebit wrote:

.Hello

Here is an evolution of patch submitted in previous post
This time I am more happy with the result because with this patch you are able with insignificant loss to launch vehicle
into wormhole at the exact moment when it about to leave the ramp of tunnel or bridge.
More comments you can read in post in forum see:
http://www.tt-forums.net/viewtopic.php?p=906213# p906213

totr.sav is small game for testing

Regards

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment8842

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 9, 2010

hackalittlebit wrote:

Hello

Removed bug;
Adding brackets (fc == _tunnel_fractcoord_6[dir] || fc == _tunnel_fractcoord_7[dir])

Made some code speed improvements.
I Changed (v->type == VEH_TRAIN && v->IsPrimaryVehicle() && DirToDiagDir(v->direction) != ReverseDiagDir(dir)) to
v->type == VEH_TRAIN && v->tile != tile && Train::From(v)->IsFrontEngine(). (A 50% instruction reduction)
and repositioned code to improve speed. (z == 0)

No known bugs at this moment.
EDIT2: rv->frame = RVC_DEFAULT_START_FRAME; is wrong in afterload.cpp in my patch
I am looking into that now.

Attached is extra zip file with disassembly and code files for comparison.
I will do more extensive testing and profiling and will keep you updated about that.

Regards Hans

EDIT1: and I used EC_STEAM, thanks Hirundo

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment8893

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Hello

Fixed setting of frames for road vehicles in afterload.cpp
Should now load any older game without problem.
No known bugs at this moment.
Edit1: still bug when loading game with tunnels.
Will do more extensive testing and profiling.

Regards Hans

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment8931

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Hello

Fixed bug in afterload.cpp, vehicles were not properly registered in vehicle hash.
Should now load any older game without problem.
No known bugs at this moment.

Regards Hans

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment8957

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 1, 2010

hackalittlebit wrote:

Hello

Update of patch
Basic functioning did not change.
Did much testing and much more to do.
Please have a look at it.
Speed improvements in train controller depend on type of game.
Many slopes is less improvement.
I really would appreciate if you would tell me if I make any mistakes.

I tested with openttd save games
The attached txt file are test results with PublicServerGame_190_Final.sav

Regards Hans

See http://www.tt-forums.net/viewtopic.php?f=33&t=50283&p=911188# p911188 to get more details

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9007

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 2, 2010

hackalittlebit wrote:

Tiny change in reversing code
removed && HasBit(VehicleEnterTile(v, gp.new_tile, gp.x, gp.y)
It was not necessary any more.
and some cosmetic changes.
Regards

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9012

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

This is minimum amount of code nescessary to make wormhole state switching work.
The handling of visibility is done separate.
Switching to and from wormhole state is always done on the moment of entering new tile.

The changes should not give any different vehicle behavior in trunk.

I can't find bugs anymore.

Explanation of various changes is done by referring to line numbers in patch.

ln 10 We are in depot so TRACK_BIT_DEPOT flag is used in controller instead of VETSB_ENTERED_WORMHOLE
ln 23 We are in depot so RVSB_IN_DEPOT flag is used in controller instead of VETSB_ENTERED_WORMHOLE

ln 36 Point where road vehicles are send when entering wormhole after leaving ramp.

ln 46 Moment road vehicles leave wormhole only difference with trunk is that state, frame, and tile are assigned here
instead in VehicleEnter_TunnelBridge proc.

ln 60 Road vehicle is entering wormhole and is going to be send to line 36.

ln 84 "if (!HasBit(r, VETS_ENTERED_WORMHOLE)) {" not necessary anymore here.

ln 99 here is were we use the correct flag mentioned in ln 23

ln 111 Here we put vehicle firmly on tunnel bridge tile it actually is.
Giving correct track bits, assigning tile and updating vehicle position hash.

ln 181 till 223 We are doing the same thing with ships as with road vehicles.

ln 236 245 The only time that flag is used is when vehicle returns back from wormhole state( and enters new tile).

TrainController:

Train treatment is more advanced.

Entering wormhole and leaving wormhole is in a way a closed loop now.

ln 257 Point of entering wormhole. exception is tunnels and bridges with 2 tiles.
In that case train does not enter wormhole state

ln 276 Point where train comes back out of wormhole to continue finding track, reserving track, switching signals etc.
Note IMPORTANT trackbits are still TRACK_BIT_WORMHOLE and very different from existing code vehicle
can have same tile as new tile. Still vehicle is entering new tile so this is the correct place.

ln 289 I was lucky here because coming out of wormhole there are no 90º turns (yet).
FindFirstTrack(v->track) will return INVALID_TRACK when v->track == TRACK_BIT_WORMHOLE.

ln 298 Moved to after vehicle enter tile proc see line 313.
Under certain circumstances tile could be reserved but not entered.
see attached foto.

ln 310 342 Just removed "if (!HasBit(r, VETS_ENTERED_WORMHOLE)) {" and brackets.

ln 364 If flag VETS_LEAVING_WORMHOLE is returned train is leaving wormhole and is returned to line 276

ln 387 and onwards is total rewrite of proc.
frame counter is increasing towards wormhole side of tunnel bridge.

Attached:
VehicleEnter_TunnelBridge proc for ease of reviewing.

More pieces of code will be committed during the comming days.

Regards HackaLitteBit

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9260

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

From now on I will submit small changes to TrainController proc and explain them.

T_B_change_on_same_tile_behavour_21532.diff
This change will make vehicles continue the loop without falling through.

ln 15 This line was removed from the end of TrainController proc (see ln 52) because when train enters new tile CheckNextTrainTile is "allways" executed.

see ln 3119 in TrainController
/* Always try to extend the reservation when entering a tile. */
CheckNextTrainTile(v);

ln 34 gv_flags are set when vehicle enters tile so when they are not set vehicle is not changing z position.
This change gives substantial overall speed improvement.

ln 40 I changed VehicleMove(v, !(v->vehstatus & VS_HIDDEN)); into VehicleMove(v, true); to eliminate graphical glitches
in tunnel entrances.

ln 42 continue loop don't fall through until end of procedure.

Next diff will I will clean up the end of TrainController proc

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9268

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Last part of T_B patch.
Basically I move all code at the end of TrainController proc to the part where old.tile != new.tile.
Changed some punctuations and style and moved code to correct location.

ln 9 was moved to ln 135 just before we start using it.

ln 15 removed because allways true when entering new tile.

ln 52 moved see ln 62 only check this for front engine.

ln 76 removed this part because not nescessary for now.

ln 120 moved ln 127 up a bit for readability.

ln 133 removed because allways true when entering new tile. see ln 15.

ln 157 chunk of code moved inside old.tile != new.tile part from lower part of TrainController proc.

Now TrainController proc has three distinct parts.

location 1 newtile == oldtile
location 2 newtile != oldtile
location 3 wormholes

order of applying patches.
T_B_rewrite_21532.patch (21 KiB)
T_B_change_on_same_tile_behavour_21532.diff (2.1 KiB)
T_B_finish_cleaning_train_controller_part2_21532.patch

Regards HackaLittleBit

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9276

@DorpsGek
Copy link
Member Author

Rubidium wrote:

In the "first" patch you seem to be adding special code for 2 tile bridges/tunnels for trains, but not for RVs/ships. Is there any particular reason for that?

+ if (!(IsTileType(v->tile, MP_TUNNELBRIDGE))) continue;
A space too many before continue and too much parentheses; it's not really misleading here but in:
+ if (!(IsTileType(gp.new_tile, MP_TUNNELBRIDGE)) || GetSlopeZ(gp.x, gp.y) != v->z_pos) {
it is misleading. I've been wondering why the check was written this way until I found out the closing parenthesis was after the IsTileType sub-expression instead of the GetSlopeZ sub-expression. Then it made sense, but the unneeded parentheses made it much harder to grasp without disecting it completely.

Some minor things:
* it's "coming", not "comming".
* } break; should be break; newline }, where break is aligned with the rest of the case's code and the } in unindented one step, so it's at the same indentation level as case.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9289

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Yes I tried to keep the amount of code to the bare minimum in order to facilitate.
Most of my attention has gone in train controller
As you can see in train controller the loop is closed in a way.
This is not the case in ship and road vehicle controller.
It should be though to get good control over the wormhole situation.
For the ship controller I will write you a small patch to correct it in order to:

  1. Stay consistent and true to the philosophy that vehicles only have wormhole state between ramps.
  2. Try to return vehicle to the same place in the code were it left when it was entering wormhole.
    I do not have enough knowledge of the road vehicle controller but it would be highly desirable if it would follow the same rules as ship and train controller.
    That is :
    new tile == old tile
    new tile != old tile
    wormhole state
    I will have a look at it to see if this is possible.
    If so also in the road vehicle controller 2 tile bridges/tunnels should be passed without entering wormhole state.

The rest of the comments I will rectify with another small patch.

Thanks :)

Comments were edited 20-12-2010

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9297

@DorpsGek
Copy link
Member Author

hackalittlebit wrote:

Here is updated ship controller.
Basic functioning the same as train controller.
Cleaned up some code.
Tested it and can't find bugs.

new tile == old tile
new tile != old tile
wormhole state

apply this patch over the previous patches

Regards HackaLittleBit

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3304#comment9304

@DorpsGek
Copy link
Member Author

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).


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

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant