Enabling users to set the soundfont on midi playback #2274

AlexanderGroeger · 2023-06-27T16:27:43Z

This PR adds a new set_soundfont function to the pygame.mixer.music module. This new feature allows game developers to leverage to pay a lower cost for storing music in their game by using smaller midi-formatted music tied with any soundfont they choose.

Technical Notice
This PR relies on SDL_mixer's Mix_SetSoundFonts function. At the initial commit, tests showed that setting the soundfont before loading and playing a MID file did not yield using the specified soundfont but instead fell back on the system default soundfont. This needs to be addressed before a merge will contribute anything.

UPDATE
A new get_soundfont function was added to pygame.mixer.music to verify whether SDL_mixer's internal soundfonts variable was correctly being updated. It turns out both functions work correctly, but there is still something in the SDL_mixer that is not immediately using the updated soundfont variable.

UPDATE 2
It appears the feature works on Raspbian, but not Windows. This means the compilation of SDL_Mixer needs to be considered in the compilation of pygame-ce because it is not compiled to support soundfonts for all operation systems. At this time, the only known working platform is Raspbian 64-bit.

UPDATE 3
set_soundfont now allows passing an empty string or no argument to clear the soundfont_paths variable in the SDL_Mixer. A stub has been added for both functions. I didn't see a MIDI used in the tests, so I wasn't sure if one existed, so both tests are left as todo tests. The docs now reflect this update too.

UPDATE 4
The todo tests have now been replaced with real tests that validate setting and getting soundfont paths will yield expected behavior.

UPDATE 5
More quick fixes. Some extra testing has been reported.
@oddbookworm and @rethanon have done some testing. On main (for both manjaro linux and mac), loading a midi yields a pygame error Couldn't open timidity.cfg which is the alternative to fluidsynth. Installing timidity with yay removes the error, but does not produce music. On this PR, (for both manjaro linux and mac), setting the soundfont before loading a midi works as intended. It seems Windows is still an issue.

UPDATE 6
Moved API to pygame.mixer, fixed some typos, and allowed set_soundfont to accept a None argument.

…ndfonts per platform.

Starbuck5 · 2023-06-29T03:14:32Z

For the two failing runs you asked about clarification about:

The code needs to be linted. Run python setup.py format after installing clang-format and black.
Type stubs need to be added for this new API. See buildconfig/stubs/pygame/

src_c/music.c

docs/reST/ref/music.rst

Starbuck5 · 2023-06-29T03:35:42Z

As well as the type stubs the CI complained about, this PR also needs an entry into the pygame-ce test suite. See the tests/ directory.

…test, updated docs

docs/reST/ref/music.rst

test/mixer_music_test.py

ankith26

First of all, thanks for contributing to pygame-ce! 🍰

Left a couple of typing related reviews. Also I'd like to discuss the API a bit more? Should we go with SDL's semicolon-separated style? I wouldn't mind making the API more pythonic and having the users deal with sequences of strings

buildconfig/stubs/pygame/mixer_music.pyi

Co-authored-by: Ankith <46915066+ankith26@users.noreply.github.com>

Starbuck5 · 2023-07-11T08:26:32Z

Question: why is this in mixer.music? Does this not apply to midi added as sounds (I'm on Windows so I can't test)? If it applies to all midi I'd think it should go in the mixer music.

AlexanderGroeger · 2023-07-11T12:43:48Z

Question: why is this in mixer.music? Does this not apply to midi added as sounds (I'm on Windows so I can't test)? If it applies to all midi I'd think it should go in the mixer music.

I'm not sure what you're asking. It is in mixer.music, and you're suggesting it should go in mixer music....

oddbookworm · 2023-07-11T12:52:34Z

Starbuck5 is probably referring to pygame.midi

Starbuck5 · 2023-07-11T16:28:41Z

Sorry I mistyped. I meant why does it not go in mixer.

AlexanderGroeger · 2023-07-11T20:47:17Z

I hadn't considered this. I tested mixer.Sound with the soundfont selected and confirmed it works on Raspbian. I can move the new API over. Which would be better to move to? pygame.mixer or pygame.midi.

src_c/mixer.c

oddbookworm

Overall, looks good. I just have a few (mostly minor) nitpicks

src_c/doc/music_doc.h

docs/reST/ref/mixer.rst

docs/reST/ref/music.rst

src_c/mixer.c

test/mixer_test.py

src_c/_pygame.h

src_c/mixer.c

docs/reST/ref/mixer.rst

AlexanderGroeger · 2023-07-26T16:19:18Z

One final note here. Perhaps we can learn something from this issue to get soundfonts working on windows.

AlexanderGroeger · 2023-07-28T11:49:31Z

I tried including fluidsynth in the windows build config but didn't have the compiling experience or intuition to make the SDL_Mixer use soundfonts on windows, so I'd prefer to wrap up this PR and make an issue regarding this.

docs/reST/ref/mixer.rst

Co-authored-by: Ankith <46915066+ankith26@users.noreply.github.com>

ankith26

LGTM, thanks for the PR 🎉

PS: to the person merging this, please squash and merge this PR (you may remove any extra co-author tags that github adds)

Starbuck5

Nice work getting this up to be fully ready!

Changes addressed.

added pygame.mixer.music.set_soundfont

29684ec

AlexanderGroeger requested a review from a team as a code owner June 27, 2023 16:27

AlexanderGroeger added 3 commits June 27, 2023 13:11

added pygame.mixer.music.get_soundfont

6b9d72c

Changed version number to 2.3.1

71e9d1e

updated docs to reflect SDL_Mixer needs to be compiled to support sou…

fcddc24

…ndfonts per platform.

Starbuck5 reviewed Jun 29, 2023

View reviewed changes

src_c/music.c Outdated Show resolved Hide resolved

Starbuck5 reviewed Jun 29, 2023

View reviewed changes

src_c/music.c Outdated Show resolved Hide resolved

Starbuck5 reviewed Jun 29, 2023

View reviewed changes

docs/reST/ref/music.rst Outdated Show resolved Hide resolved

Starbuck5 added mixer.music pygame.mixer.music New API This pull request may need extra debate as it adds a new class or function to pygame labels Jun 29, 2023

AlexanderGroeger added 2 commits June 29, 2023 10:18

minor doc word changes

fe67204

made path arg optional to allow clearing soundfont, added stub, todo …

f76b6ae

…test, updated docs

Starbuck5 reviewed Jun 30, 2023

View reviewed changes

docs/reST/ref/music.rst Outdated Show resolved Hide resolved

Starbuck5 reviewed Jun 30, 2023

View reviewed changes

test/mixer_music_test.py Outdated Show resolved Hide resolved

added proper tests to set and get soundfont

cc8058b

Starbuck5 reviewed Jul 1, 2023

View reviewed changes

test/mixer_music_test.py Outdated Show resolved Hide resolved

AlexanderGroeger added 2 commits July 1, 2023 10:55

changed versionaddedold to versionadded

04e242f

removed comment in set_soundfont test

46876ad

ankith26 reviewed Jul 5, 2023

View reviewed changes

buildconfig/stubs/pygame/mixer_music.pyi Outdated Show resolved Hide resolved

buildconfig/stubs/pygame/mixer_music.pyi Outdated Show resolved Hide resolved

AlexanderGroeger and others added 3 commits July 5, 2023 20:16

Update buildconfig/stubs/pygame/mixer_music.pyi

cb0af1d

Co-authored-by: Ankith <46915066+ankith26@users.noreply.github.com>

Update buildconfig/stubs/pygame/mixer_music.pyi

cf2a068

Co-authored-by: Ankith <46915066+ankith26@users.noreply.github.com>

removed todo prefix, added type checks to test

76d4799

specified which operating systems work and why Windows does not

e0cf7b4

removed soundfont api, again

3fbb6f6

oddbookworm reviewed Jul 22, 2023

View reviewed changes

src_c/mixer.c Outdated Show resolved Hide resolved

AlexanderGroeger added 2 commits July 22, 2023 23:04

added const to paths in get_soundfont

8ed2914

added braces to fix ambiguous else

cb1fd59

oddbookworm previously requested changes Jul 23, 2023

View reviewed changes

AlexanderGroeger and others added 6 commits July 23, 2023 12:41

added braces

0a4fda9

removed asserts for set_soundfont return value

2b9b48b

removed soundfont api doc again

d266a9a

Merge branch 'pygame-community:main' into main

a964255

Merge branch 'main' of https://github.com/AlexanderGroeger/pygame-ce

7d9e548

fixed typo in set_soundfont

a87013d

itzpr3d4t0r reviewed Jul 23, 2023

View reviewed changes

src_c/_pygame.h Show resolved Hide resolved

Starbuck5 reviewed Jul 26, 2023

View reviewed changes

src_c/mixer.c Outdated Show resolved Hide resolved

Starbuck5 reviewed Jul 26, 2023

View reviewed changes

src_c/mixer.c Show resolved Hide resolved

Starbuck5 reviewed Jul 26, 2023

View reviewed changes

docs/reST/ref/mixer.rst Show resolved Hide resolved

AlexanderGroeger added 3 commits July 26, 2023 09:16

set_soundfont simplified string_path if branches

38dd9f6

fixed typo

4d35782

trimmed up the SDL_Mixer soundfont issue notice

1560c3a

AlexanderGroeger added 2 commits July 28, 2023 07:37

removed SDL discussion from docs

aa9bdd9

format fix

5e82e79

ankith26 reviewed Jul 28, 2023

View reviewed changes

docs/reST/ref/mixer.rst Outdated Show resolved Hide resolved

Update docs/reST/ref/mixer.rst

62641aa

Co-authored-by: Ankith <46915066+ankith26@users.noreply.github.com>

ankith26 approved these changes Jul 29, 2023

View reviewed changes

Starbuck5 approved these changes Jul 29, 2023

View reviewed changes

Starbuck5 merged commit c36788a into pygame-community:main Jul 29, 2023
31 of 32 checks passed

Starbuck5 added this to the 2.3.1 milestone Jul 29, 2023

AlexanderGroeger mentioned this pull request Aug 2, 2023

pygame.mixer does not use user-specified soundfonts on windows #2375

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling users to set the soundfont on midi playback #2274

Enabling users to set the soundfont on midi playback #2274

AlexanderGroeger commented Jun 27, 2023 •

edited

Starbuck5 commented Jun 29, 2023

Starbuck5 commented Jun 29, 2023

ankith26 left a comment

Starbuck5 commented Jul 11, 2023 •

edited

AlexanderGroeger commented Jul 11, 2023

oddbookworm commented Jul 11, 2023

Starbuck5 commented Jul 11, 2023

AlexanderGroeger commented Jul 11, 2023 •

edited

oddbookworm left a comment

AlexanderGroeger commented Jul 26, 2023

AlexanderGroeger commented Jul 28, 2023

ankith26 left a comment •

edited

Starbuck5 left a comment

Enabling users to set the soundfont on midi playback #2274

Enabling users to set the soundfont on midi playback #2274

Conversation

AlexanderGroeger commented Jun 27, 2023 • edited

Starbuck5 commented Jun 29, 2023

Starbuck5 commented Jun 29, 2023

ankith26 left a comment

Choose a reason for hiding this comment

Starbuck5 commented Jul 11, 2023 • edited

AlexanderGroeger commented Jul 11, 2023

oddbookworm commented Jul 11, 2023

Starbuck5 commented Jul 11, 2023

AlexanderGroeger commented Jul 11, 2023 • edited

oddbookworm left a comment

Choose a reason for hiding this comment

AlexanderGroeger commented Jul 26, 2023

AlexanderGroeger commented Jul 28, 2023

ankith26 left a comment • edited

Choose a reason for hiding this comment

Starbuck5 left a comment

Choose a reason for hiding this comment

AlexanderGroeger commented Jun 27, 2023 •

edited

Starbuck5 commented Jul 11, 2023 •

edited

AlexanderGroeger commented Jul 11, 2023 •

edited

ankith26 left a comment •

edited