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

mainmenu: Fix minetest.after timing #4003

Closed
wants to merge 1 commit into from

Conversation

SmallJoker
Copy link
Member

@SmallJoker SmallJoker commented Apr 17, 2016

local start_time = os.clock()

minetest.after(10, function()
    print("This function was called after ".. os.clock()-start_time .."s")
end)

Returns This function was called after 7.0620000000001s, This function was called after 0.45299999999997s or any other value.
This bug was caused by the time calculation of the time last, which is initialized to zero. The time new instead starts with any value.

Tested this pull and got This function was called after 10.469s (some loading delay included).

@SmallJoker SmallJoker force-pushed the fix_minetest_after branch 3 times, most recently from 7cbe327 to dc05e27 Compare April 17, 2016 12:22
@Ekdohibs Ekdohibs added Bug Issues that were confirmed to be a bug @ Server / Client / Env. labels Apr 17, 2016
@sfan5
Copy link
Member

sfan5 commented Apr 17, 2016

👍

@sofar
Copy link
Contributor

sofar commented Apr 17, 2016

os.clock is unreliable, you can't use it for testing the timer. use minetest.get_us_time() instead

@sofar
Copy link
Contributor

sofar commented Apr 17, 2016

I've spent a few nights to make sure that the current code was in order and tested with thousands of timers etc.. I'll review this since I don't want it to regress to the bad timer performance we had before ad884f2

@sofar
Copy link
Contributor

sofar commented Apr 17, 2016

NVM, I see what's going on here. Yes, this is entirely right, thanks for spotting this edge-case.

Please merge.

@sofar
Copy link
Contributor

sofar commented Apr 17, 2016

This seems like a better solution:

diff --git a/builtin/game/misc.lua b/builtin/game/misc.lua
index 4f58710..de41cfc 100644
--- a/builtin/game/misc.lua
+++ b/builtin/game/misc.lua
@@ -6,7 +6,7 @@

 local jobs = {}
 local time = 0.0
-local last = 0.0
+local last = core.get_us_time() / 1000000

 core.register_globalstep(function(dtime)
        local new = core.get_us_time() / 1000000

@SmallJoker
Copy link
Member Author

@sofar Depends what time matters. Either it's the time when Lua loads the scripts or after it loaded all and begins to call the global functions.
Both ways are acceptable IMO

@sofar
Copy link
Contributor

sofar commented Apr 18, 2016

I'd prefer the time initialization method as it removes an entire branch.

@SmallJoker SmallJoker force-pushed the fix_minetest_after branch 2 times, most recently from 88f213d to f8c2598 Compare April 19, 2016 12:17
@paramat paramat modified the milestone: 0.4.14 Apr 20, 2016
@sofar sofar mentioned this pull request Apr 20, 2016
@sofar
Copy link
Contributor

sofar commented Apr 20, 2016

My suggested fix (proper initialization) is in #4009

paramat pushed a commit that referenced this pull request Apr 21, 2016
This fixes the problem that the first timer tick is an
overrun and causes all timers to expire immediately.

replaces #4003
@paramat
Copy link
Contributor

paramat commented Apr 21, 2016

#4009 is merged.

@paramat paramat closed this Apr 21, 2016
@SmallJoker SmallJoker deleted the fix_minetest_after branch May 9, 2016 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants