Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3600 closed defect (fixed)

[PATCH] Confusion between time and speed computation

Reported by: fatherbushido Owned by: otero
Priority: Should Have Milestone: Alpha 20
Component: UI & Simulation Keywords: patch
Cc: otero_xd@… Patch:

Description

I noticed a confusion between time and speed in some xml files.

If time evolves of a rate t (decimal positive or negative) then speed must be multiplied by 1/(1+t) If speed evolves of a rate t (decimal positive or negative) then time must be multiplied by 1/(1+t)

It affects some hero aura xml files :

.../0ad/binaries/data/mods/public/simulation/templates/units/athen_hero_pericles.xml .../0ad/binaries/data/mods/public/simulation/templates/units/ptol_hero_cleopatra.xml .../0ad/binaries/data/mods/public/simulation/templates/units/athen_hero_themistocles.xml

For example :

      <Modifications>
        <Cost.BuildTime> <Multiply>0.8</Multiply> </Cost.BuildTime>
      </Modifications>
      <AuraName>Naval Architect Aura</AuraName>
      <AuraDescription>Ships are built +20% faster during his lifespan.</AuraDescription>

I think it's

 <Cost.BuildTime> <Multiply>0.8333</Multiply> </Cost.BuildTime>

or we can modify AuraDescription.

Attachments (9)

time_rate_fix_v1.patch (6.1 KB ) - added by otero 8 years ago.
mult_operations_v1.patch (71.2 KB ) - added by Palaxin 8 years ago.
mult_operations_simple.patch (723 bytes ) - added by Palaxin 8 years ago.
@fatherbushido: it could be as simple as that (you would need to do the 0.8333 stuff as otero did for speed/time calculations, but as I said, it's the "multiply" operation that bothers me most)
time_rate_fix_v2.patch (10.6 KB ) - added by Palaxin 8 years ago.
time_rate_fix_v3.patch (10.6 KB ) - added by Palaxin 8 years ago.
time_rate_fix_v4.patch (10.5 KB ) - added by Palaxin 8 years ago.
instead of matching the values to the descriptions, descriptions are matched to the values
time_rate_fix_v5.patch (9.7 KB ) - added by Palaxin 8 years ago.
ok didn't know that…
time_rate_fix_v6.patch (8.9 KB ) - added by Palaxin 8 years ago.
that nasty rotarymill -.-
time_rate_fix_v7.patch (9.0 KB ) - added by Palaxin 8 years ago.
so propose the following: clean numbers and easy descriptions

Download all attachments as: .zip

Change History (30)

comment:1 by Palaxin, 8 years ago

I'm glad you made this ticket. I was wondering about the same, but there are even more inconsistencies in the modification of unit stats so I thought there is some intention behind it. I'm actually a physicist so the right usage of values and units is very common and important to me.

As far as I have observed, the descriptions are wrong in the following cases where stats are multiplied:

  • Speed/time modifications: you are right, +20% speed implies a 0.8333 multiplier for the respective time, -20% speed implies a 1.25 multiplier. For further explanations see below.
  • Several techs increasing the same stat: increase is determined on base value, not on current value, e.g. two bonuses described as "+20%" mean you get 100%, 120%, 140% values (actually +20%, then +16.7%) instead of 100%, 120%, 144% (+20% increase for both). IMO this is not a design question but a question of correctness. "+XX% on base stat" and "+XX%" is a huge difference. Also the multiplication operation should be modified and not the descriptions because the descriptions would actually depend on the circumstances of the tech usage (if it's currently the only tech/aura affecting the value or not).

Some explanations regarding the maths (I'm no English native, so I hope these terms are still as accurate as intended): Speed in the actual physical sense is v = ds/dt (time derivative of distance). We can simplify this to v = (Delta s) / (Delta t) (time and distance differences for constant motion). A some more general "kind of speed" is frequency (or "rate"), f = 1/t. This implies t = 1/f. f is the number of actions which occur / can be done in a specific amount of time t. In fact, train "speed", attack "speed" etc. are fitting to this category ("how many units can be trained / how many arrows can be shot in a certain amount of time"). So if we change f (or these kinds of "speed") by +20% or -20%, respectively, we get for time t a -16.7% or +25% change. In detail:

  • fnew = (1+0.2)*fold = 1.2*fold
  • tnew = 1/fnew = 1/(1.2*fold) = 1/(1.2*(1/told)) = 0.8333*told
  • fnew = (1-0.2)*fold = 0.8*fold
  • tnew = 1/fnew = 1/(0.8*fold) = 1/(0.8*(1/told)) = 1.25*told

The general rules are:

  • f = 1/t
  • t = 1/f
  • fnew = (1+bonus)*fold = (100%+bonus%)/100%*fold
  • tnew = (1+bonus)*told = (100%+bonus%)/100%*told
  • fnew = 1/((1+bonus)*told) = 100%/((100%+bonus%)*told)
  • tnew = 1/((1+bonus)*fold) = 100%/((100%+bonus%)*fold)

Edit: I just tested that auras actually calculate their bonuses on current stats (not base stats), but technologies work as described above.

Last edited 8 years ago by Palaxin (previous) (diff)

comment:2 by elexis, 8 years ago

Keywords: simple added
Milestone: Alpha 20Backlog

Patches welcome.

comment:3 by otero, 8 years ago

Cc: otero_xd@… added
Keywords: review patch added
Owner: set to otero
Status: newassigned
Summary: Confusion between time and speed computation[PATCH] Confusion between time and speed computation

Fixed some XML tags values with the correct time rates based on the discussion above. The numbers were truncated to 4 decimal digits as it keeps a good enough precision.

by otero, 8 years ago

Attachment: time_rate_fix_v1.patch added

comment:4 by otero, 8 years ago

Milestone: BacklogAlpha 20

by Palaxin, 8 years ago

Attachment: mult_operations_v1.patch added

comment:5 by Palaxin, 8 years ago

My patch adds more operations for technologies:

  • The current "multiply" operation is now called "bmultiply". To not affect balance and gameplay I renamed the operator in all technology templates. The formula is: new value = current value + (bmultiply - 1) * base value
  • "multiply" works on the current value instead: new value = current value * multiply
  • The speed operator "smultiply" simplifies the conversion to time values: new time value = current time value / smultiply

The question now is where to use which operator. For several technologies affecting the same value "multiply" yields slightly higher values than "bmultiply" since bonuses are included vs. excluded in the calculation. As I stated above I strongly prefer "multiply" instead of "bmultiply" because the %bonus of "bmultiply" actually depends on the bonuses the value already has and is not constant (leading to wrong descriptions). Funny fact: auras already use "multiply" and not "bmultiply" so the current system even is not consistent. I would still like to implement the same operators for auras (currently they only allow "add" and "multiply" so we would need "replace", "bmultiply" and "smultiply" as well). The implementation is more complex though and I currently don't know how to do it.

Last edited 8 years ago by Palaxin (previous) (diff)

comment:6 by fatherbushido, 8 years ago

Otero : nice job, you choose the hard way, the easy way was to change description. Palaxin : i think it's not a good thing to introduce those operations (kiss principle).

in reply to:  6 comment:7 by Palaxin, 8 years ago

Replying to fatherbushido:

Otero : nice job, you choose the hard way, the easy way was to change description. Palaxin : i think it's not a good thing to introduce those operations (kiss principle).

Hm sorry, but I do not understand. I think its simpler to write <smultiply>1.2</smultiply> than <multiply>0.8333</multiply> (as you said the "hard way"). And if we don't use "bmultiply" any more (as I propose) we only have (the new) "multiply" and "smultiply" in a special case. But actually that is not my main concern. I could live with one multiply operation if it was changed (like it works for auras for example). Read below to see the problem of the current "multiply" ("bmultiply" in my patch).

Regarding description changes: it seems I have to give an example to illustrate that currently %-change depend on the circumstances and that you can't make the descriptions correct in all cases. Let's assume we have a soldier with 10.0 hack damage.

  • Scenario A: We haven't built a blacksmith but a fortress and researched "Will to fight". +25% damage means our soldier now has 10.0 + 10.0*0.25 = 12.5 hack damage. The description is correct in this context. Now we build our blacksmith and research "Side Arms". The description says "+20% melee infantry attack". But in the current implementation we would get 12.5 + 10.0*0.2 = 14.5 hack damage so the increase is 2.0/12.5 = 0.16 = 16% and NOT 20%.
  • Scenario B: Now we do it the other way round. "Side Arms" yields 10.0 + 10.0*0.2 = 12.0 hack damage, and this time the description is correctly saying +20%. Adding "Will to fight" yields 12.0 + 10.0*0.25 = 14.5 hack damage, but is lying saying "+25%" because in fact we have 2.5/12.0 = 20.833% increase.

The problem is that you can't always make the description correct, you can only fit it to a special scenario. It is solved when you always multiply the current value and not the base value. 10.0 + 10.0*0.25 = 12.5 (25% increase). 12.5 + 12.5*0.2 = 15.0 (20% increase). This yields the same % values as the other way round. 10.0 + 10.0*0.2 = 12.0 (20% increase). 12.0 + 12.0*0.25 = 15.0 (25% increase). I hope this example is helpful.

Last edited 8 years ago by Palaxin (previous) (diff)

by Palaxin, 8 years ago

@fatherbushido: it could be as simple as that (you would need to do the 0.8333 stuff as otero did for speed/time calculations, but as I said, it's the "multiply" operation that bothers me most)

comment:8 by mimo, 8 years ago

I think the original reason to only apply the multiply on the base value is that you can have different techs acting on the same value with multiply and add. And thus, modifying the base value is a simple way to be sure you are independent of the order on which techs are applied.

in reply to:  8 comment:9 by Palaxin, 8 years ago

Replying to mimo:

I think the original reason to only apply the multiply on the base value is that you can have different techs acting on the same value with multiply and add. And thus, modifying the base value is a simple way to be sure you are independent of the order on which techs are applied.

Now the problem is that the descriptions depend on the order of the techs. But there is a solution. Since armor actually scales exponentially, damage(armor) = damage_initial * 0.9 armor, "add" acts like "multiply" for it. Adding 1 armor is the same as -10% damage or +11.111% effective HP. IMO using "add" only for armor and "multiply" (on current value) for all other cases would be the cleanest solution. Dependencies on the order of the techs AND on the descriptions wouldn't exist at all then. The problem with the current system is: "add" is stronger for units with lower stats and "multiply" is stronger for units without bonuses. This has two big disadvantages:

  • balancing is more difficult because different units get a different buff by the same tech
  • for the "add" operator, if a unit's stats are changed you have to change the tech as well

As I mentioned above, auras actually do not calculate on the base value, so additionally consistency is broken right now.

Last edited 8 years ago by Palaxin (previous) (diff)

by Palaxin, 8 years ago

Attachment: time_rate_fix_v2.patch added

comment:10 by Palaxin, 8 years ago

Some improvements of otero's patch, which includes a misconception:

  • You need the 0.8333-like correction only when the description talks about rate/speed and the value you are changing is a time value (or the other way round). rate-rate and time-time combinations are working fine.
  • Projectile accuracy/precision and the Attack/Ranged/Spread value behave similar to speed/rate and time values. A higher precision means a smaller spread. I therefore adjusted the bolt shooter tech.

Another thing that I'm concerned about:

  • There is a misleading value called Heal/Rate (as in the two temple techs). The description says "Healers +25% healing rate." and the Heal/Rate is multiplied by 0.8 as if it was something like healing repeat time. The value and the description are fitting in this case, but it is no rate when you have to reduce the value for faster healing.
Last edited 8 years ago by Palaxin (previous) (diff)

comment:11 by Palaxin, 8 years ago

Fixed the "no newline at end of file" changes, they were not intended.

Version 3, edited 8 years ago by Palaxin (previous) (next) (diff)

by Palaxin, 8 years ago

Attachment: time_rate_fix_v3.patch added

comment:12 by fatherbushido, 8 years ago

Palaxin, i agree you. I really think it's just a matter of description.

We just need

<AuraDescription>-20 % Attack Repeat Time of Egyptian units in her vision range.</AuraDescription>

instead of

 <AuraDescription>Egyptian units fight 20% faster in her vision range.</AuraDescription>

and we don't bother with useless computations.

For the other problem (percentage of percentage) or basis point.

We can think too that if the bank says "i will give you 10% each year", we can assume the two options (simple interest rate or compound interest rate). We can use percentage point (pp).

	"description": "Often, an infantryman would carry a secondary weapon in addition to his primary weapon of choice, to be drawn when the primary weapon has failed or been broken.",
	"tooltip": "+20 % melee infantry attack.",

becomes something like that (i think you can find a more accurate statement) :

	"tooltip": "+20 pp melee infantry attack previous % bonus.",

Let's wait otero.

Last edited 8 years ago by fatherbushido (previous) (diff)

by Palaxin, 8 years ago

Attachment: time_rate_fix_v4.patch added

instead of matching the values to the descriptions, descriptions are matched to the values

comment:13 by fatherbushido, 8 years ago

Take care ResourceGatherer.Rates.food..grain is correct.

comment:14 by Palaxin, 8 years ago

@fatherbushido sorry, I may have misunderstood you. I'm actually fine with changing the descriptions as you proposed, though I think "-20% attack repeat time for Egyptian units in her vision range." and "-25% spread for bolt shooters." may sound a bit clumsy for some players. But in any case it's better than having false descriptions.

Regarding the other point: I think I would rename "+20 % melee infantry attack." into "+20% melee infantry base attack." This is correct and still not too clumsy. In League of Legends they call it "total attack", "base attack" and "bonus attack" where base and bonus attack add up to the total attack. I think this is simple and yet precise.

As I mentioned in comment 9 there still would be problems with the current "add" and "multiply" system for techs. But actually I think this would need to be discussed in another ticket or in the forum. Sorry if I distracted too much from the actual purpose of this ticket.

by Palaxin, 8 years ago

Attachment: time_rate_fix_v5.patch added

ok didn't know that...

by Palaxin, 8 years ago

Attachment: time_rate_fix_v6.patch added

that nasty rotarymill -.-

comment:15 by fatherbushido, 8 years ago

"+20% melee infantry base attack." sounds really good.

comment:16 by elexis, 8 years ago

"-20 % Attack Repeat Time" might be more accurate mathmatically, but it is less clear to the user than "units fight 20% faster". Same goes for "-25% spread for bolt shooters" in comparison to "Bolt shooter accuracy increased 25%". The quality of the tooltips should not be degraded for this little deviation. So if we can't live with this issue, the numbers in the template should likely be changed.

by Palaxin, 8 years ago

Attachment: time_rate_fix_v7.patch added

so propose the following: clean numbers and easy descriptions

comment:17 by otero, 8 years ago

I've been reading all the comments and the patches that Palaxin has been adding, and I think he has a really good point. One of the things that I was thinking when I was making this patch was if there was a better way to make this in a more automated way, so if in the future a little change is necessary, it should be changed only in a single section instead of having a "big" change as in this patch. But at the same time I am big fan of the KISS philosophy, the simpler you can make it the easier is to understand it in the future.

In my opinion right now it should be taken the simpler patch, which is a combination of the patch I uploaded plus the patches uploaded by Palaxin where the description and mistakes I didn't notice or added are fixed, but we shouldn't discard completely the first idea Palaxin suggested where the javascript code was changed.

Now I would like to make some changes to his suggestion but I think that should be added in the future in another ticket with a rethink of the system.

For example a way I was thinking is instead of having a single tag where the final value goes, we could have a tag <Bonus>+20%</Bonus> or <Bonus>0.2</Bonus> (The second ption is easier to parse) where we can put the expected increase or decrease of the base value maybe even have something like

<Compound Base>

<Bonus>0.2</Bonus>

</Compound Base>

<Compound Current>

<Bonus>0.2</Bonus>

</Compound Current>

Where the first would imply an increase in the the original base value always, while the second would mean an increase in the current value of the unit.

And then change the base value inside javascript. I don't know if this solution is optimal but could be an starter point.

P.S. Sorry for answering late, I've been a little bussy this week and couldn't connect.

comment:18 by Palaxin, 8 years ago

Hmm this sounds interesting. I think the problem with this solution is that you add two additional lines for each value that has to be changed. If I understand right, the following pieces should do the same (otero's system top, my system below):

<XYZ>

<Compound Base>

<Bonus>0.2</Bonus

</Compound Base>

</XYZ>

<XYZ>

<bmultiply>1.2</bmultiply>

</XYZ>

Sadly, I do not have time for at least a month now and can't work on this stuff. Go ahead with your ideas, if you want :)

comment:19 by Itms, 8 years ago

Resolution: fixed
Status: assignedclosed

In 17865:

Improve some aura and technology descriptions where there was a confusion between speed and time.

Patch by Palaxin, fixes #3600

comment:20 by Itms, 8 years ago

Keywords: simple review removed

I committed Palaxin's fix above, which seems to deal correctly with the problem described above.

Please open a new ticket if you really want to implement this other idea, but to me it sounds a bit complicated. It might be useful but it's still possible to fix the tooltips like you did here.

Thanks for your work!

comment:21 by Palaxin, 8 years ago

Thanks for committing :)

I think I will discuss and refine the other idea in the forum first as it seems there are several different opinions about it... However, I don't have much time currently

Note: See TracTickets for help on using tickets.