Page MenuHomeWildfire Games

Remove the unneeded sound warnings when no sound
AbandonedPublic

Authored by Imarok on May 2 2018, 8:39 PM.

Details

Reviewers
Itms
vladislavbelov
Stan
Trac Tickets
#3285
Summary

Don't show any warnings if the game was started with --nosound.
If there is no sound device connected only show one warning at startup and no ingame warnings.

My fix may not be the best one, but it works.

Test Plan

Start one game without connected sound device and one game with sound device but with`--nosound`. (Seems only be reproducible on windows)

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6020
Build 10028: Vulcan BuildJenkins
Build 10027: arc lint + arc unit

Event Timeline

Imarok created this revision.May 2 2018, 8:39 PM
Vulcan added a subscriber: Vulcan.May 2 2018, 8:47 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/458/display/redirect

vladislavbelov added inline comments.
source/soundmanager/SoundManager.cpp
194

What do you catch here?

Stan added a subscriber: Stan.May 3 2018, 12:58 PM
Stan added inline comments.
source/soundmanager/SoundManager.cpp
194

The exception he added just below.

vladislavbelov added inline comments.May 6 2018, 9:20 PM
source/soundmanager/SoundManager.cpp
194

Yes, but the Status name is not acceptable here as the exception name (that was the original question). Because you can't understand, what's the status? And it's not the CSoundManager status, it's internal and unknown to the ISoundManager.

vladislavbelov added inline comments.May 6 2018, 9:23 PM
source/soundmanager/SoundManager.cpp
194

Also I'd prefer to not return values through exceptions at all, but use a named exception.

Imarok added inline comments.May 14 2018, 4:55 PM
source/soundmanager/SoundManager.cpp
194

I agree the type of exception I've chosen is not the best.
Any idea, what I should use?

oops, that was the wrong tab xD

elexis added a subscriber: elexis.Dec 15 2018, 4:21 PM

I didn't read the SoundManager file yet, but in general not creating a component (or passing a dummy component) if the --nosound argument is passed sounds nicer than creating a soundmanager that is determined to be unused and error out and then trying to catch and hide the result?

Stan requested changes to this revision.Jan 3 2019, 12:59 AM

Safe to assume this will need a rebase.

This revision now requires changes to proceed.Jan 3 2019, 12:59 AM
Imarok added a comment.Jan 3 2019, 1:35 AM
In D1481#67038, @elexis wrote:

I didn't read the SoundManager file yet, but in general not creating a component (or passing a dummy component) if the --nosound argument is passed sounds nicer than creating a soundmanager that is determined to be unused and error out and then trying to catch and hide the result?

IIRC the change in GameSetup.cpp makes that no component is created with --nosound.
The exception is needed for the case that sound is enabled, but no sound device is plugged in.

elexis added a comment.Jan 3 2019, 9:37 AM

So the question is: How can the code detect a missing soundcard if sound is enabled prior to creating the SoundManager?
Is alcOpenDevice really the only way to test?

Imarok added a comment.Jan 3 2019, 1:01 PM
In D1481#68698, @elexis wrote:

So the question is: How can the code detect a missing soundcard if sound is enabled prior to creating the SoundManager?
Is alcOpenDevice really the only way to test?

I think so.

elexis added a comment.Jan 3 2019, 1:50 PM
In D1481#68783, @Imarok wrote:
In D1481#68698, @elexis wrote:

So the question is: How can the code detect a missing soundcard if sound is enabled prior to creating the SoundManager?
Is alcOpenDevice really the only way to test?

I think so.

That doesn't explain the reader how the reader can reproduce the thought.
If we don't actually know, saying "I think so" may be misleading.
A websearch for that function yielded some specs and examples:
https://www.openal.org/documentation/OpenAL_Programmers_Guide.pdf
https://ffainelli.github.io/openal-example/
I didn't find a low hanging fruit there, so it seems it's unfeasible to avoid creating cpp class instances if the device is unavailable.
Perhaps something with device enumeration prior to device use could be done, but I didn't bother.
The ctor -> throw -> catch is still an anti-pattern, because by definition code should be written so that it doesn't trigger errors unless an unexpected event occurs. But this seems to be an expected event.

An alternative would also be to not catch this error and have the program exit horribly, but informing the user what to do (configuring some sound device thing),
or fixing the code to not run into this error and pick the possibly other sounddevice that works.

elexis added a comment.Jan 3 2019, 1:59 PM

The catch also catches any kind of exception, which raises the question whether we want to catch and silence other errors or rather have people experience the errors and report them.
Also agree with Vladislavs point that using the Status int as an exception type is confusing. The types don't have the exact same connotation.
Perhaps something like foo* device = alcOpenDevice; if (device) { new soundManager ... } in the JS Interface could work without using throw/catch.

source/soundmanager/SoundManager.cpp
187

(early return if this function is modified and contains more than 4 lines)

Imarok added a comment.Jan 3 2019, 2:17 PM

So what about setting a bool m_correctAlcInit to false when no device is connected and check for that after constructing CSoundManager instead of catching an exception?

In D1481#68798, @elexis wrote:

The catch also catches any kind of exception, which raises the question whether we want to catch and silence other errors or rather have people experience the errors and report them.

With my patch an unconnected device throws exactly one warning, but then remains silence (in contrast to the error spam we currently have), which is the behaviour we want.

Imarok added a comment.Jan 3 2019, 2:20 PM
In D1481#68794, @elexis wrote:

The ctor -> throw -> catch is still an anti-pattern, because by definition code should be written so that it doesn't trigger errors unless an unexpected event occurs. But this seems to be an expected event.

We don't expect the user not to connect a sound device. But we use the exception to handle that unexpected possibility.

An alternative would also be to not catch this error and have the program exit horribly, but informing the user what to do (configuring some sound device thing),
or fixing the code to not run into this error and pick the possibly other sounddevice that works.

The program shouldn't exit, because it's no fatal error. We can't pick an other device, because if the exception is triggered no device at all is connected.

wraitii added a subscriber: wraitii.Jan 3 2019, 2:41 PM

So what about setting a bool m_correctAlcInit to false when no device is connected and check for that after constructing CSoundManager instead of catching an exception?

An un-recoverable error in the constructor is _the_ use case for an exception and the above would be a really ugly workaround imo.

Two solutions I think:

  • Either go rummaging through our custom exception types somewhere in /lib for something reasonably unique, throw that, catch it - alternatively create your own custom type. Then we're OK.
  • Do two-step initialisation where you first create the object unfailingly then call an 'Init' function that can fail (return false, whatever) and then destroy the object.
elexis added a comment.Jan 3 2019, 2:55 PM

We don't expect the user not to connect a sound device. But we use the exception to handle that unexpected possibility.

If the only case that is dealt with in this diff is a user not having any sound device for real and not some bug or misconfiguration, then the code should expect that. That's very much expected now that we know that users with that configuration exist. throw has the intention of dealing with program errors and program errors should be avoided. (Not so important, still.)

It should detect that situation and then notice that it doesn't need a soundmanager and whatever else is involved that --nosound disables (I see some more stuff about that than g_SoundManager, ISoundManager::SetEnabled(false)?).

Tried this thing https://code.wildfiregames.com/D1481#68798 ?

Imarok added a comment.Jan 3 2019, 7:13 PM

So what about setting a bool m_correctAlcInit to false when no device is connected and check for that after constructing CSoundManager instead of catching an exception?

An un-recoverable error in the constructor is _the_ use case for an exception and the above would be a really ugly workaround imo.

That!

In D1481#68817, @elexis wrote:

throw has the intention of dealing with program errors and program errors should be avoided. (Not so important, still.)

No, the intention of throw is to escape the usual code path in case something unusual happens. (One example from Java: If you try to open a file you don't have the rights to, it throws an exception. It is not necessary a program error, but it is unusual, so an exception gets thrown.)

Tried this thing https://code.wildfiregames.com/D1481#68798 ?

That means you want to initialize the alcDevice before initializing the soundmanager and then pass that to the constructor of the soundmanager. (Or you would need to init the device twice) That would result in ugly code.

Using an exception is just the cleanest way to go.

Stan added inline comments.Jan 3 2019, 7:16 PM
source/soundmanager/SoundManager.cpp
194

Can we create a named exception for this ?

Imarok marked an inline comment as done.Jan 3 2019, 7:17 PM
Imarok added inline comments.
source/soundmanager/SoundManager.cpp
194

Sure, I just ran out of names.

Did you try to check the specs or just like what you see and end it there?
Using throw like you do isn't the uncleanest solution, but in general you can't tell me that creating a class to manage sound, then noticing that we don't have a soundcard, then deleting the soundmanager again is a fully clean pattern.
If we know in advance that something will fail, we should not attempt to do it, rather than trying to recover from unrecoverable errors.
So the question is only whether we can test whether a sounddevice exists or not without an ugly workaround.
I don't know anything about OpenAL so I can't rule out that it's impossible to check whether a sound device exists.
Did someone check so far before deeming this the cleanest solution? I'm not writing the patch so it shouldn't be my task, but a quick search shows alcGetString to test whether a default device exists:
https://wiki.delphigl.com/index.php/alcOpenDevice
We can see that this API function is also used in the same function to test whether it worked or not. It sounds a bit intersecting. Perhaps something can be done with refactoring (changing the order of alcGetString and alcOpenDevice).

source/soundmanager/SoundManager.cpp
346

Check also this below

Imarok abandoned this revision.Jan 6 2019, 11:44 PM

Seems like I can't reproduce that bug anymore.
I also don't have any working VS available.
So if anyone wants, please take it over. I cannot work on this further. :(

Stan added a comment.Jan 7 2019, 9:35 AM

If you still have Windows. Just disable your soundcard :)

elexis added a comment.Jan 7 2019, 2:08 PM

(I was thinking about a static HasSoundcard function.)