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

New DrMp3AudioImporter plugin using dr_mp3. #60

Closed
wants to merge 6 commits into from

Conversation

williamjcm
Copy link
Contributor

'Tis my first time contributing a plugin, so I might have made mistakes here and there. 😅

Review/feedback welcome!

@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #60 into master will decrease coverage by 0.09%.
The diff coverage is 78.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #60     +/-   ##
=========================================
- Coverage   90.72%   90.63%   -0.1%     
=========================================
  Files          47       49      +2     
  Lines        4205     4238     +33     
=========================================
+ Hits         3815     3841     +26     
- Misses        390      397      +7
Impacted Files Coverage Δ
...c/MagnumPlugins/DrMp3AudioImporter/DrMp3Importer.h 100% <100%> (ø)
...MagnumPlugins/DrMp3AudioImporter/DrMp3Importer.cpp 78.12% <78.12%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 335c792...6e362f0. Read the comment docs.

@mosra mosra added this to the 2019.0b milestone May 24, 2019
@mosra mosra self-assigned this May 24, 2019
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

I might have made mistakes here and there. 😅

Doesn't seem so, at all :) Code looks great. Could you enable this plugin in all of package/ci (except appveyor-rt.bat and travis-android-arm.sh because those don't have OpenAL) so we can see what the CI says?

Besides that, can you add the plugin to the lists in

  • doc/building-plugins.dox
  • doc/cmake-plugins.dox
  • doc/namespaces.dox (one @dir entry)

and, finally, to FindMagnumPlugins.cmake? Search for DrWav to see where it needs to be added, it's three places in that file.

@mosra
Copy link
Owner

mosra commented May 25, 2019

Merged as fe8b709. This was great, thank you a lot!

I have to admit I didn't do a hearing test -- I assume you did ;)

@mosra mosra closed this May 25, 2019
@williamjcm
Copy link
Contributor Author

I have to admit I didn't do a hearing test -- I assume you did ;)

Yeah, I did test with a bunch of audio files on my end. A game's OST, and a few loose files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants