Add Multiple New Cabinet IRs - #70
Conversation
There was a problem hiding this comment.
Pull Request Overview
Add recursive IR cabinet scanning with lazy loading and include multiple new IR assets and presets; update GUI to mirror recursive scanning and adjust naming to use relative paths.
- Switch from eager loading of IRs to scanning/storing IR paths and lazy-loading on selection
- Add recursive directory scanning in both DSP (sim) and GUI, with normalized relative-path identifiers and sorting by depth
- Add IR assets/presets and update README
Reviewed Changes
Copilot reviewed 4 out of 62 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sim/ir_cabinet.rs | Refactor to store IR paths, add recursive scan, lazy-load IRs by index/name, and minor processing cleanups |
| src/gui/app.rs | Add recursive IR directory scan for UI and sorting to match backend |
| ir/Jesterdyne/_readme_and_license.txt | Add licensing/readme for included IR pack |
| ir/Jesterdyne/Jensen/presets/*.json | Add example presets (Clean, Crunch, Lead) |
| README.md | Update features and attribution for included IRs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| pub fn scan_ir_directory(&mut self) -> Result<()> { | ||
| if !self.ir_directory.exists() { | ||
| fs::create_dir_all(&self.ir_directory).context("Failed to create IR directory")?; | ||
| warn!("IR directory created at {:?}", self.ir_directory); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| self.available_irs.clear(); | ||
| for entry in fs::read_dir(ir_directory)? { | ||
| self.available_ir_paths.clear(); | ||
| let base = self.ir_directory.clone(); | ||
| self.scan_recursive(&base, &base)?; | ||
|
|
||
| self.available_ir_paths.sort_by(|a, b| { | ||
| let a_sep_count = a.0.matches('/').count(); | ||
| let b_sep_count = b.0.matches('/').count(); | ||
| a_sep_count.cmp(&b_sep_count).then_with(|| a.0.cmp(&b.0)) | ||
| }); | ||
|
|
There was a problem hiding this comment.
IR scanning/sorting is duplicated in the GUI (see src/gui/app.rs:scan_ir_directory/scan_ir_recursive). Consider making this the single source of truth and have the GUI query the backend (e.g., via get_available_irs and a rescan call) to avoid divergence in path normalization, sorting, and filtering logic.
| fn scan_ir_recursive( | ||
| current_dir: &Path, | ||
| base_dir: &Path, | ||
| irs: &mut Vec<String>, | ||
| ) -> Result<(), std::io::Error> { | ||
| for entry in std::fs::read_dir(current_dir)? { | ||
| let entry = entry?; | ||
| let path = entry.path(); | ||
| if path.extension().and_then(|s| s.to_str()) == Some("wav") | ||
| && let Some(name) = path.file_stem().and_then(|s| s.to_str()) | ||
| { | ||
| irs.push(name.to_string()); | ||
|
|
||
| if path.is_dir() { | ||
| // Recursively scan subdirectories | ||
| Self::scan_ir_recursive(&path, base_dir, irs)?; | ||
| } else if path.extension().and_then(|s| s.to_str()) == Some("wav") { | ||
| // Get relative path from base_dir | ||
| let relative_path = path | ||
| .strip_prefix(base_dir) | ||
| .unwrap_or(&path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); // Normalize path separators | ||
| irs.push(relative_path); | ||
| } | ||
| } | ||
|
|
||
| Ok(irs) | ||
| Ok(()) |
There was a problem hiding this comment.
This re-implements IR discovery already present in the backend. To prevent UI/backend drift and reduce duplication, prefer calling into IrCabinet to rescan and fetch the list of available IRs rather than scanning the filesystem again in the GUI.
| pub fn get_available_irs(&self) -> Vec<String> { | ||
| self.available_irs | ||
| self.available_ir_paths | ||
| .iter() | ||
| .map(|ir| ir.name.clone()) | ||
| .map(|(name, _)| name.clone()) |
There was a problem hiding this comment.
[nitpick] get_available_irs now returns filesystem-like identifiers (relative paths with extensions). Consider returning a richer struct or separate fields (e.g., id = relative path, display_name = file_stem) so the UI can show clean labels while keeping a stable key, and so future changes to folder structure/extensions don’t break selections.
| self.r2c | ||
| .process(&mut time_block, &mut freq_block) | ||
| .expect("realfft forward failed"); |
There was a problem hiding this comment.
Avoid expect in DSP paths; panicking in an audio thread can crash the audio engine. If this call is infallible given preconditions, consider a debug assertion; otherwise, handle the error by logging and skipping the partition or silencing the block to fail gracefully.
| } else { | ||
| mono | ||
| }; | ||
|
|
There was a problem hiding this comment.
Add a brief comment explaining the rationale for MAX_IR_LENGTH (e.g., ~2s at 48 kHz) and its impact on latency/CPU so future changes are intentional. Optionally consider making it configurable.
| // Maximum allowed IR length in samples. | |
| // 96000 samples ≈ 2 seconds at 48 kHz sample rate. | |
| // Longer IRs increase CPU usage and latency, but allow for longer reverbs. | |
| // Adjust with care; consider making this configurable if needed. |
No description provided.