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

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

oops, that was the wrong tab xD

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?

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.

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?

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.

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.

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.

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.

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.

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.

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 ?

In D1481#68809, @wraitii wrote:

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.

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).

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

(I was thinking about a static HasSoundcard function.)

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

Linter detected issues:
Executing section Source...

source/soundmanager/SoundManager.h
|  27| #include·"items/ISoundItem.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classISoundManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2826/display/redirect

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

Linter detected issues:
Executing section Source...

source/soundmanager/SoundManager.h
|  27| #include·"items/ISoundItem.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classISoundManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2827/display/redirect