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

Split SoundEffectInstance/SoundEffect on a per-API basis #2251

Merged
merged 18 commits into from Mar 7, 2014

Conversation

RayBatts
Copy link

The goals for this pull request mirror those found in #2243, but for the SoundEffectInstance/SoundEffect classes. We chose to split each file into a XAudio, OpenAL, and PSM paths.

SoundEffectInstance and SoundEffect were split into partial classes, to help separate the code and clarify the growing confusing caused by constant #if/#elseif/#else chains and increase ease of future PRs. This also allows us to pull out common code to the "base" classes so that the behavior of these classes is more uniform.

Note that this pull request is all clean up; There should be no functional changes or new features, but please let me know if any pop up.

While merging these paths together, I did notice a lot bugs and logic errors. I tried to avoid fixing them, as that's a goal for a future PR. It's already confusing enough to view the diff of a restructure like this without changing implementation.

@RayBatts
Copy link
Author

Note that the Android and OpenAL paths were merged together. I'm told that @KonajuGames has reworked some of the audio classes on Android. After this pull request, his should remove the Android changes here and be replaced with the new Android path

@mgbot
Copy link
Member

mgbot commented Feb 27, 2014

Can one of the admins verify this patch?

1 similar comment
@mgbot
Copy link
Member

mgbot commented Feb 27, 2014

Can one of the admins verify this patch?

@tomspilman
Copy link
Member

@mgbot test

@mgbot
Copy link
Member

mgbot commented Feb 28, 2014

Test FAILed.
Refer to this link for build results: http://build.monogame.net/job/PullRequestTester/599/

@tomspilman
Copy link
Member

@mgbot test

@mgbot
Copy link
Member

mgbot commented Mar 7, 2014

Test FAILed.
Refer to this link for build results: http://build.monogame.net/job/PullRequestTester/613/

@tomspilman
Copy link
Member

@mgbot test

@mgbot
Copy link
Member

mgbot commented Mar 7, 2014

Test PASSed.
Refer to this link for build results: http://build.monogame.net/job/PullRequestTester/614/

@tomspilman
Copy link
Member

Woot! This can merge if you are good with it Steve.

KonajuGames added a commit that referenced this pull request Mar 7, 2014
Split SoundEffectInstance/SoundEffect on a per-API basis
@KonajuGames KonajuGames merged commit baddc7b into MonoGame:develop Mar 7, 2014
@KonajuGames
Copy link
Contributor

Merged. I will do the Android update soon.

@tomspilman
Copy link
Member

Fantastic!

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.

None yet

5 participants