#495 throttle.lua floor problem

Reporter pierre.gotab
Owner MattJ
Created
Updated
Stars ★ (1)  
Tags
  • Milestone-0.9
  • Status-Fixed
  • Type-Defect
  • Priority-Medium
  1. pierre.gotab on

    *What steps will reproduce the problem?* 1. th = throttle.create(1, 1) 2. call th:poll(1) more than once per second 3. these poll call will *never* return true *What is the expected output? What do you see instead?* It is expected that poll will finish to return true, whatever the call frequency of :update() *What version of the product are you using? On what operating system?* Any versions on any OSes :) *Please provide any additional information below.* See https://github.com/maranda/metronome/issues/223 for a full description of the problem. And please be smarter than Marco Cirillo :D

  2. MattJ on

    Hi, thanks for the report! I see the problem. However I'm worried it's not so simple. The floor() was not originally there, I added it in this commit: https://hg.prosody.im/trunk/rev/0e9a5b63206a I am quite annoyed at myself for not adding an explanation in a comment or in the commit message, but I'm sure there was a reason I added it. We also don't have any tests for this module, so I'm disinclined to make the obvious change without 1) tests or 2) understanding why I added floor() in the first place. But I agree the current behaviour is certainly wrong. Hmm. I'm setting the milestone as 0.9 because this deserves to be fixed in the stable branch.

    Changes
    • owner MattJ
    • tags Milestone-0.9 Status-Accepted
  3. pierre.gotab on

    Maybe the floor is there to keep the balance an integer. What I propose to ensure throttle works as intended, and keep balance an integer is: local growth = self.rate * elapsed; if growth >= 1 then local int_growth = floor(growth); self.t = newt - (growth - int_growth) / self.rate; local balance = int_growth + self.balance; if balance > self.max then ... ... -- else dont do anything

  4. MattJ on

    Wouldn't just: - if balance > self.max then + if floor(balance) > self.max then suffice for that?

  5. pierre.gotab on

    No I don't think this condition should be changed. The problem is: b 0 1 |---------|--- t 1 2 ^ :update() call t=2.3 and balance=1.3 ; but keep balance floored so: ^ \_ back in time here t=2 and balance=1 This is the aim of newt - (growth - int_growth) / self.rate = 2.3 - (0.3)/1 = 2 It keeps internal self.t consistent with balance flooring. Like if update was called at t=2 (And obviously if growth < 0 nothing has to be done, just wait for a significant delta time)

  6. pierre.gotab on

    *growth < 1 sorry

  7. MattJ on

    This has (finally) been fixed in trunk, with tests. It has also been backported to 0.10, and if no issues show, we'll consider it for 0.9 as well. Another issue was also fixed, where the initial balance is not correctly calculated. https://hg.prosody.im/trunk/rev/9499db96c032

    Changes
    • tags Status-Fixed
  8. MattJ on

    And indeed, this fix uncovered the likely reason for that floor() commit: https://hg.prosody.im/0.10/rev/25237002aba4

New comment