News:

The Forum Rules and Guidelines
Our forum has Rules and Guidelines. Please, be kind and read them ;).

Branch 11x-fixes

Started by neroden, May 27, 2013, 03:24:33 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

neroden

James, I think you finally found the place where the freight distance for origins is checked.  (Destinations will take a bit more work.)  However, there was a *really* nasty bug in your last "fix", where you had the sign reversed, so the factory would only ship to halts OUTSIDE the coverage radius.

Anyway, I fixed that. 

...And then I got going on simfab.cc.  I cleaned it up a *lot*, and removed several subtle logic errors.  This compiles.  I haven't had a chance to test it (have to run off to a Memorial Day BBQ) but the logic is very carefully worked out so it should be fine.

This is on my branch 11x-fixes.  I suggest you merge it with your 11.x branch.

jamespetts

#1
Thank you - that is very helpful. What are the outstanding issues with the destinations that you have found?

Enjoy the barbecue!

Edit: I notice that the performance of this seems to be even worse than 11.9006 - it seems that calling recalc_nearby_halts(); every step (as void fabrik_t::recalc_factory_status() is called every step) is excessive; but there is not an easy answer as to when, if not every step, this should be recalculated. Any thoughts on how to improve the performance here would be welcome.
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.

neroden

#2
I'm not sure what errors with destinations remain now because I'm not sure how many of the outstanding problems I just fixed.  Give me a chance to check...

Quote from: jamespetts on May 27, 2013, 05:16:57 PM
Edit: I notice that the performance of this seems to be even worse than 11.9006 - it seems that calling recalc_nearby_halts(); every step (as void fabrik_t::recalc_factory_status() is called every step) is excessive; but there is not an easy answer as to when, if not every step, this should be recalculated. Any thoughts on how to improve the performance here would be welcome.

For now, just delete the call to recalc_nearby_halts in ::recalc_factory_status entirely.  It's only used there to update the color of the bar on the factory window.  I think it's OK if that's out of date (I think it can be out of date for other reasons anyway).

The data is used but not updated in ::info_conn().  We could put a call to recalc_factory_status there, but that would violate the expectation that viewing the info window shouldn't change the internal game state (and violate const-correctness).  I also think it's OK if the info window information is out of date.

The data will still be updated with every call to verteile_waren, which is currently the only place which is using it for *game logic* rather than simple display information.  This should be good for now. 

---
In fact, this is actually far more often than it needs to be updated.  It should really only be updated at the following times:
(1) when planquadrat_t::add_to_haltlist is called on any tile on which the factory is sitting
(2) when planquadrat_t::remove_from_haltlist is called on any tile on which the factory is sitting
(3) when a new factory is created (or a factory is replaced)

This would speed things up a lot.  However, rearranging the code to do this is more work, and somewhat finicky work, so it's not going to get done today!

...
oh, and the BBQ was fun, thanks!

---
(edit)
Do you (or someone else) know how to find out: if we have a particular planquadrat_t, whether a factory is sitting on it, and if so, what the fabrik_t object for it is?

---
(edit)
Mmm.  It would also need to be rebuilt on game loading.

jamespetts

Thank you for that suggestion of removing the call to recalc_nearby_halts - now implemented. The performance has not been greatly improved, although some profiling might be necessary; I suspect that it will take me some time to work out how to set it up, though, as I use MSVC++ Express, which does not have profiling support built in.

To answer your question, you need to call fabrik_t::get_fab(welt, pos);, where "pos" is the koord of the planquadrat_t object that you wish to interrogate. It returns the pointer to the factory if there is one, or NULL if there is not.
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.

neroden

#4
Quote from: jamespetts on May 28, 2013, 01:06:23 AM
Thank you for that suggestion of removing the call to recalc_nearby_halts - now implemented. The performance has not been greatly improved, although some profiling might be necessary; I suspect that it will take me some time to work out how to set it up, though, as I use MSVC++ Express, which does not have profiling support built in.

To answer your question, you need to call fabrik_t::get_fab(welt, pos);, where "pos" is the koord of the planquadrat_t object that you wish to interrogate. It returns the pointer to the factory if there is one, or NULL if there is not.
No good, too slow.  That cannot possibly be how it works when I click on a tile and it pops up with the "Look!  There's a factory here!" info window.

----
Hmmm.....  I spoke too soon.  Maybe that subroutine implements exactly the same procedure, looking at it...
I can't, unfortunately, recover the koord from the planquadrat_t object, which means I have to find all the places which make the function calls.

----
OK, 11x-fixes branch now has the version which only updates the list on an as-needed basis.  :-)  I also fixed what I believe is a bug which exists in standard, a stray remove_from_haltlist line in a place where it doesn't belong (bauer/hausbauer.cc).

Next question.  Could you explain to me what's going on with the arguments for haltestelle_t::find_route -- the long version.  Specifically, what is the destination_pos argument supposed to signify when it is present?  I suspect there's a logic bug buried in here, but I can't figure out what was intended.

neroden

-poke- James, I'm not sure if you noticed that I updated the branch to be less computation-intensive (see above)

jamespetts

#6
Have not had much time to-day - will look into this and other things later. Thank you very much for your efforts!

Edit: Now incorporated and pushed. Thank you very much for that - most helpful. Still rather limited for time, so cannot respond in full this evening, but the efforts are most appreciated.
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.

neroden

By the way, thanks (a) for responding quickly, and (b) thanks again for introducing me to git, which I love.  :-)  It makes it easy to work on multiple different simutrans projects without having them interfere with each other.

jamespetts

Thank you for the latest fixes on this - much appreciated. Does your fix to the routing algorithm deal with the issue identified by Michael Hauber relating to passengers destined for industries getting stuck?

My time is somewhat short this evening, too: am about to go away for a few days, during which time I shall not be able to access my development environment, but should be able to access the forums and possibly the Bridgewater-Brunel server to try to test the latest RC there.
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.

Michael Hauber

I'll test if the stuck passengers bug is fixed if someone supplies me with an executable.

jamespetts

Thank you for the offer - I am afraid that I am currently staying away from my development environment until next week, so shan't be able to upload an executable for a little while, however.

This should be tested one way or another before long, though.
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.

neroden

Quote from: jamespetts on May 29, 2013, 08:48:42 PM
Thank you for the latest fixes on this - much appreciated. Does your fix to the routing algorithm deal with the issue identified by Michael Hauber relating to passengers destined for industries getting stuck?
Not sure.  I did fix something related to that, though it was a different fix.