Fix mod sounds from local, Android SoundPool issues, more commenting (#4310)

core/src/com/unciv/UncivGame.kt

@@ -15,10 +15,7 @@ import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.tilesets.TileSetCache
 import com.unciv.models.translations.Translations
 import com.unciv.ui.LanguagePickerScreen
-import com.unciv.ui.utils.CameraStageBaseScreen
+import com.unciv.ui.utils.*
-import com.unciv.ui.utils.CrashController
-import com.unciv.ui.utils.ImageGetter
-import com.unciv.ui.utils.center
 import com.unciv.ui.worldscreen.PlayerReadyScreen
 import com.unciv.ui.worldscreen.WorldScreen
 import java.util.*
@@ -180,8 +177,9 @@ class UncivGame(parameters: UncivGameParameters) : Game() {
 
     override fun dispose() {
         cancelDiscordEvent?.invoke()
+        Sounds.clearCache()
 
-        // Log still running threads (should be only this one and "DestroyJavaVM")
+        // Log still running threads (on desktop that should be only this one and "DestroyJavaVM")
         val numThreads = Thread.activeCount()
         val threadList = Array(numThreads) { _ -> Thread() }
         Thread.enumerate(threadList)

core/src/com/unciv/ui/utils/Sounds.kt

@@ -1,22 +1,51 @@
 package com.unciv.ui.utils
 
+import com.badlogic.gdx.Application
 import com.badlogic.gdx.Gdx
 import com.badlogic.gdx.audio.Sound
 import com.badlogic.gdx.files.FileHandle
 import com.unciv.UncivGame
 import com.unciv.models.UncivSound
 import java.io.File
+import kotlin.concurrent.thread
+
+/*
+ * Problems on Android
+ * 
+ * Essentially the freshly created Gdx Sound object from newSound() is not immediately usable, it
+ * needs some preparation time - buffering, decoding, whatever. Calling play() immediately will result
+ * in no sound, a logcat warning (not ready), and nothing else - specifically no exceptions. Also,
+ * keeping a Sound object for longer than necessary to play it once will trigger CloseGuard warnings
+ * (resource failed to clean up). Also, Gdx will attempt fast track, which will cause logcat entries,
+ * and these will be warnings if the sound file's sample rate (not bitrate) does not match the device's
+ * hardware preferred bitrate. On a Xiaomi Mi8 that is 48kHz, not 44.1kHz. Channel count also must match.
+ * 
+ * @see "https://github.com/libgdx/libgdx/issues/1775"
+ * logcat entry "W/System: A resource failed to call end.":
+ *  unavoidable as long as we cache Gdx Sound objects loaded from assets
+ * logcat entry "W/SoundPool: sample X not READY":
+ *  could be avoided by preloading the 'cache' or otherwise ensuring a minimum delay between
+ *  newSound() and play() - there's no test function that does not trigger logcat warnings.
+ * 
+ * Current approach: Cache on demand as before, catch stream not ready and retry. This maximizes
+ * logcat messages but user experience is acceptable. Empiric delay needed was measured a 40ms
+ * so that is the minimum wait before attempting play when we know the sound is freshly cached
+ * and the system is Android.
+ */
 
 /**
  * Generates Gdx [Sound] objects from [UncivSound] ones on demand, only once per key
  * (two UncivSound custom instances with the same filename are considered equal).
- * 
+ *
  * Gdx asks Sound usage to respect the Disposable contract, but since we're only caching
  * a handful of them in memory we should be able to get away with keeping them alive for the
- * app lifetime.
+ * app lifetime - and we do dispose them when the app is disposed.
  */
 object Sounds {
-    private enum class SupportedExtensions { mp3, ogg, wav }    // Gdx won't do aac/m4a
+    private const val debugMessages = true
+
+    @Suppress("EnumEntryName")
+    private enum class SupportedExtensions { mp3, ogg, wav }    // Per Gdx docs, no aac/m4a
     
     private val soundMap = HashMap<UncivSound, Sound?>()
     
@@ -29,16 +58,25 @@ object Sounds {
         if (!UncivGame.isCurrentInitialized()) return
         val game = UncivGame.Current
 
-        // Get a hash covering all mods - quickly, so don't map cast or copy the Set types
+        // Get a hash covering all mods - quickly, so don't map, cast or copy the Set types
         val hash1 = if (game.isGameInfoInitialized()) game.gameInfo.ruleSet.mods.hashCode() else 0
         val newHash = hash1.xor(game.settings.visualMods.hashCode())
+
         // If hash the same, leave the cache as is
         if (modListHash != Int.MIN_VALUE && modListHash == newHash) return
 
         // Seems the mod list has changed - clear the cache
+        clearCache()
+        modListHash = newHash
+        if (debugMessages) println("Sound cache cleared")
+    }
+
+    /** Release cached Sound resources */
+    // Called from UncivGame.dispose() to honor Gdx docs
+    fun clearCache() {
         for (sound in soundMap.values) sound?.dispose()
         soundMap.clear()
-        modListHash = newHash
+        modListHash = Int.MIN_VALUE
     }
 
     /** Build list of folders to look for sounds */
@@ -50,40 +88,88 @@ object Sounds {
         val game = UncivGame.Current
 
         // Allow mod sounds - preferentially so they can override built-in sounds
-        // audiovisual mods first, these are already available when game.gameInfo is not 
+        // audiovisual mods after game mods but before built-in sounds
-        val modList: MutableSet<String> = game.settings.visualMods
+        // (these can already be available when game.gameInfo is not)
+        val modList: MutableSet<String> = mutableSetOf()
         if (game.isGameInfoInitialized())
             modList.addAll(game.gameInfo.ruleSet.mods)  // Sounds from game mods
+        modList.addAll(game.settings.visualMods)
 
+        // Translate the basic mod list into relative folder names so only sounds/name.ext needs
+        // to be added. Thus the empty string, added last, represents the builtin sounds folder.
         return modList.asSequence()
             .map { "mods$separator$it$separator" } +
-            sequenceOf("")  // represents builtin sounds folder
+            sequenceOf("")
     } 
 
-    fun get(sound: UncivSound): Sound? {
+    /** Holds a Gdx Sound and a flag indicating the sound is freshly loaded and not from cache */
+    private data class GetSoundResult(val resource: Sound, val isFresh: Boolean)
+
+    /** Retrieve (if not cached create from resources) a Gdx Sound from an UncivSound
+     * @param sound The sound to fetch
+     * @return `null` if file cannot be found, a [GetSoundResult] otherwise
+     */
+    private fun get(sound: UncivSound): GetSoundResult? {
         checkCache()
-        if (sound in soundMap) return soundMap[sound]
+        // Look for cached sound
+        if (sound in soundMap) 
+            return if(soundMap[sound] == null) null
+            else GetSoundResult(soundMap[sound]!!, false)
+
+        // Not cached - try loading it
         val fileName = sound.value
         var file: FileHandle? = null
-        for ( (modFolder, extension) in getFolders().flatMap { 
+        for ( (modFolder, extension) in getFolders().flatMap {
+            // This is essentially a cross join. To operate on all combinations, we pack both lambda
+            // parameters into a Pair (using `to`) and unwrap that in the loop using automatic data
+            // class deconstruction `(,)`. All this avoids a double break when a match is found.
             folder -> SupportedExtensions.values().asSequence().map { folder to it } 
         } ) {
             val path = "${modFolder}sounds$separator$fileName.${extension.name}"
+            file = Gdx.files.local(path)
+            if (file.exists()) break
             file = Gdx.files.internal(path)
             if (file.exists()) break
         }
-        val newSound =
-            if (file == null || !file.exists()) null
-            else Gdx.audio.newSound(file)
 
-        // Store Sound for reuse or remember that the actual file is missing
+        @Suppress("LiftReturnOrAssignment")
-        soundMap[sound] = newSound
+        if (file == null || !file.exists()) {
-        return newSound
+            if (debugMessages) println("Sound ${sound.value} not found!")
+            // remember that the actual file is missing
+            soundMap[sound] = null
+            return null
+        } else {
+            if (debugMessages) println("Sound ${sound.value} loaded from ${file.path()}")
+            val newSound = Gdx.audio.newSound(file)
+            // Store Sound for reuse
+            soundMap[sound] = newSound
+            return GetSoundResult(newSound, true)
+        }
     }
 
+    /** Find, cache and play a Sound
+     *
+     * Sources are mods from a loaded game, then mods marked as permanent audiovisual,
+     * and lastly Unciv's own assets/sounds. Will fail silently if the sound file cannot be found.
+     * 
+     * This will wait for the Stream to become ready (Android issue) if necessary, and do so on a
+     * separate thread. No new thread is created if the sound can be played immediately.
+     * 
+     * @param sound The sound to play
+     */
     fun play(sound: UncivSound) {
         val volume = UncivGame.Current.settings.soundEffectsVolume
         if (sound == UncivSound.Silent || volume < 0.01) return
-        get(sound)?.play(volume)
+        val (resource, isFresh) = get(sound) ?: return
+        val initialDelay = if (isFresh && Gdx.app.type == Application.ApplicationType.Android) 40 else 0
+
+        if (initialDelay > 0 || resource.play(volume) == -1L) {
+            thread (name = "DelayedSound") {
+                Thread.sleep(initialDelay.toLong())
+                while (resource.play(volume) == -1L) {
+                    Thread.sleep(20L)
+                }
+            }
+        }
     }
-}
+}