small changes to names in state machine

DorpsGek commented 10 years ago

hackalittlebit opened the ticket and wrote:

I am tinkering with the state machine for planes. I had some difficulties understanding the code. For my self I changed some var names to make the code more readable. The changes give a better idea what the plane wants or is doing. So why not share it. ;-)

Very little changes: AircraftEventHandler_StartTakeOff ==> AircraftEventHandler_StartClimb AircraftEventHandler_EndTakeOff ==> AircraftEventHandler_StartCruise AircraftEventHandler_Flying ==> AircraftEventHandler_StartDescent AircraftEventHandler_Landing ==> AircraftEventHandler_TouchDown AircraftEventHandler_EndLanding ==> AircraftEventHandler_PlatformArrive

TAKEOFF ==> DEPARTURE STARTTAKEOFF ==> TAKEOFF ENDTAKEOFF ==> CLIMBING LANDING ==> DESCENDING ENDLANDING ==> TOUCHDOWN

Apply the attached patch and things will fall in place.

Regards Hans

Attachments

statemachine_vars_update.diff (33.65 KiB)

Reported version: trunk Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/5833

DorpsGek commented 10 years ago

Alberth wrote:

Sorry, but this is unlikely to happen. Names that make sense for one make no sense for someone else, and it's pretty much impossible to get universal agreement on it. If we add this, someone else might make a patch with other names that you don't like, in the worst case leading to rename wars. It's good for the commit count, but otherwise mostly just a waste of time.

Secondly, we avoid doing changes for the sake of changes. It's however usually fine to re-organize names as a part of changing or extending functionality.

When I find names not making a lot of sense, I usually add doxygen documentation with a good explanation (in my view :) ), or keep a piece of paper with the meaning in my words. In time, I find that the names used in the code do get the meaning they have without needing that piece of paper :)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5833#comment12829

DorpsGek commented 10 years ago

hackalittlebit wrote:

Albert said "When I find names not making a lot of sense, I usually add doxygen documentation with a good explanation (in my view :) ), or keep a piece of paper with the meaning in my words. In time, I find that the names used in the code do get the meaning they have without needing that piece of paper :)"

I know, I just had a little hope you guys would swallow this one without to much work.:-) I am busy with little patch to improve flying behaviour. When I submit I will give full explanation. Thanks Hans

This comment was imported from FlySpray: https://bugs.openttd.org/task/5833#comment12830

DorpsGek commented 10 years ago

hackalittlebit wrote:

Hello hereby explanations about modifications: Vars for helicopters were not changed.

in airport.h changed vars.

TAKEOFF => DEPART Depart was chosen because aircraft want to leave terminal to taxi to take-off runway. STARTTAKEOFF => TAKEOFFROLL Plane is requesting to enter take-off runway. (Limiting the amount of times word TAKEOFF is used) ENDTAKEOFF => TAKEOFF Moment aircraft is losing conact with the runway.

LANDING => DESCEND Plane is requesting free runway to make final aproach (e.g. descend till runway). ENDLANDING => LANDING Point of touchdown and breaking.

In patch line 184 you can see type (%Vehicle is flying in the air.) Some comments are improved.

event procs:

In sequential order AircraftEventHandler_TakeOff => AircraftEventHandler_TakeOffRoll AircraftEventHandler_StartTakeOff => AircraftEventHandler_TakeOffClimb AircraftEventHandler_EndTakeOff => AircraftEventHandler_TopOfClimb (Official IAA abraviation is TOC) Were Confusing

AircraftEventHandler_Flying => AircraftEventHandler_StartDescend (Called when permission to make final approach) AircraftEventHandler_EndLanding => AircraftEventHandler_PlatformArrive

See all.png to see sequential calling of procs.

And have a look at mindbogeling plane speed on the runway. (Good for the tire factories) Included setting png show plane settings no aditional grfs used see included savegame

Hope to be of any help. Regards Hans

Attachments

statemachine_vars_update2.diff (34.01 KiB)
All.PNG (407.04 KiB)
settings.png (8.05 KiB)
testtrunk2.sav (28.88 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5833#comment12840

DorpsGek commented 10 years ago

hackalittlebit wrote:

Ok some more patches for airports. entry_point_hold_pattern_r26281.diff The holding pattern is improved with this patch. Entry points are added for most of the airplane airports. So now a true holding pattern with speed limit is established over each airport. See PDF. yellow: entry points blue: holding points Helicopter setting were not touched. Doubts:

Length of climb and descend , should this e equal for every airport? Maybe city airports with short take off and landing capacity?

Intercontinental airports should have other holding pattern. Holding and starting plane are crossing and does not look good. I did not manage to use points 69 and 76 also for holding. If I would be allowed to use those points a better holding pattern could be estabelished.

Overhead by introdusing more points.

If length between entrypoint and first holding point would be standard, let say 10 tiles then that space could be used to decelerate the plane to holding speed.

Included are a zipfile that holds some games for testing. The patches have to be applied in sequential order.

Hans

Attachments

entry_point_hold_pattern_r26281_p1.patch (27.20 KiB)
shift_intercontinental_2px_r26281 p2.patch (15.21 KiB)
airport save games.7z (96.00 KiB)
pages.pdf (167.49 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5833#comment12860

DorpsGek commented 10 years ago

hackalittlebit wrote:

Ok a new try.

Please consider the above code null and void. This time an attempt to improve airplane behavior. I realized that controlling any code changes in the state machine is tedious and time consuming. Therefore I made a tool that makes it much more easy to check the various points at airports. Attached in the zipfile (airport_movement.7z) you will find a scalable vector graphics (SVG) representation of the airports in ottd. Also included is Perl file that will generate that file. The best program for opening the SVG file is Firefox. I tested on Opera also. Moving with the mouse over a point will show you the relations. When you toggle F12 in FF you get the console. If you click on a point and hit c key, the information about that point is shown in the console. Move the points with key's 'w, a, s, d'.

To make the Perl file work you have to change the path parameters. Point $path_org to original airport_movement.h and $path_new to patched version. I opted for an external JavaScript file to make it possible to generate the points file automatically with an other program. Maybe this SVG file should be included in the docs file.

The patches: (patchqueue.7z)

In general an attempt has been made to standardize plane behavior around airports. This means equal approach distance, equal takeoff distance, the same setoff on landing and take of strips, equal holding height. I saw that you tried to rotate airports. The points should be in the middle or centre line of the tile eg 8 for tile 16 or 16 for tile 32. (7 or 15) This would give the newgrf designers a chance to uniformisize the sprite offset.(It must have been a headache for them) The offsets in this patch are not on the ideal place. (see SVG) I tried to find a optimum and they are the same for each airport. It is however relatively easy to shift the points all together (e.g. all airports) some pixels in any direction. To do this however would need the consensus including the grf designers. Maybe it is possible to do a batch change for the offsets in each grf??? I don't know. Nor do I know if it possible.

The way I see aircraft behavior is that it has 11 different stages. 1 Taxi to runway. (fixed speed) 2 Accelerate. (increase speed) 3 Take off with steep climb. (increase speed, height) 4 Ascend till cruise altitude. (increase speed, height) 5 Cruise. (fixed speed, height) 6 Slow descent and prepare for holding.(reduce speed, height) 7 Holding till landing slot found. (fixed speed, height) (All Aircraft!!) 8 Steep descent and touch down. (fixed speed, reduce height) 9 Decelerate on landing strip. (reduce speed) 10 Taxi to terminal or hangar. (fixed speed) 11 Loading unloading. (zero speed)

The altitude thingy I took care of in my patches. The way the things are in trunk now is that, if airplane takes off on mountain, it keeps a different
cruise altitude then from a plane that took of at sea level.(resulting in funny landings) Cruise altitude is now equal regardless of take-off height. For most airports I added extra points that are airport entry points. Once reached the plane goes in a 'holding stage' until suitable landing strip is found. Holding pattern above airports are all the same height regardless of altitude. I took 184 as minimum height to avoid 911 effect. (makes me think that towns should not be allowed to build high-rise in front of landing strip) This was fixed with the new var 'platformheight'. Planes will slowly rise after take off and descend when coming closer to destination.

The acceleration is an other pain in the a.. I would like to see only one! :-) I understand that this will be not so easy. (maybe another batch update in the various nwgrf's?) I still have difficulties to understand that piece of code. :-(

So now some points of doubt and detail.

var 'platformheight' this I introduced to reduce the amount of iterations to determine platform height. and for code readability. Doubts that I have is the way I initialized it in 'int platformheight = MIN_FLIGHT_ALTITUDE'. This was done in order to have a descent height when airport has invalid tile.

I changed 2 points from the intercontinental airport. Point 69 and 76. They are now the same as all other airports.(but have to be checked) This gave two more point for the holding pattern. I am not happy with the holding pattern at this airport because planes are crossing each other after take-off.

I used 'v->tick_counter & 1' to change altitude every other tick. (I am not sure if this is legal)

I introduced the var 'AMED_TAXI' value = 1 << 9. I did not use it and can be taken out. However it would be nice to change 0 into AMED_TAXI if you want i'll do it. just ask.

Question:

AMED_SLOWTURN I see is allways used when plane is flying. Maybe it could be taken out and it should be standard behavior. only when AMED_LAND is used it should be combined with AMED_EXACTPOS to point aircraft in the right direction. What do you think of that?

As you can see I made some minor changes to the naming of some procedures and vars. AircraftEventHandler_TakeOff and AircraftEventHandler_StartTakeOff are absolutely confusing. and were changed into AircraftEventHandler_TakeOffRoll and AircraftEventHandler_TakeOff respectively. I myself are not yet 100% happy with the proc names. I think it should be something like trytoland, trytostart etc.(AircraftEventHandlerTry maybe?)

Please have a look at that again.

The patches have a short description included and were made with Mercurial.

Regards Hans

Attachments

airport_movement.7z (38.98 KiB)
patchqueue.7z (15.54 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5833#comment12962

DorpsGek commented 7 years ago

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no recent commentary. 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). Thanks for the patch, sorry it didn't make it.

OpenTTD / OpenTTD