Fix incorrect delta reset in Native.Time.fpsWhen.

[jwmerrill]

Fixes issue mentioned in:

elm-lang@274cd7e#commitcomment-9425007

Also designed to avoid creating a new function on every frame.

[jvoigtlaender]

Yes, I'm all for this. :)

Though I think the !wasOn && isOn in there should be simplified, given that we know isOn to be true at that point.

[jvoigtlaender]

Yes, I'm all for this. :)

Though I think the !wasOn && isOn in there should be simplified, given that we know isOn to be true at that point.

[jwmerrill]

@jvoigtlaender do you have a good idea for how to test that this works correctly? I don't really understand how to test impure code in Elm yet.

[jwmerrill]

@jvoigtlaender do you have a good idea for how to test that this works correctly? I don't really understand how to test impure code in Elm yet.

[jvoigtlaender]

I don't think it would be doable as a unit test or so that could be included for running in the test builts.

I'd write a small interactive test and check it live.

The thing to look out here is that an fpsWhen signal should really show 0 as delta in the update round where its isOn input signal switches from false to true.

[jvoigtlaender]

I don't think it would be doable as a unit test or so that could be included for running in the test builts.

I'd write a small interactive test and check it live.

The thing to look out here is that an fpsWhen signal should really show 0 as delta in the update round where its isOn input signal switches from false to true.

[jwmerrill]

I did verify that the following simple program worked incorrectly before, and works correctly after this commit:

import Time
import Signal
import Text
import Mouse

display n = Text.plainText (toString n)

input = Time.fpsWhen 60 Mouse.isDown

main = Signal.map display (Signal.foldp (+) 0 input)

[jwmerrill]

I did verify that the following simple program worked incorrectly before, and works correctly after this commit:

import Time
import Signal
import Text
import Mouse

display n = Text.plainText (toString n)

input = Time.fpsWhen 60 Mouse.isDown

main = Signal.map display (Signal.foldp (+) 0 input)

[jvoigtlaender]

What about my additional https://github.com/elm-lang/core/pull/76#issuecomment-70839236 that the var wasOn = true; line would better be var wasOn = isOn.value;?

[jvoigtlaender]

What about my additional https://github.com/elm-lang/core/pull/76#issuecomment-70839236 that the var wasOn = true; line would better be var wasOn = isOn.value;?

[jwmerrill]

What about my additional #76 (comment) that the var wasOn = true; line would better be var wasOn = isOn.value;?

Sorry, I don't follow what you mean here. 61c8dbc was meant to address your comment that the conditional could be simplified.

isOn is currently a boolean, so it doesn't have a .value property.

[jwmerrill]

What about my additional #76 (comment) that the var wasOn = true; line would better be var wasOn = isOn.value;?

Sorry, I don't follow what you mean here. 61c8dbc was meant to address your comment that the conditional could be simplified.

isOn is currently a boolean, so it doesn't have a .value property.

[jvoigtlaender]

This is about a comment (https://github.com/elm-lang/core/pull/76#issuecomment-70839236) I made to Evan in my original PR, not the other comment about simplifying the conditional I made on your PR and which you have adressed. Anyway:

There are two isOn in that function. The one that is current in the line where wasOn = true; happens is not a boolean. It is a signal of booleans. And I think wasOn should initially be set to the initial value of that signal instead of to the constant true.

[jvoigtlaender]

This is about a comment (https://github.com/elm-lang/core/pull/76#issuecomment-70839236) I made to Evan in my original PR, not the other comment about simplifying the conditional I made on your PR and which you have adressed. Anyway:

There are two isOn in that function. The one that is current in the line where wasOn = true; happens is not a boolean. It is a signal of booleans. And I think wasOn should initially be set to the initial value of that signal instead of to the constant true.

[evancz]

To put it another way, maybe the initial value of wasOn should be false because it has never been on before? Or maybe it should depend on whether things are on or off at the beginning as @jvoigtlaender is suggesting?

[evancz]

To put it another way, maybe the initial value of wasOn should be false because it has never been on before? Or maybe it should depend on whether things are on or off at the beginning as @jvoigtlaender is suggesting?

[jwmerrill]

I understand now. Added 297b548 implementing @jvoigtlaender's suggestion.

[jwmerrill]

I understand now. Added 297b548 implementing @jvoigtlaender's suggestion.

[jwmerrill]

the following simple program

BTW, I get a thrill every time I look at code this short that manages to put something on the screen that depends on dynamic mouse input. So much nicer than wrangling 3 languages and event listeners to do the same.

[jwmerrill]

the following simple program

BTW, I get a thrill every time I look at code this short that manages to put something on the screen that depends on dynamic mouse input. So much nicer than wrangling 3 languages and event listeners to do the same.

[jvoigtlaender]

@evancz, I think it shouldn't be false initially if isOn is true initially. The reason being that when the overall return signal of fpsWhen gets intialized, it will call startStopTimer with the initial value of isOn. If that one is true but wasOn was initially set to false, then startStopTimer will set up a first timeout, to occur after msPerFrame, but using notifyTrue, thus forcing the first delta to be reported as 0 (instead of the proper amount of msPerFrame milliseconds which will have passed by then).

(That would even happen then with just fps instead of fpsWhen.)

[jvoigtlaender]

@evancz, I think it shouldn't be false initially if isOn is true initially. The reason being that when the overall return signal of fpsWhen gets intialized, it will call startStopTimer with the initial value of isOn. If that one is true but wasOn was initially set to false, then startStopTimer will set up a first timeout, to occur after msPerFrame, but using notifyTrue, thus forcing the first delta to be reported as 0 (instead of the proper amount of msPerFrame milliseconds which will have passed by then).

(That would even happen then with just fps instead of fpsWhen.)

[jvoigtlaender]

@jwmerrill, Thanks for catching what I had done there in my original PR!

I learned to be more careful about refactoring JavaScript.

[jvoigtlaender]

@jwmerrill, Thanks for catching what I had done there in my original PR!

I learned to be more careful about refactoring JavaScript.

[jwmerrill]

I have been bitten by exactly this problem so many times...

[jwmerrill]

I have been bitten by exactly this problem so many times...

[evancz]

Alright, it looks good to me, thank you so much!

[evancz]

Alright, it looks good to me, thank you so much!