Physics patches

hertogjan · Post by **hertogjan** » 21 Feb 2006 21:20

I cannot guarantee that it will not interfere with any other of the patches in the Integrated Nightly. This patch replaces and adds quite large pieces of code.
For instance, I foresee problems with PBS. I have not incorporated any PBS behaviour into the train acceleration functions, so what will happen when a train encounters a PBS signal is unsure. Maybe it will be stuck there forever.
I am not planning to build new code based on the old PBS. If PBS will return to the trunk, then I will certainly look at it, but at this point there is nothing I can do.
But maybe if there are enough people who wish that this patch be incorporated into the next Integrated Nightly, then maybe I will put some effort in it to make it compatible with the other patches.

Basje · Post by **Basje** » 22 Feb 2006 08:22

The PBS thing is indeed a point. But I hope it will be solved soon. I myself currently use the latest Integrated Nightly Build which still contains PBS (and I use it A LOT with merely no problems).

Perhaps you could contact richk67 who is the creator of the Integrated Nightly Build to see what possibilities there are? It would give you a lot more feedback and debugging info.

Post by **peter1138** » 26 Feb 2006 09:53

Hertogjan, could you please update your patch properly?

hertogjan · Post by **hertogjan** » 26 Feb 2006 15:55

In which sense do you mean "properly"?
Do you mean so that it is compatible with the latest revision?

(Actually, I chose it not to update it during the last week, since the updates of the trunk were too small. In the sense that the conflicts when merging are only minimal. Maybe next week I will fix all those little things.)

Post by **peter1138** » 26 Feb 2006 18:06

By properly I mean merging it with trunk changes instead of having the trunk changes reverted...

hertogjan · Post by **hertogjan** » 26 Feb 2006 21:46

Yes, what I thought was indeed true: There were numerous changes in the trunk code which involved mostly code style changes (whitespace, tabbing, etc.)
Where possible I changed the code* so most differences should be gone now.
I did not add new features.

Peter1138: Please tell me if there are still problems in this version (and if so, where they are).

*Not every difference. At a few points I overruled the trunk changes because they are silly, i.e. breaking the style of the surrounding code. And since I am a perfectionist, having those lines as in the trunk wouldn't help, since I will automatically correct all lines which "break the style" upon sight.

Darkvater · Post by **Darkvater** » 26 Feb 2006 21:55

hertogjan wrote:*Not every difference. At a few points I overruled the trunk changes because they are silly, i.e. breaking the style of the surrounding code. And since I am a perfectionist, having those lines as in the trunk wouldn't help, since I will automatically correct all lines which "break the style" upon sight.

Which is a bad thing for inclusion

You see, even if we sometimes fail to do so, it is very desirable to have only the RELEVANT changes in the diff to add and not random code-beautifyings. It makes it hard to read afterward what was changed and why.

For the style may I point you to http://www.tt-forums.net/viewtopic.php?t=7933 and http://wiki.openttd.org/index.php/Coding_style

See changes are the one below is just cruft making the diff much much harder to read. Now there is nothing wrong with 'fixing' this...seperately as a style-change.

Code: Select all

-		case 0x1A: // Alter purchase list sort order.
+		case 0x1A: /* Alter purchase list sort order */

hertogjan · Post by **hertogjan** » 27 Feb 2006 11:57

Sometimes I really do get confused since some style changes of the trunk are actually against the style conventions in the wiki (i.e. something good is changed to something bad, according to the conventions).

The reason I change some of the trunk style is that the code should be easy to read. As I said, if I am programming some piece of code, then I will automatically correct all things that are wrong*. I don't care much about how the diff looks, since at the end of the day we will be working with code, not diffs.
And really, the style changes are intended to be part of the diff. I will not release a patch that contains ugly code when it can be avoided. For proper functionality, clean code is needed** (thus the style changes are relevant in some way).

But as I said, there are only a few instances of where I did this. Still more than 99% of the patch is "relevant".

*The changes are not completely random, but the only pieces that I change are in the direct surroundings of the patched parts. What I do, is that after I created the code but before I create the diff, that I (visually) scan for style errors (excessive whitespaces, tabbing, etc.). Since the code does not tell me what was the old code and what wasn't, this may have some side-effects on the diff.
**With some experience in physics calculations, I can assure you that not writing down everything clearly will inevitably lead to errors.

Post by **peter1138** » 27 Feb 2006 13:00

Do not include style changes in code you are not modifying. You may not care how the diff looks, but we do.

Darkvater · Post by **Darkvater** » 27 Feb 2006 14:02

hertogjan wrote:The reason I change some of the trunk style is that the code should be easy to read. As I said, if I am programming some piece of code, then I will automatically correct all things that are wrong*. I don't care much about how the diff looks, since at the end of the day we will be working with code, not diffs.
And really, the style changes are intended to be part of the diff. I will not release a patch that contains ugly code when it can be avoided. For proper functionality, clean code is needed** (thus the style changes are relevant in some way).

I really don't want to be rude, but I think you are missing the point of a diff. If I, or even somebody else for that matter looks through your changes, or the SVN commit log to see what was added in this or that revision because something might be buggy, style changes are VERY VERY confusing. Yes, write your code with the proper style, but leave the rest alone, producing a CLEAN diff. Anything else is 'rubbish' as far as the diff is concerned. Of course if the diff is never intended for a commit, but just for your own fun/exercise, it doesn't matter. But to be added to SVN it does.

And I know this from experience. I used to do this as well and when I look back to some of the changes I see a jungle of little style-fixes that have nothing to do with the actual change making it for me a lot more difficult to figure out what is going on. I try to avoid this as much as possible nowadays

hertogjan · Post by **hertogjan** » 27 Feb 2006 20:41

Darkvater wrote:(...)
Of course if the diff is never intended for a commit, but just for your own fun/exercise, it doesn't matter. But to be added to SVN it does.
(...)

The question is, who really wants to have this piece of code added to SVN?

I have discovered that the broken style in newgrf.c has been fixed by peter1138 today (revision r3678). Later this week (maybe tomorrow) I will change my patch to be compatible with those changes.
There are also a few style changes in roadveh_cmd.c, which I will track down and delete. But there are only a few of them so this wouldn't take more than 30 minutes I think.

hertogjan · Post by **hertogjan** » 08 Mar 2006 22:16

Okay, somewhat later than I promised, a renewed version of the physics patch. I did some code changes for better compatibility with the trunk code. Also, I fixed a little bug (in the display of train wagon weights).

There are still some technical things that I want to have solved before I may consider it "ready"*:
-The first issue is a common problem about the indexing of the patches GUI entries. This should be invariants, not hardcode numbers (see the other topic somewhere).
-The second issue is the support for the miscellaneous flags in the EngineInfo struct. In order to have tilting vehicles, the flag should be in RailVehicleInfo. It would be possible to set it in EngineInfo, but this requires a little change of the code. Another option is to discard the tilting flag for the standard vehicles**, and use it only as an option for newGRFs. My opinion is that the flags in RailVehicleInfo are better for this purpose, since this flag is specific to rail vehicles.

If some developer has some comment on this, please tell me so these issues can be solved. Also, if there are other issues, please let me know.

*i.e. to call it "ready" (from my side) to attempt to have it merged with the SVN trunk code.
**since part of the problem is that defining it in table/engines.h in EngineInfo is a little harder than in RailVehicleInfo.

DaleStan · Post by **DaleStan** » 09 Mar 2006 05:31

hertogjan wrote:-The second issue is the support for the miscellaneous flags in the EngineInfo struct. In order to have tilting vehicles, the flag should be in RailVehicleInfo.

All four vehicle types have misc. flags. Any particular reason you want the flags in RailVehicleInfo/RoadVehicleInfo/ShipInfo/PlaneInfo instead of in EngineInfo?

Actually, you should probably ask Peter1138 where he stores the 2CC flag, and use that; 2CC is one of the other misc flags.

hertogjan · Post by **hertogjan** » 09 Mar 2006 20:32

Currently, the misc flags are stored in EngineInfo. I have thought a little about it, and I think it is better to remove the tilting flag from the RailVehicleInfo flag (which was already existent for some other purposes). If I do it this way, I will have to remove the tilting flag for the standard engines. This also solves another issue (i.e. having to remove the tilting flag before loading a GRF file).
Those who want to play with tilting wagons for the standard vehicles should probably just use a GRF which just copies all vehicles, but with extra options (may be tilting cars, dual company colours, etc.)

Doing it this way has as a consequence that no tilting vehicles will be available wihout extra GRFs, but I think that the advantages (smaller chance of errors) outweigh the disadvantages.

hertogjan · Post by **hertogjan** » 22 Mar 2006 21:46

I have done some work on the patch. It has been a while since the latest version, and many things have changed in the SVN trunk. I adapted my code to be compatible with those changes. Also, I have fixed the issue I described above (about the miscellaneous flags).

There is one little new feature. For aircraft, you can choose to show the speed that matches the speed of the other vehicles. For instance, the fastest aircraft go as fast as a train going 512 km/h (approximately). With the patch disabled, you just see 2300 km/h (or something close to that value) in the aircraft status bar; When enabled, you see 512 km/h. It might look a little bit silly sometimes, since aircraft are way too slow. (A lot have been said about this in other topics.) Now it's only a GUI patch. Maybe in the future it will do more.

Also, I provide a Dutch language file. I will have more languages included if someone provides me the translations.

hertogjan · Post by **hertogjan** » 01 Apr 2006 21:45

The version of the patch I post here is compatible with a number of changes in the trunk. The two most important are electric rail and the new unit system.
About the first: The physics file was updated to be able to handle the new railtype. Also, I have made a little change in the power handling, which now also includes tractive effort, i.e. the tractive effort of a train composed of diesel (or steam) and electric engines will drop on non-electric tracks (just as the power does).
Everything in the GUI parts should now be compatible with the new unit system. I also added some things to this system, for instance to be able to show forces for maximum tractive effort. There are now five unit systems instead of three: Imperial (original), Imperial (American), Imperial (British), Metric, and SI (mks). Also the data for the road vehicles realistic acceleration patch* is displayed using this method.

For the ones who download the patch, please look closely at the numbers and try if you can spot some errors. I hope all displays correctly, but I am not sure. Especially about all weights being displayed properly (read the comment below).

*I had to make a workaround for displaying weights which are fractions (1/16) of tons. The code is a bit ugly, but it works (I think). The problem is that cargo weights are expected to be in tons (i.e. 1 cargo unit is set equal to 1 ton, for most of the bulk cargo). I will try to clean it up when I have time.

Brianetta · Post by **Brianetta** » 01 Apr 2006 23:33

That's cool - you can add an electric loco to your diesel consist to help it up hills, and only electrify the steep bits.

Shame the running costs will likely be the same whatever.

AlienDNA · Post by **AlienDNA** » 04 Apr 2006 09:46

Not to mention that it would be rather useless to have the diesel lugging the electric loco with it on the rest of the track, only having it's benefit on a short piece of track going up, and just it being dead weight on all the rest...

But indeed, possibilities are there

madman2003 · Post by **madman2003** » 04 Apr 2006 19:11

Does anyone have problems applying the patch to latest svn?

madman2003 · Post by **madman2003** » 05 Apr 2006 16:41

There was a change in include files (both the normal and the 32bpp branch didn't patch completely).

I modified the patch and then made a new patch, it's a diff between the 32bpp branch, but it'll probably work on the main brach as well.

I wanted to try it before the author could fix it and now i can

Transport Tycoon Forums

Physics patches

Who is online