News:

Simutrans Sites
Know our official sites. Find tools and resources for Simutrans.

[BUG] Cost estimates are wrong for cut-and-cover tunnels

Started by Matthew, June 04, 2022, 05:48:52 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Matthew

Steps to reproduce

Build a cut-and-cover tunnel using the new tunnel construction system.

Expected results

The construction cost is correctly estimated before construction.

Actual results

Only a tiny fraction of the cost is displayed:

(Signature being tested) If you enjoy playing Simutrans, then you might also enjoy watching Japan Railway Journal
Available in English and simplified Chinese
如果您喜欢玩Simutrans的话,那么说不定就想看《日本铁路之旅》(英语也有简体中文字幕)。

jamespetts

It looks as though the cost estimates may not be taking into account the add-on costs for depth, being beneath buildings or similar.
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.

PJMack

Unfortunately that is not the only problem.  I checked with ordinary bored tunnels and am still getting underestimates.  I also checked an older version (from January) and am also getting drastic underestimates.  Furthermore the problem is inconsistent in that the first tunnel built in underground mode has a cost estimate of about 50-80% of the actual cost whereas the second tunnel of the same type has a cost estimate of less than 10% of the actual cost.

I am still trying to sort out what exactly is going on but looking at the code, the tooltip is set in both the mark_tile and check_pos functions of the tool, each using a different formula.  In order to total the actually costs, the full route would be needed, which is not available in the check_pos function, but is in the mark_tile function.

The easiest "solution" would be to remove the estimate tooltip entirely as in reality tunneling costs are somewhat unpredictable, however I think players would like at least some estimate.  One approach would be to give an estimate of the minimum costs (made clear to be a minimum), however I have not yet given up on using the actual costs.

PJMack

I managed to find the problem.  For tunnel tiles with no ground, it was only using the way costs.  I adjusted it to use the the same tunnel building cost function as is used during construction and pushed a draft PR.

The reason the PR is in draft mode is that the tooltip now overestimates the costs slightly, which I believe may have to do with way and/or forge costs as that is the only other costs in the calculations. 

PJMack

The good news is I managed to get the estimate correct and the PR is ready.

The bad news is that it turns out that way cost is (and was prior to recent tunnel improvements) ignored while tunnel building.  I am not sure if this was by design or if I uncovered another bug.

jamespetts

Quote from: PJMack on June 04, 2022, 08:29:49 PMThe good news is I managed to get the estimate correct and the PR is ready.

The bad news is that it turns out that way cost is (and was prior to recent tunnel improvements) ignored while tunnel building.  I am not sure if this was by design or if I uncovered another bug.
Thank you for that. The way cost certainly should not be ignored while building tunnels. Do you mean that it is not shown in the preview, not deducted from the player's balance, or both?
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.

PJMack

When I added the sub-costs for tunnels, I simply replaced all calls of tunnel_desc_t->get_value() with calls to a get_total_cost(tunnel_desc_t,koord3d) and likewise with maintenance, as I did not want to refactor working code.  In trying to fix the aforementioned lack of way costs, I found that the existing code has more issues.  In addition to not deducting the tunnel way cost from the players ballence, when the control key is pressed while tunnel building or upgrading a tunnel in underground mode, the tunnel cost is applied twice and maintenance is not adjusted at all (a reload would fix the maintenance though).  Furthermore, there are two tunnel construction functions, one for single click and one for two click underground mode, both with sections of the same code.  In the case of the two click function, that same code appears in two places and in some cases will execute twice (including the build cost). 

I believe that a refactor may indeed be necessary at this point.

jamespetts

Quote from: PJMack on June 05, 2022, 11:04:18 PMWhen I added the sub-costs for tunnels, I simply replaced all calls of tunnel_desc_t->get_value() with calls to a get_total_cost(tunnel_desc_t,koord3d) and likewise with maintenance, as I did not want to refactor working code.  In trying to fix the aforementioned lack of way costs, I found that the existing code has more issues.  In addition to not deducting the tunnel way cost from the players ballence, when the control key is pressed while tunnel building or upgrading a tunnel in underground mode, the tunnel cost is applied twice and maintenance is not adjusted at all (a reload would fix the maintenance though).  Furthermore, there are two tunnel construction functions, one for single click and one for two click underground mode, both with sections of the same code.  In the case of the two click function, that same code appears in two places and in some cases will execute twice (including the build cost). 

I believe that a refactor may indeed be necessary at this point.
Thank you for identifying these issues - that is helpful. I agree that some reworking of this code may be desirable.  Did you want to look at doing this? I am currently staying away from home, and will not be able to look at coding (as opposed to pakset) work for about a week.
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.

PJMack

I was planning on giving it a go. 

I did find the source of one of the bugs.  The double cost is a result of a merge conflict from 2013-12-31 where in one branch the code as inside the if statement whereas the other the code was outside of it.  The code ended up in both places after the merge, and each instance was further edited, sometimes not identically, in the past 8½ years.

jamespetts

Quote from: PJMack on June 06, 2022, 07:21:48 PMI was planning on giving it a go. 

I did find the source of one of the bugs.  The double cost is a result of a merge conflict from 2013-12-31 where in one branch the code as inside the if statement whereas the other the code was outside of it.  The code ended up in both places after the merge, and each instance was further edited, sometimes not identically, in the past 8½ years.
Excellent, thank you for this: it is much appreciated.

I think that 2013 may have been very shortly before routine merging from Standard came to an end because of how difficult that it had become as a result of code divergences. This perhaps demonstrates why this was wise.

Thank you for looking into this.
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.

PJMack

I have pushed a refactor which should fix all the aforementioned issues and hopefully not break anything else.  I would like to continue testing more before declaring it ready, and have left the PR in non-draft mode to make it easier for others to see and test as well.

jamespetts

Quote from: PJMack on June 08, 2022, 10:31:29 PMI have pushed a refactor which should fix all the aforementioned issues and hopefully not break anything else.  I would like to continue testing more before declaring it ready, and have left the PR in non-draft mode to make it easier for others to see and test as well.
Excellent, thank you. Do let me know when you think that this is ready.
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.


jamespetts

Quote from: PJMack on June 12, 2022, 09:09:00 PMI am through with my testing.
Thank you for your work on this, and apologies for the delay in looking into this. When I look at this on Github, however, I see that there appears to be a merge conflict in tunnelbauer.cc. I should be grateful if you could look into that; thank you.
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.

PJMack

I did a manual merge.  It should work now.

jamespetts

Quote from: PJMack on June 19, 2022, 11:22:54 PMI did a manual merge.  It should work now.
Thank you very much - now incorporated.
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.