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

Extending the OSC API? #16

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Simon-L
Copy link

@Simon-L Simon-L commented Nov 8, 2022

Hello!

I had a try at adding some new commands to the OSC API and it turned out to be really simple, thank you for laying out all the code as neatly as it is!

New things are:

  • change sequence length, beat per measure, beat length and measures
  • add and remove notes, snapping at 64th notes

I'm opening a PR for anyone to test and as I wish to contribute but this is more of a discussion about how and why extending the API. What are your thoughts about that?

My initial idea was to make editing sequences while playing live using MIDI controllers or other interactions. I'm mostly working on making modules for VCV Rack although seq192 is exciting to use and experiment with as it is very lightweight.

Speaking of lightweight, as a bonus and a little Makefile exercise, I've made seq192 into a lib in a separate branch here. Would there be any interest in stripping it even more? I'm thinking even removing jack and gtk to have a very cool embeddable sequencer engine.

@jean-emmanuel
Copy link
Owner

jean-emmanuel commented Nov 8, 2022

Hey, thanks a lot for your contribution ! Here are a few remarks:

  • setting the sequence length using ticks looks a bit risky to me as there's no guarantee the ppqn won't change someday. Wouldn't it be better to allow specifying a length in decimal beat units (quarter notes or eighth notes) ?
  • in the same spirit, the edit actions could use beat units instead of ticks
  • constants such as SEQ_MODE_BEATS could be named SEQ_EDIT_MODE_BEATS, it looks more readable and consistent to me
  • perform.hpp at line 370 looks a bit out of place / doesn't do what its name suggests

@jean-emmanuel
Copy link
Owner

Regarding the other question, I just pushed some changes to allow building without jack and gtk (see readme).

@@ -475,7 +475,8 @@ void perform::osc_status( char* address, const char* path)
json += "\"queued\":" + std::to_string(m_seqs[nseq]->is_queued()) + ",";
json += "\"playing\":" + std::to_string(m_seqs[nseq]->get_playing()) + ",";
json += "\"timesPlayed\":" + std::to_string(m_seqs[nseq]->get_times_played()) + ",";
json += "\"recording\":" + std::to_string(m_seqs[nseq]->get_recording());
json += "\"recording\":" + std::to_string(m_seqs[nseq]->get_recording()) + ",";
json += "\"events\":" + m_seqs[nseq]->to_json();
Copy link
Owner

@jean-emmanuel jean-emmanuel Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's very likely going to exceed the maximum udp packet size and thus breaking osc_status.

Copy link
Author

@Simon-L Simon-L Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch and thanks! What was I thinking trying to do that... To be honest I pushed that only for testing and showing the purpose if there's any interest, I didn't realise it would get reviewed immediately.

I just made a quick test and an UDP payload around 60kB (liblo default max size being 64kB) can contain more than 3000 events, and notes are a single event with both note on and note off timestamps.
May I suggest to turn this into a per-sequence status instead of global so that only one sequence is dumped to JSON? That'd hold a 10 minutes long sequence at 4/4 120bpm averaged to 10 notes per bar.

Copy link
Owner

@jean-emmanuel jean-emmanuel Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got notified and thought it was addressed to me somehow, sorry, please do mess around with your branch :).

A per-sequence status would make sense I guess, I did a quick attempt to make it possible via /sequence/status in the osc_seq_status branch, it only required a tiny bit of refactoring and so far the limitation I see is that the possibility to target mutliple sequences with either row/col numbers or string patterns doesn't play well with the optional reply address argument that's currently implemented in /status, but I think this argument could be dropped or replaced with an editable/stored reply address.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue I mentioned is now fixed in the osc_seq_status branch (reply address can be specified or not as first argument of /sequence/status)

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.

2 participants