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

"Typo" in documentation for range of midi channel #42

Closed
soerensen3 opened this issue Nov 4, 2022 · 5 comments
Closed

"Typo" in documentation for range of midi channel #42

soerensen3 opened this issue Nov 4, 2022 · 5 comments

Comments

@soerensen3
Copy link

This is a bit of a smart ass "issue" but in "https://github.com/robbert-vdh/nih-plug/blob/master/src/midi.rs" the channel of NoteEvent (For messages that have a channel) is declared as:

    /// The note's channel, from 0 to 16.
    channel: u8,

It should probably read 0 to 15 (4-Bit) if it is zero based or 1-16. From my experience it is zero based.

https://www.cs.cmu.edu/~music/cmsip/readings/MIDI%20tutorial%20for%20programmers.html

@robbert-vdh
Copy link
Owner

It's 0..16. The to is exclusive here. I'll probably rewrite all of these comments at some point to use Rust's range syntax at some point.

@soerensen3
Copy link
Author

Thanks for the response! Should I close the issue?

@robbert-vdh
Copy link
Owner

Can keep it open, I'll need change this at some point. 'x to y' means that y is exclusive to most people (as opposed to 'x through y'), but since that's apparently still ambiguous it's probably best to just use Rust range syntax here.

@robbert-vdh
Copy link
Owner

I tried a couple things but all the alternatives just read worse. So instead I added a note on what from x to y means and I changed all floating point ranges to be in the standard [x, y] interval notation.

@soerensen3
Copy link
Author

Hi, thank you for the fixes. I hate to say that now it is wrong in another place :-/, because in the next line where the note value is defined, it says it goes from 0 to 127. But in this case it 127 should be included. If I may make a suggestion:

pub enum NoteEvent {
    /// A note on event, available on [`MidiConfig::Basic`] and up.
    NoteOn {
        timing: u32,
        /// A unique identifier for this note, if available. Using this to refer to a note is
        /// required when allowing overlapping voices for CLAP plugins.
        voice_id: Option<i32>,
        /// The note's channel, with values from 0 to 15 inclusive.
        channel: u8,
        /// The note's MIDI key number, with values from 0 to 127 inclusive.
        note: u8,
        /// The note's velocity, in the range `[0, 1]`. Some plugin APIs may allow higher precision
        /// than the 127 levels available in MIDI.
        velocity: f32,
    },
   //...
}

I think it is better to keep it simple and unambigious. Also it is might be better to keep it in one place, because if I look it up in the reference I will usually not look at the beginning of the file or read the full text. A reference is not meant to be read completely but to look up specific things. Also if you hover it in any IDE it will not show the explanation in the beginning.

If you agree I can try to make a PR if you like.

robbert-vdh added a commit that referenced this issue Nov 8, 2022
Apparently `from x to y` is ambiguous, so we now use the Rust `x..y`
syntax instead.
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

No branches or pull requests

2 participants