Skip to content

Add delay#8

Closed
jsfillman wants to merge 7 commits into
mainfrom
add_delay
Closed

Add delay#8
jsfillman wants to merge 7 commits into
mainfrom
add_delay

Conversation

@jsfillman

@jsfillman jsfillman commented May 27, 2025

Copy link
Copy Markdown
Owner

PR Type

Enhancement, Documentation


Description

  • Added dual 3-tap stereo delay effect to DSP and UI

    • 18 new OSC-controllable delay parameters (L/R, 3 taps each, feedback, mix)
    • Delay can be enabled/disabled per channel
    • Integrated delay into main DSP signal path
  • Major UI update: new Effects tab with full delay controls

    • All delay parameters exposed as knobs/sliders in PyQt6 interface
    • MIDI CC mapping for all delay controls
    • Improved layout and usability for synthesis/effects
  • Added comprehensive documentation for dual 3-tap delay implementation

  • Added flake8 config and minor code style improvements


Changes walkthrough 📝

Relevant files
Enhancement
2 files
legato_synth.dsp
Add dual 3-tap stereo delay to DSP, 18 OSC params               
+68/-1   
main_window.py
Add Effects tab and full delay UI controls                             
+482/-23
Documentation
1 files
dual-3tap-delay-implementation.md
Add detailed dual 3-tap delay implementation doc                 
+242/-0 
Configuration changes
1 files
.flake8
Add flake8 config for code style enforcement                         
+4/-0     
Formatting
10 files
melody.py
Add noqa for E402 import order linting                                     
+1/-1     
murnau_ui.py
Add noqa for E402 import order linting                                     
+3/-3     
ramp_test.py
Add noqa for E402 import order linting                                     
+1/-1     
run_murnau.py
Add noqa for E402 import order linting                                     
+3/-3     
ramp_test.py
Minor print formatting improvement                                             
+3/-1     
widgets.py
Minor import cleanup and code style                                           
+2/-4     
test_dsp_init.py
Add noqa for E402 import order linting                                     
+1/-1     
test_melody.py
Add noqa for E402 import order linting, remove unused code
+1/-14   
test_osc_client.py
Add noqa for E402 import order linting                                     
+1/-1     
test_ramp_test.py
Add noqa for E402 import order linting, remove unused code
+1/-8     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review

    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Feedback Implementation

    The delay implementation is missing proper feedback loops despite having feedback parameters. The current implementation creates taps from the input signal only, not from the delay line output.

    delay_L_process(input) = output_L
    with {
        // Three taps using @ operator for delays
        tap1_L = input : @(ms_to_samples(delay_L_tap1_time)) : *(delay_L_tap1_level);
        tap2_L = input : @(ms_to_samples(delay_L_tap2_time)) : *(delay_L_tap2_level);
        tap3_L = input : @(ms_to_samples(delay_L_tap3_time)) : *(delay_L_tap3_level);
    
        // Sum taps
        taps_sum_L = tap1_L + tap2_L + tap3_L;
    
        // Dry/wet mix
        output_L = select2(delay_L_enable,
            input,  // Bypass when disabled
            input * (1 - delay_L_mix) + taps_sum_L * delay_L_mix
        );
    };
    Delay Enable State

    The delay enable checkboxes are created but their initial state is not set to match the default disabled state sent in init_parameters.

    self.delay_L_enable = QCheckBox("Enable Left")
    self.delay_L_enable.setStyleSheet(
        """
        QCheckBox {
            color: #E0E0E0;
            font-weight: bold;
            font-size: 12px;
            spacing: 5px;
        }
        QCheckBox::indicator {
            width: 18px;
            height: 18px;
        }
        QCheckBox::indicator:checked {
            background-color: #5D5236;
            border: 2px solid #D4BF8A;
        }
    """
    )
    self.delay_L_enable.stateChanged.connect(self.on_delay_L_enable_change)
    enable_layout.addWidget(self.delay_L_enable)
    
    self.delay_R_enable = QCheckBox("Enable Right")
    self.delay_R_enable.setStyleSheet(
        """
        QCheckBox {
            color: #E0E0E0;
            font-weight: bold;
            font-size: 12px;
            spacing: 5px;
        }
        QCheckBox::indicator {
            width: 18px;
            height: 18px;
        }
        QCheckBox::indicator:checked {
            background-color: #5D5236;
            border: 2px solid #D4BF8A;
        }
    """
    )
    self.delay_R_enable.stateChanged.connect(self.on_delay_R_enable_change)
    enable_layout.addWidget(self.delay_R_enable)

    @qodo-code-review

    qodo-code-review Bot commented May 27, 2025

    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Implement feedback loop

    The delay implementation is missing the feedback loop described in the
    documentation. Without feedback, the delay effect will be limited to simple
    echoes rather than the more complex ambient textures mentioned in the
    specification.

    src/murnau/dsp/legato_synth.dsp [110-126]

    -// Left channel delay processing (basic 3-tap, no feedback for now)
    +// Left channel delay processing with feedback
     delay_L_process(input) = output_L
     with {
    -    // Three taps using @ operator for delays
    -    tap1_L = input : @(ms_to_samples(delay_L_tap1_time)) : *(delay_L_tap1_level);
    -    tap2_L = input : @(ms_to_samples(delay_L_tap2_time)) : *(delay_L_tap2_level);
    -    tap3_L = input : @(ms_to_samples(delay_L_tap3_time)) : *(delay_L_tap3_level);
    +    // Create feedback loop with delay line
    +    delay_line = de.sdelay(max_delay_samples, 512);
    +    feedback_input = input + (feedback_signal * delay_L_feedback);
    +    delay_signal = feedback_input : delay_line;
         
    -    // Sum taps
    -    taps_sum_L = tap1_L + tap2_L + tap3_L;
    +    // Three taps from the delay line
    +    tap1_L = delay_signal(ms_to_samples(delay_L_tap1_time)) : *(delay_L_tap1_level);
    +    tap2_L = delay_signal(ms_to_samples(delay_L_tap2_time)) : *(delay_L_tap2_level);
    +    tap3_L = delay_signal(ms_to_samples(delay_L_tap3_time)) : *(delay_L_tap3_level);
    +    
    +    // Sum taps for feedback and output
    +    feedback_signal = tap1_L + tap2_L + tap3_L;
         
         // Dry/wet mix
         output_L = select2(delay_L_enable,
             input,  // Bypass when disabled
    -        input * (1 - delay_L_mix) + taps_sum_L * delay_L_mix
    +        input * (1 - delay_L_mix) + feedback_signal * delay_L_mix
         );
     };
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that feedback is missing from the delay implementation, but the code comment explicitly states "no feedback for now", indicating this may be intentionally omitted in this implementation phase. The suggested improvement is technically sound but not critical.

    Low
    Prevent zero-delay issues

    The current implementation doesn't ensure a minimum delay time greater than
    zero, which could cause issues with the delay line. Adding a small offset to the
    delay time calculation will prevent potential zero-delay problems.

    src/murnau/dsp/legato_synth.dsp [106-108]

    -// Convert milliseconds to samples
    -ms_to_samples(ms) = ms * ma.SR / 1000;
    +// Convert milliseconds to samples with safety offset
    +ms_to_samples(ms) = max(1, ms * ma.SR / 1000);
     max_delay_samples = ms_to_samples(2000);
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion provides a minor safety improvement by ensuring minimum delay of 1 sample, but since delay parameters have defined ranges starting from 0, zero delay might be intentionally allowed. This is a defensive programming practice but not critical.

    Low
    • Update

    @jsfillman jsfillman closed this May 27, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant