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

Split controller OSC off from SuperDirt handshake OSC (Redux for 1.9) #1051

Merged
merged 17 commits into from
Feb 5, 2025

Conversation

matthewkaney
Copy link
Collaborator

This cherry-picks the changes in #1027 into the 1.9-dev branch, since they aren't specifically related to 2.0 and would be nice to include in 1.9.5 (the most practical benefit is that this removes the "Tidal can't connect to SuperDirt without a control port" error state that occasionally scares new users).

Marking this as a draft until I test this locally and verify that everything works. This duplicates an already-merged PR, so it should be fine, but it would be awesome if anyone wants to give it a quick look over to make sure there's nothing questionable happening.

@matthewkaney matthewkaney changed the title Split osc code Split controller OSC off from SuperDirt handshake OSC (Redux for 1.9) Nov 29, 2023
@matthewkaney
Copy link
Collaborator Author

I'm holding this off until 1.10 so it can be incorporated into #1059

@matthewkaney matthewkaney marked this pull request as ready for review April 19, 2024 13:34
@matthewkaney
Copy link
Collaborator Author

Did some testing and it looks like this is all working on my end. To recap: SuperDirt handshakes/bus mappings are handled by individual targets, rather than by the stream. This means that each handshake occurs on a separate UDP port, and SuperDirt targets no longer use the Tidal control listener port for sending messages.

Fixes #1030 as well—a bug where the control bus mappings weren't actually being properly used to address bus messages (and therefore bus params wouldn't work if SuperDirt allocated a different block control buses than 0-n)

In general, this is part of a broader effort to separate out different pieces of stream functionality (ie, consolidate the listener logic, consolidate SuperDirt-specific features, etc). I know @polymorphicengine has been thinking along these lines, so let me know if you have any thoughts!

@polymorphicengine
Copy link
Collaborator

polymorphicengine commented Apr 21, 2024

i think this is looking good!
are you also planning to move into the direction of the extendable OSC controller? (#982)

i mean it is actually pretty much there, we only need to abstract over the act function inside ctrlResponder so people can extend it like

act' :: O.Message -> Stream -> IO ()
act' matchingMessage str = customAction str
act' m str = defaultAct m str

@matthewkaney
Copy link
Collaborator Author

matthewkaney commented Apr 21, 2024

are you also planning to move into the direction of the extendable OSC controller?

Yes, though I think in a separate PR. My (maybe controversial) thought is that the Stream object shouldn't own the controller/listener at all, since the listener is just a wrapper around functions that the Stream object exposes. Or else we move to a point where all of the relevant code is split across a bunch of small logical components (a clock, a listener, some targets, a playmap abstraction) and Stream is just a set of MVars that holds together a default (but entirely optional) Tidal setup.

@sss-create
Copy link
Collaborator

uh nice, @matthewkaney!

@yaxu
Copy link
Member

yaxu commented Feb 3, 2025

Ready for merge?

@ahihi
Copy link
Contributor

ahihi commented Feb 4, 2025

i did some testing on this just now, it looks good to me and solves my control bus woes 😍

@matthewkaney
Copy link
Collaborator Author

Oh great! I wasn't sure if everything still worked after the messy merge, but that's good to hear.

I'll test a bit this evening to see if I catch anything amiss in how it behaves and look into why the builds are failing. After that, this should be good to merge tomorrow morning!

@matthewkaney
Copy link
Collaborator Author

@yaxu Yes, I think that can be merged! I guess the latest build issue is some error with the Ormolu action, possibly related to the fact that this PR is from a fork rather than a branch on the main repository? Good to address at some point, but doesn't seem like it will be an issue here after merge

@yaxu yaxu merged commit 619aa91 into tidalcycles:dev Feb 5, 2025
@yaxu
Copy link
Member

yaxu commented Feb 5, 2025

Excellent thanks! I think the build issue was a small formatting thing that was already automatically fixed.

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