simplify the clock logic by removing LinkOperations

[polymorphicengine]

it was pretty hard to track down what was causing #1089, so i found it easier just to remove the LinkOperations datatype and replace it with a bunch of functions that act on a given session state / a clock reference / the clock config

i think this is a good next step in simplifying the stream/clock logic.

tested everything and seems to be fine, but please have a look @Zalastax, @mindofmatthew, @yaxu :)

[Zalastax]

This is a tricky area and it's good that you try to simplify!

By using setCPS, the change will not be effective until next cycle. This could cause a difference in the performance. What do you think about this potential difference?

[polymorphicengine]

thanks for looking into it!
are you sure it wont be effective until the next cycle?
when i try it, it seems instant. i'm doing something like

ghci> import Sound.Tidal.Context 
ghci> tidal <- startTidal superdirtTarget defaultConfig 
ghci> :set -XOverloadedStrings 
ghci> streamReplace tidal 1 $ s "ho sn bd sn"
ghci> streamOnce tidal $ cps 2

[Zalastax]

I expect this to behave differently? Could cause subtle differences when events change cps and the events coming afterwards won't use the new cps until next tick.

[polymorphicengine]

yes it might be a difference of a tick, the reason i did here like this is to avoid carrying around an additional sessionstate that would keep track of time changes (this was previously possible, since the LinkOperations would carry this specific version of setTempo)
i'm not so sure if the complication is worth it here

[Zalastax]

I'm not really active in this project at the moment so someone else need to make the call of whether the complexity is worth it. But now you see the motivation for the current design.

[Zalastax]

Yes, I am quite confident. This is because setCPS just adds a SetTempo entry to the actions list. The actions are processed between the ticks. So with this change, events that use cps won't immediately affect the timing. I think this could be problematic for some people, while most won't notice.

[polymorphicengine]

hmm now that i think of it, would a version of setTempo work that creates a new session state, calls Link.setTempo and commits the session state?

[Zalastax]

hmm now that i think of it, would a version of setTempo work that creates a new session state, calls Link.setTempo and commits the session state?

It could possibly work if it also calls setTempo on the original session state that's used for the tick. This is needed since commits on the new session state don't affect the original. For the single tick case I see no issues because the original session state is not committed but I don't know how the normal tick case is affected when we set tempo in two session states and commit both.

[Zalastax]

It seems unsafe to use ss after it is destroyed. If we want to ensure ss is freed, then I think we need to move flow control into this function. Here we could accept a function (Link.SessionState -> IO a) as argument and return IO a. We would create ss, run the function, then destroy ss, then return.

[Zalastax]

Alternatively, could we move the destroySessionState into a callback function which we include in the return? New return type becomes IO (Link.SessionState, IO ()).

[Zalastax]

Doesn't this mean we will lose the tempo changes that happen in the single tick case since we don't (and can't) commit the zeroed session state?

[polymorphicengine]

i thought so too, but streamOnce tidal $ cps 1 is still working.. not completely sure why?

[Zalastax]

Could this be the source of the bug? We are running commitAndDestroy immediately - before the IO where the session states are used. Compare to clockProcess where I understand we first process the arc using liftIO, then we run commitAndDestroy.

[Zalastax]

The suggestion in https://github.com/tidalcycles/Tidal/pull/1090/files/81c19870a81294e4b5def9cdc24365fa0b23e726#r1645196629 could be applied here. The callback would invoke both destroy functions.

[polymorphicengine]

hmm yes that makes sense, the callback is probably a good idea! thanks!

[polymorphicengine]

this should be good to go now.

i introduced an additional session state to keep track of tempo changes:
in case of the doTick call inside of the running clock, it will just be twice the same session state, but in case of doSingleTick it will be a zeroed session state and the new additional one.

also, as you suggested, they are deleted after the call to doTick now.

do let me know what you think @Zalastax !

[Zalastax]

This looks good to me!

I think there could be some more encapsulation so that Clock is in charge of all the calls to Link, but I don't find that so important. I would be OK with merging this.

[polymorphicengine]

I think there could be some more encapsulation so that Clock is in charge of all the calls to Link

yes i agree - added one more commit that moves the code to a function clockOnce in the Clock module that you simply pass a TickAction.

also bumped tidal-links version to 1.0.4

i think this is good to go now!

[matthewkaney]

This all looks good to me, although I admit I'm not very knowledgeable about the Link integration.

A direction I'd like to see with encapsulation is the ability to swap in a deterministic mock clock for writing more Stream tests. That can definitely wait until a future PR of course.

[yaxu]

Will try it out in the wild, thanks!