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

Refactoring AL entry points loading #6516

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@mrhelmut
Copy link
Contributor

commented Oct 30, 2018

This is a first pass at addressing #6515. It refactors the entry points so that DllNotFoundException and NoAudioHardwareException are correctly thrown upon OpenAL initialization failures.

cc @cra0zy @Jjagg

If this gets merged, I'll work on another PR to make the Audio namespace optional.

@cra0zy

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

This is a first pass at addressing #6515. It refactors the entry points so that DllNotFoundException and NoAudioHardwareException are correctly thrown upon OpenAL initialization failures.

We are trying to do the opposite, no exception should be thrown if audio stuff is missing and instead everything should continue as normal.

PS. You can achieve the same thing you did by adding a if (ret == IntPtr.Zero) check here: https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Audio/OpenAL.cs#L238 , if I am not mistaken.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

That's what I did here: https://github.com/MonoGame/MonoGame/pull/6516/files#diff-8c76ca43b718d6fa4dfb764513677375R200

I moved every static initialization to make sure that nothing gets statically initialized and throw an exception outside of the try/catch loop.

@cra0zy

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

I moved every static initialization to make sure that nothing gets statically initialized and throw an exception outside of the try/catch loop.

It never throws an exception... If it does not get initialized, the default value is set, the only thing that could throw an exception at that point is actually trying to call the delegate.

https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Utilities/FuncLoader.Desktop.cs#L48

@tomspilman

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

I'll work on another PR to make the Audio namespace optional.

Note on console this a real concern at times.

On one console if MonoGame initialized the audio system itself and you happen to use FMOD it would not work correctly. In that project i had to hack the MG audio system to not initialize on startup to avoid the problem.

Using a 3rd party sound system should be expected with MG. So MG should play nice with that and not initialize audio unless it is actually needed/used. How best to do that while preserving XNA behavior i'm not sure.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

It never throws an exception... If it does not get initialized, the default value is set, the only thing that could throw an exception at that point is actually trying to call the delegate.

Oh ok! Though we still need to get them out of the static initializer so that we are sure that the native library pointer gets loaded before the delegates.

On one console if MonoGame initialized the audio system itself and you happen to use FMOD it would not work correctly. In that project i had to hack the MG audio system to not initialize on startup to avoid the problem.

That's what I figured looking at the consoles. This should be workable.

@cra0zy

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Oh ok! Though we still need to get them out of the static initializer so that we are sure that the native library pointer gets loaded before the delegates.

It always does as NativeLibrary is getting set on its declaration: https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Audio/OpenAL.cs#L194

You don't need to move any initialization code.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

We do, if we want to make sure that DllNotFoundException and NoAudioHardwareException are correctly thrown. With static initialization it can get initialized anywhere and most importantly, outside the scope of these statements:

https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Audio/OpenALSoundController.cs#L152

catch (DllNotFoundException ex)

Which makes those try/catch blocks useless, because any exception is uncatchable due to its static ctor nature (and a missing library is currently not a silent error), which makes the detection of an audio initialization failures impossible (or at least, not consistent at all).

We need to make sure that things are done in an ordered and timed manner. We also need to have a control over whether or not the library is getting loaded if we want to avoid it be loaded while using third party audio engines.

@tomspilman

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

We need to make sure that things are done in an ordered and timed manner. We also need to have a control over whether or not the library is getting loaded if we want to avoid it be loaded while using third party audio engines.

100% agree.

@cra0zy

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@mrhelmut you can just ask if the NativeLibrary is IntPtr.Zero at any time to know if it got loaded.

@tdeeb

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

Have there been any updates regarding this?

@Jjagg

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@tdeeb Anything that happens is public. If it's not here it hasn't happened.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

I've been off to other stuff, but I'm planning to go back to this and make audio optional. But since there are multiple audio backend to deal with, it's most probably going to take some time before anything gets merged.

@cra0zy

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

@mrhelmut Not sure if you got what changes I requested, anyway want me to make a new PR with those changes?

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@cra0zy I'm going to revamp this PR later, but feel free if they are changes you'd like to make, I'll branch from there.

@mrhelmut mrhelmut closed this Dec 3, 2018

@mrhelmut mrhelmut deleted the mrhelmut:audio_refactor branch Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.