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

[wip] Supporting more midi messages in MidiDrop plugin #1399

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

Conversation

smoothdeveloper
Copy link

This is wip of a first iteration on supporting a bit more of the midi messages in Score.

current scope

  • parsing
    • cc
    • pitchbend
  • ui
    • display enveloppes in the track
    • show notes with different colour per midi channel
  • playback
    • output the additional events

Guidance appreciated, including pointers for:

  • how to deal with the streaming of midi data on the output port of the track
  • how to deal with the UI, to add a display of enveloppes for cc and pitchbend data

};
struct MidiTrackEvent {
static MidiTrackEvent make_note_off(const double start, const midi_size_t ch, const midi_size_t n, const midi_size_t v){
return MidiTrackEvent{m_start: start, m_message: Midi::NoteOffData{channel: ch, note: n, velocity: v}};
Copy link
Member

Choose a reason for hiding this comment

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

 {m_start: start,

is not valid C++ syntax AFAIK, it should be

 .m_start = start

:)

Copy link
Author

Choose a reason for hiding this comment

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

image

Mmmh, ok, learning that I like GNU old-style (probably useful when C/C++ didn't agree yet on non positional saving lives in codebases terms) 🙂

How can I make sure my PR isn't going to add warnings?

@@ -18,11 +18,101 @@ class DropHandler final : public Process::ProcessDropHandler
const score::DocumentContext& ctx) const noexcept override;
};

struct PitchbendData{
Copy link
Member

Choose a reason for hiding this comment

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

MAybe all of these could go into a more general Midi.hpp ... or, I wonder if it wouldn't be best in libremidi, and then try to consume those directly ? not sure...

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I'll start moving it soon then, please let me know what makes most sense.

I was going with MidiTrackEvent as type name (which is the core object along MidiNote which has "pointers" to optional? pair), and considering a "domain" folder:

/domain/MidiNote.hpp moving the current one
/domain/MidiTrackEvent.hpp the detailed header
/domain/Midi.hpp a logical header, including several others, for convenience in other modules.

Do you like this?

@@ -65,11 +65,8 @@ void DropHandler::dropData(
const double ratio = song_t / actualDuration.msec();
if (ratio != 1.)
{
for (auto& note : track.notes)
{
Copy link
Member

Choose a reason for hiding this comment

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

👍

midi_size_t velocity;
};
struct MidiTrackEvent {
static MidiTrackEvent make_note_off(const double start, const midi_size_t ch, const midi_size_t n, const midi_size_t v){
Copy link
Member

Choose a reason for hiding this comment

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

these functions look a lot like the ones in libremidi, maybe it should go there ?

@jcelerier
Copy link
Member

hello ! just so you know - as this was requested in many places I added a very small separate "midi reader" object which will take a midi file explicitly and output every message, as well as a "midi filter" object which is able to filter a specific CC, pitch bend, after touch, etc... message. It's availble in the current nightlies

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