Displayed speed (on item tooltip) is different from actual speed

ryzom-pipeline commented 7 years ago

Original report by Guillaume DUPUY (Bitbucket: [Guillaume DUPUY](https://bitbucket.org/Guillaume DUPUY), ).

This issue affects both winchgate server and a RC server : simply take a weapon (for example, a 60 hit per minutes dagger), hit a target for 10 minutes, and you'll find that you don't do 60 hit per minutes, but more 54/55. This is actually three differents problems / bugs :

Float and double mixing

In rares cases, the use of a float and a double in the same expression would result in an incorrect value due to precision error (when 10 / hitrate is an integer). This is fixed in 0f4c946, using a cast to uint16 and not a round() (to be consistant with how others stats are calculated in Ryzom)

State transition taking place only once per tick

All attacks after the first one (any sabrina phrase actually) two ticks longer (and there is a one tick delay in the first one), because state transition are made only once per tick. An usual attack goes like this (with a 60cpm / 10 ticks per attack dagger) :

t=0 ; action is created. State goes New -> Evaluated -> Validated -> ExecutionInProgress
t=1 ; launch() (where the damage is done and action is launched). State goes ExecutionInProgress -> SecondValidated -> Latent
t=2 to t=10 (latency/number of hits/tick), nothing. state is Latent
t=11 ; latency ticks has passed since launch(), we can go out of sleep and renew phrase (because it's cyclic). State goes Latent -> LatencyEnded -> Evaluated
t=12 ; Basically the same process as t=0 (except we start from Evaluated instead of New). State goes Evaluated -> Validated -> ExecutionInProgress
t=13, Damage is done, launch() is called. State goes ExecutionInProgress -> SecondValidated -> Latent
t=14 to t=22, nothing. state is Latent.
t=23 ; Same as t=11. State goes Latent -> LatencyEnded -> Evaluated
t=24 ; Same as t=12. State goes Evaluated -> Validated -> ExecutionInProgress
t=25 ; Damage is done, same as t=13. State goes ExecutionInProgress -> SecondValidated -> Latent
etc

I ommited the fact that the action actually happens 4 ticks after launch ; there is a slight delay so damage will match visual effect (ie you won't deal damage before seeing your attack animation). This has no impact on this however, so not mentionned.

So we can clearly see that the first action actually happens 1 tick after we started it (tick t=1), and that all actions are actually separated by 12 ticks and not 10 ticks, because we need one tick to mark the phrase as ended, one to mark it as needing execution, and one tick to actually execute it.

This can be easily seen by checking the state at each ticks (see attached tracking.patch for an easy example how to do that).

My fix is simple : instead of updating the current action only once per tick, simply update it as long as the state changes. see f0295d2 for an example implementation.

I tested on a ryzomcore server, with melee attacks, offensive spells, digging and crafting and everything works perfectly fine.

Displayed speed is an integer, actual speed for calculation is a float

Take for example a dagger ; to have a latency of 10 ticks, you'll need to have between [10;11[ (due to how cast are made).With 600 ticks per second, it means that the breakpoint is

600 / x = 11 <=> x = 54.5454(...)

So if we take a weapon slighty slower than this value and one slightly faster, they will display the same speed but have actually different speeds.

(For reference, 54.54 speed is 0.818 precraft speed, calculated like this

speed = speedMin + (speedMax - speedMin) * precraft <=> precraft = (54.54 - 30) / (60-30) = 0.818)

There is three ways to change that, with the third way beeing the only 100% fixes (the first two proposals will fix some of the problems but not all)

Use speed as a int for calculation (see commit 6221e35 for an example on how you can do that). It's still not a perfect solution because the displayed speed would still be wrong : for example, a 60 hit per minute dagger will be actually 60 hit per minute, but a 58 hit per minute dagger will also be 60 hit per minutes.
Display speed as a float, rounded to 2 or 3 decimals (and using the same rounded value for calculations). It makes little sense to have a value like 54.54 hit per minutes. It still doesn't solve the problem of the 60/58 hit per minutes dagger
use a float latency. This should solve the 60/58 hit per minute dagger, however it's a big modification who could break a lot of bug

ryzom-pipeline commented 7 years ago

Original comment by Cédric Ochs (Bitbucket: [Cédric OCHS](https://bitbucket.org/Cédric OCHS), ).

Nice catches and thanks for your fixes :p

ryzom-pipeline commented 7 years ago

Original comment by Guillaume DUPUY (Bitbucket: [Guillaume DUPUY](https://bitbucket.org/Guillaume DUPUY), ).

Actually using a float latency / latencyEndDate isn't that big of a change, see 5f3baed for an example implementation. Using this + speed as an int for calculation works perfectly fine, displayed speed is matching actual speed.

ryzom-pipeline commented 7 years ago

Original comment by Guillaume DUPUY (Bitbucket: [Guillaume DUPUY](https://bitbucket.org/Guillaume DUPUY), ).

Switched to a double instead of a float because with high values of GameCycle (like on Winchgate's live server), float precision would result in comparison error, thus making launch() beeing called inifinitively, resulting in EGS crashing. See 8d78674

ryzom-pipeline commented 7 years ago

Original comment by Cédric Ochs (Bitbucket: [Cédric OCHS](https://bitbucket.org/Cédric OCHS), ).

Merged in Glorf/ryzomcore/fix_speed (pull request #141)

Displayed speed is now actual speed, fix #295

ryzom-pipeline commented 7 years ago

Original comment by Cédric Ochs (Bitbucket: [Cédric OCHS](https://bitbucket.org/Cédric OCHS), ).

Merged in Glorf/ryzomcore/fix_speed (pull request #141)

Displayed speed is now actual speed, fix #295

ryzom / ryzomcore