tidalcycles / Tidal

Pattern language
http://tidalcycles.org/
GNU General Public License v3.0
2.27k stars 255 forks source link

simplify the clock logic by removing LinkOperations #1090

Closed polymorphicengine closed 3 months ago

polymorphicengine commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

polymorphicengine commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

Will try it out in the wild, thanks!