Code refactoring etc. list

[neroden]

OK, I think I found the most recent "vanishing freight" bug and squashed it.  Haven't tested yet but the logic reads right.

We should probably make a list somewhere of cleaning up/recoding/refactoring things that need to be done or might need to be done so that we can organise them, prioritise them, and assign them.

That's a good idea.  Here's a start, feel free to copy and move it. Mod note: Topic split for discussion.

My personal top priority which I'm NOT currently working on is actually something which Standard needs fixed.  We have to implement a "viewport" which can be rotated without rotating the world.  A while back I worked out how to do this.
* Step one is separating all the uses of "koord" in the GUI, for pixels, from the "real" uses of koord, for map tiles.  Unfortunately prissi failed to review the final version of this patch promptly or commit it, and several weeks work went down the drain as it became out of date.  This is why I don't like trying to get patches into standard.
* Step two: once this is done, it becomes clear that the conversions between pixels and map files are only done in a few places: one for converting from mouseclicks to map position, one for converting from map position to display, a pair for converting to the "minimap", a pair for converting to the "world map", and a couple of places where the code was duplicated.  These conversion routines can then have a "twist" put into them to take a "rotation" parameter, rotating only in the GUI. 
* Step three: The rotation parameter must then be fed through to all the display (currently "zeichnen") routines to the "besch" level, and added (modulo 4) to the existing rotation of buildings to pick the right image to look up -- *at display time*.  Similar work will fix convoy visuals quite easily.
*Step four: Some extra tweaking may be needed to the display routines to make sure multi-tile buildings display in the right order and from the right starting point.
*Step five: fix the world-enlarging code to allow for enlarging on the "north" and "west" sides (which can currently only be done by rotating, enlarging, re-rotating)
*Step six: rip out the old "rotate90" world rotation code entirely.  It's thousands of lines of code which shouldn't exist at all.  It creates trouble for doing, basically, anything whatsoever.

---
Another cleanup which ought to be done in standard is to convert ribi and hang into actual classes, in order to get rid of a huge number of unsafe typedefs.  They would still be POD (plain old data) classes, but they'd have type-checked conversion routines instead of the not-type-checked stuff currently used.  This could uncover and fix a large number of lurking bugs.  Unfortunately, the first step here is again to separate the uses of koord in the gui (which don't have anything to do with ribi and hang) from the uses for the map (which do).

---
And then, of course, there's the time/distance/speed cleanups which I've been trying to do/hoping to do for years now. 

---
There's another category: integer math cleanups.  There are an awful lot of these in the experimental finance code, where stuff is multiplied by something and then later divided by it.  It makes it hard to test for overflow, among other things.  It might be advisable to rewrite a bunch of this stuff using Bernd's float32_e8_t portable floating point class.

---
And then there's the maintenance & cost stuff, where bugs keep showing up.  This is my first draft of a possible structure for that stuff:

(1) Each object has an owner (as they do now)
(2) There is a subclass of ding_t for all objects with maintenance costs.  Objects with maintenance costs inherit from this subclass, not from ding_t directly.  They will store monthly maintenance costs (running costs are another matter).  There will be a virtual "alter_maintenance_costs" method for each object whose maintenance cost changes over time (such as obsolete vehicles); this will carefully remove the old maintenance costs from the owner and add the new maintenance costs, and should be called monthly. These objects should naturally have a current value, as well -- the current value may also be a virtual method, and an "adjust_current_values" method can update them monthly too. This allows for accounting for asset values, as well as being available for use for other purposes.
(3) A call to change the owner (set_besitzer) will automatically remove the maintenance costs from the former owner and automatically add them to the new owner.
(4) A "new construction" operation will consist of, first checking that the construction is permitted; then constructing the ding_t with no owner; and finally changing its owner.
(5) A "deletion" operation will consist of, first checking that the deletion is permitted; then changing the ding_t to have no owner; and finally deleting it.
(6) For bridges and tunnels, they contain surface ways which are not supposed to be charged for.  I would very much like to restructure things so that the ways are separate from the bridges and tunnels, and charged separately.  Perhaps this should be done *before* the finance cleanup as it would make it all much simpler. 
(7) If this is not done, then certain objects (such as these ways on bridges) must have an alternate effective status for "owner" -- rather than being owned by a player, they must be "owned" by the bridge itself.  Not being owned by the player, the costs will not accrue to the player; the bridge may pass them on if and only if it wants to.  I know how to write this, but it's complicated.
( Other objects, like electrification, also need to be "attached" to a track.  Many of these can reasonably be owned by someone other than the track owner (particularly if the track owner is "unowned").  However, the track owner should have the right to (a) buy them or (b) delete them (after paying off the original owner, generally).  In the special case of electrification, other players should probably also be allowed to create electrification over someone else's line, but not to remove existing electrification.   These need to be handled carefully.  I've seen some funny behavior related both to "unowned" track and to the actions which the public player can do. 
(9) Another group of objects need to be restricted quite sharply to being owned by the track owner, particularly signs and signals, which can cause an awful mess for other players if they go on the other player's roads, or if they go on public roads and nobody else has the power to delete or replace them.  These should be transferred to the track owner whenever the track owner changes, using code in the set_owner routine for the track.
(10) The "buy (house)" tool would double up to buy roads, and to buy electrification etc. sitting on a base track
(11) The "deletion" tools would work by first purchasing all the property, and *then* deleting it (and either charging demolition cost or refunding scrap value); this would eliminate some confusing anomalies related to deleting stuff owned by "unowned".
(12) Almost everything would have positive value, and most ownership transfers would pay the current value.  There would be a "pay for it" and a "don't pay for it" version of the ownership transfer, because for example giving stuff to the public player should cost money.   
(13) Construction costs and Demolition costs / scrap refund must be charged and tracked separately, outside this automatic framework, partly because upgrades may require their own special rules.  Demolition costs / scrap refund should be set individually per besch; some of them should be positive (scrap value more than demolition cost) while others should be negative (scrap value less than demolition cost).  Most of the cost should be in the "buying" part of it, though.
(14) The maintenance costs should flow through to the tooltips automatically.  Somehow.

Thank you for all your bug-squashing work. This is not a good time to be a bug in Experimental, I think...

Heh. :-)

[jamespetts]

Very interesting thoughts - thank you for setting this all out! (Thank you also for your latest bug fix).

It would be helpful if you could set out which of these changes that you think are sensible to implement only on Experimental even if they are not implemented in Standard: the answer to that is clear for some of the items, but not all.

About the electrification, however: I do not think it a sensible idea to let one player electrify another's tracks. Having one type of electrification (say D. C. overhead lines) can prevent another type of electrification (A. C. overhead lines, for example) from being constructed. Further, if one player was allowed to build electrification on another's lines but not delete it (and the not deleting thing is for good reason), then the player who owns the track might prohibit access to the owner of the electrification, and take advantage of the electrification indefinitely, whilst the other player has to keep paying for it indefinitely, because it cannot be deleted. I think that it is preferable simply to let people only build way objects on their own ways.

[neroden]

About the electrification, however: I do not think it a sensible idea to let one player electrify another's tracks. Having one type of electrification (say D. C. overhead lines) can prevent another type of electrification (A. C. overhead lines, for example) from being constructed.

Ah, OK.  I was thinking of some real-world situations.  :-)

[jamespetts]

Yes, I suppose that it can work in real life where there is a far more fine-grained control over these things. Maybe this will be possible one day in Simutrans, but the sort of precision in control of assets necessary to do this would probably be very fiddly both to code and to use as a player, and would in any event be a very low priority.

[Markohs]

While I can agree to some of your points, I'd like to note here most of that changes are substantially deep, and will change experimental enough to make very hard the porting of future changes incorporated in standard to experimental.

But ofc, that's only my personal opinion.

[Dwachs]

To your thoughts on the rotation code: Pretty much agree with it.

In my opinion, this is independent of the separation of koord between map and screen coordinates. I think, the rotate-viewport stuff should be done in the routines that actually calculate the image of an object (calc_bild). That is, images do not have to be computed all the time only after change of rotations. Of course this forbids to have multiple viewports with different rotation each.

The real show-stopper I see is the following code in vehikel_t::hop (present in standard and experimental for ages):

Code Select Expand


if( check_for_finish & ist_erstes && (fahrtrichtung==ribi_t::nord || fahrtrichtung==ribi_t::west))
{
steps_next = (steps_next/2)+1;
}

It is a decision based on the current rotation of the viewport to influence the simulation behavior. In essence this implies rotation-dependent simulation. This piece of code was put there to minimize graphical glitches and all existing vehicle graphics are adjusted to this behavior.

If this part would be changed to take current viewport orientiation into account this means:
* The vehicles in stations might jump during rotation (ie their images jumps)
* Network games force one rotation upon all clients

If this part would be left out entirely would mean complete realigning of all existing vehicle graphics. I tried to do that automatically but the result was not that great.

[isidoro]

[...]
It is a decision based on the current rotation of the viewport to influence the simulation behavior. In essence this implies rotation-dependent simulation.
[...]

And that implies that ST has quantum mechanical behavior... 
The observer, while doing observations, can alter the state of the system...

[prissi]

With the (not so recent any more) improvements of the drawing code, we might test an offset automatic of vehivles on load time to compensate for the finishing position. This would also make stops at signals nicer.

[jamespetts]

With the (not so recent any more) improvements of the drawing code, we might test an offset automatic of vehivles on load time to compensate for the finishing position. This would also make stops at signals nicer.

That seems to be a good idea.

[neroden]

To your thoughts on the rotation code: Pretty much agree with it.

In my opinion, this is independent of the separation of koord between map and screen coordinates. I

It is possible to do it without separating the coordinates, but separating map and screen coordinates means that the compiler will check whether you have gotten confused!

(To clarify: the only conversions between screen coordinates and map coordinates should happen in generic "display" routines and generic "click on display" routines.  With separate screen and map coordinate types, it becomes possible to verify that you are correctly handling rotation, zoom level, etc. etc. because if you try to pass coordinates without going through one of the conversion routines, the compiler complains!)

think, the rotate-viewport stuff should be done in the routines that actually calculate the image of an object (calc_bild). That is, images do not have to be computed all the time only after change of rotations.

I don't think this saves any computational effort.  The images are all loaded into memory from the pak in any case.  "Calculating the image" is mostly a matter of grabbing the correct image number, which is an extremely cheap operation.

The way I see it, calc_bild will compute all four images.  The get_bild() routines will take an *argument* with value from 1 to 4 specifying the orientation to retrieve.  I realize this is a somewhat deeper piece of design change than doing it the way you proposed, but it's the right way to do it.

Don't make another design error, please; if you do, we'll be back here rewriting the code to allow for multiple simultaneous viewports with different rotations in a few years.  We can afford to pass a small integer specifying orientation through the "zeichnen", display, and get_bild routines.

Of course this forbids to have multiple viewports with different rotation each.

Which we would like to allow!

The real show-stopper I see is the following code in vehikel_t::hop (present in standard and experimental for ages):

Code Select Expand


if( check_for_finish & ist_erstes && (fahrtrichtung==ribi_t::nord || fahrtrichtung==ribi_t::west))
{
steps_next = (steps_next/2)+1;
}

It is a decision based on the current rotation of the viewport to influence the simulation behavior. In essence this implies rotation-dependent simulation. This piece of code was put there to minimize graphical glitches and all existing vehicle graphics are adjusted to this behavior.

IMNSHO, this piece of code has needed to be *removed* for a long time.  Creating logical gameplay glitches in order to "minimize graphical glitches" is simply poor design, IMNSHO.  Vehicle movement already has glitchiness (see below), and it is better to address this in a clean manner rather than messing with the gameplay logic. 

If I'm not mistaken, this code actually helps in creating some bizarre glitches in trying to click on vehicles, which are routinely not on the tiles they appear to be on.

If this part would be left out entirely would mean complete realigning of all existing vehicle graphics. I tried to do that automatically but the result was not that great.

This should be done.  It could be done on loading based on some flag in the .dat file for the vehicle, telling whether it needs to be "realigned" or not.  Vehicles with newer graphics could have "no realignment needed" flags set.

[neroden]

While I can agree to some of your points, I'd like to note here most of that changes are substantially deep, and will change experimental enough to make very hard the porting of future changes incorporated in standard to experimental.

I'd very much like to get a bunch of these changes into standard, but the people who decide what goes in standard have historically, in the past, had their own preferences. 

For instance, a preference for allowing "pretty graphics" to damage gameplay, and a strong preference for embedding trouble deep in code design (for instance, in the rotation code, or in the time and distance code) rather than doing clean code design.  As you know I strongly disagree with those preferences and my preferences are nearly contrary.  I am not sure whether my preferences are compatible with the preferences of the standard developers at all.

My preferences also differ from James Petts's preferences but appear to be *compatible* with his preferences. 

But ofc, that's only my personal opinion.

[prissi]

There are a lot of other issues with rotations and multi-tile building: Like certain rotations may have animations while other have not; i.e. some building are in sync_step and other not. Or certain tiles are occupied while other are not (most of this has be solved recently; but having different maps will lead to this).  Also directions are used as tie breaker, and actually vehicles have to be sorted by their offset (i.e. direction) into the tile list. Because a background car is a foreground car after 180 degree rotation. Sorting those each step will very likely affect performance notabley.

Thus there are quite a few places which need changes or even redesign. Actually my first attempt at implementing rotations I had been used only drawing; but I had realized that it would not work without a big effort after a week or so.

Simutrans was build without rotations in mind, and with a much different drawing system. Still, objects reaching in the tile left to them (which is the main reason vehicles are set back) may still produce errors in certain occasions (not least tunnels and slopes).

[neroden]

There are a lot of other issues with rotations and multi-tile building: Like certain rotations may have animations while other have not; i.e. some building are in sync_step and other not.

Why would anyone do this in a pak? 

Or certain tiles are occupied while other are not (most of this has be solved recently; but having different maps will lead to this).

This should be fairly straightforward to solve, and I need to solve it for other reasons anyway.  (Sigh.)

Also directions are used as tie breaker, and actually vehicles have to be sorted by their offset (i.e. direction) into the tile list. Because a background car is a foreground car after 180 degree rotation.

The display order is the key problem, yes.

Sorting those each step will very likely affect performance notabley.

My instinct says that we should store all four orders at all times.  This will keep performance fast but will increase *memory usage* substantially.  There may be some way to save on this; for instance to only store one list for things like ways and buildings, and to store four lists only for trees, pillars, and vehicles.  But that would have memory overhead too.

For the map as a whole, it's pretty easy to simply run through the tiles in a different order.

I suppose it might make sense, in terms of saving memory, to only store one order at a time -- but only if two games with different rotations, and therefore different orders, could be synced over the network.  :-)  This calls for making the "display order" separate from the "savefile order".

Thus there are quite a few places which need changes or even redesign.

Definitely true.  I think the work should be done from the "bottom up", fixing these places first.

Actually my first attempt at implementing rotations I had been used only drawing; but I had realized that it would not work without a big effort after a week or so.

Simutrans was build without rotations in mind, and with a much different drawing system. Still, objects reaching in the tile left to them (which is the main reason vehicles are set back) may still produce errors in certain occasions (not least tunnels and slopes).

It appears that simutrans goes through the list of objects on each tile "back to front", and it appears that simutrans goes through the list of tiles from "back" to "front" as well.  It seems as if this is done once for the background and (I am confused by the "display_after" routines, which appear to go front to back.)

Vehicles heading "up and left" or "up and right" on the screen should have their front ends in the "correct" tile, the one displayed first.  Vehicles heading "down and right" or "down and left" on the screen should have their *back* ends in the "correct" tiles. 

But we want to keep track of "front ends" for gameplay purposes. 

One natural way to handle this: make the display routines for the "tile in the background" hook into the list of vehicles for the "tile in the foreground" and display the ones whose back ends are on the "tile in the background", while the "tile in the foreground" ignores those vehicles.

--
But perhaps the most natural way to handle this: make it so that a vehicle which is present in two tiles is actually in the list of vehicles for both tiles. This could allow for the player to click on the vehicle from either tile, which seems desirable to me. 

Displaying it could be the responsibility of the tile in the "background"; the tile in the foreground would check its length and location and choose not to display anything.  (Alternatively, the two tiles could each clip the vehicle and display only their part of it, but this seems silly.)

Its official "position" (vehicle_t.get_pos) would still be the position of the front end.  However, it would be added to the list for the tile it is entering before being deleted from the list for the tile it is leaving.  This would require some fairly deep and subtle changes, but would probably give the cleanest solution.