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

Add initial Event to switchE #165

Merged
merged 7 commits into from
Jan 9, 2022
Merged

Add initial Event to switchE #165

merged 7 commits into from
Jan 9, 2022

Conversation

mitchellwrosen
Copy link
Collaborator

This patch changes the type of switchE from

switchE :: MonadMoment m => Event (Event a) -> m (Event a)

to

switchE :: MonadMoment m => Event a -> Event (Event a) -> m (Event a)

as briefly discussed in #162

Consider this a second attempt at a feature request, this time with code attached :)

I'm not fully convinced this change is necessary, as I'm quite new to FRP in general, but this is a pattern that seems to exist in other frameworks. Happy to hear reasons why this is not needed, or if it is, I hope I've implemented it properly! Thanks.

@mitchellwrosen
Copy link
Collaborator Author

Is there anything I can do to help move this along?

@mitchellwrosen
Copy link
Collaborator Author

@HeinrichApfelmus, any thoughts on this one?

lp <- stepperL never pp
switchP :: Pulse a -> Pulse (Pulse a) -> Build (Pulse a)
switchP p pp = mdo
lp <- stepperL p pp
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, the latch-of-pulse had an initial pulse never. Now, p.

p1 <- newPulse "switchP_in" switch :: Build (Pulse ())
p1 `dependOn` pp
p2 <- newPulse "switchP_out" eval
p2 `dependOn` p
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, p2 was initialized with no parents, and would not have a parent until pp fired. p1, a child of pp, would switch p2's parent to the newly fired event.

Now, p2 does have an initial parent - p. This, like before, will get switched away to a new parent by p1 when pp fires.

@mitchellwrosen
Copy link
Collaborator Author

In addition to the use case I mentioned in #162, I think I've hit another. In brief:

I have a "game session" that's modeled as:

gameSessionMoment :: Event Input -> Moment (Behavior GameState, Event ())

where the output Behavior GameState is rendered to the screen on changes, and the output Event () fires when the player dies.

I thought about turning this into an infinite game, by simply restarting the whole thing when the player dies. This would mean the final game creation function becomes

gameMoment :: Event Input -> Moment (Behavior GameState)

(with some out-of-band mechanism for quitting the game, like pressing Esc, which is not fed into the FRP network).

I got stuck writing the second function in terms of the first. Here's an attempt:

gameMoment :: Event Input -> Moment (Behavior GameState)
gameMoment eInput = do
  (bFirstGameState, eFirstGameOver) <- gameSessionMoment eInput

  -- Event that fires when the current game is over
  let eGameOver = undefined :: Event ()

  let eNewGameSession :: Event (Behavior GameState, Event ())
      eNewGameSession = observeE (gameSessionMoment eInput <$ eGameOver)

  switchB bFirstGameState (fst <$> eNewGameSession)

In English:

  • Create the first game session.
  • When the first game session ends, switch to a new game session.

However I could not figure out how to define eGameOver above. If switchE took an initial event, then it would be defined as:

eGameOver <- switchE eFirstGameOver (snd <$> eNewGameSession)

@Invisible-Rabbit-Hunter
Copy link
Contributor

Invisible-Rabbit-Hunter commented Jun 29, 2021

Hi, will this ever be merged? I found a case where I think something like this is needed, namely when trying to create a type akin to Dynamic in reflex, specifically when creating a join operator.

My attempt would look approximately like this

data Dynamic a = Dynamic { dynBehavior :: Behavior a, dynEvent :: Event a }
  deriving Functor

joinDyn :: forall m a. MonadMoment m => Dynamic (Dynamic a) -> m (Dynamic a)
joinDyn (Dynamic behDyn evDyn) = do
  d1 <- valueB behDyn
  e <- switchE (dynEvent d1) (dynEvent <$> evDyn)
  b <- switchB (dynBehavior d1) (dynBehavior <$> evDyn)
  return $ Dynamic b e

which I think should yield the correct semantics, though I'm actually quite new to FRP in general.

@mitchellwrosen
Copy link
Collaborator Author

I'm not sure. Despite having written the patch, I now forget most of what I (thought I) knew about the internals, and it's tough to say without an expert reviewer whether this patch could be considered correct (is there a space leak now? etc).

Perhaps once I have more time I'll continue my side-project of stripping down and refactoring reactive-banana (in a separate repo) until the implementation becomes clear, but for now, I can't push this along any further.

@ocharles
Copy link
Collaborator

To move this forward, I'd like to look at a standalone example of where we really really need this change. It's hard to be sufficiently motivated with where things stand at the moment. This isn't to say "let's not do this", but more "let's make sure we really understand why we're doing this". The game example you provide @mitchellwrosen looks like a great start here, so maybe we should turn that into something we can run and try and find some solutions there. I understand @Invisible-Rabbit-Hunter's suggestion about Dynamic, but I'd rather reason in terms of an actual application, rather than an abstraction.

@ocharles
Copy link
Collaborator

ocharles commented Sep 14, 2021

Actually, I'm coming around to this. In #162, @HeinrichApfelmus acknowledges that he thinks this is probably something new. In #74 (comment), @ChristopherKing42 shows that once could be implemented in terms of this. And for me personally I'm (yet again) building a little UI-toolkit in reactive-banana, and wanted a switchUI :: UI a -> Event (UI a) -> MomentIO (UI a). I won't go into the details of what a UI is, but suffice to say it's basically Dynamic (a UI is a Behavior and an Event), which @Invisible-Rabbit-Hunter also expressed a desire for.

So maybe we do have sufficient evidence to move on this. @HeinrichApfelmus, any thoughts?

@ocharles
Copy link
Collaborator

ocharles commented Sep 14, 2021

It may also be worth contrasting other FRP systems. Notably I looked at reflex and they have:

switchHold :: (Reflex t, MonadHold t m) => Event t a -> Event t (Event t a) -> m (Event t a) 

which is precisely the new switchE we're talking about here. Interestingly this isn't primitive:

switchHold ea0 eea = switch <$> hold ea0 eea

where

switch :: Behavior t (Event t a) -> Event t a
hold :: a -> Event t a -> m (Behavior t a)

are primitive (at least in the sense they are methods of Reflex and MonadHold, respectively). hold here is our stepper, but we don't have a switch in this form. So I wonder if that switch is also something to consider. I think with that, our current switchE is switchE e = switch =<< stepper never e.

Eitherway, I would probably still want to provide users with switchE :: Event a -> Event (Event a) -> MomentIO (Event a), so maybe a small step is to just add that and we can revisit the Behavior (Event a) switch another time.

@ChristopherKing42
Copy link

ChristopherKing42 commented Sep 16, 2021

switch :: Behavior (Event a) -> Event a does seem more general then the proposed switchE.

@ocharles
Copy link
Collaborator

@mitchellwrosen I've merged master into your branch and also updated the model documentation for switchE. It's a bit hairy, maybe there's a better formulation.

Copy link
Collaborator

@ocharles ocharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board with this, so I'm going this my approval. Tagging in @HeinrichApfelmus for the next review!

@HeinrichApfelmus
Copy link
Owner

(Sorry for the long delay on this.) I think that I'm onboard with the change, especially as @PaulJohnson has given an implementation in terms of existing combinators, #162 (comment) , but the new version does have better garbage collection (as upTo and once are still open).

I main question I have is: Do we want to change the type of switchE and break backwards compatibility, or do we want to introduce a new name to keep backwards compatibility for a little longer?

@ocharles
Copy link
Collaborator

Do we want to change the type of switchE and break backwards compatibility, or do we want to introduce a new name to keep backwards compatibility for a little longer?

A tricky question! A quick search on Hackage search suggests that there is no Hackage usage of switchE - all the results are other libraries defining their own switchE. To me the appeal of reactive-banana is its compact API, so introducing a new combinator and keeping switchE means we are essentially exporting switchE = switchE2 never, which seems overly specific. Also, we are the only FRP library that has switchE without an initial event, so this change essentially brings us inline with the rest.

If it were me, I'd be inclined to make a breaking change.

Comment on lines 280 to 284
-- > switchE ee = \time0 -> concat [trim t1 t2 e | (t1,t2,e) <- intervals ee, time0 <= t1]
-- > switchE e0 ee = \time0 -> [(t, a) | (t,a) <- e0, t < t1] ++ concat [trim t1 t2 e | (t1,t2,e) <- intervals ee, time0 <= t1]
-- > where
-- > t1 = head [t | (t, _) <- ee]
-- > intervals e = [(time1, time2, x) | ((time1,x),(time2,_)) <- zip e (tail e)]
-- > trim time1 time2 e = [x | (timex,x) <- e, time1 < timex, timex <= time2]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither is correct, I believe, not the least because t1 is not in scope. 😅 The correct semantics is

switchE e0 ee0 time0 =
    concat [ trim t1 t2 e | (t1,t2,e) <- intervals ee ]
  where
    laterThan e time0  = [(timex,x) | (timex,x) <- e, time0 < timex ]
    ee                 = [(time0, e0)] ++ (ee0 `laterThan` time0)
    intervals ee       = [(time1, time2, e) | ((time1,e),(time2,_)) <- zip ee (tail ee)]
    trim time1 time2 e = [x | (timex,x) <- e, time1 < timex, timex <= time2]

Formulated in this way, it becomes clearer how e aligns with the occurrences in ee, and the trim function indicates that <= instead of < is the right choice.

The other main question is: What happens right at time0? If the ee event has an occurrence at this moment, then this will be ignored — that's why we need the initial event e in the first place. Any occurrence of e at this moment will also be ignored, however, also by virtue of trim.

(That said, the semantics was never quite correct in the case where ee is a singleton or the empty list. 🙈 But we can exclude this corner case by proclaiming that the semantics are only supposed to hold in the other cases.)

Copy link
Owner

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the pull request, @mitchellwrosen ! 😊

If somebody could

  • Change the comment for switchE to use the correct semantics
  • Add a major version bump to the .cabal and update the changelog

then I think we're ready to merge!

@ocharles
Copy link
Collaborator

ocharles commented Jan 9, 2022

@HeinrichApfelmus Done!

@ocharles ocharles merged commit d06310e into HeinrichApfelmus:master Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants