Patch: start to separate out unit conversions

[neroden]

This is a code cleanup patch.  It should *not change any behavior*.  It compiles and appears to indeed, not change any behavior.

I have done several things here, but the main one is the introduction of simunits.h.  I am hoping to straighten out some of the many undocumented unit conversions within simutrans.

This involves introducing new symbolic constants.  I have been very careful; they are all either static const values defined in classes header files or #defines, so they should be just as fast as the hardwired constants.  There is also some arithmetic of constants which I wrote out, which any good compiler should do constant folding on.  I introduce a few new routines, but they are all inline-expanded and should again keep things fast.

I also rearranged the variable usage in a few key routines for clarity, separating out local variables which were conceptually different rather than reusing them.  I was very careful; any modern optimizing compiler which tracks variable life and handles the named return value optimization should ensure that this is just as fast as the pre-existing code, and I know that Visual C++ and GCC both do (I actually know the internals of GCC so I know this for certain).  I mention this because these are critical-section routines.

I have defined CARUNITS_ related conversions which are different from TILE_STEPS, for clarity.  TILE_STEPS is currently used for some confusing rasterization processes which I am not yet ready to clean up (my scheme for cleaning it up depends on separating the use of koord for screen/gui stuff from the use of it for map coordinates) but I needed to straighten out the part involving carlengths.

I am planning to introduce more defined constants and routines for unit conversions like this, in hopes of cleaning up more of the very ugly formulas scattered around the code, but I think this is enough for one patch.

[prissi]

TILE_STEPS is mainly nowadays for old game loading, since in the old days a car could only go 16 units on a tile. It has nothing to do with steps any more. Thus calling something get_length_in_steps() is misleading, imho.

---
Some personal comments to this good idea:

Maybe one should stick to VEHICLE_xxx thus VEHICLE_STEP_LENGTH or so.

Using lower case constants in classes is imho not good. Why not sticking to the uppercase convention? Also, since it is direly derived from a define, why not using this. Less confusion to find the matching parameter, or?

The internal pixels is probably more confusing than just leaving it away.

Microns? What does this mean? You mean the speed calculation? If any, this is the internal hour, as time(tick) + speed = distance (step)

km is not defined as a distance in simutrans. It should be better left out.

day are not derived from ticks; the only exist in certain settings of show month. THeir length also depends on the setting of show month. Some for hour and minutes. THere is no correlation between hours and km/h. Hour will be longer with longer bits_per_month setting while the speed is not affected.

Z_HEIGHT_STEP is for double height extension. (In the old days, z koordinate was unit16 and a height step were not 1 but 16.)

But it may indeed help to have this clarified within a file.

[neroden]

TILE_STEPS is mainly nowadays for old game loading, since in the old days a car could only go 16 units on a tile. It has nothing to do with steps any more.

Thanks very much for that information!  I still haven't figured out its use in rasterization, which I am leaving alone for now.  Should I rename it to a name more useful for its use in rasterization?

Thus calling something get_length_in_steps() is misleading, imho.

Ah, but it does get length in steps -- in the 256-step-per-tile steps actually used today!

Maybe one should stick to VEHICLE_xxx thus VEHICLE_STEP_LENGTH or so.

I actually tried that but decided (a) the names were too long (b) I wasn't sure the 256 steps per tile were really "vehicle steps" in any sense.  And they are called "steps" throughout the fahre_basis code.  But I could go back to the VEHICLE_STEP names if you like.  There is an entirely different sort of step (sync_step / step) so I can see that just calling them "steps" is problematic.  If you can think of a better, short name for the unit that would be perfect.

Using lower case constants in classes is imho not good. Why not sticking to the uppercase convention?

orthogonal_length is parallel to diagonal_length and belongs next to it in the same class.  Happy to make it upper case if you really want me to.

I actually think both of them are misnamed, and should be orthogonal_steps_max and diagonal_steps_max.  Or orthogonal_vehicle_steps_max and diagonal_vehicle_steps_max, to use the longer names.  That seemed like more renaming work, but shall I try it?

Also, since it is direly derived from a define, why not using this. Less confusion to find the matching parameter, or?

It was important to have both a #define for 256 and one for 255, because both of them get used heavily. This caused me a great deal of trouble figuring out appropriate names, and I don't think I succeeded.  But once I get the names right, I'm happy to put them in the same place and make them both #defines, if you like.

The internal pixels is probably more confusing than just leaving it away.

That is just my attempt to document things for myself, I don't think I actually did anything with that, I'll clean that up.

Microns? What does this mean? You mean the speed calculation?

I had to come up with a name for the distance unit passed to fahre_basis, since it doesn't have a name.  It is very small, and appears to be defined so that 1 step = 2^12 microns or 1 tile = 2^20 microns.  I am open to suggestions as to what to call it, but it's used in function interfaces so it needs a name.

If any, this is the internal hour, as time(tick) + speed = distance (step)

Huh?  Your formula is wrong and doesn't make sense.  I reviewed the code.  Time (ticks) * speed (ticks/micron) = distance (in "micron" units of 2^20 per tile, NOT steps of 256 per tile).

km is not defined as a distance in simutrans. It should be better left out.

Huh?  Yes, km is implicitly defined in simutrans.  As long as you have km/h, you have km, and "km" is all over the translations.  I have to mention it, one way or another.  I can rearrange the way it's described.

day are not derived from ticks; the only exist in certain settings of show month. THeir length also depends on the setting of show month. Some for hour and minutes. THere is no correlation between hours and km/h. Hour will be longer with longer bits_per_month setting while the speed is not affected.

That's not the kind of hours I meant, I'm trying to define the hours implied by km/h.  I'll make that clearer.

I realize that the initial design of simutrans involved a completely fake and bogus km/h where there were neither hours nor km involved.  This was a bad design and confuses everyone.  However it can be straightened out by having some sort of meaning for km and h, and once that is done, these can be shown to the player in an info window (in a "1 vehicle km = 1 tile, one vehicle hour = .055 of a game month" sort of way), which would be VERY HELPFUL as a player when trying to figure out how far a vehicle can travel in a month.  I am trying to get to the point where I can write that, but it's very confusing right now.

Z_HEIGHT_STEP is for double height extension. (In the old days, z koordinate was unit16 and a height step were not 1 but 16.)

I know, and I'm not dealing with that yet.

But it may indeed help to have this clarified within a file.

You're **** right it helps.  It's absolutely necessary to fix this sort of stuff if we want maintainable code.  Your very comments have shown that the existing situation confuses *you*! :-)

Could you please answer these questions:
(1) What should I call the very small unit for distance which is passed to fahre_basis?
(2) What should I call the (length in steps -1) version of the length of a tile (255 orthogonally, 180 diagonally)?
(3) What should I call the (length in steps) version of the the length of a tile (256 orthogonally, 181 diagonally)?
(4) What should I call the steps actually used in fahre_basis, 256 per tile.  "vehicle steps", "steps", what?
(5) Should I rename TILE_STEPS to something less misleading for its use in rasterization?

[prissi]

First one comment:
private static const inside classes are not working in all compilers (Or forgot whether static or private was the issue). We had troubles with this in the past, therefore I would use either global const, enums in classes or defines.

Second comment:
Even if used heavily, giving constants-1 another name is making things difficult. It invites all sort of subtle errors like using the wrong constant; and because it is only wrong by one it is nearly undetected.

About the microns: Now I see what shift do you mean. This was actually a shift by 16 before, but then due to the new system it was shifted to 12. You can call it yards ... Honestly, one should just state that this is an empirical divider to make game speed appealing on the display. Much like:

To convert the speed per time into vehicle_steps the following formula is used: ((ms * speed) >> 12)
This might be even more useful directly as a comment in fahre_basis and leaving_depot

I just checked again: There is no km or h information within the translations, and this is by design. The simutrans engine cannot model real world, the scales are too different. (This is one of the main difference I have with experimental.) I could live with an indicator 120km/h means xx tiles per month, which could be indicated in the speed bonus dialogue (assuming no diagonals etc. and of course caluclated for the bit_per_month setting) Any other distance information is not present in the game text intentionally.

(The actual formula is very simple, as you can derived from you work:
tile_per_month=(((1<<(bits_per_month-12))-1)*speed)/(TILE_STEP*16)
)


Could you please answer these questions:
(1) What should I call the very small unit for distance which is passed to fahre_basis?
Nothing: A comment in the file should be enough. It is the same arbitary choice of kmh_to_speed uses ...
(2) What should I call the (length in steps -1) version of the length of a tile (255 orthogonally, 180 diagonally)?
Nothing, use the long version -1 as mentioned above
(4) What should I call the steps actually used in fahre_basis, 256 per tile.  "vehicle steps", "steps", what?
I am not the right person here to ask here. I usually never confuse function step with variable steps. Moreover, for me doing steps in a function step is somewhat logical.
(5) Should I rename TILE_STEPS to something less misleading for its use in rasterization?
OLD_VEHICLE_STEPS and VEHICLE_STEPS (or whatever you the small steps within a tile want to name) seems a sensible choice for me.

[neroden]

First one comment:
private static const inside classes are not working in all compilers (Or forgot whether static or private was the issue). We had troubles with this in the past, therefore I would use either global const, enums in classes or defines.

Interesting.  There are problems with global static const not doing constant-folding properly in some compilers.  I think it was private which was the issue, not public static const, which really should work.  Enums cause type-handling problems.  I guess I'll just use #defines.

Second comment:
Even if used heavily, giving constants-1 another name is making things difficult. It invites all sort of subtle errors like using the wrong constant; and because it is only wrong by one it is nearly undetected.

Sigh.  I've already FOUND a whole bunch of off-by-one errors like that, with the EXISTING code, and I'm open to better ideas on how to prevent further such errors.

This is a psychological question.  I think using "diagonal_steps_max" instead of "diagonal_length" is the less likely error.  I think writing "diagonal_length" instead of "diagonal_length-1" is the more likely error.  You think the other one is the more likely error.  I don't know which is really more likely.

I do know that writing "diagonal_length" instead of the correct "diagonal_length +1" has happened repeatedly throughout the vehicle overtaking code, so *that* error has already been made.

I think now, after what you've said, that the biggest problem is that the name diagonal_length is too misleading, and must be changed.

I could use diagonal_vehicle_steps_per_tile (== 181) and VEHICLE_STEPS_PER_TILE (==256).  This requires, unfortunately, a very large number of instances of "diagonal_vehicle_steps_per_tile -1", and I fear that people will accidentally leave out the "-1", causing off-by-one errors.

Or, if it's stored in a uint8 and we are using the 256, a very serious overflow error, which would be detected by the compiler, or would cause very dramatically wrong behavior.  So I guess that is a good argument for doing it this way, now that I think about it!  I will do it this way.

About the microns: Now I see what shift do you mean. This was actually a shift by 16 before, but then due to the new system it was shifted to 12. You can call it yards ... Honestly, one should just state that this is an empirical divider to make game speed appealing on the display.

It's used too heavily not to name it.  I realize it's an arbitrary unit for internal purposes, designed mostly for having greater "smoothness" of vehicle motion, and in fact it seems to be the smallest power of 2 unit which allows for the distance information to not overflow a uint32.

To convert the speed per time into vehicle_steps the following formula is used: ((ms * speed) >> 12)
This might be even more useful directly as a comment in fahre_basis and leaving_depot

Too many locations in the code.  The shift by 12 is really in far too many locations, the 12 needs to be a named constant.

I just checked again: There is no km or h information within the translations, and this is by design. The simutrans engine cannot model real world, the scales are too different. (This is one of the main difference I have with experimental.) I could live with an indicator 120km/h means xx tiles per month

That's what I'm really hoping to get.  When I play, I currently pull out a bus, run it for a month on a straight path, and count how many tiles it's travelled to figure the tiles per month, which is kind of a stupid thing to do.

, which could be indicated in the speed bonus dialogue (assuming no diagonals etc. and of course caluclated for the bit_per_month setting)

OK, I was very much not sure where to put the tiles/month number even when I figured it out.  :-)

(The actual formula is very simple, as you can derived from you work:
tile_per_month=(((1<<(bits_per_month-12))-1)*speed)/(TILE_STEP*16)
)

Gack.  Not simple enough to do in my head as I'm playing :-)

(1) What should I call the very small unit for distance which is passed to fahre_basis?
Nothing: A comment in the file should be enough. It is the same arbitary choice of kmh_to_speed uses ...

I really need a name for it, we're using the unit in a function interface.  Also this is the source of the "magic 12" all over the code, which really deserves a named constant, because as you say it used to be 16 -- and we might have to change it again later.

You suggested "yards", I'll call it "yards".  It should only be seen by programmers.

(2) What should I call the (length in steps -1) version of the length of a tile (255 orthogonally, 180 diagonally)?
Nothing, use the long version -1 as mentioned above

I can do this and I agree it's the right thing to do.  I will have to a large patch to change every use of diagonal_length by 1.  I have to adjust old_diagonal_length too, if I do it this way.  And the loading and saving routines.... It will be a very large and ugly patch, so it may take me a while to finish it.

(4) What should I call the steps actually used in fahre_basis, 256 per tile.  "vehicle steps", "steps", what?
I am not the right person here to ask here. I usually never confuse function step with variable steps. Moreover, for me doing steps in a function step is somewhat logical.

Then I will call them "vehicle_steps".  I suppose it never hurts to be verbose, and you suggested that earlier.

(5) Should I rename TILE_STEPS to something less misleading for its use in rasterization?
OLD_VEHICLE_STEPS and VEHICLE_STEPS (or whatever you the small steps within a tile want to name) seems a sensible choice for me.

OK, I will use OLD_VEHICLE_STEPS.  I still intend to separate this from CARUNITS, since the use in rasterization currently seems to have nothing to do with the use of the unit for the length of vehicles.

Thank you for your very helpful feedback.  I will come back with a rather different second patch, since adjusting diagonal_length will involve a lot of code changes.

[prissi]

It is good to get external input on this. SOmetimes I am too much into thing I am too familiar with and then are very conservative about it.

In principle one can also use
#if  XX !== XXX-1
#error "XXX != XX"
#endif
or use assert() to ensure both are really off by one ... but since you agree now with me on -1 this is not needed any more.

About the >>12 and "yards": At which place in the code this is used? I only found convoi in step() LEAVING_DEPOT and vehicle_basis_t fahre routine. Aparently I missed some other places then.

But again I agree to your suggestions. ANd please take your time, as the next weekend hopefully sees the release as soon as the Macintosh error is solved. Then this is a very good point in time to incorporate such a patch.

[inkelyad]

Or, if it's stored in a uint8 and we are using the 256, a very serious overflow error, which would be detected by the compiler, or would cause very dramatically wrong behavior.

Why so much uint8 and uint16 in simutrans code? I can understand it in 'on-disk' format, but in running code? why not use plain int?

[Spike]

Why so much uint8 and uint16 in simutrans code? I can understand it in 'on-disk' format, but in running code? why not use plain int?

In the early days, memory buses were rather slow and narrow, so it was important to reduce the amount of data transferred between CPU and memory. Simutrans uses to read and write a lot of data to/from memory, so this was a bottle neck in the past.

The uint8 and uint16 often were used to keep the in-memory size of objects small, and therefore transfer more quickly to/from memory.

I don't know if this still is a bottleneck in modern systems and Simutrans versions.

Edit: I once did tests and random meory reads were really a problem there:

http://www.funkelwerk.de/site/index.php/articles/programming/code-optimization

If you scroll down quite a lot to find a section "Read/write data linearly" you can find an example program that I used for testing and also a table of results.

While linear read on that machine gave about 200MB/s random byte reads from memory just came in at below 5MB/s, and a large map on Simutrans is a multiple of 5MB, and therefore it was important to transfer as little data as possible to make it run somewhat efficiently.

I guess on modern systems the numbers are much different ... also there always could be the chance that my way to measure the transfer speed was wrong. Source code for the testing code is available, so you can look and try by yourself if you want to. A link to the code is in the second paragraph below the result table.

[prissi]

The main reason for uint8 is that those consume memory. Not so much if you think of convois. But think of pedestrian, cars, movingobj and suddenly size matters, as there are millions on larger maps. Even more, a 32 byte object fits into a Level 1 cacheline, a 36 might require 2. (This gets of course worse with 64 bit systems).

The data structures in simutrans are packed and optimized for 32 bit Intel GCC, as this is major target on 99% of the releases.

[neroden]

But again I agree to your suggestions. ANd please take your time, as the next weekend hopefully sees the release as soon as the Macintosh error is solved. Then this is a very good point in time to incorporate such a patch.

Sorry it's taken me so long to get back to it... had to go on a trip and deal with medical stuff.  Hope it will still be a good time to incorporate such a patch when I finish it.  :-(

EDIT: NEW VERSION OF PATCH READY.

This incorporates previous comments.  It corrects a number of off-by-one errors and replaces a number of uses of raw numbers with #defined constants.  This is a patch against current SVN.  There's more code cleanup I could do afterwards, but I think this does enough for a single patch.

[jamespetts]

Ahh, 110.0 was released recently, so hopefully this is indeed a good time! Welcome back, and I hope that you are well.

[neroden]

Erp, crossed messages with James!  New version of patch is attached to previous message.

Thank you for the wishes of good health.  I am... much better than I was, if not completely well.

EDIT: James, the adaptation of this patch to experimental is in the "experimental-time" branch on github.

[neroden]

HEY!  SIMUTRANS DEVELOPERS!

There's a patch I submitted back on March 6th, several comments up (Reply #9).  Could someone please review it?  It sometimes seems like it's as hard as pulling teeth to get cleanup patches into simutrans standard.

[Dwachs]

It does not apply cleanly anymore. Could you please upload an updated patch?

[neroden]

Updated patch.

[Dwachs]

One complaint so far: The constant name 'OLD_VEHICLE_STEPS' is not very instructive. Am I right that this constant is linked to offset calculations of object on a tile? Maybe OBJECT_OFFSET_STEPS or DING_OFFSET_STEPS is more descriptive?

[neroden]

One complaint so far: The constant name 'OLD_VEHICLE_STEPS' is not very instructive. Am I right that this constant is linked to offset calculations of object on a tile? Maybe OBJECT_OFFSET_STEPS or DING_OFFSET_STEPS is more descriptive?

I was not clear on what it was still being used for when I separated it out; I simply separated out the vehicle related stuff and saw what was left.  You're right, it seems to be used for offsets of objects on tiles.  OBJECT_OFFSET_STEPS is a great name, I'll change the patch to use that.  Thanks for the feedback!

EDIT: Revised patch attached (also updated to patch against current HEAD)

(PS to James Petts: thanks again for getting me introduced to git.  I'd never be able to maintain multiple different patches, some depending on others, and keep them up-to-date when HEAD changes, without it.)

EDIT: Grr, incomplete patch, here's version 6.

EDIT: Now that I'm focusing on the uses of OBJECT_OFFSET_UNITS, I'm spotting a very large amount of repeated rendering code for, basically, rotating from rectangular to isometric.  Perhaps if this patch goes in I can make a followup patch to abstract that repeated code out into inline functions.

[Dwachs]

Thank you, incorporated in r4520.

EDIT: Now that I'm focusing on the uses of OBJECT_OFFSET_UNITS, I'm spotting a very large amount of repeated rendering code for, basically, rotating from rectangular to isometric.  Perhaps if this patch goes in I can make a followup patch to abstract that repeated code out into inline functions.

Please go ahead