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

Optional sound system #6629

Merged
merged 6 commits into from Feb 10, 2019
Merged

Optional sound system #6629

merged 6 commits into from Feb 10, 2019

Conversation

@mrhelmut
Copy link
Contributor

mrhelmut commented Jan 6, 2019

Hey there,

This PR should make Microsoft.Xna.Framework.Media and Microsoft.Xna.Framework.Audio optional so that no sound system is initialized if none of those namespace are used. This should make MonoGame friendly to third party sound engine like FMOD or Wwise.

It addresses:

  • #6515 exception types are now consistent (it's either DllNotFoundException or NoAudioHardwareException)
  • #6107 if no SoundEffect Song MediaPlayer Microphone or DynamicSoundEffectInstance are used, OpenAL won't get loaded and the dll can be removed from release packages
  • #4717 this error can now be caught through SoundEffect.Initialize() and developers can make it so it is not fatal

As suggested previously, sound engine initialization is now done whenever SoundEffect Song MediaPlayer Microphone or DynamicSoundEffectInstance are constructed.
It is however possible to manually initialize the SoundEffect API by calling SoundEffect.Initialize(). This allows developers to get any initialization errors and act accordingly (e.g. disabling sound, or make it fatal).

Fixes #6515 #6107 #4717

@Jjagg @cra0zy
@tomspilman if this goes through, I'll try to get the console repos' compliant.

Copy link
Contributor

Jjagg left a comment

Awesome PR! Thanks @mrhelmut :)
I made some comments, actually partly about existing code but made obvious in this PR.

MonoGame.Framework/Audio/OpenALSoundController.cs Outdated Show resolved Hide resolved
MonoGame.Framework/Audio/SoundEffect.OpenAL.cs Outdated Show resolved Hide resolved
MonoGame.Framework/Audio/SoundEffect.cs Show resolved Hide resolved
MonoGame.Framework/Media/Song.NVorbis.cs Outdated Show resolved Hide resolved
@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Jan 7, 2019

Thanks! I'm going to refactor the controller instance management, it has been a bit messy for quite too long.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Jan 7, 2019

@Jjagg I think I got things clean up. Let me know how it goes for you.

EDIT: forgot Android, brb.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Jan 7, 2019

Ready for review.

@Jjagg
Jjagg approved these changes Jan 7, 2019
@Jjagg

This comment has been minimized.

Copy link
Contributor

Jjagg commented Jan 7, 2019

Looks good to me! Gonna hold off a little bit in case anyone else wants to go over this, but will merge if not.

@@ -128,7 +129,7 @@ public static ReadOnlyCollection<Microphone> All
/// </summary>
public static Microphone Default
{
get { return _default; }
get { SoundEffect.Initialize(); return _default; }

This comment has been minimized.

Copy link
@harry-cpp

harry-cpp Jan 8, 2019

Member

Write code like this in the static initializer instead of static property.

This comment has been minimized.

Copy link
@harry-cpp

harry-cpp Jan 8, 2019

Member

Just so there is no confusion, ie:

static Microphone()
{
    SoundEffect.Initialize();
}

This comment has been minimized.

Copy link
@harry-cpp

harry-cpp Jan 8, 2019

Member

You could also use static constructors in few other places.

This comment has been minimized.

Copy link
@Jjagg

Jjagg Jan 8, 2019

Contributor

I think it's better to check if initialization was done properly on every call, rather than do it only once. It makes behavior more predictable. Static initializers complicate control flow because you can't be sure when they'll be called.

This comment has been minimized.

Copy link
@harry-cpp

harry-cpp Jan 8, 2019

Member

They get called during first access to their respective class.

This comment has been minimized.

Copy link
@mrhelmut

mrhelmut Jan 8, 2019

Author Contributor

The thing is, Default and ```All`` are the microphone entry points, but you have to initialize the sound API to populate the microphones, and the Microphone class is static by design...
Unless we change the design of the Microphone class, I don't think we can do anything else than having this in a static field. I'm not a fan of static classes because this kind of design problems.

Alternative proposition: not initializing the sound API at all in the Microphone class, and throw if All/Default are accessed while not initialized. Forcing users to either call SoundEffect.Initialize() or to use the auto initialization when loading content.

I've seen static classes initializing without obvious calls to them, so I'd feel more comfortable with not having any library loading happening inside static ctor/fields.

For the same reason, I don't quite like OpenAL.NativeLibrary being initialized statically. I think I'm going to change that too.

This comment has been minimized.

Copy link
@Jjagg

Jjagg Jan 8, 2019

Contributor

We should not change the API by forcing explicit initialization. If we could have a fresh API, I'd definitely want to make it explicit though.

This comment has been minimized.

Copy link
@harry-cpp

harry-cpp Jan 8, 2019

Member

How about just checking if Audio systems can be initialized on game run?

For the same reason, I don't quite like OpenAL.NativeLibrary being initialized statically. I think I'm going to change that too.

Please don't, those are just wrappers. Think of them as DllImport statements :P

This comment has been minimized.

Copy link
@mrhelmut

mrhelmut Jan 8, 2019

Author Contributor

Sure, but they do load the dll if the static ctor kicks in, which we want to avoid to not mess up with third party sound engine.

But if we can't get the sound api to initialize in a non static way for Microphone, there's no point in removing the static initialization from OpenAL anyway because it'll still be around within Microphone.

This comment has been minimized.

Copy link
@mrhelmut

mrhelmut Jan 9, 2019

Author Contributor

I am more suspicious of Microphone than I am of OpenAL. I'd prefer to not add a static constructor to Microphone.

If we'd like to be 100% safe, I'd suggest to move static initialization out of OpenAL func loading and manually load them (to have a perfect control on the "when" and "how").

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Jan 14, 2019

After giving some thoughts about static initializers, I believe that we are 100% fine with OpenAL statically loading the binary. I assume that people not willing to use OpenAL will remove the dll from their distribution, which doesn't trigger any error if OpenAL tries to load anyway (unless SoundEffect.Initialize() is called, but then it's the expected behavior).

I am however not in favor of having Microphone use a static initializer to initialize the sound API. As of now everything would be fine, but we have no control over what could reference Microphone and possibly trigger its initializer. For safety, I prefer to have the initialization upon accessing the microphones list.

@Jjagg @cra0zy What do you think? If it's ok to you, then it should be good to go.

@Jjagg

This comment has been minimized.

Copy link
Contributor

Jjagg commented Jan 14, 2019

That's fine by me.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Feb 7, 2019

@cra0zy Could you share your thoughts about the current status of this PR? It would be cool to move forward.

I've been running on this PR for quite some time now, using FMOD.

@harry-cpp

This comment has been minimized.

Copy link
Member

harry-cpp commented Feb 9, 2019

Looks fine to me, sorry for the delay.

@Jjagg

This comment has been minimized.

Copy link
Contributor

Jjagg commented Feb 10, 2019

Thanks @mrhelmut! Merging this :D

@Jjagg Jjagg merged commit 72919dc into MonoGame:develop Feb 10, 2019
7 checks passed
7 checks passed
Build Mac, iOS, and Linux Finished TeamCity Build MonoGame / Build Mac : Running
Details
Build Windows, Web, Android, and OUYA Finished TeamCity Build MonoGame / Build Windows : Running
Details
Package Mac and Linux Finished TeamCity Build MonoGame / Package Mac and Linux : Running
Details
Package NuGet Finished TeamCity Build MonoGame / Package NuGet : Running
Details
Package Windows SDK Finished TeamCity Build MonoGame / Package Windows : Running
Details
Test Mac Finished TeamCity Build MonoGame / Test Mac : Tests passed: 1148, ignored: 46
Details
Test Windows Finished TeamCity Build MonoGame / Test Windows : Tests passed: 1252, ignored: 20
Details
@mrhelmut mrhelmut deleted the mrhelmut:optionalSound branch Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.