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

Integration with midly for full MIDI message parsing / support using "Owned" message variants #36

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

masonium
Copy link
Contributor

@masonium masonium commented Nov 19, 2023

Initial branch based on the discussion in #35 .

Docs are probably a little sparse; I wasn't sure what should be copied / re-worded vs. what isn't really necessary.

I went with OwnedLiveEvent and OwnedSystemCommand as anologues to the midly variants, but I'm definitely not wedded to those names. I do think it's better to have different names than simply e.g. LiveEvent; the user can differentiate with qualified paths (midly::LiveEvent vs. bevy_midi::LiveEvent) but that strikes me as clunky.

src/input.rs Show resolved Hide resolved
Copy link
Owner

@BlackPhlox BlackPhlox left a comment

Choose a reason for hiding this comment

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

Nice! Looking good!
As mentioned, I added a debug custom message for OwnedLiveEvent when NoteOn, NoteOff & AfterTouch.
Also fixed a bug in the piano example.
Haven't had time to check the input and output example yet, but I'll do that next 👍

src/types.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
examples/piano.rs Outdated Show resolved Hide resolved
@BlackPhlox
Copy link
Owner

I noticed that in the piano example, the function display_press translated the keys to an absolute value instead of a relative one, so we should do this instead:

fn display_press(mut query: Query<(&mut Transform, &Key), With<PressedKey>>) {
    for (mut t, k) in &mut query {
        if t.translation.y == k.y_reset{
            t.translation.y += -0.05;
        }
    }
}

masonium and others added 3 commits November 19, 2023 11:50
fix for piano example, better message debugging

Co-authored-by: Mikkel Rasmussen <theluja@gmail.com>
@masonium masonium requested a review from BlackPhlox November 19, 2023 20:22
Comment on lines +380 to +390
// let pitch = data.message.msg[1];
// let octave = pitch / 12;
// let key = KEY_RANGE[pitch as usize % 12];

// if data.message.is_note_on() {
// debug!("NoteOn: {}{:?} - Raw: {:?}", key, octave, data.message.msg);
// } else if data.message.is_note_off() {
// debug!("NoteOff: {}{:?} - Raw: {:?}", key, octave, data.message.msg);
// } else {
// debug!("Other: {:?}", data.message.msg);
// }
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// let pitch = data.message.msg[1];
// let octave = pitch / 12;
// let key = KEY_RANGE[pitch as usize % 12];
// if data.message.is_note_on() {
// debug!("NoteOn: {}{:?} - Raw: {:?}", key, octave, data.message.msg);
// } else if data.message.is_note_off() {
// debug!("NoteOff: {}{:?} - Raw: {:?}", key, octave, data.message.msg);
// } else {
// debug!("Other: {:?}", data.message.msg);
// }

@BlackPhlox
Copy link
Owner

BlackPhlox commented Nov 20, 2023

I noticed that in the piano example, the function display_press translated the keys to an absolute value instead of a relative one, so we should do this instead... #36 (comment)

Made a pr to your branch with some updates to the piano example along with the above-mentioned issue: masonium#1

Added Orthographic projection and made keypress relative in the piano example
@BlackPhlox BlackPhlox mentioned this pull request May 9, 2024
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