Fix timestamp function and its interaction with every/fps/fpsWhen #76

jvoigtlaender · Jan 2, 2015

making sure that events occurring in the same update cycle all get the
same timestamp;

also, making sure that timestamp (every t) will not carry events with different pair components;

this fixes issue #75

I agree that it should be a global thing. Do you mind making that change?

Have you tested that this works in the cases you mentioned? I am messing with elm-reactor at the moment, and I am not certain that the timestamp argument is getting passed in properly in all cases.

I was playing around with initializing the signal graph separately from specifying it. So instead of calling map and actually creating a node at that moment, it is just some data to be dealt with later. This is more in the spirit of solving the recursive-values / looping problem described here, but the point is that in that world, having a "global" start time that gets threaded through the graph initialization makes total sense.

evancz · Jan 5, 2015

I agree that it should be a global thing. Do you mind making that change?

Have you tested that this works in the cases you mentioned? I am messing with elm-reactor at the moment, and I am not certain that the timestamp argument is getting passed in properly in all cases.

I was playing around with initializing the signal graph separately from specifying it. So instead of calling map and actually creating a node at that moment, it is just some data to be dealt with later. This is more in the spirit of solving the recursive-values / looping problem described here, but the point is that in that world, having a "global" start time that gets threaded through the graph initialization makes total sense.

Yes, I can introduce the global programStart. I'll add that change to this PR.

Yes, I've tested the PR. Specifically, I've tested that:

Signal.map2 (\(a,_) (b,_) -> (a==b)) (timestamp (Signal.map f s)) (timestamp (Signal.map g s)) only produces Trues
uncurry (==) <~ timestamp (every second) only produces Trues
(\(_,d1,d2) -> d1 == d2) <~ foldp (\(ns,d) (os,_,_) -> (ns,ns-os,d)) (0,0,0) (timestamp (fps 1)) only produces Trues (except in the first event, where this is simply not possible, because the first delta computed in the foldp will compare the current time against initial value 0 instead of against a reasonable previous time)

jvoigtlaender · Jan 5, 2015

Yes, I can introduce the global programStart. I'll add that change to this PR.

Yes, I've tested the PR. Specifically, I've tested that:

Signal.map2 (\(a,_) (b,_) -> (a==b)) (timestamp (Signal.map f s)) (timestamp (Signal.map g s)) only produces Trues
uncurry (==) <~ timestamp (every second) only produces Trues
(\(_,d1,d2) -> d1 == d2) <~ foldp (\(ns,d) (os,_,_) -> (ns,ns-os,d)) (0,0,0) (timestamp (fps 1)) only produces Trues (except in the first event, where this is simply not possible, because the first delta computed in the foldp will compare the current time against initial value 0 instead of against a reasonable previous time)

Okay, programStart is now set globally.

jvoigtlaender · Jan 5, 2015

Okay, programStart is now set globally.

laszlopandy · Jan 5, 2015

laszlopandy reviewed Jan 5, 2015

View changes

laszlopandy · Jan 5, 2015

src/Native/Time.js

+      curr = fst(event);
+      zero = snd(event);
+      diff = zero ? 0 : curr - prev;
+      return Utils.Tuple2(diff, curr);


These variables should all be local to this function because they are not used anywhere else now.

Yes, I had not made them local purely because I wanted to create as small as possible a conceptual diff against the old version of this function, where these variables were declared at the top.

But yes, I'll refactor now as you suggest.

laszlopandy · Jan 5, 2015

laszlopandy reviewed Jan 5, 2015

View changes

laszlopandy · Jan 5, 2015

src/Native/Time.js

    }
+    var clock = A2( Signal.map, fst, NS.timestamp(ticker) );


I think there should be a function Time.timeOf : Signal a -> Signal Time, which samples the current time of each signal update. It seems like there are many cases where you call timestamp but actually only want the first argument.

The question is, will this be a function defined on the Elm side, or something added as a native signal on the JS side.

I would write Time.timeOf in native and then define Time.timestamp in terms of it.

timestamp sig = Signal.map (,) (timeOf sig) sig

So this would be a change in the standard library. Probably something that should be considered independently of this PR.

Good point. Let's merge this one first.

Please merge with master, so you get the new run-test.sh script. Otherwise this branch will keep failing on Travis.

laszlopandy · Jan 6, 2015

Please merge with master, so you get the new run-test.sh script. Otherwise this branch will keep failing on Travis.

Merged (only a small conflict needed resolving for the new programStartvalue), and Travis build pass looks fine.

jvoigtlaender · Jan 6, 2015

Merged (only a small conflict needed resolving for the new programStartvalue), and Travis build pass looks fine.

laszlopandy · Jan 6, 2015

laszlopandy reviewed Jan 6, 2015

View changes

laszlopandy · Jan 6, 2015

src/Native/Time.js

+      return Utils.Tuple2(snd(event) ? 0 : curr - snd(old), curr);
+    }
+    var deltas = A2( Signal.map, fst, A3( Signal.foldp, F2(g), Utils.Tuple2(0, elm.timer.programStart), NS.timestamp(ticker) ) );
+    return A3( Signal.map2, F2(f), isOn, deltas );
  }


These last few lines are really hard to read for me.

f and g should have proper names. (I thought of startStopTimer and calcDelta, but you can choose).

The tuple coming out of NS.timestamp and the tuple from g are not the same, but I thought they were. Since this will not be exposed to Elm, you can use an object, it doesn't have to be an Elm type: { delta: 0, timestamp: elm.timer.programStart }

jvoigtlaender · Jan 6, 2015

How about this?

To set expectations on timing, I am not sure how this will interact with elm-reactor which was the primary thing I want to fix with 0.14.1. I want to do the 0.14.1 release quite soon, so I'd like to merge this right after that.

evancz · Jan 7, 2015

To set expectations on timing, I am not sure how this will interact with elm-reactor which was the primary thing I want to fix with 0.14.1. I want to do the 0.14.1 release quite soon, so I'd like to merge this right after that.

Sounds good.

jvoigtlaender · Jan 7, 2015

Sounds good.

Bumping this, now that the dust has settled around the 0.14.1 release.

jvoigtlaender · Jan 20, 2015

Bumping this, now that the dust has settled around the 0.14.1 release.

Do we have any certainty at all that this will work with Elm Reactor? I did a review just now, and it's not clear to me at all. If it does not work, are you responsible for making the changes there?

I have some style concerns that I'll add inline.

evancz · Jan 21, 2015

Do we have any certainty at all that this will work with Elm Reactor? I did a review just now, and it's not clear to me at all. If it does not work, are you responsible for making the changes there?

I have some style concerns that I'll add inline.

evancz · Jan 21, 2015

evancz reviewed Jan 21, 2015

View changes

evancz · Jan 21, 2015

src/Native/Signal.js

+    this.value = Utils.Tuple2(localRuntime.timer.programStart, input.value);
+    this.kids = [];
+    this.recv = function(timestep, changed, parentID) {
+      if (changed) { this.value = Utils.Tuple2(timestep, input.value); }


Use more modern style. I did a bunch of messy things when I was writing this code.

if (changed) { this.value = Utils.Tuple2(timestep, input.value); }

Sure, I can do these style changes. I had refrained from doing any of that, both to make the syntactic diff as small as possible and since you somewhere expressed that contributors should as much as possible preserve syntactic style they encounter in the files they edit.

evancz · Jan 21, 2015

evancz reviewed Jan 21, 2015

View changes

evancz · Jan 21, 2015

src/Native/Signal.js

  }

+  function timestamp(input) { return new Timestamp(input); }


function timestamp(input) { return new Timestamp(input); }

evancz · Jan 21, 2015

evancz reviewed Jan 21, 2015

View changes

evancz · Jan 21, 2015

src/Native/Time.js

@@ -12,37 +12,36 @@ Elm.Native.Time.make = function(elm) {

  function fpsWhen(desiredFPS, isOn) {


I find the new version of this function very very confusing. At least it should not have 170+ character lines, but the ideal version might actually move wasOn and timeoutID into a signal so that it all gets picked up by Elm Reactor?

The same style stuff with {} applies here as well with the function and object. Prefer to bring things down a line.

Do you find the old version of this function less confusing? That would be https://github.com/elm-lang/core/blob/master/src/Native/Time.js#L13-39. It has the same basic structure with the non-exposed signals (so nothing for Elm Reactor to pick up).

The crucial commit concerning this function is this one: jvoigtlaender@274cd7e. All subsequent commits are just renamings or moving around of variables, prompted by @laszlopandy's comments.

Should I talk you through the mentioned commit? I'd say it very much simplifies this function compared to how it was previously.

Re: "Do we have any certainty at all that this will work with Elm Reactor? I did a review just now, and it's not clear to me at all. If it does not work, are you responsible for making the changes there?"

Do you have specific concerns about what might potentially break? I did think about how this might interact with the reactor while writing the PR. Specifically, I was considering hot-swapping and convinced myself that that will be okay (basing that judgement on an analysis of what hotSwap in Runtime.js does). Also, I had taken into account in my first version of the PR how the timer indirection stuff worked prior to elm-lang@d4a6d93 (with inducedDelay and all), and then when that commit came out, thought about how that would affect my PR and adapted it accordingly. What guarantees beyond those considerations can I give? Will I feel responsible for making changes in the reactor if they become necessary because of what I introduced here? To the best of my abilities and understanding, yes.

Aside: the PR is divided into small-scale commits. Some of these can easily be identified as mere refactorings which certainly have no potential to break anything. I could talk you through the remaining commits in order, to explain why each of them is okay (not changing semantics in an adverse way).

jvoigtlaender · Jan 21, 2015

Re: "Do we have any certainty at all that this will work with Elm Reactor? I did a review just now, and it's not clear to me at all. If it does not work, are you responsible for making the changes there?"

Do you have specific concerns about what might potentially break? I did think about how this might interact with the reactor while writing the PR. Specifically, I was considering hot-swapping and convinced myself that that will be okay (basing that judgement on an analysis of what hotSwap in Runtime.js does). Also, I had taken into account in my first version of the PR how the timer indirection stuff worked prior to elm-lang@d4a6d93 (with inducedDelay and all), and then when that commit came out, thought about how that would affect my PR and adapted it accordingly. What guarantees beyond those considerations can I give? Will I feel responsible for making changes in the reactor if they become necessary because of what I introduced here? To the best of my abilities and understanding, yes.

Aside: the PR is divided into small-scale commits. Some of these can easily be identified as mere refactorings which certainly have no potential to break anything. I could talk you through the remaining commits in order, to explain why each of them is okay (not changing semantics in an adverse way).

Thanks for the fix! I did some style changes that were needed in the files, but I'll trust that things work in a good way.

One simple way to see if things work well with Elm Reactor is to use Elm Reactor with the new code. If you run through some programs that use fps and pause, rewind, and swap, that's a pretty good indicator.

evancz · Jan 21, 2015

Thanks for the fix! I did some style changes that were needed in the files, but I'll trust that things work in a good way.

One simple way to see if things work well with Elm Reactor is to use Elm Reactor with the new code. If you run through some programs that use fps and pause, rewind, and swap, that's a pretty good indicator.

Cool that you merged this.

Also, I agree that the commit elm-lang@79cd80e made the working of the fpsWhen function clearer. In fact, given that clearer form I now think a further change is warranted, in this line: https://github.com/elm-lang/core/blob/master/src/Native/Time.js#L37. Namely, it would be better to have wasOn = isOn.value there. The reason is that if the initial value of the passed isOn signal is false (otherwise the proposed new line is anyway equivalent to the current line), then the current implementation unnecessarily does a clearTimeout(0) at signal initialization time, whereas the proposed change would prevent that.

jvoigtlaender · Jan 21, 2015

Cool that you merged this.

Also, I agree that the commit elm-lang@79cd80e made the working of the fpsWhen function clearer. In fact, given that clearer form I now think a further change is warranted, in this line: https://github.com/elm-lang/core/blob/master/src/Native/Time.js#L37. Namely, it would be better to have wasOn = isOn.value there. The reason is that if the initial value of the passed isOn signal is false (otherwise the proposed new line is anyway equivalent to the current line), then the current implementation unnecessarily does a clearTimeout(0) at signal initialization time, whereas the proposed change would prevent that.

Re: "One simple way to see if things work well with Elm Reactor is to use Elm Reactor with the new code", am I correct that I cannot do this by testing with the elm-reactor executable that was installed by the Elm-0.14.1 installer? Because its debugger mode will run with some "compiled-in" version of the standard library, that is, it will still run the implementations of timestamp, fps etc. that were current when that elm-reactor executable was built?

jvoigtlaender · Jan 21, 2015

Re: "One simple way to see if things work well with Elm Reactor is to use Elm Reactor with the new code", am I correct that I cannot do this by testing with the elm-reactor executable that was installed by the Elm-0.14.1 installer? Because its debugger mode will run with some "compiled-in" version of the standard library, that is, it will still run the implementations of timestamp, fps etc. that were current when that elm-reactor executable was built?

jwmerrill · Jan 23, 2015

I think this change introduces incorrect behavior for wasOn and isOn. It's subtle, but the conditional used to be executed immediately inside f, and is now executed in the setTimeout callback. That means that by the time !wasOn && isOn runs, line 34 will have executed, setting wasOn = isOn.

I haven't tested this yet, but I think that conditional is now always false, because by the time it executes, wasOn will always be equal to isOn.

Argh, mutable state, defeating refactoring habits grown in referentially transparent Haskell land.

The fix should be to assign !wasOn && isOn to an intermediate variable, and pass that one there, right?

I wonder why this didn't come up in my testing, but I do believe now that a test can be written that shows the problem.

@jwmerrill, this commit should fix it for good: jvoigtlaender@fee51f3

Ah, just put up a PR of my own: https://github.com/elm-lang/core/pull/135

IMHO, my version reads a little cleaner, and has the advantage of not creating a new function on every frame.

Yeah, I commented in your PR that I like that version, modulo an additional simplification.

jvoigtlaender referenced this pull request Jan 2, 2015
Closed
Time.timestamp does not seem to satisfy the semantics implied in documentation #75

small refactoring (and comments about programStart)

e74a00f

jvoigtlaender changed the title from Fix timestamp function to Fix timestamp function and its interaction with every/fps/fpsWhen Jan 2, 2015

make initial timestamp unique throughout
... and thus also fix every/fps/fpsWhen behaviour at signal start

c2f87e0

jvoigtlaender added some commits Jan 5, 2015

refactoring variables as brought up by #laszlopandy

68be574

further simplification

620b1fb

Merge branch 'master' into fix-timestamp
Conflicts: src/Native/Runtime.js merge master

8d614b5

renaming/refactoring

94bed9d

Merge branch 'master' into fix-timestamp

79d743d

style changes

e9da1e1

evancz pushed a commit that referenced this pull request Jan 21, 2015

Merge pull request #76 from jvoigtlaender/fix-timestamp
Fix timestamp function and its interaction with every/fps/fpsWhen

36962ec

evancz merged commit 36962ec into elm:master Jan 21, 2015
1 check passed

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

jvoigtlaender deleted the jvoigtlaender:fix-timestamp branch Jan 21, 2015

This was referenced Jan 23, 2015

Merged

Fix incorrect delta reset in Native.Time.fpsWhen. #135

Closed

Fix semantics of fpsWhen. #142

jvoigtlaender referenced this pull request Feb 6, 2015
Merged
Don't make timeoutId in fpsWhen part of the state stored in signal graph. #160

jvoigtlaender referenced this pull request May 2, 2015
Closed
foldp' breaks debugger in 0.15? #11

elm/core

Join GitHub today

Fix timestamp function and its interaction with every/fps/fpsWhen #76

Conversation

jvoigtlaender commented Jan 2, 2015

jvoigtlaender added some commits Jan 2, 2015

jvoigtlaender referenced this pull request Jan 2, 2015

jvoigtlaender changed the title from Fix timestamp function to Fix timestamp function and its interaction with every/fps/fpsWhen Jan 2, 2015

This comment has been minimized.

evancz commented Jan 5, 2015

This comment has been minimized.

jvoigtlaender commented Jan 5, 2015

This comment has been minimized.

jvoigtlaender commented Jan 5, 2015

laszlopandy reviewed Jan 5, 2015 View changes

This comment has been minimized.

This comment has been minimized.

laszlopandy reviewed Jan 5, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jvoigtlaender added some commits Jan 5, 2015

This comment has been minimized.

laszlopandy commented Jan 6, 2015

This comment has been minimized.

jvoigtlaender commented Jan 6, 2015

laszlopandy reviewed Jan 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

jvoigtlaender commented on 94bed9d Jan 6, 2015

This comment has been minimized.

evancz commented Jan 7, 2015

This comment has been minimized.

jvoigtlaender commented Jan 7, 2015

This comment has been minimized.

jvoigtlaender commented Jan 20, 2015

This comment has been minimized.

evancz commented Jan 21, 2015

evancz reviewed Jan 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

evancz reviewed Jan 21, 2015 View changes

This comment has been minimized.

evancz reviewed Jan 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jvoigtlaender commented Jan 21, 2015

evancz pushed a commit that referenced this pull request Jan 21, 2015

Hide details View details evancz merged commit 36962ec into elm:master Jan 21, 2015 1 check passed

1 check passed

This comment has been minimized.

evancz commented Jan 21, 2015

jvoigtlaender deleted the jvoigtlaender:fix-timestamp branch Jan 21, 2015

This comment has been minimized.

jvoigtlaender commented Jan 21, 2015

This comment has been minimized.

jvoigtlaender commented Jan 21, 2015

This comment has been minimized.

jwmerrill commented on src/Native/Time.js in 274cd7e Jan 23, 2015

This comment has been minimized.

jvoigtlaender replied Jan 23, 2015

This comment has been minimized.

jvoigtlaender replied Jan 23, 2015

This comment has been minimized.

jvoigtlaender replied Jan 23, 2015

This comment has been minimized.

jwmerrill replied Jan 23, 2015

This comment has been minimized.

jwmerrill replied Jan 23, 2015

This comment has been minimized.

jvoigtlaender replied Jan 23, 2015

This was referenced Jan 23, 2015

jvoigtlaender referenced this pull request Feb 6, 2015

jvoigtlaender referenced this pull request May 2, 2015

laszlopandy reviewed Jan 5, 2015

View changes

laszlopandy reviewed Jan 5, 2015

View changes

laszlopandy reviewed Jan 6, 2015

View changes

jvoigtlaender commented on `94bed9d` Jan 6, 2015

evancz reviewed Jan 21, 2015

View changes

evancz reviewed Jan 21, 2015

View changes

evancz reviewed Jan 21, 2015

View changes

evancz merged commit `36962ec` into elm:master Jan 21, 2015
1 check passed

jwmerrill commented on `src/Native/Time.js` in `274cd7e` Jan 23, 2015