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

Fixes for tracker music support, add test #2152

Merged
merged 8 commits into from
May 14, 2023
Merged

Conversation

Starbuck5
Copy link
Member

@Starbuck5 Starbuck5 commented May 9, 2023

Continued from #2146

Problem: pygame-ce 2.2.1 has a regression where XM music doesn't work on Windows.

Regression caused by SDL_mixer, switching back down to 2.6.2 fixes it. Reported upstream: libsdl-org/SDL_mixer#518.

Solution: go back to using SDL_mixer 2.6.2 (on all platforms, for consistency), and add a unit test to ensure support on all platforms.

Problem 2: apparently xm music wasn't working on Mac? -> fixed by adjusting the build flags to disable shared libmodplug like all the other audio codec libraries.

Problem 3: manylinux build was doing something with docker that fails on PR branches unless they are direct branches of pygame-ce, so had to move my stuff to this new branch.

@Starbuck5 Starbuck5 requested a review from a team as a code owner May 9, 2023 07:22
@Starbuck5 Starbuck5 marked this pull request as draft May 9, 2023 07:45
@Starbuck5 Starbuck5 marked this pull request as ready for review May 10, 2023 04:19
@Starbuck5 Starbuck5 changed the title Fixes for tracker music support, unit test Fixes for tracker music support, add test May 10, 2023
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

So if I understand what's going on here as of now, just removing mp3 tests somehow makes everything work again including xm loading? This is so suspicious hehe

@Starbuck5
Copy link
Member Author

So if I understand what's going on here as of now, just removing mp3 tests somehow makes everything work again including xm loading? This is so suspicious hehe

Just to be clear, I removed mp3 tests after adding them also in this PR. In the overall diff of this PR, no mp3 tests were removed. It's not that this PR removes mp3 tests.

I opened an issue about the mp3 loading failing (#2154), and cool-guy-dev confirmed this problem does not occur on real built pygame wheels. It seems to only be a problem when getting SDL_mixer from apt on the Ubuntu sdist runs.

This PR is totally ready for review, just focusing on xm music. (As a proxy for all the different tracker music formats, like mod and it).

@novialriptide
Copy link
Member

LGTM, all tests pass on my machine.
image

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The code changes fixes the failing xm tests on Windows for me.

@MyreMylar MyreMylar merged commit 9aed969 into main May 14, 2023
28 checks passed
@Starbuck5 Starbuck5 added this to the 2.3 milestone May 14, 2023
@Starbuck5 Starbuck5 deleted the starbuck5-tracker-music branch May 15, 2023 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildconfig mixer.music pygame.mixer.music
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants