SGSampleGroup::remove(): immediately remove from _samples by default

Code in AudioIdent::setIdent() expects that SGSampleGroup::remove() does
remove the corresponding item from _samples, however SG commit 47aae88ab
broke this assumption, which caused the bug reported at [1].

Restore the immediate removal from _samples by default. For the special
case where we do need delayed removal (i.e., while
SGSampleGroup::update() is iterating over _samples), the new optional
parameter is passed the value true. For a longer explanation, see [2].

This fixes a crash when swapping the selected and standby frequencies
for the NAV1 or NAV2 instrument (more precise description at [1]).

[1] https://forum.flightgear.org/viewtopic.php?f=25&t=40890
[2] https://sourceforge.net/p/flightgear/mailman/message/37721699/

simgear/sound/sample_group.cxx

@@ -94,7 +94,8 @@ void SGSampleGroup::check_playing_sample(SGSoundSample *sample)
         sample->no_valid_source();
         _smgr->release_source( sample->get_source() );
         _smgr->release_buffer( sample );
-        remove( sample->get_sample_name() );
+        // Delayed removal because this is called while iterating over _samples
+        remove( sample->get_sample_name(), true );
     } else if ( sample->has_changed() ) {
         if ( !sample->is_playing() ) {
             // a request to stop playing the sound has been filed.
@@ -133,15 +134,10 @@ void SGSampleGroup::update( double dt ) {
         testForMgrError("update");
     }
 
-    for (auto current : _removed_samples) {
+    for (const auto& refname : _refsToRemoveFromSamplesMap) {
-        for (auto sample = _samples.begin(); sample != _samples.end(); ) {
+        _samples.erase(refname);
-            if (sample->second == current) {
-                sample = _samples.erase(sample);
-            } else {
-                ++sample;
-            }
-        }
     }
+    _refsToRemoveFromSamplesMap.clear();
 }
 
 // add a sound effect, return true if successful
@@ -160,8 +156,14 @@ bool SGSampleGroup::add( SGSharedPtr<SGSoundSample> sound,
 
 
 // remove a sound effect, return true if successful
-bool SGSampleGroup::remove( const std::string &refname ) {
+//
-
+// TODO: the 'delayedRemoval' boolean parameter has been specifically added
+// for check_playing_sample() which is only called while update() is iterating
+// over _samples. It may be better style to remove this parameter and add a
+// separate member function for use in check_playing_sample(), whose behavior
+// would be the same as when passing delayedRemoval=true here.
+bool SGSampleGroup::remove( const std::string &refname, bool delayedRemoval )
+{
     auto sample_it = _samples.find( refname );
     if ( sample_it == _samples.end() ) {
         // sample was not found
@@ -171,8 +173,12 @@ bool SGSampleGroup::remove( const std::string &refname ) {
     if ( sample_it->second->is_valid_buffer() )
         _removed_samples.push_back( sample_it->second );
 
-    // Do not erase within the loop
+    // Do not erase within the loop in update()
-//  _samples.erase( sample_it );
+    if (delayedRemoval) {
+        _refsToRemoveFromSamplesMap.push_back(refname);
+    } else {
+        _samples.erase(refname);
+    }
 
     return true;
 }

simgear/sound/sample_group.hxx

@@ -91,9 +91,11 @@ public:
     /**
      * Remove an audio sample from this group.
      * @param refname Reference name of the audio sample to remove
+     * @param delayedRemoval If true, don't modify _samples before the main
+     *        loop in update() is finished
      * @return return true if successful
      */
-    bool remove( const std::string& refname );
+    bool remove( const std::string& refname, bool delayedRemoval = false );
 
     /**
      * Test if a specified audio sample is registered at this sample group
@@ -240,6 +242,7 @@ private:
 
     sample_map _samples;
     std::vector< SGSharedPtr<SGSoundSample> > _removed_samples;
+    std::vector<std::string> _refsToRemoveFromSamplesMap;
 
     bool testForMgrError(std::string s);
     bool testForError(void *p, std::string s);