Add GUI with parameter controls and pitch readout

[user1303836]

Phase 5: GUI implementation.

Changes

• 5 rotary sliders (Mix, Rate Mult, Manual Rate, Smoothing, Sensitivity) with APVTS attachments for bidirectional host sync
• 2 combo boxes (Mode, Waveform) with APVTS attachments
• Pitch readout label updated via Timer at 30Hz from processor atomics -- displays note name + frequency (e.g. "E2 82.4 Hz") when tracking, "--" when silent or low confidence
• NoteNames.h utility -- frequency-to-note-name conversion using standard MIDI math (A4 = 440Hz reference)

Layout

600x400 window:

• Title bar at top
• Pitch readout below title
• 5 rotary knobs in a row
• Mode and Waveform dropdowns at bottom

[user1303836]

PR #9 Review: Add GUI with parameter controls and pitch readout

Verdict: One important safety issue to address. DAW automation/modulation compatibility is good overall.

This PR implements Phase 5 (GUI) with APVTS-attached controls, meaning all 7 parameters are fully accessible to the host DAW (Ableton, Logic, Reaper, etc.) for automation, modulation, MIDI learn, and preset recall. The implementation follows the standard JUCE pattern correctly.

DAW Automation / Modulation Compatibility Assessment

Capability	Status	Notes
Parameters visible in DAW automation lanes	OK	All 7 params registered via APVTS in PR #2 with display names and labels
Automation write (user moves knob -> DAW records)	OK	SliderAttachment/ComboBoxAttachment handle bidirectional sync
Automation read (DAW drives param -> knob moves)	OK	Attachments update slider position from host changes
Ableton "Configure" parameter capture	OK	Touching any knob will register the parameter
Ableton modulation (LFO/envelope on params)	OK	APVTS params are standard VST3 parameters, fully modulatable
Preset save/recall via host	OK	APVTS XML serialization in processor (PR #2) handles this
Parameter state on plugin close/reopen	OK	getStateInformation/setStateInformation round-trip via APVTS

Issues

Priority	Item	Location
High	Attachment destruction order -- currently safe by coincidence (member declaration order), but fragile. Explicitly .reset() attachments in destructor before implicit member destruction to prevent DAW crashes on plugin close.	PluginEditor.h:23-30
Medium	ComboBox items duplicated between editor and processor parameter layout. If choices drift out of sync, DAW automation could select wrong mode/waveform. Read choices from the APVTS parameter instead.	PluginEditor.cpp:40
Low	Unused titleArea variable	PluginEditor.cpp:69

What's done well

• APVTS SliderAttachment/ComboBoxAttachment pattern used correctly for all 7 parameters -- this is the standard approach that guarantees DAW compatibility
• RotaryHorizontalVerticalDrag slider style works well in DAW contexts (mouse drag, wheel, double-click reset)
• Timer-based pitch readout at 30Hz using atomics -- real-time safe, no locking on the audio thread
• NoteNames utility uses correct MIDI math with A4=440Hz reference
• Layout is clean and organized with visual grouping (title, pitch readout, knobs, dropdowns)
• Destructor calls stopTimer() -- correct, prevents timer callbacks after destruction begins

Merge order

This is the only open PR. Targets main. I recommend addressing the attachment destruction order issue before merging, as it could cause crashes during plugin teardown in a DAW session.

[user1303836]

DAW automation/modulation concern: Attachment objects are created AFTER slider setup -- this is correct.

The JUCE APVTS attachment pattern requires that the slider/combobox is fully set up (style, text box, added to component hierarchy) before the attachment is created. Creating the attachment connects the UI control to the host parameter bidirectionally:

• GUI knob change -> updates the APVTS parameter -> host sees the change (automation write)
• Host automation/modulation changes parameter -> APVTS notifies attachment -> slider moves visually

This ordering is correct. The attachments ensure that all 7 parameters are fully accessible to Ableton (or any DAW) for:

• Automation lanes (drawing/recording automation)
• Modulation (Ableton's LFO/envelope modulators applied to VST parameters)
• MIDI learn / mapping
• Preset save/recall via the host

One thing to verify in Ableton specifically: the parameter names shown in the DAW come from the AudioParameterFloat/AudioParameterChoice constructor's display name (e.g., "Mix", "Rate Multiplier", "Manual Rate") -- these were set up in PR #2 and look good.

[user1303836]

Critical: Attachment destruction order matters for DAW stability.

JUCE's SliderAttachment and ComboBoxAttachment destructors remove the listener from the slider/combobox. If the slider is destroyed before the attachment, the attachment destructor will try to access a dangling reference, causing a crash -- potentially during Ableton session teardown.

C++ destroys members in reverse declaration order. In the current layout:

juce::Slider mixSlider, ...          // declared first
...
std::unique_ptr<SliderAttachment> mixAttach, ...  // declared after

This means attachments are destroyed before the sliders. This is correct and safe.

However, this is a subtle invariant. If someone reorders these member declarations in a future refactor, it could cause crashes on plugin close in the DAW. Consider adding a comment to guard this, or explicitly calling .reset() on the attachments in the destructor before the implicit member destruction occurs:

HdnRingmodAudioProcessorEditor::~HdnRingmodAudioProcessorEditor()
{
    stopTimer();
    // Destroy attachments before their associated UI components
    mixAttach.reset();
    rateMultAttach.reset();
    // ... etc
}

This makes the destruction order explicit and safe regardless of member declaration order. This is the most important feedback in this review -- a wrong destruction order can crash the DAW.

[user1303836]

RotaryHorizontalVerticalDrag is a good choice for DAW compatibility. This slider style supports:

• Mouse drag (vertical and horizontal) for direct interaction
• Mouse wheel for fine adjustment
• Double-click to reset to default (JUCE built-in)
• Host parameter mapping / automation write-back via the attachment

In Ableton, when a user clicks "Configure" on the plugin and touches a knob, Ableton captures the parameter for automation/modulation. The APVTS attachment ensures this works seamlessly.

One UX consideration: JUCE's default rotary slider doesn't show the current automation value when the host is overriding the parameter. This is handled by the attachment -- the slider will visually track wherever the host drives the parameter to. Good.

[user1303836]

ComboBox items are duplicated between the editor and processor.

The choices "Pitch Track" / "Manual" and "Sine" / "Triangle" / "Square" / "Saw" are defined in both createParameterLayout() (PR #2) and here. If they ever get out of sync (e.g., someone adds a "Sawtooth" alias or a new waveform), the ComboBox indices won't match the parameter values, causing wrong mode/waveform selection.

Consider reading the choices from the APVTS parameter itself:

if (auto* param = dynamic_cast<juce::AudioParameterChoice*>(p.apvts.getParameter(ParameterIDs::mode)))
    modeBox.addItemList(param->choices, 1);

This guarantees the UI always matches the parameter definition and prevents DAW automation from selecting wrong values. This applies to both modeBox and waveformBox.

[user1303836]

Timer-based pitch readout at 30Hz is correct for GUI updates. This reads the processor's atomic values without locking, which is real-time safe. The update rate is fast enough to feel responsive but doesn't waste CPU.

The 0.1f confidence threshold for showing pitch vs "--" is a reasonable display threshold. Note that this is separate from the DSP sensitivity parameter (which gates whether the oscillator frequency updates). The display might show "--" even when the oscillator is still ring modulating at the last good frequency. This is actually correct -- the display shows "what the detector currently sees" while the DSP shows "what's being used."

[user1303836]

Potential negative octave display for very low frequencies. If rounded is negative (which happens for frequencies below ~8.18 Hz, i.e., MIDI note 0 = C-1), the modulo and division could produce odd results. The noteIndex < 0 guard handles the modulo correctly, but octave could be negative (e.g., -2), producing display strings like "C-2".

Given the YIN detector only outputs 20-5000 Hz, the practical minimum is around E1 (41 Hz, MIDI ~28), which gives octave 1. So this won't happen in practice. Just noting the edge case.

The static const char* names[] array inside the function is fine -- it's initialized once and never modified. Using static here avoids re-creating the array on every call.

[user1303836]

titleArea is extracted but never used. This variable captures the top 40px but nothing is placed in it -- the title is drawn directly in paint() at hardcoded coordinates (0, 8, getWidth(), 30). The removeFromTop(40) call correctly reserves the space so sliders don't overlap the title, but the variable itself is unused. A minor cleanup:

area.removeFromTop(40); // reserved for title drawn in paint()