Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rating bonus for default economy also #6525

Closed
DorpsGek opened this issue Dec 7, 2016 · 10 comments
Closed

Rating bonus for default economy also #6525

DorpsGek opened this issue Dec 7, 2016 · 10 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay stale Stale issues

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Dec 7, 2016

SirkoZ opened the ticket and wrote:

I suggest rating bonus for default economy also because I feel it needs it.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/6525
@DorpsGek
Copy link
Member Author

adf88 wrote:

"bonus for default economy also" - what do you mean by that? What is your patch supposed to do?


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14320

@DorpsGek
Copy link
Member Author

SirkoZ wrote:

Well same as with smooth economy - if over 80% is transported, industry is even more likely to increase production.
Adds to playability.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14324

@DorpsGek
Copy link
Member Author

Alberth wrote:

Not sure how much sense it makes to improve non-smooth economy, but I can see the point.
@adf88: Compare smooth case with non-smooth case (industry_cmd.cpp, lines 2536-ish versus 2573-ish).

I think the 'else' branch needs to be cleaned up though, an "if (only_decrease || ..)" and a "if (!only_decrease && ..)" inside doesn't make a lot of sense there. Your patch now adds a hack rather than a proper fix.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14440

@DorpsGek
Copy link
Member Author

SirkoZ wrote:

You have to admit, Alberth, that non-smooth portion is well-condensed and therefore might look as hacked together, however you have much inside:

Line 2572 - first if temperate Oil Wells (or other industry with only_decrease behaviour) or 1 out of 3 (as it was in TTD - I've gone through the Marcin's exe) then

again Line 2574 - if !only_decrease because you don't want to increase only_decrease industry (same as in TTD exe) and of course the
chance16 portion that I extended without adding all kinds of extra code - it should be concise and to the point, don't you agree?

Regards S/Z


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14441

@DorpsGek
Copy link
Member Author

Alberth wrote:

The entire code should be easily readable, which is not the same as concise as possible. I believe the current code is too dense, too many cases folded into one.

You have to think which cases exist, and how the execution path runs for each case. If you change a statement, you have to think very careful which cases are affected. It's too easy to mess up there.
By expanding the various cases clearly, it's trivial to understand what code belongs in what case, much less room for errors.

No doubt the compiler will smash it all back into something smaller and more efficient like what we have now, but I'd like that to be a compiler problem and not mine.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14444

@DorpsGek
Copy link
Member Author

SirkoZ wrote:

Again - no doubt it's condensed, however it's easy to comprehend.
There is no room for errors in the current code. Try expanding - oh there shall be errors. :-)
I did try and not that it can't be done, it's unnecessarily complicated in my opinion.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14446

@DorpsGek
Copy link
Member Author

SirkoZ wrote:

In the last sentence I should have said it becomes unnecessarily complicated.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14447

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 7, 2017

SirkoZ wrote:

Perhaps the simplest fix to date would be to replace this:
if (!only_decrease && (i->last_month_pct_transported[0] > PERCENT_TRANSPORTED_60) != Chance16(1, 3)) {

with this (calculate adjustment based on last_month_%_transported):
if (!only_decrease && Chance16((85 + ((i->last_month_pct_transported[0] * 2) / 3)), 256)) {

Achieves the purpose and simplifies the code. First you take 1/3 as with default line (for non-used industries) and
then add the remaining 2/3rds based on how much industry is actually used...


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14796

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 7, 2017

SirkoZ wrote:

LOL - lipstick on a pig - good one. :-)
Well I tried. =)


This comment was imported from FlySpray: https://bugs.openttd.org/task/6525#comment14797

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
@frosch123 frosch123 removed the Core label Apr 14, 2018
@andythenorth andythenorth added the stale Stale issues label Jan 5, 2019
@andythenorth
Copy link
Contributor

Thanks for this. There's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. I'm closing it as we try to keep the issue count low for OpenTTD, it helps us focus on things that are important and fun. Feel free to discuss in irc or request re-opening if you disagree. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay stale Stale issues
Projects
None yet
Development

No branches or pull requests

3 participants