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

MIDI Import feature ported to drumstick and new MIDI Export feature #3224

Closed
wants to merge 13 commits into from

Conversation

senseab
Copy link
Contributor

@senseab senseab commented Jan 9, 2017

Now I have changed origin portsmf of MIDI Import to drumstick
And with some new features.

Origin MidiImport plugin changed to SmfImport

# check for drumstick-file
PKG_CHECK_MODULES(DRUMSTICK_FILE REQUIRED drumstick-file>=1.1.0)
IF(NOT DRUMSTICK_FILE_FOUND)
MESSAGE(FATAL_ERROR "LMMS requires libdrumstick-file and drumstick-dev >= 1.0.11 - please install, remove CMakeCache.txt and try again!")
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the as of now unreleased Ubuntu 17.04 will have libdrumstick 1.0.2. Is there something in 1.0.11 that is absolutely required? (https://launchpad.net/ubuntu/+source/libdrumstick)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well,i was using Archlinux,drumstick may newer. i will fix it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it's typo

@Umcaruje
Copy link
Member

Umcaruje commented Jan 9, 2017

@tonychee7000 locale updates are done over transifex now, this should not be in the commit for other languages, just English. CC @liushuyu

@senseab
Copy link
Contributor Author

senseab commented Jan 9, 2017

Sorry, i would fix it

@Umcaruje
Copy link
Member

Umcaruje commented Jan 9, 2017

Also you'll need to add drumstick to packages needed for travis, and mingw packages will need to be made. What are the advantages of drumstick over our current implementation?

@senseab
Copy link
Contributor Author

senseab commented Jan 9, 2017

Reply @Umcaruje

  1. I have not understood travis yet, and I don't know how to make mingw packages. So it will be better if there were someone help.
  2. As you see, drumstick can read Overture and Cakewalk project. Then we can support more file types.
  3. Drumstick is Qt-based library for MIDI, using Signal/Slot mechanism.
  4. It would be easy to procceed with RPN Event like pitch bend range was changed. See MIDI import: Pitch bend range CCs ignored and wrong default #1967 and Add pitch range CC handling #3206

If any doubts, ask @liushuyu

@zonkmachine
Copy link
Member

MMS requires libdrumstick-file and drumstick-dev >= 1.0.2

Debian Jessie use drumstick 0.5.0

Changes for v1.0.2
...
Compilation minimum requirements for all platforms: CMake 3.0 and Qt 5.1 

We still use Qt 4 on Linux

@senseab
Copy link
Contributor Author

senseab commented Jan 9, 2017

is there any libdrumstick newer version at Debian ?if not, i will test it

@zonkmachine
Copy link
Member

is there any libdrumstick newer version at Debian ?

In experimental only. 1.0.2 https://packages.debian.org/search?suite=experimental&arch=any&searchon=names&keywords=drumstick
I don't know if Debian folks have a problem using packages from experimental.

QT 4 is about to get dropped anyway so that is probably not a big thing. Also I don't know if the libstick version in Debian is a problem. I only use Ubuntu based systems so I'm used to using ppa's. It's a thing with Debian to not update their stable releases so I guess they are cool with running an older lmms too.

@senseab
Copy link
Contributor Author

senseab commented Jan 9, 2017

Got that, I would say that Qt 5 might be more effective.
But I still wonder the lowest version for my codes, to get the most widely compatibility

@zonkmachine
Copy link
Member

But I still wonder the lowest version for my codes, to get the most widely compatibility

Then it looks like maybe 0.5.0 is the best candidate.

@senseab
Copy link
Contributor Author

senseab commented Jan 10, 2017

i will test it

@liushuyu
Copy link
Member

liushuyu commented Jan 10, 2017

I would like to say:

  1. This PR should be squashed into one commit, for the early commit messages were non-sense.
  2. Need more testing, for the changes affected many aspects
  3. Test drumstick package under Windows. Seems like drumstick is intended for Linux only, so we may need some workarounds to port or compile it for Windows (via MSYS or Cygwin, or just dig out drumstick-file components), Darwin, BSDs, Haiku, etc.
  4. For the sake of new comers, @tonychee7000 had expressed the hardness of finding "clues" of each and every APIs in LMMS, let's just try to leave more comments in our code. Not only for developers. A bunch of translators asked me about the context of messages in LMMS, I feel like we need to do something about this.

@liushuyu liushuyu force-pushed the port_to_drumstick branch 2 times, most recently from d67da79 to bfaaa46 Compare January 10, 2017 04:56
@senseab
Copy link
Contributor Author

senseab commented Jan 10, 2017

Tested on Debian with drumstick 0.5.0, works well.

@senseab
Copy link
Contributor Author

senseab commented Jan 10, 2017

@liushuyu It seems that I overwrite your commit for mistake. so could you repair it again?

* Added Cakewalk file format import support

* Added Overture file format import support

* Enhanced MIDI CC Message handling

* Merge liushuyu's commit.
@senseab
Copy link
Contributor Author

senseab commented Jan 26, 2017

Just a suggestion, you can create a new branch if necessary.

@ghost
Copy link

ghost commented Apr 9, 2017

Object::connect: No such signal ����::signalSMFTimeSig( int, int, int, int )
Object::connect: No such signal ����::signalSMFTempo( int )
Object::connect: No such signal ����::signalSMFText( int, QString )
Object::connect: No such signal ����::signalSMFError( QString )
Object::connect: No such signal ����::signalSMFCtlChange( int, int, int )
Object::connect: No such signal ����::signalSMFPitchBend( int, int )
Object::connect: No such signal ����::signalSMFNoteOn( int, int, int )
Object::connect: No such signal ����::signalSMFNoteOff( int, int, int )
Object::connect: No such signal ����::signalSMFProgram( int, int )
Object::connect: No such signal ����::signalSMFHeader( int, int, int )
Object::connect: No such signal ����::signalSMFTrackStart()

Just tried building the PR, and it looks like it segfaults when trying to import any MIDI file.

fi

cmake ../ -DUSE_WERROR=OFF -DLIB_SUFFIX='' -DSTATIC_DRUMSTICK=$STATIC_OPTS
make && sudo make install
Copy link
Member

Choose a reason for hiding this comment

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

This file is near identical to Linux. They should be consolidated. It can live in linux until this library is ported to homebrew.

@@ -0,0 +1,54 @@
drumstick_ver=$1
Copy link
Member

Choose a reason for hiding this comment

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

This can stay as-is, but this should probably be ported to https://github.com/LMMS/lmms/blob/master/cmake/msys/msys_helper.sh like @dragoneagle did in #3369 or alternately added directly to the mingw mirros that Toby maintains for us. Toby generally does this on-request.

I think first though we can pull Qt4 support, which is an open task, so we wouldn't hold up the merge for this script alone.

@tresf
Copy link
Member

tresf commented Apr 10, 2017

it segfaults when trying to import any MIDI file.

Seems to be similar to this issue... http://stackoverflow.com/a/14774274/3196753

@tonychee7000 may need to add the class name in front of the slots ::signalSMFTimeSig, etc.

I assume you're on the right build of drumstick? Qt5 builds against drumstick 1.1.x. Qt4 builds against drumstick 0.5.x.

@senseab
Copy link
Contributor Author

senseab commented Apr 11, 2017

@tonychee7000 may need to add the class name in front of the slots ::signalSMFTimeSig, etc.

I have added that class name above ::signal***, but it won't work with output

QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFTimeSig( int, int, int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:49
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFTempo( int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:52
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFText( int, QString ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:55
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFError( QString ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:58
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFCtlChange( int, int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:61
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFPitchBend( int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:64
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFNoteOn( int, int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:67
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFNoteOff( int, int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:70
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFProgram( int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:73
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFHeader( int, int, int ) in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:76
QObject::connect: No such signal drumstick::QSmf::drumstick::QSmf::signalSMFTrackStart() in /home/tonychyi/工程/lmms/plugins/SmfImport/midiReader.cpp:79

Here are duplicate drumstick::QSmf

Here is Archlinux

@ghost
Copy link

ghost commented Apr 11, 2017

I assume you're on the right build of drumstick? Qt5 builds against drumstick 1.1.x. Qt4 builds against drumstick 0.5.x.

Yep, building with Qt5 seems to have solved the issue.

On another note, perhaps the instrument used when exporting a MIDI could be determined with a dropdown on the instrument window, instead of forcing all instruments to be a sf2 player.

@Umcaruje Umcaruje added this to the 1.3.0 milestone May 14, 2017
@PhysSong
Copy link
Member

Now there are a bunch of merge conflicts. If this work still has some values, someone may redo this work.
@tonychee7000 Do you have any ideas/opinions what should we do?

@senseab
Copy link
Contributor Author

senseab commented May 7, 2019

Reply @PhysSong

Oops, several years.

Well, someone can redo this feature referring to my code or just check where conflict is and merge them by manual. Might a lot work to do.

In my code, MidiImport was replaced by SmfImport for drumstick supports Overture(.ove) and old Cakewalk(.wrk) project file. I just want a accurate name.

MidiExport was totally ported to drumstick. Abandon the current code might do.

A new branch might needed?

@PhysSong
Copy link
Member

PhysSong commented May 7, 2019

Well, there have been several changes:

There are some more changes. Anyway, it may be very difficult to resolve merge conflicts...

@senseab
Copy link
Contributor Author

senseab commented May 8, 2019

@PhysSong

  • MidiExport I haven't considered about B&B tracks or more, just export tracks which use sf2player. It might be redo.
  • I tested Qt5 and all codes was based by Qt5, just forget Qt4.
  • I have no idea about MSVC, even it worked on Travis at that time.

I will show what I have done on MidiExport, all based on drumstick.

  1. Call setFileFormat() and set MIDI file version to 1
  2. Call setDivision(960)
  3. Scan all tracks which use sf2player and call setTracks() + 1 on drumstick
  4. AutomationTrack was bonded to MIDI channel.
  5. Write drum track with MIDI channel 10.
  6. Write global event which had track 0
  7. List all notes and automations by order
  8. Calculate the time difference between 2 events.
  9. Write to some channel
  10. Write EndOfTrack event finally

Considering MidiExport on master has changed a lot, Redo might be an option.

@PhysSong
Copy link
Member

Considering MidiExport on master has changed a lot, Redo might be an option.

Some remarks in case someone wants to redo works in this PR:

  • The license of drumstick has been changed to GPL3+ in r464.
  • Overture support is removed in r437.

Note that those changes are in their trunk, but not in their latest release(r391).

@PhysSong PhysSong added the rework required Pull requests which must be reworked to make it able to merge or review label Jan 4, 2021
@PhysSong
Copy link
Member

Updates:

  • The latest Drumstick release is 2.7.2(as of 2022.11.12), which defaults to Qt 6(Qt 5 option is also available, but some distributions build it with Qt 6)
  • As of Drumstick 2.0, it's licensed under GPL3+ while LMMS is still under GPL2+

@zonkmachine
Copy link
Member

As of Drumstick 2.0, it's licensed under GPL3+ while LMMS is still under GPL2+

What does this mean practically?

@tresf
Copy link
Member

tresf commented May 7, 2024

What does this mean practically?

It just means we need to bump LMMS to GPL3+ in order to bundle it. Any dependencies that don't have the "+" in "GPL2+" would no longer be able to be bundled. I think we went over this very specifically in another thread, but my seaching was not fruitful. https://github.com/LMMS/lmms/issues?q=is%3Aissue+gpl3

@zonkmachine
Copy link
Member

It just means we need to bump LMMS to GPL3+ in order to bundle it. Any dependencies that don't have the "+" in "GPL2+" would no longer be able to be bundled. I think we went over this very specifically in another thread, but my seaching was not fruitful. https://github.com/LMMS/lmms/issues?q=is%3Aissue+gpl3

OK. So, if I understand this correctly, big if..., we wouldn't be able to ship it with the AppImage but for the rest we could link it to a package.

@tresf
Copy link
Member

tresf commented May 7, 2024

It just means we need to bump LMMS to GPL3+ in order to bundle it. Any dependencies that don't have the "+" in "GPL2+" would no longer be able to be bundled. I think we went over this very specifically in another thread, but my seaching was not fruitful. https://github.com/LMMS/lmms/issues?q=is%3Aissue+gpl3

OK. So, if I understand this correctly, big if..., we wouldn't be able to ship it with the AppImage but for the rest we could link it to a package.

When you say "wouldn't be able to ship", I'm not sure if you mean the GPL2 (without +) or GPL3 (before we upgrade), but I'll try to make it as simple as possible, based on my understanding of the license.

  • GPL2+ can upgrade to GPL3+ at any time. without a license change. You can just call the completed work GPL3+, however many will consider this misleading.
  • GPL3+ cannot downgrade to GPL2+.
  • GPL2 (without the +) cannot upgrade to GPL3+.
  • Regardless of this, our plugin system can fall into the "Standard Interface" clause of either GPL license.
    • This means that technically, as long as our plugin-specific code is compatible, we're OK.
    • This may cause confusion or be considered misleading if certain directories of the project were a different license, but this is technically allowed using an interface or plugin system.
    • At time of writing this, I do not believe we use a standard interface for the MIDI import, so this library specifically would require either a refactor to leverage the "Standard Interface" clause, or an upgrade to the project as a whole to comply with GPL3+.
  • Switching the project to GPL3+ is already actively being discussed for VST3 support, so this is probably the simplest and least confusing path. We just need to make sure we check each plugin and dependency for compatibility, and if exceptions exist, try to leverage the "Standard Interface" clause, or ask the author to relicense. Anything not using a plugin system (e.g. uses headers and linking and would crash LMMS if missing) is a hard-requirement to be GPL3/GPL3+ compatible or needs a significant refactor to be used as a standardized plugin.

With regards to the word "shipping"... the bundle of GPL3+ and non-GPL3+ components is referred to as "aggregate" in the license and seems to be allowed, although I've seen projects such as FoxIt leverage install-time downloaders (e.g. GhostScript) to avoid any potential legal issues.

@zonkmachine
Copy link
Member

With regards to the word "shipping".

I think I meant "bundle".

Thanks for the clarification. I'm removing this from the current milestone.

@zonkmachine zonkmachine modified the milestones: 1.3, 1.3+ May 9, 2024
@Rossmaxx
Copy link
Contributor

@Veratil didn't you rewrite midi import (and also plan to fix midi export) so is there anything you need here?

@Rossmaxx
Copy link
Contributor

I see way too much for this PR to be fixed up. If @senseab wants to continue (I assume otherwise because they seem to have forgotten), better to start from scratch. Will close in 2 days if no one complains

@Rossmaxx
Copy link
Contributor

Well, someone can redo this feature referring to my code or just check where conflict is and merge them by manual. Might a lot work to do.
In my code, MidiImport was replaced by SmfImport for drumstick supports Overture(.ove) and old Cakewalk(.wrk) project file. I just want a accurate name.
MidiExport was totally ported to drumstick. Abandon the current code might do.
A new branch might needed?

I see this as an indication for closing.

@Rossmaxx Rossmaxx closed this May 29, 2024
@Veratil
Copy link
Contributor

Veratil commented May 29, 2024

@Veratil didn't you rewrite midi import (and also plan to fix midi export) so is there anything you need here?

Yes, MIDI import is pretty much done locally (but I don't like the code in the current state it is). Export is not done yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rework required Pull requests which must be reworked to make it able to merge or review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants