*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
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
tags Milestone-0.9 Status-Accepted
owner MattJ
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
MattJ
on
Wouldn't just:
- if balance > self.max then
+ if floor(balance) > self.max then
suffice for that?
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)
pierre.gotab
on
*growth < 1 sorry
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
*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
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.
ChangesMaybe 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
Wouldn't just: - if balance > self.max then + if floor(balance) > self.max then suffice for that?
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)
*growth < 1 sorry
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
ChangesAnd indeed, this fix uncovered the likely reason for that floor() commit: https://hg.prosody.im/0.10/rev/25237002aba4