Bug in bridge maintenance costs... reveals deeper design bugs

[neroden]

Really, there are so many bugs showing up.  The code needs a good clean-up.

Anyway, this one's easy to trigger.  Pop open the finance window, and look at the monthly fixed costs.

Build a bridge and watch them go up.
Demolish it and watch them go down.  But NOT BY THE SAME AMOUNT.

* At the moment, this is a rather nice way to reduce maintenance costs because the costs are subtracted roughly twice when deleting the bridge.  Pathway to profitability!   

This is a pretty serious bug. 

But further than that, it's actually not good that this kind of bug is *possible*.

*  I believe that currently, the accounting is "manually" adjusted every time something is built and every time something is destroyed.  That's a really bad way of doing things.  There should be a full list somewhere of all objects owned by the player, which can be indexed through to compute the fixed costs, and the fixed costs should be recomputed from scratch once a month when they're actually use.  That means that computation errors won't propagate and multiply, as they currently do.

* Finally, the bridge pricing is STILL per tile rather than per kilometer.  Everything else is per kilometer. Fix this.  Right now, the meters per kilometer can't be changed without skewing the price ratio between elevateds and bridges.  I realize this will require changing the pakset price numbers, but that's (a) not too hard, and (b) needs to be done anyway.

[neroden]

This one is *excitingly* abusable.  I can make the monthly costs negative.  Freeplay for everyone!

Not so good for multiplayer games...

----
The costs reset on save-and-load, at least.  Still sucks.

[jamespetts]

Thank you for the report - found and fixed on the Github branch.

I agree about the code design. From what I understand, it is all very old and has been in this state for years. The difficulty is that I can't practically make major architectural changes from Standard (whilst preserving function), or else it would be near impossible to merge changes in from Standard. I generally take the view that I only make changes from the Standard code where I want to change the function, so as to maximise the similarities between the two code-bases to facilitate easy merging of the improvements that are so often brought about to Standard.

Ideally, instead of having bespoke accounting code for each specific type of transaction, we'd have a base class called "asset" from which everything buyable and sellable was derived and the player class would have generic buy/sell/build/remove methods which would handle all of the accounting (which would be over-ridden but call the base method as appropriate). It would be quite painful to try to merge in changes from Standard if Experimental had this and Standard did not, however.

As to the scaling of bridge prices per kilometre: this is done - see simworld.cc

Code Select Expand


void karte_t::set_scale()
{
   const uint16 scale_factor = get_settings().get_meters_per_tile();
   
   // Vehicles
   for(int i = road_wt; i <= air_wt; i++)
   {
      if(&vehikelbauer_t::get_info((waytype_t)i) != NULL)
      {
         FOR(slist_tpl<vehikel_besch_t*>, & info, vehikelbauer_t::get_info((waytype_t)i))
         {
            info->set_scale(scale_factor);
         }
      }
   }

   // Ways
   stringhashtable_tpl <weg_besch_t *> * ways = wegbauer_t::get_all_ways();

   if(ways != NULL)
   {
      FOR(stringhashtable_tpl<weg_besch_t *>, & info, *ways)
      {
         info.value->set_scale(scale_factor);
      }
   }

   // Tunnels
   stringhashtable_tpl <tunnel_besch_t *> * tunnels = tunnelbauer_t::get_all_tunnels();

   if(tunnels != NULL)
   {
      FOR(stringhashtable_tpl<tunnel_besch_t *>, & info, *tunnels)
      {
         info.value->set_scale(scale_factor);
      }
   }

   // Bridges
   stringhashtable_tpl <bruecke_besch_t *> * bridges = brueckenbauer_t::get_all_bridges();

   if(bridges != NULL)
   {
      FOR(stringhashtable_tpl<bruecke_besch_t *>, & info, *bridges)
      {
         info.value->set_scale(scale_factor);
      }
   }

   // Way objects
   FOR(stringhashtable_tpl<way_obj_besch_t *>, & info, *wayobj_t::get_all_wayobjects())
   {
      info.value->set_scale(scale_factor);
   }

   // Stations
   ITERATE(hausbauer_t::modifiable_station_buildings, n)
   {
      hausbauer_t::modifiable_station_buildings[n]->set_scale(scale_factor);
   }

   // Goods
   const uint16 goods_count = warenbauer_t::get_waren_anzahl();
   for(uint16 i = 0; i < goods_count; i ++)
   {
      warenbauer_t::get_modifiable_info(i)->set_scale(scale_factor);
   }

   // Settings
   settings.set_scale();
}


and bruecke_besch.h:

Code Select Expand


void set_scale(uint16 scale_factor)
   {
      const uint32 scaled_price_preliminary =  set_scale_generic<uint32>(preis, scale_factor);
      const uint32 scaled_maintenance_preliminary =  set_scale_generic<uint32>(maintenance, scale_factor);
      scaled_price = scaled_price_preliminary > 0 ? scaled_price_preliminary : 1;
      scaled_maintenance = scaled_maintenance_preliminary > 0 ? scaled_maintenance_preliminary : 1;
   }


[kierongreen]

Having a list of all assets owned by a player would:
a) Take more memory
b) Take some time to sum maintenance.
c) Slow down building, and more particularly demolishing as items would have to be added/removed from the list.

[jamespetts]

Yes, I daresay - I don't think that that solution would provide good performance at all. An "asset" base class would not have these difficulties, however, as it would not change significantly the low-level computation of maintenance costs.

[neroden]

Yes, I daresay - I don't think that that solution would provide good performance at all. An "asset" base class would not have these difficulties, however, as it would not change significantly the low-level computation of maintenance costs.

Hmm.  Good idea.  This could be done with a mix-in class, since C++ has multiple inheritance.  This is actually one of the few good uses for multiple inheritance.

I wish the standard development cycle was... well, more bazaar-like, more git-style.  So that it would be *possible* to get fixes like this into Standard, in reasonable amounts of time.

I just did a nice little improvement which could almost certainly go into standard, but it's simply not worth my time to do the work to get it into standard.  (See other thread.)  I did that piece of code in about 8 hours, and I estimate that getting it into standard would take ANOTHER eight hours.  In contrast, getting it into experimental is basically a matter of pointing you to the correct branch in my git repository.

[Dwachs]

I just did a nice little improvement which could almost certainly go into standard, but it's simply not worth my time to do the work to get it into standard. 

which one? these translations in simcity et al? They look like they could applied to standard unchanged. Lets see if it is worth my time ...

[neroden]

which one? these translations in simcity et al? They look like they could applied to standard unchanged. Lets see if it is worth my time ...

The "pretty cities" stuff, yeah.  There's more than just translations.  If you've got the right tools, you might be able to do it a lot quicker than I could...