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

Update portsmf + some MIDI loading updates #5653

Merged
merged 2 commits into from
Sep 13, 2020
Merged

Conversation

Veratil
Copy link
Contributor

@Veratil Veratil commented Aug 23, 2020

This updates portsmf to the latest commit from sourceforge, r234. I downloaded the snapshot and copied all the files we have over. A lot of changes here are function parameters adding const.

With the portsmf update brought an error when trying to use our Alg_seq constructor with stringstream. The constructor takes a std::istream which I tried to convert the stringstream to stringistream, but everything kept yelling that a constructor couldn't be found (during runtime). So I converted it to use ifstream, and everything calmed down and started working again. I've tested loading about 10 MIDIs I have saved for testing purposes, and all seemed to load without crashing or missing tracks (maybe) after the next change below.

I converted the static 256 count array of smfMidiChannel to an unordered_map. This is pretty much a drop in replacement as its operator[] will create a new element if none exists. This change has let me load previously unloadable MIDI files (found in Discord from others reporting it won't load). There are still some fixes to do, such as making sure all tracks are actually loaded. I find some MIDIs will load and have extra tracks created, but no data. I'm not certain if this is an error or a result of looping through a track that doesn't exist in the MIDI.

Finally, I added the seqnames attribute check since I've noticed before that track names won't load on some that use that attribute. This check is pretty much the same that's used in the allegro code.

@LmmsBot
Copy link

LmmsBot commented Aug 23, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8696-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.696-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8696?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8694-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.696-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8694?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8698-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.696-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8698?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/s3gdjpin3517s5ho/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35172960"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/l64w1v6ehg75kc80/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35172960"}]}, "commit_sha": "737f652cca046ecffba2febf32f15937eeef1751"}

@Veratil Veratil force-pushed the midi-fixes branch 6 times, most recently from 21045bb to 25815a5 Compare August 23, 2020 23:36
@PhysSong
Copy link
Member

You missed a for loop at the end which still uses 256.

@Veratil Veratil requested a review from PhysSong August 30, 2020 15:16
@PhysSong
Copy link
Member

PhysSong commented Sep 5, 2020

I saw that MidiImport::tryImport() references chs[9] at the end. This should probably be changed to take mod 16 for all existing channels.
The other part of MidiImport change looks good.

portsmf fixes:
* Fix allegro warnings as errors
* Fix msvc missing max. Use MAX macro 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

Successfully merging this pull request may close these issues.

3 participants