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

Update performance helptext for buildings producing more wares #5861

Merged
merged 32 commits into from Aug 3, 2023

Conversation

gpalino
Copy link
Contributor

@gpalino gpalino commented Apr 20, 2023

Continuation of #5778
My goal is now to add/update help texts, so that it is clear how fast are wares produced under two common conditions:

  • economy needs all wares produced by building
  • economy need only one ware produced by building

Second case is usual for tool buildings and unevently generated wares, like barbarian inns, crystal mine or honey bread bakery.

Suggestion from tothxa for full cycle:
"If all wares are needed by the economy and it is fully supplied, then this building can produce on average one in , then one in , … and finally one <ware[n]> in <time[n]>, for a total of <total_time>."
I'm not sure how you calculate with the sleep time between cycles, but in this case it should probably not be spread over the wares, but included directly after ware[n], like "then it sleeps for , for a total…"

Suggestion from hessenfarmer:
"When this building is fully supplied, production of <ware_a> takes <time_1> and <ware_b> takes <time_b>. If all wares are needed this results in a total time of <time_tot> for <wares_a> and <wares_b>."

My suggestion:
"When this building is fully supplied and all wares are needed by the economy, production of <ware_a> takes <time_a> and <ware_b> takes <time_b> on average. If only one ware is needed by the economy, production of <ware_a> takes <time_a1> and <ware_b> takes <time_b1> on average."

I don't think specifying total time or sleep times is helpful, the time of ware production when economy needs only one ware can't be calculated precisely from full cycle parameters.

Example for atlantean crystal mine:
Total time = 387.4 sec
Basic production time for 1 granite = 15+3.6 = 18.6 sec
Basic production time for 1 quartz = 10+3.6 = 13.6 sec
Basic production time for 1 diamond = 25+3.6 = 28.6 sec
All sleep times = 38.4+30+40 = 108.4 sec
Full cycle produces 9 granite, 4 quartz and 2 diamonds

When e.g. only diamonds are needed:
2 diamonds take 57.2 sec
sleep time is just 40 sec
but also one granite is produced, which delays diamonds production by another 18.6 sec
So if we would like to reflect everything what is happening, it would lead to complex help text.

When I am considering what I expect from performance help text, I can imagine these 3 cases:

  • getting comparison of the production speeds between buildings in production chain, e.g.:
    If I have 2 iron mines, how many smelting works should I build to have full speed of iron production?
    If I have 5 lumberjack’s houses, how many forester’s houses should I build to produce logs at full speed?
  • compare speed between tribes:
    Can atlanteans build one ship faster than barbarians?
    Have all 5 barracks the same speed of new soldier production?
  • compare speed between upgraded and non-upgraded buildings:
    I have 3 miners - how can I produce coal faster, with 3 coal mines or 1 deeper coal mine?

For all these cases time of production of one unit is important, but full cycle time or sleep times are not important.

Give me your opinion and I'll continue with another buildings.

For Crystal Mine, Blacksmithy and Small Armor Smithy.
@hessenfarmer
Copy link
Contributor

I added some wording improvements:

  • if possible use a more specific description then the word ware (i.e. all wares are of the same type e.g. tool)
  • always use a number before a ware

But I am happy with the calculations scheme. So from my Side please go on. @tothxa @frankystone

@tothxa
Copy link
Member

tothxa commented Apr 21, 2023

But I am happy with the calculations scheme. So from my Side please go on. @tothxa @frankystone

I'm OK with it too.

gpalino and others added 5 commits April 23, 2023 00:21
Co-authored-by: hessenfarmer <43039887+hessenfarmer@users.noreply.github.com>
Co-authored-by: hessenfarmer <43039887+hessenfarmer@users.noreply.github.com>
@gpalino
Copy link
Contributor Author

gpalino commented May 1, 2023

Some of buildings have few wares with the same timing, example atlantean weaving mill. Which performance help text do you prefer?

When this building is fully supplied and all kinds of clothing are needed by the economy, production of one spidercloth takes %1$s, one tabard takes %2$s and one golden tabard takes %3$s on average. If only one kind of clothing is needed by the economy, production of one spidercloth takes %4$s, one tabard takes %5$s and one golden tabard takes %6$s on average.
(where %1 == %2 == %3, and %4 == %5 == %6)

When this building is fully supplied and all kinds of clothing are needed by the economy, production of one of each clothing takes %1$s on average. If only one kind of clothing is needed by the economy, its production takes %2$s on average.

First is more flexible, when timing will be changed in future. Second is shorter.

@hessenfarmer
Copy link
Contributor

Some of buildings have few wares with the same timing, example atlantean weaving mill. Which performance help text do you prefer?

When this building is fully supplied and all kinds of clothing are needed by the economy, production of one spidercloth takes %1$s, one tabard takes %2$s and one golden tabard takes %3$s on average. If only one kind of clothing is needed by the economy, production of one spidercloth takes %4$s, one tabard takes %5$s and one golden tabard takes %6$s on average. (where %1 == %2 == %3, and %4 == %5 == %6)

When this building is fully supplied and all kinds of clothing are needed by the economy, production of one of each clothing takes %1$s on average. If only one kind of clothing is needed by the economy, its production takes %2$s on average.

First is more flexible, when timing will be changed in future. Second is shorter.

I'd prefer the second one taking the risk of need to change if timings change. But it is yor decision.

@gpalino
Copy link
Contributor Author

gpalino commented May 2, 2023

I've noticed strange conditions in empire building Marble Mine. There are two actions mine_granite and mine_marble, both having the same return condition:
"return=skipped unless economy needs marble or economy needs granite"

If I understand it well, granite and marble are mined in the same ratio, regardless of economic needs. The original commit which brings these conditions is quite old - 4d20e28.

If we want to keep existing logic, I suggest simplifying script to be similar to Deep Marble Mine, to prevent confusion.
If we want to have economic preference (e.g. more marble mined when only marble is needed by economy), we can update two conditions to

"return=skipped unless economy needs granite"
"return=skipped unless economy needs marble"

(similar to e.g. atlantean Crystal Mine)

What do you think?

@gpalino gpalino marked this pull request as ready for review May 10, 2023 05:47
@hessenfarmer
Copy link
Contributor

I've noticed strange conditions in empire building Marble Mine. There are two actions mine_granite and mine_marble, both having the same return condition: "return=skipped unless economy needs marble or economy needs granite"

If I understand it well, granite and marble are mined in the same ratio, regardless of economic needs. The original commit which brings these conditions is quite old - 4d20e28.

If we want to keep existing logic, I suggest simplifying script to be similar to Deep Marble Mine, to prevent confusion. If we want to have economic preference (e.g. more marble mined when only marble is needed by economy), we can update two conditions to

"return=skipped unless economy needs granite"
"return=skipped unless economy needs marble"

(similar to e.g. atlantean Crystal Mine)

What do you think?

I think you discovered a bug that was maybe introduced by me some time ago (copy and paste most probably)
so please change the condition i9n a way that the ware that is really needed is preferred (i.e. for granite only depend on economy need of granite and for marble only depoend on marble neededness).
furthermore could you please change the descname of the granite program in the empire deep mine to reflect the mining for granite.
Thanks for spotting this and for the changes you made.

@tothxa
Copy link
Member

tothxa commented May 13, 2023

please change the condition in a way that the ware that is really needed is preferred (i.e. for granite only depend on economy need of granite and for marble only depoend on marble neededness).

That would make the small mine better for marble production than the deep mine, because it would then give it 3 to 1, while the deep mine always gives them 3 to 2. Also, the current program only allows the efficient production of marble, if the mine always has 2 rations and 2 mugs of wine. So this would be a major change for the Imperial economy, making them stronger, if I'm not mistaken.

The Atlantean crystal mine has a very different logic with different yields of the precious stones.

@gpalino
Copy link
Contributor Author

gpalino commented May 13, 2023

please change the condition in a way that the ware that is really needed is preferred (i.e. for granite only depend on economy need of granite and for marble only depoend on marble neededness).

That would make the small mine better for marble production than the deep mine, because it would then give it 3 to 1, while the deep mine always gives them 3 to 2. Also, the current program only allows the efficient production of marble, if the mine always has 2 rations and 2 mugs of wine. So this would be a major change for the Imperial economy, making them stronger, if I'm not mistaken.

The Atlantean crystal mine has a very different logic with different yields of the precious stones.

Yes, hessenfarmer's suggestion will improve Imperial economy. For me it seems logical if both marble mines work by the same principle - either both having fix ratio of mined marble & granite (like before), or both preferring economic needs (like atlantean mine). The second option improves economy.

@hessenfarmer
Copy link
Contributor

well, I agree that just adopting the if condition would improve empire a lot. How about the following suggestion:
making the programs depend on their proper ecomomy need and reduce the output of the main ware by one (resulting in 2 marble 1 granite or vice versa)
reasons:

  • the necessity of having always 2 rations and wine to gain proper marble production is not documented anywhere and can only be known by looking in the data files. This is something that I would not expect from a "normal" player. We teach the players that they can control production with the economy settings which is currently not the case for this building.
  • the marblemine delivers currently 4 wares per ration which is more then every other basic mine, so I believe reducing this would be fair in exchange for the better control

But perhaps this should be discussed a bit broader @widelands/developers any thoughts on this?

@tothxa
Copy link
Member

tothxa commented May 14, 2023

How about the following suggestion: making the programs depend on their proper ecomomy need and reduce the output of the main ware by one (resulting in 2 marble 1 granite or vice versa) reasons:

the necessity of having always 2 rations and wine to gain proper marble production is not documented anywhere and can only be known by looking in the data files. This is something that I would not expect from a "normal" player. We teach the players that they can control production with the economy settings which is currently not the case for this building.

To compare (assuming marble is always needed):

  • slow supply, granite is needed: marble is same as before, -1 granite
  • slow supply, only marble is needed: +1 marble, -2 granite – no immediate loss, but in the long term may result in no more oversupply of granite: needs testing
  • good supply, granite is needed: -1 marble, -1 granite
  • good supply, only marble is needed: marble is same as before, -2 granite

So overall the improvement is minimised and limited to low food conditions, and it is offset by possible loss when granite is needed and the general reduction of granite.

the marblemine delivers currently 4 wares per ration which is more then every other basic mine, so I believe reducing this would be fair in exchange for the better control

That's a good point. I didn't check that.

gpalino and others added 2 commits May 21, 2023 07:40
Co-authored-by: hessenfarmer <43039887+hessenfarmer@users.noreply.github.com>
Copy link
Contributor

@hessenfarmer hessenfarmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now

@hessenfarmer
Copy link
Contributor

Many thanks for this large piece of work

Copy link
Contributor

@hessenfarmer hessenfarmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I just found there are 2 open issues with the code of the barbarians ax Workshop and the barbarians warmill, where my comments were not resolved. could you please update these aswell then it is good to go

data/tribes/initialization/amazons/units.lua Show resolved Hide resolved
data/tribes/initialization/amazons/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/atlanteans/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/atlanteans/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/frisians/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/atlanteans/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/barbarians/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/empire/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/empire/units.lua Show resolved Hide resolved
data/tribes/initialization/empire/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/frisians/units.lua Show resolved Hide resolved
data/tribes/initialization/frisians/units.lua Outdated Show resolved Hide resolved
data/tribes/initialization/frisians/units.lua Outdated Show resolved Hide resolved
@hessenfarmer
Copy link
Contributor

@gpalino I reviewed all suggestions of @tothxa If he has no objections on my corrections and comments. Could you please implement the agreed sentences to get this in finally.

Thanks a lot for your effort and patience

@hessenfarmer
Copy link
Contributor

@gpalino
I hope we (@tothxa and me) did not frustrate you too much to abstain from further commiting. It is definitly not because of your work, but it is due to none of us being a native speaker and our drive to et texts as perfect as possible to not being ambiguous in translation for example. And it is unavoidable that sometimes a sentence that should have been good for one reviewer might sound not good for another one as every judgement is subjective.

@gpalino
Copy link
Contributor Author

gpalino commented Jun 10, 2023

@gpalino I hope we (@tothxa and me) did not frustrate you too much to abstain from further commiting. It is definitly not because of your work, but it is due to none of us being a native speaker and our drive to et texts as perfect as possible to not being ambiguous in translation for example. And it is unavoidable that sometimes a sentence that should have been good for one reviewer might sound not good for another one as every judgement is subjective.

No problem with that, I just had lot of tasks in my personal life last weeks. I hope I'll be back next week or two.

@gpalino
Copy link
Contributor Author

gpalino commented Aug 1, 2023

Sorry for big delay last months, now all your reviews should be included to the code.

@hessenfarmer hessenfarmer enabled auto-merge (squash) August 3, 2023 06:08
@hessenfarmer hessenfarmer merged commit 46a7a63 into widelands:master Aug 3, 2023
33 checks passed
@Noordfrees Noordfrees added this to the v1.2 milestone Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants