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

AUDIO: MT32: Update MT32 emulation code to mt32emu 2.7.0 #4265

Closed
wants to merge 1 commit into from

Conversation

lotharsm
Copy link
Member

@lotharsm lotharsm commented Sep 12, 2022

This patch (finally) updates the MT32 emulation code to mt32emu 2.7.0.

No warnings in GCC. I re-applied our patch for silencing MSVC warnings, but unfortunately, I can't test them right now.

Submitting this as PR since it touches all ports.

@@ -1,5 +1,5 @@
/* Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009 Dean Beeler, Jerome Fisher
* Copyright (C) 2011-2021 Dean Beeler, Jerome Fisher, Sergey V. Mikayev
* Copyright (C) 2011-2022 Dean Beeler, Jerome Fisher, Sergey V. Mikayev
Copy link
Member

@bluegr bluegr Sep 12, 2022

Choose a reason for hiding this comment

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

Since these changes produce a lot of noise, can you put them in a separate commit?


template <class Sample>
void produceOutput(Sample *outStream, const Sample *nonReverbLeft, const Sample *nonReverbRight, const Sample *reverbDryLeft, const Sample *reverbDryRight, const Sample *reverbWetLeft, const Sample *reverbWetRight, Bit32u outLength) {
if (outStream == nullptr) {
if (outStream == NULL) {
Copy link
Member

@bluegr bluegr Sep 12, 2022

Choose a reason for hiding this comment

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

Please don't replace nullptr (which is C++ style) with NULL (which is C style).

Check here why NULL was replaced with nullptr:
https://stackoverflow.com/questions/20509734/null-vs-nullptr-why-was-it-replaced

Copy link
Member Author

@lotharsm lotharsm Sep 13, 2022

Choose a reason for hiding this comment

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

I didn't do the modifications on my own - I took the code directly from mt32emu (as always)...

Copy link
Member Author

@lotharsm lotharsm Sep 13, 2022

Choose a reason for hiding this comment

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

I wonder what is the better approach here: modify the sources we import (and deviate from the original) or keeping the imported code as-is, "trusting" the original developers? I don't think they made this change without a reason though...

Copy link
Member

@eriktorbjorn eriktorbjorn Sep 13, 2022

Choose a reason for hiding this comment

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

Looks like upstreams always used NULL, and we changed it in af529f5

@lotharsm
Copy link
Member Author

lotharsm commented Sep 13, 2022

@lotharsm
Copy link
Member Author

lotharsm commented Sep 15, 2022

Yikes, I accidentally pushed the code to master via d94505d.

Should we revert this or is it acceptable as-is, including the "untouched" sources?

@lotharsm
Copy link
Member Author

lotharsm commented Sep 15, 2022

Closing, fixing this in-tree if necessary.

@lotharsm lotharsm closed this Sep 15, 2022
@lotharsm lotharsm deleted the mt32emu-update branch Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants