Re: [Flightgear-devel] Audio paths in add-ons

[Florent R. <f.r...@fr...>]

Hi,

James Turner <ja...@fl...> wrote:

> Yep, apply the sound-manager patch for sure.

Mostly for Erik:

Before applying my fix for FGSoundManager::playAudioSampleCommand(), I
propose little changes to simgear/sound/sample.{hxx,cxx}:

Currently, the SGSoundSample constructor used by
FGSoundManager::playAudioSampleCommand() is as follows[1]:

SGSoundSample::SGSoundSample(const char *file, const SGPath& currentDir) :
    _is_file(true)
{
    SGPath p = simgear::ResourceManager::instance()->findPath(file, currentDir);
    _refname = p.utf8Str();
}

however its use in FGSoundManager::playAudioSampleCommand() is awkward
in several ways. First, FGSoundManager::playAudioSampleCommand() starts
from 'path' and 'file' strings. After the already-discussed fix for the
existence check, it will build an SGPath from these 'path' and 'file'
components. Then it uses globals->resolve_maybe_aircraft_path() on this
path, which goes through
simgear::ResourceManager::instance()->findPath(). For consistency, and
because ResourceManager::findPath() replaces the special [addon=...]
prefix with a real filesystem path,
FGSoundManager::playAudioSampleCommand() will then have to decompose the
SGPath obtained from ResourceManager::findPath() into directory and file
parts only because the existing SGSoundSample constructor wants these.
And what will this constructor do? Feed the two parts to
simgear::ResourceManager::instance()->findPath() a second time (quicker
though, because the second arg will be non-null)! This is a bit
convoluted and ugly.

I propose to add a straightforward, explicit constructor from SGPath:

SGSoundSample::SGSoundSample(const SGPath& file) :
    _is_file(true)
{
    _refname = file.utf8Str();
}

and reimplement the existing one using delegation:

// Delegating constructor that goes through the ResourceManager
SGSoundSample::SGSoundSample(const string& file, const SGPath& currentDir) :
    SGSoundSample(
        simgear::ResourceManager::instance()->findPath(file, currentDir))
{ }

As you can see, I also changed the first arg of that one from const
char* to const std::string&. This is already what
ResourceManager::findPath() expects; the current SGSoundSample
constructor forces FGSoundManager::playAudioSampleCommand() in
soundmanager.cxx to use a .c_str() because of this, which is quite
awkward since ResourceManager::findPath() will then rebuild a new
identical std::string instance from the obtained const char*.

Patch is attached and tested; it appears to work fine.

When fgValidatePath() is moved from FlightGear to SimGear (a priori,
soon), we could also add a check for read permission to the
SGSoundSample(const SGPath& file) constructor---which would therefore
also benefit the delegating one.

Regards

[1] https://sourceforge.net/p/flightgear/simgear/ci/next/tree/simgear/sound/sample.cxx#l83

-- 
Florent