-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
replace Tempo module by an ableton-link synched clock that comes with tidal-link #1059
Conversation
…ly on tidals types and can act as a general purpose ableton link based clock for timed processes
…reintegrate everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! This makes sense to me. I've added some comments and questions I had, but overall I think this is a good direction to move in.
,cQuantum = 4 | ||
,cBeatsPerCycle = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a @Zalastax questions, but what's the difference between cQuantum
and cBeatsPerCycle
? To me they seem redundant: if someone wants to define Tidal "bpm" in terms of a 4-beat cycle, is there a case where they wouldn't also want cycles phase-aligned with other 4-beat measures (loops, etc) on the network (which is what quantum is for)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly redundant. I thought it could possibly be useful for some people but wasn't sure if it would be used. I was thinking people think of cycles in interesting and different ways and that I don't want to impose my way. It could also act as a workaround for the limits of Link BPM.
Btw, can you update the BootTidal.hs file so that it's easier to test this branch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of my goals when I introduced Link was to sequentialize everything that can affect time. I did so by moving the handling of Transition and once
into Tempo and putting everything in a queue which is processed between the processing of the normal clock. This is inspired by my experience with Erlang actors and I believe it often reduces bugs. We are here moving away from that pattern. I do see benefits with this new approach, like once
happening more immediately and ClockActions being a cleaner interface. I also don't think the drawbacks are very significant and I notice I didn't even fully complete that vision because pmap is modified from multiple threads. So I quite like this change but wanted to provide some context. Might be room for some future tidying up, but not sure much benefits can be gained.
} | ||
|
||
-- | possible actions for interacting with the clock | ||
data ClockAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this focuses the actions to be time related instead of the mix that TempoAction had.
@@ -37,11 +39,30 @@ import Sound.Tidal.Utils (enumerate) | |||
along with this library. If not, see <http://www.gnu.org/licenses/>. | |||
-} | |||
|
|||
type TransitionMapper = Time -> [ControlPattern] -> ControlPattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
src/Sound/Tidal/Config.hs
Outdated
@@ -25,31 +24,21 @@ data Config = Config {cCtrlListen :: Bool, | |||
cCtrlAddr :: String, | |||
cCtrlPort :: Int, | |||
cCtrlBroadcast :: Bool, | |||
cFrameTimespan :: Double, | |||
cEnableLink :: Bool, | |||
cProcessAhead :: Double, | |||
cTempoAddr :: String, | |||
cTempoPort :: Int, | |||
cTempoClientPort :: Int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this PR would be a good place to get rid of cTempoAddr
, cTempoPort
and cTempoClientPort
, since it's already changing the Config
type? They're for a tempo-sharing mechanism that was replaced with Link, and are unused everywhere else.
i got around to the restructuring of the stream module now. it is seperated into 7 sub-modules now all of which are re-exported by the old
i didn't make any changes to the code yet, it was just a matter of copy and pasting things into the right places. hope this makes sense to you, let me know what you think! @mindofmatthew @yaxu @Zalastax |
This reorganization makes sense to me! I was curious: are you thinking of doing a major restructuring of the Stream code in this PR? I could see it getting unwieldy, but since there are only a couple of us who will likely comment, I think it's manageable. If that's the case, would you mind if I submitted pull requests against your branch? I have some ideas of my own (eg #1051), and would generally be interested in helping out with the effort! |
i thought about it, but i think we could actually close this PR if everyone is happy with it and then work on a new, seperate PR to actually make some changes to the code, aswell as add some things etc. maybe that would make it a bit cleaner. what do you think? |
That sounds good with me, although I'd wait to merge it in until we can get a 1.9.5 release out. Unless @yaxu objects, then I'm going to look into merging the build fixes (#1062) and then releasing 1.9.5 later this week! In the meantime, I think you're safe to treat this PR as approved and start a new branch off of this one for a future Stream PR |
Hi sorry I'm still away from things until next week, but had a quick look and can't see anything controversial so please go ahead @mindofmatthew ! |
Shall we merge this now? |
@yaxu Sounds great! Go ahead and do this before you do your branch tidying |
to make tidal-link and tidals clock functionality more widely useable (i.e. not dependend on types from tidal) i decided to reimplement the Tempo.hs module in a new module Sound.Tidal.Clock, that is part of tidal-link. The functionality is still the same, but i rewrote the code to make the clock independed of tidal and added an API so it can be used more easily.
i also restructured the code into a Clock monad, which is essentialy just the IO monad with some read-only memory for the configuration and mutable state, imo this makes the code much easier to comprehend and maintain.
i also rewrote some of the Stream and Transition modules to use the Clock instead of Tempo.hs, here i tried to make as little changes as possible, most of the code is exactly the same. the most notable changes are
streamReplace
:updatePattern
is no longer handled by the clock, it was only that way to tag the patterns with the time of evaluation, this time can be gotten from the clock through the APIstreamFirst
:onSingleTick
is also no longer handled by the clock, the zeroedLinkOperations needed for the action can also be gotten through the clock APItransiton
: also this function is no longer handled directly by the clockthe next step would be to restructure the Stream module, possibly splitting it into a couple of modules, maybe also rewriting Stream as an IO based monad.
what do you think @yaxu @Zalastax @mindofmatthew ?