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 Ableton Link support (re-rebased) #4331

Merged
merged 32 commits into from Mar 4, 2019
Merged

Add Ableton Link support (re-rebased) #4331

merged 32 commits into from Mar 4, 2019

Conversation

@jamshark70
Copy link
Contributor

@jamshark70 jamshark70 commented Feb 24, 2019

Purpose and Motivation

Adds Ableton Link support into sclang.

This is the third version of this PR.

  1. #3351 was @smiarx Maxime Sirbu's first implementation.

  2. #4081 rebased #3351 against a development branch that was current as of September 2018 (also archiving long discussion).

  3. This PR adds a latency compensation parameter, makes the dependency-hook interface consistent with TempoClock, implements additional hooks (start/stop synchronization), updates help and unit tests. Also, the 50+ commits are now squashed down to 4 and rebased against February 2019's development branch.

(I squashed to 4 commits to note some milestones -- especially to keep a record of the current Link submodule version -- and also to keep Maxime's name in the commit history. He deserves most of the credit; I just cleaned up some things.)

Types of changes

  • Documentation
  • New feature

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review
smiarx and others added 4 commits Nov 5, 2017
added Ableton/link submodule

included Ableton::Link library in cmake for sclang

basic launch of link in TempoClock

link beat & tempo synchronisation

use link::requestBeatAtTime when beats is changed

notify mCondition when link starts

switch back correctly when link is disabled

add numPeers function to TempoClock

use beatsPerBar as quantum

Link tempo change callback function in sclang

Make LinkClock as a subclass of TempoClock

Match spacing convention

Make linkTimeOfInitialization static

Add *newFromTempoClock to LinkClock

Create a new LinkClock object which starts with the same tempo, beats
and seconds properties as a TempoClockObject.
It also stops the TempoClock and reschedules its tasks, creating a
seamless transition between the two clocks.

LinkClock: notify condition when SetAll

beatsPerBar_ sets Link quantum

Update clock in PauseStream when awoken

Each new call to the awake method of a PauseStream will update its clock
property. We do not need to update stream.beats, because stream is a
Routine and its beat value is updated after each call to next

Moving LinkClock into a separate compilation unit

Add CMake option SC_ABLETON fot link support

Fix typo in SC_ABLETON option definition

Move LinkClock into External/Ableton/

Update link submodule to lastest commit

solve issue with windows build

Match style guidelines

Remove unnecessary guarded functions

Properly handle exception in LinkClock constructor

Make TempoClock members private

Rename SC_Clock.h to match conventions

Remove prTempoChanged symbol optimization

Set mTempo and mBeatDur directly

mTempo & mBeatDur were only changed during the tempo callback of link,
so there was a slight delay when changing tempo.
Now the values are also changed directly during primivive calls.

Use thisThread.clock in EventStreamPlayer.play

Instead of refering to the instance var clock. Prevent error when
another clock awakes the player.

Add UnitTest for LinkClock

Check if LinkClock is running in primitives

Add basic help file for LinkClock
lang: LinkClock: Add latency compensation parameter

lang: LinkClock SetQuantum primitive throws Wrong Type error

We have a specific error code for wrong type; we should use it
instead of the generic `errFailed`.

lang: Change LinkClock:SetTempoAt*() to wake up the clock

TempoClock SetTempo functions do `mCondition.notify_one()` to
refresh timing on tempo change. LinkClock should do the same.

lang: Set-tempo callback doesn't need to commit the timeline

We are only reading the timeline, no need to commit with no changes
classlib: Notify dependants if LinkClock tempo changes externally

lang/classlib: Ableton Link: Provide start/stop callbacks

classlib: LinkClock:setMeterAtBeat should leverage 'super'

LinkClock:setMeterAtBeat missed some of the TempoClock:setMeterAtBeat
logic. It should only handle the Link quantum and then forward
to 'super' to be sure everything gets done.

classlib: LinkClock onStart/onStop hooks receive 'this' as an argument

help: Update LinkClock help with new methods and latency handling

lang/classlib: LinkClock: Add 'numPeers' notification

Help: Add section on LinkClock notifications, plus GUI example

classlib: LinkClock: Remove callbacks, use 'changed' consistently

Help: LinkClock: Remove callbacks, clarify notifications, start/stop example

Testsuite: TestLinkClock: Replace tempo callback with notifications

Testsuite: TestLinkClock: Fix stream-player reschedule bug

The pattern needs to wake up more often. As originally written,
'awake' is never called on the stream player so it never
registers that the clock changed.

Testsuite: Add missing whitespace before closing brace (TestLinkClock)

testsuite: TestLinkClock: Comments for Ableton's test plan; rename methods

- "Test Plan - TEMPO-1" is not descriptive. Add Ableton's explanation
  of each test.
- Test method names now conform to guidelines.
@brianlheim
Copy link
Member

@brianlheim brianlheim commented Feb 24, 2019

(I squashed to 4 commits to note some milestones -- especially to keep a record of the current Link submodule version -- and also to keep Maxime's name in the commit history. He deserves most of the credit; I just cleaned up some things.)

FYI https://help.github.com/en/articles/creating-a-commit-with-multiple-authors

@brianlheim brianlheim self-requested a review Feb 24, 2019
HelpSource/Classes/LinkClock.schelp Outdated Show resolved Hide resolved
HelpSource/Classes/LinkClock.schelp Show resolved Hide resolved
SCClassLibrary/External/Ableton/LinkClock.sc Outdated Show resolved Hide resolved
SCClassLibrary/External/Ableton/LinkClock.sc Outdated Show resolved Hide resolved
lang/LangPrimSource/SC_Clock.hpp Outdated Show resolved Hide resolved
lang/LangPrimSource/SC_Clock.hpp Outdated Show resolved Hide resolved
lang/LangPrimSource/SC_Clock.hpp Outdated Show resolved Hide resolved
lang/LangPrimSource/SC_Clock.hpp Outdated Show resolved Hide resolved
testsuite/classlibrary/TestLinkClock.sc Outdated Show resolved Hide resolved
@brianlheim
Copy link
Member

@brianlheim brianlheim commented Feb 25, 2019

(Bear with me, commenting on these is how I know which ones I did and didn't do yet.)

@jamshark70 no worries, that's a good process. btw, at work sometimes we leave a 👍 reaction on a review comment to signal it's been dealt with. if you need/want any help on this just let me know (preferably in a comment on the thread and not on one of the review comments). I'd be happy to make some of these changes myself

@jamshark70
Copy link
Contributor Author

@jamshark70 jamshark70 commented Feb 25, 2019

OK, I've pushed most of the changes to SC_Ableton_Link.cpp.

There are 30+ review comments. It's going to take a day or two to get through all of them.

I'm not very confident in C++, so I left two for others:

  • Moving linkTimeOfInitialization into a static member variable.
  • "A line or two of documentation."

I'll continue later today.

@colinsullivan
Copy link
Contributor

@colinsullivan colinsullivan commented Feb 25, 2019

* Moving linkTimeOfInitialization into a static member variable.

* "A line or two of documentation."

I should be able to pitch in this week.

@jamshark70
Copy link
Contributor Author

@jamshark70 jamshark70 commented Feb 26, 2019

OK, I think I got them all except for the two points I mentioned yesterday (moving the Link initialization time variable, and a comment on the class).

Aside: Having TestLinkClock and TestTempoClock was very helpful -- at several stages, I built the C++ changes and ran the tests, just to be sure.

@jamshark70 jamshark70 force-pushed the topic/LinkLatencyDev branch from 2ef0cd8 to d7e9398 Feb 26, 2019
…cClass

Moved definition of LinkClock into hpp, use static methods for init
@brianlheim
Copy link
Member

@brianlheim brianlheim commented Mar 2, 2019

Looks like the Appveyor build failures are due to an update in MSVC on Appveyor's end: https://www.appveyor.com/updates/, which for some reason causes htonll to be no longer defined (hmmmmmmm). In the interest of getting this in, I am going to do the following:

  • switch to an older build image on AppVeyor on a separate branch
  • push the rest of my changes as commits on the same branch, and create a PR against this one

this is only delaying the problem until the next AppVeyor platform update. after this is merged, I will make sure to determine the real cause of this situation. it may be that we need to define this function ourselves past a certain version of MSVC. without too much speculation, i think it may be that we're asking for an earlier version of Windows compatibility than this function is allowed to have (https://docs.microsoft.com/en-us/windows/desktop/api/winsock2/nf-winsock2-htonll), and only recently was that oversight rectified on MS's side. not sure

brianlheim added 7 commits Mar 2, 2019
Ableton Link: final review changes
@brianlheim brianlheim merged commit 63783ae into develop Mar 4, 2019
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@brianlheim brianlheim deleted the topic/LinkLatencyDev branch Mar 4, 2019
@brianlheim
Copy link
Member

@brianlheim brianlheim commented Mar 4, 2019

thanks everyone for their collaboration on this!

@jamshark70
Copy link
Contributor Author

@jamshark70 jamshark70 commented Mar 5, 2019

After code formatting is done, I think I should add a note to the LinkClock helpfile advising users to open port 20808 in their firewall. I couldn't get it working for days until I remembered that I'm running ufw on my machine and it was blocking the traffic.

@nhthn nhthn added this to the 3.11 milestone Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants