helgoboss / reaper-rs

Rust bindings for the REAPER C++ API
MIT License
78 stars 8 forks source link

What is state of MIDI take? State of API and what to use right now? #64

Closed Levitanus closed 1 year ago

Levitanus commented 1 year ago

I'm not sure of how things are going with MIDI, especially with MIDI-Take right now. From the one side — I see no clean «that is MIDI» API in medium-level. From the other side, reaper-high builds some docs for helgoboss-midi which look from the first side like a deep dive into the API.

And, if with reaper API itself for the current moment, I've taken a strategy of:

But the question of working with MIDI-Take is not small: last time I solved this problem, it took about of a week.

It's not so "technical", as major of functionality of reaper functions. And I can build some code, that would be useless for the community.

And I have feeling, that the problem is already solved, but I can not realize, where and how. Especially, I'm interested of midi-source audio accessor, which sounds for me, came from Python, a bit strange...

In one phrase — how to be with MIDI now?

helgoboss commented 1 year ago

In general, I add something to reaper-rs whenever I need something new for ReaLearn/Playtime (I don't add anything which I don't need because then it would be untested). Usually, I add it to the medium-level API first with an eye on quality and consistency, and then build a more sloppy high-level API on top of it.

Concerning MIDI: Medium-level API supports enumerating incoming MIDI device messages from an audio hook (Reaper::get_midi_input), sending MIDI to hardware devices from an audio hook (Reaper::get_midi_output), reading and providing MIDI from PCM sources in real-time (PcmSourceTransfer) and whatever MIDI-related function you can find on the Reaper struct.

Looking at MIDI PCM sources and enumerating the contained MIDI events is not something I've needed so far, so you would need to implement it on your own.

Levitanus commented 1 year ago

OK, I'll loot at it next days and try to make PR. If You have time, please, check my previous two, I want to have feedback if I need to change approach for something.

Levitanus commented 1 year ago

Hmm. I've checked current types, and, definitely, there is need for a separate struct for MIDI event in take source. But it also holds standard midi message inside. Example from reapy:

class MIDIEventDict(te.TypedDict):
    ppq: float
    selected: bool
    muted: bool
    cc_shape: CCShapeFlag
    buf: ty.List[int]

And I'm thinking on difference between already implemented midi message and midi message from Take.

What is bothering me — that in midi Take there are a lot of big messages, like Lyrics, Text and Notations (which I make in 100-200 character in length). I don't feel an array as a good type to hold these. But consider vector instead.

Maybe there is a point of making Trait instead of Struct? And implement it for two structs: real-time with arrays and got from take with vectors?

Levitanus commented 1 year ago

OK, since I really want to get into this functions as short as possible, I will not touch yet /medium/midi.rs, but, instead will build draft module /medium/source_midi.rs. Then we can take a brief look at both and decide how the final implementation should look like.

helgoboss commented 1 year ago

Yes, it's better. It'll probably take a while until I can take a thorough look at the take stuff.

Levitanus commented 1 year ago

I was too confused by attempt of making iterator from raw Vec<u8>. So, at first came back to try native Reaper enumerations (e.g. MIDI_GetCC, MIDI_SetCC etc.). Still there are some concerns about of underlying types for midi messages. For the moment I decieded to use just U4 and U7 from helgoboss::midi:

#[derive(Debug)]
pub struct SourceMidiEvent<T> {
    position_in_ppq: PositionInPpq,
    is_selected: bool,
    is_muted: bool,
    event: T,
}
impl<T> SourceMidiEvent<T> {
    pub fn new(position_in_ppq: PositionInPpq, selected: bool, muted: bool, event: T) -> Self {
        Self {
            position_in_ppq,
            is_selected: selected,
            is_muted: muted,
            event,
        }
    }
}

#[derive(Debug)]
pub struct CcEvent {
    pub channel_message: U4,
    pub channel: U4,
    pub cc_num: U7,
    pub value: U7,

Going to make first tests. As I know, in every scripted API (at least, lua and Python) there were a lot of problems using these enumerations. That's why we've made custom iterators on the raw midi source data.

helgoboss commented 1 year ago

Yes, if you want this to end up in medium-reaper, please use helgoboss-midi types.

You could have a look at reaper_high::Project::tracks() to see how I turn a typical REAPER API enum function into an iterator. I would do the same for iterating over events of a MIDI source. Performance-wise, that's better than putting everything into a vec first because it's done on the fly and you can stop iterating at any time.

Levitanus commented 1 year ago

After a couple of iterations, I've managed how to get size of midi message: call the function twice, if this is not regular 3-byte message.

But, also, I have profiled arbitrary "FFI" MIDI_GetEvt and MIDI_GetAllEvts. And, yeah, arbitrary single call is already an result of iteration. So, there is no real sense of having these functions in medium-level API, but make an iteration in rust instead.

Profiling results:

[2022-11-12T17:05:36Z DEBUG rea_score_test] SourceMidiEvent { position_in_ppq: PositionInPpq(1200.0), is_selected: false, is_muted: false, event: GenericMessage { size: 3, message: [144, 60, 103] } }
[2022-11-12T17:05:38Z DEBUG rea_score_test] passed with single-access: Some(1.377263539s)
[2022-11-12T17:05:38Z DEBUG rea_score_test] [240, 0, 0, 0, 0, 3, 0, 0, 0, 144, 60, 96, 240, 0, 0, 0, 0, 3, 0, 0, 0, 128, 60, 0, 240, 0, 0, 0, 0, 3, 0, 0, 0, 144, 62, 96, 240, 0, 0, 0, 0, 3, 0, 0, 0, 128, 62, 0, 240, 0, 0, 0, 0, 3, 0, 0, 0, 144, 60, 103, 240, 0, 0, 0, 0, 3, 0, 0, 0, 128, 60, 0, 240, 0, 0, 0, 0, 3, 0, 0, 0, 144, 62, 96, 0, 0, 0, 0, 0, 38, 0, 0, 0, 255, 15, 78, 79, 84, 69, 32, 48, 32, 54, 50, 32, 97, 114, 116, 105, 99, 117, 108, 97, 116, 105, 111, 110, 32, 115, 116, 97, 99, 99, 97, 116, 105, 115, 115, 105, 109, 111, 240, 0, 0, 0, 0, 3, 0, 0, 0, 128, 62, 0, 224, 1, 0, 0, 32, 3, 0, 0, 0, 176, 1, 108, 160, 5, 0, 0, 0, 3, 0, 0, 0, 176, 123, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[2022-11-12T17:05:38Z DEBUG rea_score_test] 240
[2022-11-12T17:05:38Z DEBUG rea_score_test] passed with collection-access: Some(132.552726ms)

source:

let event = rpr.midi_get_evt(take, 4).expect("should take event");
log::debug!("{:?}", event);
let now = Instant::now();
for _ in 0..(10_i32.pow(6)) {
    let idx = rand::random::<i32>().abs() % 9;
    let event = rpr
        .midi_get_evt(take, idx as u32)
        .expect(&format!("should take event, idx={:?}", idx));
}
log::debug!(
    "passed with single-access: {:?}",
    Instant::now().checked_duration_since(now)
);
// 
let events = rpr
    .midi_get_all_events_raw(take, 500)
    .expect("should be vector of msgs");
let event = events.get(4 * 12).unwrap();
log::debug!("{:?}", events);
log::debug!("{:?}", event);
let now = Instant::now();
for _ in 0..(10_i32.pow(5)) {
    let events = rpr
        .midi_get_all_events_raw(take, 200)
        .expect("should be vector of msgs");
    let idx = rand::random::<i32>().abs() % 9;
    let event = events.get(idx as usize).unwrap();
}
log::debug!(
    "passed with collection-access: {:?}",
    Instant::now().checked_duration_since(now)
);
Levitanus commented 1 year ago

And, I'm sorry for asking in this issue, but I feel separate will be too much for this question.

Now, when medium-level API covers about of 30% of low-level (which I found a bit of optimistic ;) ), the reaper.rs module is already of 8 000 lines in length... So, when the whole API covered, it would be about of 25 000 lines... My VSCode on my working station is already thinking a lot during the navigation... Maybe there is a point of making of some refactoring?

Intuitively, I would separate medium-level Reaper functions in modules with, probably, one global plugincontext or something like this.

What do you think about this?

helgoboss commented 1 year ago

Last time I checked it was more like 13%, not 30%. That's also the number I mentioned in the README.

Mmh, medium-level API is intended to be 1:1 with the original REAPER function list. I want to keep it that way. Separation of functions into different groups/structs is usually a subjective matter, so not something that fits to the medium-level API philosophy. Also, having to think (and agree with other contributors) where a function goes before you port it to that API is takes time that could better be spent elsewhere. I actually think, the reason why REAPER's API exists and is so comprehensive is also due to the fact that they don't have to think of perfect API design before adding new functionality. I like that pragmatism.

Concerning performance of the IDE, large files really shouldn't be a problem for a good IDE. Especially if they are as simple as a flat list of functions. Maybe you've set up something wrong. I can't really help in this regard because I use IntellJ Rust, there it's fast at least. Also, I think it wouldn't make a difference. I mean, look at the low-level Reaper struct! It's ten times larger. And you don't get rid of that simply by slicing up the medium-level API.

However, what you suggest is exactly what I was intending for higher-level APIs (reasonable grouping of functions, improving things, fixing quirks, ...).

Levitanus commented 1 year ago

I consider reaper_plugin_functions.h as an autogenerated file, that does not represent the structure of the Reaper project itself.. As well, as I understand, the major part of the low-level API of your workspace.

But medium-level is something, that should be human-made and human-readable. Even if not talking of speed of IDE navigation, with every 1000 lines of code it will raise the threshold of getting into project, or modification after a brake. It seems to me, that this type of overhead will then take more resources, than navigating through modules. For example, I hardly can find a function I'm working on, when it does not produce error\warning message.

For the moment, I don't see a big issue in semantics of separation, as API is already quite separated by naming conventions.

There're MIDI_* functions, *Proj* functions, *MediaTrack* etc.

So... Maybe, there is a point in considering of some separation. And from the user-view with autocomplete it still can be just a function list, being reexported by the parent module.

helgoboss commented 1 year ago

with every 1000 lines of code it will raise the threshold of getting into project, or modification after a brake. It seems to me, that this type of overhead will then take more resources, than navigating through modules. For example, I hardly can find a function I'm working on, when it does not produce error\warning message.

Can you elaborate? Not sure I understood that.

I don't know if you read the Rust docs of the reaper-medium crate. They go into utmost detail why I have designed it the way it is designed. If not, I encourage you to read it.

TL;DR The medium-level API should reflect the original API as much as possible, that's simply how I defined it. I defined it that way so that people who know ReaScript or the C++ API feel right at home and find stuff quickly. And another reason why I defined it that way, is to avoid exactly this kind of discussion about how things are done ... well, that didn't quite work out ;)

Look, the original REAPER C++ SDK has the same "issue" (which I find to be a non-issue): Every function is top-level, no modularization. The medium API simply inherits that feature.

I can understand your thinking. I also like a good API, nicely modularized. REAPER's original API is not a beauty in terms of API design, even Justin admits that. That's exactly why I - right from the start - envisioned a higher-level API on top of that which has all the freedom to make things right, not just in terms of modularization.

If VSCode has a really bad time with the API and it's clear that this is because of the sheer size of the Reaper struct, then something needs to be done, I agree. But even then, I think it should be done on IDE side. It doesn't have to be slow, as JetBrains IDEs clearly show. I use this API a lot, in really large extensions such as ReaLearn and the Clip Engine and I never felt it's an issue.

So... Maybe, there is a point in considering of some separation. And from the user-view with autocomplete it still can be just a function list, being reexported by the parent module.

They can't just be top-level module functions. All of these functions need context (the REAPER function pointers), so they should go into a struct. If you would want to modularize, you would need to create a struct for each module which borrows low::Reaper.

Levitanus commented 1 year ago

Can you elaborate? Not sure I understood that.

When you can not get what is happening in the file, You can not work with it. Beside the Reaper struct there are other types and functions. Also, even with knowledge of what is being put inside the Reaper struct, it is already quite a task to find it. These CamelCase to snake_case translations, free order of functions etc. do not help to find, what you have just modified.

I can not imagine a situation, where scrolling through tens of thousands lines of code is more comfortable, than quick browsing through a dozen of files...

They can't just be top-level module functions.

We can, actually, profile, is there sensitive overhead in usage of global plugin context (like top-level Reaper::get()) in comparison to accessing is as struct field. If this is a matter of under 100ms in a million iterations — why not to use just global state?

As you can see above, some intuitive performance considerations can be not as they seem :)

helgoboss commented 1 year ago

Can you elaborate? Not sure I understood that.

When you can not get what is happening in the file, You can not work with it. Beside the Reaper struct there are other types and functions. Also, even with knowledge of what is being put inside the Reaper struct, it is already quite a task to find it. These CamelCase to snake_case translations, free order of functions etc. do not help to find, what you have just modified.

I'm quite in favour of putting all those extra structs in other files. I also wouldn't have a real problem with distributing the functions to multiple impl Reaper blocks in multiple files as it's not a breaking change for consumers ... although I don't find it very useful and therefore am reluctant to actually do it - unless you show that this simple change would indeed solve your VSCode performance issues. But splitting it into multiple modules, no. Breaking change, no significant benefit, sorry.

I can not imagine a situation, where scrolling through tens of thousands lines of code is more comfortable, than quick browsing through a dozen of files...

I'm pretty sure that's a matter of taste.

They can't just be top-level module functions.

We can, actually, profile, is there sensitive overhead in usage of global plugin context (like top-level Reaper::get()) in comparison to accessing is as struct field. If this is a matter of under 100ms in a million iterations — why not to use just global state?

As you can see above, some intuitive performance considerations can be not as they seem :)

Yes, with a static it can be done, but that would be a quiet major change and deviation of the crate philosophy (again, read the docs if you are interested in the rationale). I'm not worried about the performance of that solution (I use it always with a static in reaper-high). I just don't see the significant benefit of doing that.

To be honest, this discussion doesn't go into a direction I would like to further pursue. You have your views on this, I have mine. This issue is not a showstopper of any kind and there are so many things about reaper-rs that can be discussed/done, which are far more urgent than this. If you seek perfection, we would go on discussing like this for days. And I don't have time for that. reaper-rs is for me not an end in itself, it's my tool for writing ReaLearn and for this it works great so far.

Therefore, let's just leave it at that for now. And if you would like to see the function result structs in separate files, no problem.

Levitanus commented 1 year ago

I also wouldn't have a real problem with distributing the functions to multiple impl Reaper blocks in multiple files as it's not a breaking change for consumers

Is it really possible? :) Sounds good!

The problem I see, that «every in one place» is the common way of consuming. But it would be hard to maintain, e.g. write this biiig file. I've carefully red docs.

P.S. I'm sorry for focusing on this. I'm just looking to the perspective of holding everything, while wrapping the rest of the low-level API. I'm focusing now on this task as a target. And, as communication takes time, I'm trying to ask several questions forehead.

helgoboss commented 1 year ago

I also wouldn't have a real problem with distributing the functions to multiple impl Reaper blocks in multiple files as it's not a breaking change for consumers

Is it really possible? :) Sounds good!

Yes. Look at reaper.rs and reaper_simple.rs in reaper_high. Makes use of that Rust feature.

The problem I see, that «every in one place» is the common way of consuming. But it would be hard to maintain, e.g. write this biiig file. I've carefully red docs.

Why? You just look for an empty slot (the space between functions) and "slap" a new function there, done :D

Levitanus commented 1 year ago

The issue will be resolved with closing of #66