path and sound handling improvements - #504
Open
Trass3r wants to merge 9 commits into
Open
Conversation
Consistent and simple cross-platform path handling. Remove File dependency from PathUtils in favor of Path.
- replace FILENAME_CACHE HashMap with ConcurrentHashMap to fix data race - replace PathTree/PathNode custom trie with flat ConcurrentHashMap map - replace FileFinder.walkFileTree with simple DirectoryStream loop - remove unused PathNode getters, hashCode, equals
Replaced recursive getNext() with a loop that safely returns null when all sound groups have empty file lists. The caller playBackground() now handles null gracefully instead of crashing. This fixes an infinite recursion that occurs with the DK2 demo where the MUSIC category exists but contains no sound files.
Instead of unconditionally calling setAudioRenderer(null), check whether all three audio categories (MusicEnabled, VoiceEnabled, SfxEnabled) are disabled. Only then set the audio renderer to null. This removes the need for a separate AudioEnabled config option.
Collaborator
Author
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes asset/path string handling around forward slashes and improves audio robustness (including running without an audio device by disabling the audio renderer when all audio categories are off).
Changes:
- Replace many
File.separatorconcatenations with forward-slash paths and trailing-slash folder constants for asset keys. - Refactor
PathUtilscase-insensitive path resolution/caching implementation (ConcurrentHashMap-based) and simplify separator conversion. - Prevent sound initialization/playback paths from throwing when audio is disabled or background music selection runs out of files.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/toniarts/openkeeper/view/map/Water.java | Simplifies texture asset key concatenation. |
| src/main/java/toniarts/openkeeper/view/KeeperHandState.java | Uses forward-slash asset key building for icons. |
| src/main/java/toniarts/openkeeper/view/control/TrapFlowerControl.java | Simplifies texture key construction for flower icons. |
| src/main/java/toniarts/openkeeper/utils/PathUtils.java | Reworks path resolution caches and standardizes separators to /. |
| src/main/java/toniarts/openkeeper/utils/MapThumbnailGenerator.java | Uses a forward-slash palette asset path constant. |
| src/main/java/toniarts/openkeeper/utils/AssetUtils.java | Updates asset key construction to match new folder constants and / normalization. |
| src/main/java/toniarts/openkeeper/tools/modelviewer/SoundsLoader.java | Updates sound asset paths to match new folder constant format. |
| src/main/java/toniarts/openkeeper/tools/modelviewer/ModelViewerScreenController.java | Simplifies resource texture lookup paths. |
| src/main/java/toniarts/openkeeper/tools/modelviewer/ModelViewer.java | Disables audio renderer when audio is disabled; path concatenation simplifications. |
| src/main/java/toniarts/openkeeper/tools/convert/wad/WadFile.java | Updates separator parsing to use /. |
| src/main/java/toniarts/openkeeper/tools/convert/map/KwdFile.java | Normalizes art resource names to /; adjusts basePath handling. |
| src/main/java/toniarts/openkeeper/tools/convert/KmfModelLoader.java | Updates model/material/texture asset key building to match new folder constants. |
| src/main/java/toniarts/openkeeper/tools/convert/Bf4Extractor.java | Uses / when composing output paths. |
| src/main/java/toniarts/openkeeper/tools/convert/AssetsConverter.java | Standardizes converted-assets folder constants to include trailing / and simplifies task path composition. |
| src/main/java/toniarts/openkeeper/Main.java | Adds isAudioDisabled() and disables audio renderer when all audio categories are off; tweaks GL debug filtering; uses / in user folders. |
| src/main/java/toniarts/openkeeper/gui/nifty/NiftyUtils.java | Updates sound asset path concatenation. |
| src/main/java/toniarts/openkeeper/gui/Cursor.java | Updates cursor texture path construction to match new constants/separators. |
| src/main/java/toniarts/openkeeper/game/state/SoundState.java | Fixes background selection iteration and handles no-next-file case; updates sound paths. |
| src/main/java/toniarts/openkeeper/game/state/PlayerScreenController.java | Standardizes GUI icon paths and skips sound playback when audio disabled. |
| src/main/java/toniarts/openkeeper/game/state/MainMenuState.java | Updates thumbnail folder constant usage. |
| src/main/java/toniarts/openkeeper/game/state/MainMenuScreenController.java | Skips menu sound registration when audio disabled; updates sound path building. |
| src/main/java/toniarts/openkeeper/game/sound/SoundGroup.java | Avoids NPE on blank SDT entries and standardizes separator handling. |
| src/main/java/toniarts/openkeeper/game/sound/SoundCategory.java | Simplifies folder path building and separator usage. |
| .github/workflows/gradle.yml | Updates action versions, Java version, and Gradle invocation. |
Comments suppressed due to low confidence (1)
src/main/java/toniarts/openkeeper/utils/PathUtils.java:245
- In the one-segment shortcut, resolveFileName() also returns p.toRealPath().toString() without normalizing separators. This can again introduce backslashes into caches and break later split("/") logic on Windows.
// Try one-segment shortcut: look it up directly from certainPath
if (nonEmpty.size() == 1 && !certainPath.equalsIgnoreCase(realPath)) {
Path p = Paths.get(certainPath, nonEmpty.get(0));
if (Files.exists(p))
return p.toRealPath().toString();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
104
to
109
| public static String fixFilePath(final String folderPath) { | ||
| if (!folderPath.endsWith(File.separator)) { | ||
| return folderPath.concat(File.separator); | ||
| if (!folderPath.endsWith("/")) { | ||
| return folderPath + '/'; | ||
| } | ||
| return folderPath; | ||
| } |
Comment on lines
+218
to
+222
| // Try exact match first | ||
| Path testFile = Paths.get(fileName); | ||
| if (Files.exists(testFile)) | ||
| return testFile.toRealPath().toString(); | ||
|
|
Comment on lines
+339
to
+344
| // Preserve trailing '/' for directory results (matches original FileFinder behavior) | ||
| String result = current.toRealPath().toString(); | ||
| if (Files.isDirectory(current)) { | ||
| result = result + '/'; | ||
| } | ||
| return result; |
| throw new RuntimeException("Failed to read the file " + file + "!", e); | ||
| } | ||
| this.basePath = PathUtils.fixFilePath(basePath); | ||
| this.basePath = basePath; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplify path handling, fix sound-related exceptions and support systems/containers without audio device.