News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

[passenger-generation] factories don't save/restore jobs data

Started by Philip, September 16, 2013, 08:07:31 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Philip

In the passenger-generation branch, available_jobs_by_time is stored in the gebaeude_t structure. That's fine for attractions and city limits, but not for factories, because a factory's building is not saved and restored. After loading a game, all jobs will always be immediately available.

Let me first say that I'm very impressed with your work on the passenger-generation branch. I've been reading through the diff, and most changes are definitely improvements.

My proposal is to save and restore factory buildings like all other buildings, and things seem to be in the right order for that to work. The patch is a little clumsy, but it appears to work (admittedly, with some other changes to let me see jobs statistics for factories). If you decide just to leave things as they are for now, that's perfectly understandable—it's not that big a deal.


diff --git a/dataobj/dingliste.cc b/dataobj/dingliste.cc
index 670af2d..2c71f9b 100644
--- a/dataobj/dingliste.cc
+++ b/dataobj/dingliste.cc
@@ -1162,8 +1162,6 @@ void dingliste_t::rdwr(karte_t *welt, loadsave_t *file, koord3d current_pos)
||  d->get_typ()==ding_t::async_wolke
// fields will be built by factory
||  d->get_typ()==ding_t::field
- // do not save factory buildings => factory will reconstruct them
- ||  (d->get_typ()==ding_t::gebaeude  &&  ((gebaeude_t *)d)->get_fabrik())
// things with convoi will not be saved
||  (d->get_typ()>=66  &&  d->get_typ()<82)
||  (umgebung_t::server  &&  d->get_typ()==ding_t::baum  &&  file->get_version()>=110001)
diff --git a/dings/gebaeude.cc b/dings/gebaeude.cc
index 248386b..bafb02a 100644
--- a/dings/gebaeude.cc
+++ b/dings/gebaeude.cc
@@ -947,8 +947,6 @@ void gebaeude_t::new_year()

void gebaeude_t::rdwr(loadsave_t *file)
{
- // do not save factory buildings => factory will reconstruct them
- assert(!is_factory);
xml_tag_t d( file, "gebaeude_t" );

ding_t::rdwr(file);
@@ -1106,8 +1104,15 @@ void gebaeude_t::rdwr(loadsave_t *file)
file->rdwr_byte(dummy);
}

+ if(file->get_experimental_version() >= 12) {
+ bool f = is_factory;
+ file->rdwr_bool(f);
+ is_factory = f;
+ ptr.fab = NULL; // set_fab will be called soon enough, hopefully
+ }
+
// restore city pointer here
- if(  file->get_version()>=99014  ) {
+ if(  file->get_version()>=99014 && !is_factory  ) {
sint32 city_index = -1;
if(  file->is_saving()  &&  ptr.stadt!=NULL  ) {
city_index = welt->get_staedte().index_of( ptr.stadt );
diff --git a/simfab.cc b/simfab.cc
index 37d5f45..12f8bb7 100644
--- a/simfab.cc
+++ b/simfab.cc
@@ -968,7 +968,9 @@ void fabrik_t::baue(sint32 rotate, bool build_fields, bool force_initial_prodbas
{
this->rotate = rotate;
pos_origin = welt->lookup_kartenboden(pos_origin.get_2d())->get_pos();
+ if(!building) {
building = hausbauer_t::baue(welt, besitzer_p, pos_origin, rotate, besch->get_haus(), this);
+ }
pos = building->get_pos();
pos_origin.z = pos.z;

@@ -1462,6 +1464,15 @@ DBG_DEBUG("fabrik_t::rdwr()","loading factory '%s'",s);
}
}
}
+
+ if(file->get_experimental_version() >= 12)
+ {
+ grund_t *gr = welt->lookup(pos_origin);
+ gebaeude_t *gb = gr->find<gebaeude_t>();
+
+ building = gb;
+ building->set_fab(this);
+ }
}





(BTW, one of the other changes is to the new generate_passengers_or_mail routine, where passenger_trips_per_month is sometimes 0, which causes an arithmetic exception when we divide by it. I'm assuming you've probably fixed that already).

jamespetts

Thank you very much for that - that is most helpful. Fix incorporated (with some further changes to fix some bugs that then occurred when unloading maps). I had not yet spotted the divide by zero issue, but have now fixed it. Thank you for spotting that, too!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Philip

I'm terribly sorry, but I appear to have introduced two new bugs in fixing that one: setting the factory pointer to NULL when saving a file is wrong, of course. We can just rely on init() to do the right thing. Furthermore, we only set the factory pointer on the first tile of the factory; I'm confused by that: are we meant to have separate jobs statistics for each factory tile?

Looking at the code now, I also need to test it with fields, that might also be broken.

Here's a partial fix, though it might be best to go back to the old code for now.


diff --git a/dings/gebaeude.cc b/dings/gebaeude.cc
index 1c03b47..a8d573b 100644
--- a/dings/gebaeude.cc
+++ b/dings/gebaeude.cc
@@ -1109,7 +1109,6 @@ void gebaeude_t::rdwr(loadsave_t *file)
bool f = is_factory;
file->rdwr_bool(f);
is_factory = f;
- ptr.fab = NULL; // set_fab will be called soon enough, hopefully
}

// restore city pointer here


Again, sorry about that,

jamespetts

Philip,

thank you very much for the fix - that is appreciated. Don't worry about having made a mistake - it's easy to do with C++, especially with code that's been around as long as Simutrans and has been developed by amateurs.

As to factory tiles, the position is this: each individual factory tile is a separate gebaeude_t object. The same goes for attractions, and all multi-tile buildings. However, for any multi-tile building, there is a method get_first_tile() which returns one specific tile of that building that is treated as the principal tile. Because multi-tile buildings only need to store one set of statistics, I use the statistics in the tile returned by get_first_tile(), and ignore the statistics in the other tiles. I hope that this clarifies things - do ask if anything remains unclear.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Philip

Thank you for that explanation, I hadn't been fully aware of the get_first_tile() thing. In playing with the code, it seems when loading a file, we're calling add_ausflugsziel() before a building's position is set, so the first-tile-only logic doesn't work. Moving that call to laden_abschliessen seems to help:


diff --git a/dings/gebaeude.cc b/dings/gebaeude.cc
index a8d573b..6cb043c 100644
--- a/dings/gebaeude.cc
+++ b/dings/gebaeude.cc
@@ -1164,12 +1164,6 @@ void gebaeude_t::rdwr(loadsave_t *file)

adjusted_jobs = welt->calc_adjusted_monthly_figure(jobs);
adjusted_mail_demand = welt->calc_adjusted_monthly_figure(mail_demand);
-
- // Hajo: rebuild tourist attraction list
- if(tile && tile->get_besch()->ist_ausflugsziel())
- {
- welt->add_ausflugsziel(this);
- }
}
}

@@ -1211,6 +1205,12 @@ void gebaeude_t::laden_abschliessen()
ptr.stadt = NULL;
}
}
+
+ // Hajo: rebuild tourist attraction list
+ if(tile && tile->get_besch()->ist_ausflugsziel())
+ {
+ welt->add_ausflugsziel(this);
+ }
}




What's the situation for halts, depots, and HQs? All those return something from ->get_jobs(), so I'm assuming they should count as commuter targets? That doesn't appear to be happening for depots.

If I've traced this correctly, it's because depots can't be found with gr->find<gebaeude_t>()—I think this explains the "This sometimes happens for unknown reasons" comment in simworld.cc...and is probably causing problems elsewhere.

Another thing I don't understand about multi-tile buildings is that they get added, in add_building_to_world_list, to commuter_targets etc. multiple times, each time with their full weight. They're thus much likelier to be picked than 1x1 buildings, and also we end up with duplicate entries. Should gb = gb->get_first_tile() be replaced by if(gb != gb->get_first_tile()) return;?

I've attached a patch with that and a few more minor fixes to simworld.cc.

Thanks again,
Philip

jamespetts

Philip,

thank you very much for that. I have applied your second set of fixes. I do wonder about the issue with  if(gb != gb->get_first_tile()) return, and in particular whether if some method only added a single tile to the list which was not the first tile, this would not work: but my brief testing suggests that multiple tiles are only sought to be added with town halls, and there were no other instances, loading demo.sve, at least, in which "return" was hit, so I have included this for now at least.

I tried your first patch, but it caused access violations for me, I am afraid, which I do not have time to track down fully now. Is this actually necessary given that "return" in the code snippet identified above is not actually hit, at least on loading demo.sve, which has plenty of town based attractions (look at the football stadium and greenhouse in Christminster, for example), except in appropriate places for the town hall?

As to depots, headquarters and stops: I added code to add headquarters this morning. Depots should already be added, but are subject to the problem that you have pointed out and I had not noticed: thank you for spotting that. I have not yet had chance to check: does your second .diff fix the issue with depots? I notice that you replaced a number of instances to gr->find<gebaeude_t>() with much simpler references to destination.building (and I was rather silly for missing the possibility of doing that, due, I think, to having put in all the gr->find<gebaeude_t>() code before all destinations had a pointer to a gebaeude_t object, rather than just, as in 11.x, city building destinations). This seems to improve performance a little, too. Stops should be added, too (at least extension buildings): see line 3995 of simwerkz.cc.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Philip

James,
the access violation is curious. I suspect what might be happening is related to multi-threaded loading. Leaving out my change is probably best for now. If I'm thinking about this correctly, we're now at the point where all gebaeudes' first tiles are added to the world lists (I'm not sure about fisheries and shipyards, though), so it might be best to move that code entirely in the next post-paxgen release (13.x?).

I'm still running into one division by zero when starting some games, and I'm also running into problems with the multi-thread code, which I believe is because karte_t::load and karte_t::save got desynched (maybe that explains the access violations, too?)

Suggested fix:

diff --git a/simworld.cc b/simworld.cc
index dd6bfdc..1547096 100644
--- a/simworld.cc
+++ b/simworld.cc
@@ -4384,7 +4384,7 @@ void karte_t::step_passengers_and_mail(long delta_t)
const uint32 mail_packets_per_month_hundredths = calc_adjusted_monthly_figure(10u);

const uint32 passenger_trips_per_month = max((passenger_origins_weight * passenger_trips_per_month_hundredths) / 100u, 1u);
- const uint32 mail_packets_per_month = (mail_weight * mail_packets_per_month_hundredths) / 100u;
+ const uint32 mail_packets_per_month = max((mail_weight * mail_packets_per_month_hundredths) / 100u, 1);

passenger_step_interval = ticks_per_world_month / passenger_trips_per_month;
mail_step_interval = ticks_per_world_month / mail_packets_per_month;
@@ -6269,14 +6269,25 @@ DBG_MESSAGE("karte_t::speichern(loadsave_t *file)", "saved messages");
file->rdwr_long(dummy);

if(file->get_version()>=99018) {
+ int adapted_max_world_cost;
+ if(file->get_experimental_version() >= 12)
+ {
+ adapted_max_world_cost = MAX_WORLD_COST;
+ }
+ else
+ {
+ // Before Experimental version 12, private car ownership data were saved in cities.
+ adapted_max_world_cost = MAX_WORLD_COST - 1;
+ }
+
// most recent version is 99018
for (int year = 0;  year</*MAX_WORLD_HISTORY_YEARS*/12;  year++) {
- for (int cost_type = 0; cost_type</*MAX_WORLD_COST*/12; cost_type++) {
+ for (int cost_type = 0; cost_type<adapted_max_world_cost; cost_type++) {
file->rdwr_longlong(finance_history_year[year][cost_type]);
}
}
for (int month = 0;month</*MAX_WORLD_HISTORY_MONTHS*/12;month++) {
- for (int cost_type = 0; cost_type</*MAX_WORLD_COST*/12; cost_type++) {
+ for (int cost_type = 0; cost_type<adapted_max_world_cost; cost_type++) {
file->rdwr_longlong(finance_history_month[month][cost_type]);
}
}

jamespetts

Thank you very much for that - I have implemented your fixes. I am not sure that I quite understand what you mean by moving "that code" into the next version - to which code are you referring here?

Incidentally, on a slightly different but somewhat related topic, have you considered getting an account at Github? You might find it easier to branch from my repository, make changes, and then issue pull requests for me automatically to merge them than post patch files here.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.