emscripten-ports / SDL2

Other
166 stars 64 forks source link

SDL_AddTimer doesn't work properly #67

Open 9chu opened 5 years ago

9chu commented 5 years ago
SDL_TimerID
SDL_AddTimer(Uint32 interval, SDL_TimerCallback callback, void *param)
{
    return EM_ASM_INT({
        return Browser.safeSetTimeout(function() {
            Runtime.dynCall('iii', $1, [$0, $2]);
        }, $0);
    }, interval, callback, param);
}

SetTimeout only emit the callback once, it should be setInterval.

kripken commented 5 years ago

I think that's what the SDL API says, though: that it will be called once?

https://wiki.libsdl.org/SDL_AddTimer

Or maybe I'm misreading that... For reference emscripten's SDL1 support also just calls it once, but maybe that's wrong too...

9chu commented 5 years ago

I think that's what the SDL API says, though: that it will be called once?

https://wiki.libsdl.org/SDL_AddTimer

Or maybe I'm misreading that... For reference emscripten's SDL1 support also just calls it once, but maybe that's wrong too...

To be honest, setInterval is still not correct. Accroding to the SDL manual, the returned value of the timer's callback function indicates the next interval of the timer. If the returned value from the callback is 0, the timer is canceled.

So, I think the correct implementation may be:

// Cause I'm new to emscripten, I wrote it in pure js :)
mapping = {};
next_id = 0;

function SDL_AddTimer(interval, callback, param) {
    var id = next_id++;
    var internal_callback = function(interval) {
        var ret = callback(interval, param);
        if (ret == 0) {
            del mapping[id];
            return;
        }
        mapping[id] = setTimeout(function() { internal_callback(ret) }, ret); 
    };
    mapping[id] = setTimeout(function() { internal_callback(interval) }, interval);
    return id;
}

function SDL_RemoveTimer(id) {
    if (!(id in mapping))
        return false;

    clearTimeout(mapping[id]);
    del mapping[id];
    return true;
}
Daft-Freak commented 5 years ago

https://github.com/emscripten-ports/SDL2/commit/09b829ba77136cf16ba2ce8885f184b391a8a76e should be it.

Beuc commented 5 years ago

FYI 09b829b broke RenPyWeb. Somehow SDL_AddTimer returned 0 while it didn't use to, causing an application error.

In addition there's no SDL_SetError, so I got an unrelated error which was quite confusing.

Daft-Freak commented 5 years ago

Oops, looks like I need to init SDL_TimerData.nextID to 1...

Daft-Freak commented 5 years ago

Or just move two characters... a8a4fdee312efffb6bea85e0a5aa913e49bf0810

Beuc commented 5 years ago

This appears to work, thanks a lot!