Added SoundEffect.FromStream #1122

Merged
merged 4 commits into from Jan 6, 2013

Projects

None yet

5 participants

@chaosnhatred
Contributor

Hey,
This is my first pull request, so please be gentle.

I came across this function in the xna docs and it looked like it was for me, so i implemented it here as it as i needed it to test on Android with as well.

The only platforms i tested with were Windows7 and Android 2.3

@KonajuGames
Contributor

Overall a clean pull request. Some comments on the implementation.

The Desktop version should be implemented slightly differently, as the SoundEffect ctor used simply creates a MemoryStream from the byte[] passed to it, resulting in two in-memory copies of the WAV file at the same time. I would suggest adding a private SoundEffect(string, Stream) ctor that takes the stream directly and passes that stream to LoadAudioStream without the intermediate MemoryStream.

The Android version is a no-op at the moment because the Sound(byte[]. float, bool) ctor used by SoundEffect(string, byte[]) does nothing at this stage. This is because we currently use Android's SoundPool. Also, there is no need to copy from MemoryStream into a byte[] as MemoryStream has a GetBuffer method that returns the byte[] used internally.

unknown Implemented an internal ctor that accepts a Stream and set FromStream…
… to use that for Desktops.

I have FromStream throw NotImplementedEX for Android and everything else processes like normal.
0eaa956
@chaosnhatred
Contributor

My fault for not checking that out more. :-X

I implemented an internal ctor that accepts a Stream and set FromStream to use that for Desktops.

for everything else, i have FromStream throw NotImplementedEX for Android and everything else processes like normal.

@elisee
Contributor
elisee commented Jan 2, 2013

The Mac port has its own MacOS/Audio/SoundEffect.cs class that will need a FromStream method too.

@chaosnhatred
Contributor

alright, i have no issue with implementing it, i just have no way of testing it.
So any of the many MacOS/iOS devs out there will have to test this on my behalf.

@tomspilman
Member

You can do one of two things:

  1. Blindly implement the Mac version and let others test for you.
  2. Don't implement the Mac version, but be sure your changes don't break the Mac build.
@elisee
Contributor
elisee commented Jan 3, 2013

I looked at it myself but the Mac has no AudioLoader class unlike the other desktop ports and works directly with byte arrays rather than a stream (because it uses native Cocoa APIs to load sound effects). I guess we could get the buffer from the stream and pass that? Not sure.

Anyway this pull request doesn't break anything on Mac and works fine on Linux (and most likely Windows too) so I'd say don't block on the missing method on Mac.

unknown Implemented an internal ctor that accepts a Stream and set FromStream…
… to use that for all other platforms.

I have also implemented a ctor for MacOS that accepts a stream and added FromStream for it as well.
5cf915d
@chaosnhatred
Contributor

I added FromStream for MacOS, please test it :-)

I have also modified MonoGame.Framework\Audio\SoundEffect.cs by adding a ctor that accepts a stream.

@elisee
Contributor
elisee commented Jan 4, 2013

Just tested on MacOS X, works great.

@chaosnhatred
Contributor

I guess if no one has any objections, this can be merged.

I was also doing some digging around concerning OpenAL and android and opened an issue for its discussion.

#1146

@KonajuGames
Contributor

The new ctors should be private. In Desktop and MacOS they are marked public.

@chaosnhatred
Contributor

Fixing now, should have it committed in a few minutes.

@KonajuGames
Contributor

Thanks for taking care of all the suggestions. Merging now.

@KonajuGames KonajuGames merged commit 635882b into MonoGame:develop3d Jan 6, 2013
@dellis1972

This new constructor has broken the Windows 8 and the WP8 build. Under those platforms _data and _sound are not defined as they are not used.

Contributor
@chaosnhatred
Contributor

alright, i'll get that moved into the non winrt blocks

@chaosnhatred chaosnhatred deleted the unknown repository branch Jan 6, 2013
@KonajuGames
Contributor

I've submitted a PR, before I saw your reply.

@chaosnhatred
Contributor

its all good :-)

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