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

Confounding behavior of Time signal #145

Closed
imeckler opened this Issue Jan 27, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@imeckler
Contributor

imeckler commented Jan 27, 2015

I just happened upon the following extremely confusing behavior. Consider the following program:

module Bad where

import Keyboard
import Time
import Text
import Signal

type Update
  = Key {x : Int, y : Int}
  | Tick Time.Time

updates =
  let time = Time.every 30 in
  Signal.mergeMany
  [ Signal.map Key (Signal.sampleOn time Keyboard.arrows)
  , Signal.map Tick time
  ]

update u s = case u of
  Key arr -> {s | x <- s.x + arr.x, y <- s.y + arr.y}
  Tick t  -> {s | time <- t}

main = 
  Signal.foldp update {x=0,y=0,time=0} updates
  |> Signal.map (Text.plainText << toString)

If you try running it, you'll notice that the time field never changes! Somehow those updates aren't making it to the merged signal. Replacing on of the instances of time with a "new" Time.every 30 makes things work as expected.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 27, 2015

Contributor

This is exactly as mandated by the documentation of Signal.mergeMany: "When multiple updates come in at the same time, the left-most update wins, just like with merge." Where "just like with merge" means, again per the documentation: "If an update comes on both signals at the same time, the left update wins (i.e., the right update is discarded)."

Since in your case both updates arrive exactly when the time from let time = Time.every 30 has an event, only one of the two updates makes it through, namely the left one.

As to why replacing one instance of time with a "new" Time.every 30 changes the behavior, see this thread: https://groups.google.com/d/msg/elm-discuss/KzevSWc0gfU/kfRNiZKLxP8J

Contributor

jvoigtlaender commented Jan 27, 2015

This is exactly as mandated by the documentation of Signal.mergeMany: "When multiple updates come in at the same time, the left-most update wins, just like with merge." Where "just like with merge" means, again per the documentation: "If an update comes on both signals at the same time, the left update wins (i.e., the right update is discarded)."

Since in your case both updates arrive exactly when the time from let time = Time.every 30 has an event, only one of the two updates makes it through, namely the left one.

As to why replacing one instance of time with a "new" Time.every 30 changes the behavior, see this thread: https://groups.google.com/d/msg/elm-discuss/KzevSWc0gfU/kfRNiZKLxP8J

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 27, 2015

Contributor

So the upshot is that what you are seeing is not a bug, but the specified behavior of mergeMany. One thing, though, is that it is not currently clear from the documentation that a second Time.every 30 gives you completely independent times. From a message (mine) on the linked thread:

"Yes, having two signals both defined as Time.fps 30 not produce identical events is a kind of violation of referential transparency. Likewise with calls of Time.every. Probably that should be mentioned in the documentation."

Contributor

jvoigtlaender commented Jan 27, 2015

So the upshot is that what you are seeing is not a bug, but the specified behavior of mergeMany. One thing, though, is that it is not currently clear from the documentation that a second Time.every 30 gives you completely independent times. From a message (mine) on the linked thread:

"Yes, having two signals both defined as Time.fps 30 not produce identical events is a kind of violation of referential transparency. Likewise with calls of Time.every. Probably that should be mentioned in the documentation."

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender
Contributor

jvoigtlaender commented Jan 27, 2015

@imeckler

This comment has been minimized.

Show comment
Hide comment
@imeckler

imeckler Jan 27, 2015

Contributor

I see, thanks for the quick response. The behavior I was used to from other FRP implementations in the case of simultaneous signals was to trigger the right one just after the left.

What are the intended semantics of Signal? Is it something like Signal a = Stream (Time, a) where the Time is strictly increasing, or is it Stream (Time, a) where the Time is increasing, not necessarily strictly? If it's the former I understand this decision (although it's not clear that those semantics are an appropriate choice), if it's the latter it seems extremely strange to just drop events.

Contributor

imeckler commented Jan 27, 2015

I see, thanks for the quick response. The behavior I was used to from other FRP implementations in the case of simultaneous signals was to trigger the right one just after the left.

What are the intended semantics of Signal? Is it something like Signal a = Stream (Time, a) where the Time is strictly increasing, or is it Stream (Time, a) where the Time is increasing, not necessarily strictly? If it's the former I understand this decision (although it's not clear that those semantics are an appropriate choice), if it's the latter it seems extremely strange to just drop events.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 27, 2015

Contributor

This can be an endless topic for philosophical discussion on the mailing list. In fact, it already has been. :)

Anyway, probably not useful to have that discussion here.

Contributor

jvoigtlaender commented Jan 27, 2015

This can be an endless topic for philosophical discussion on the mailing list. In fact, it already has been. :)

Anyway, probably not useful to have that discussion here.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 27, 2015

Contributor

But maybe writing a PR with additional documentation would be useful. Anything that would have helped you to not be surprised by this behavior you ran into.

Contributor

jvoigtlaender commented Jan 27, 2015

But maybe writing a PR with additional documentation would be useful. Anything that would have helped you to not be surprised by this behavior you ran into.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jan 27, 2015

Contributor

I tried to address the documentation need in https://github.com/elm-lang/core/pull/146.

I think the issue here can be closed.

@imeckler, I didn't answer your explicit question about the intended semantics of Signal before. The answer is: It is something like Signal a = Stream (Time, a) where the Time is strictly increasing.

Contributor

jvoigtlaender commented Jan 27, 2015

I tried to address the documentation need in https://github.com/elm-lang/core/pull/146.

I think the issue here can be closed.

@imeckler, I didn't answer your explicit question about the intended semantics of Signal before. The answer is: It is something like Signal a = Stream (Time, a) where the Time is strictly increasing.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 30, 2015

Contributor

Closing this, since no further activity happened here after the improvement to the docs this triggered.

Contributor

jvoigtlaender commented Aug 30, 2015

Closing this, since no further activity happened here after the improvement to the docs this triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment