-
Notifications
You must be signed in to change notification settings - Fork 7
refactor: add unified InstrumentEngine interface and factory #1038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * Unified interface for all instrument engines (subtractive synth, sampler, FM). | ||
| * | ||
| * Every instrument engine implementation must conform to this interface so that | ||
| * playback, recording, and automation code can operate on any instrument kind | ||
| * without branching on the concrete type. | ||
| */ | ||
| export interface InstrumentEngine { | ||
| /** Trigger note-on for a track (for live playing / recording). */ | ||
| noteOn(trackId: string, pitch: number, velocity: number): void; | ||
|
|
||
| /** Trigger note-off for a track. */ | ||
| noteOff(trackId: string, pitch: number): void; | ||
|
|
||
| /** Play a note with a fixed duration (for sequenced playback). */ | ||
| triggerAttackRelease(trackId: string, pitch: number, duration: number, velocity: number): void; | ||
|
|
||
| /** | ||
| * Set an engine-specific parameter by name. | ||
| * | ||
| * This is a generic escape hatch for automation and preset changes. | ||
| * Implementations may ignore unknown parameter names. | ||
| */ | ||
| setParameter(trackId: string, name: string, value: number | string | boolean): void; | ||
|
|
||
| /** Release all currently sounding notes across every track. */ | ||
| releaseAll(): void; | ||
|
|
||
| /** Tear down resources associated with a single track. */ | ||
| removeTrack(trackId: string): void; | ||
|
|
||
| /** Dispose the entire engine and release all resources. */ | ||
| dispose(): void; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,138 @@ | ||||||||||||||||
| import type { TrackInstrument } from '../types/project'; | ||||||||||||||||
| import type { InstrumentEngine } from './InstrumentEngine'; | ||||||||||||||||
| import { synthEngine } from './SynthEngine'; | ||||||||||||||||
| import { samplerEngine } from './SamplerEngine'; | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * Adapter that wraps the legacy {@link SynthEngine} singleton to conform to | ||||||||||||||||
| * the {@link InstrumentEngine} interface. | ||||||||||||||||
| */ | ||||||||||||||||
| class SynthEngineAdapter implements InstrumentEngine { | ||||||||||||||||
| noteOn(trackId: string, pitch: number, velocity: number): void { | ||||||||||||||||
| synthEngine.noteOn(trackId, pitch, velocity); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| noteOff(trackId: string, pitch: number): void { | ||||||||||||||||
| synthEngine.noteOff(trackId, pitch); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| triggerAttackRelease(trackId: string, pitch: number, duration: number, velocity: number): void { | ||||||||||||||||
| // Delegate to the singleton which handles MIDI-to-freq conversion and preset routing. | ||||||||||||||||
| void synthEngine.playNote(trackId, pitch, velocity, duration, 'piano'); | ||||||||||||||||
|
Comment on lines
+20
to
+21
|
||||||||||||||||
| // Delegate to the singleton which handles MIDI-to-freq conversion and preset routing. | |
| void synthEngine.playNote(trackId, pitch, velocity, duration, 'piano'); | |
| // Use the already-configured synth for this track instead of forcing a specific preset. | |
| this.noteOn(trackId, pitch, velocity); | |
| setTimeout(() => { | |
| this.noteOff(trackId, pitch); | |
| }, duration); |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SamplerEngineAdapter.triggerAttackRelease forwards velocity directly, but SamplerEngine.triggerAttackRelease appears to expect a normalized 0–1 velocity (call sites clamp note.velocity to 0–1, and noteOn converts MIDI 0–127 to 0–1). If a consumer uses MIDI velocity (0–127) like SynthEngineAdapter does, this will drive the sampler gain envelope far above 1. Please standardize the velocity scale for InstrumentEngine and convert here accordingly.
| samplerEngine.triggerAttackRelease(trackId, pitch, duration, velocity); | |
| // InstrumentEngine uses MIDI velocity (0–127); SamplerEngine.triggerAttackRelease expects 0–1. | |
| const clampedVelocity = Math.max(0, Math.min(127, velocity)); | |
| const normalizedVelocity = clampedVelocity / 127; | |
| samplerEngine.triggerAttackRelease(trackId, pitch, duration, normalizedVelocity); |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SamplerEngineAdapter.setParameter is a no-op even though SamplerEngine now defines a setParameter method. Delegating to samplerEngine.setParameter(...) would keep parameter-handling behavior in one place and prevents the adapter and engine from drifting as real-time automation gets implemented.
| setParameter(_trackId: string, _name: string, _value: number | string | boolean): void { | |
| // Sampler parameters (ADSR, root note, etc.) are set via ensureTrackSampler config. | |
| // A future PR will wire individual param updates here. | |
| setParameter(trackId: string, name: string, value: number | string | boolean): void { | |
| samplerEngine.setParameter(trackId, name, value); |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FM adapter’s doc says kind: 'fm' tracks fall back to the subtractive synth using FmTrackInstrument.fallbackPreset, but the implementation never receives or applies fallbackPreset (it always delegates to the subtractive adapter behavior). Either remove/adjust this documentation or add a mechanism to pass/apply fallbackPreset (e.g., via a standardized setParameter name).
| * `kind: 'fm'` fall back to the subtractive synth using {@link FmTrackInstrument.fallbackPreset}. | |
| * Once a dedicated FM engine exists this adapter will delegate to it. | |
| * `kind: 'fm'` currently fall back to the subtractive synth via | |
| * {@link SynthEngineAdapter}. FM-specific parameters (including any | |
| * fallback preset) are not yet applied here. Once a dedicated FM engine | |
| * exists this adapter will delegate to it. |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FmEngineAdapter creates a new SynthEngineAdapter instance for fallback even though the factory already maintains a singleton subtractiveAdapter. Reusing the existing subtractive adapter avoids duplicated wrapper instances and reduces the risk of divergent behavior if the adapter later gains per-track state/configuration.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| import { describe, it, expect } from 'vitest'; | ||
| import { | ||
| getEngineForInstrument, | ||
| SynthEngineAdapter, | ||
| SamplerEngineAdapter, | ||
| FmEngineAdapter, | ||
| } from '../InstrumentFactory'; | ||
| import type { InstrumentEngine } from '../InstrumentEngine'; | ||
| import type { | ||
| SubtractiveTrackInstrument, | ||
| SamplerTrackInstrument, | ||
| FmTrackInstrument, | ||
| } from '../../types/project'; | ||
|
|
||
| // ── Fixtures ─────────────────────────────────────────────────────────────── | ||
|
|
||
| const subtractiveInstrument: SubtractiveTrackInstrument = { | ||
| kind: 'subtractive', | ||
| preset: 'piano', | ||
| name: 'Test Piano', | ||
| settings: { | ||
| oscillator: { waveform: 'triangle', octave: 0, detuneCents: 0, level: 1 }, | ||
| ampEnvelope: { attack: 0.005, decay: 0.3, sustain: 0.2, release: 1.2 }, | ||
| filter: { enabled: false, type: 'lowpass', cutoffHz: 20000, resonance: 1, drive: 0, keyTracking: 0 }, | ||
| filterEnvelope: { attack: 0.01, decay: 0.1, sustain: 1, release: 0.3, amount: 0 }, | ||
| lfo: { enabled: false, waveform: 'sine', target: 'off', rateHz: 1, depth: 0, retrigger: false }, | ||
| unison: { voices: 1, detuneCents: 0, stereoSpread: 0, blend: 0 }, | ||
| glideTime: 0, | ||
| outputGain: 1, | ||
| }, | ||
| }; | ||
|
|
||
| const samplerInstrument: SamplerTrackInstrument = { | ||
| kind: 'sampler', | ||
| preset: 'sampler', | ||
| name: 'Test Sampler', | ||
| settings: { | ||
| audioKey: 'test-key', | ||
| rootNote: 60, | ||
| trimStart: 0, | ||
| trimEnd: 1, | ||
| playbackMode: 'classic', | ||
| loopStart: 0, | ||
| loopEnd: 1, | ||
| ampEnvelope: { attack: 0.005, decay: 0.1, sustain: 1, release: 0.3 }, | ||
| }, | ||
| }; | ||
|
|
||
| const fmInstrument: FmTrackInstrument = { | ||
| kind: 'fm', | ||
| preset: 'fm', | ||
| name: 'Test FM', | ||
| fallbackPreset: 'organ', | ||
| settings: { | ||
| carrier: { waveform: 'sine', ratio: 1, level: 1 }, | ||
| modulator: { waveform: 'sine', ratio: 2, level: 0.5 }, | ||
| modulationIndex: 5, | ||
| feedback: 0, | ||
| ampEnvelope: { attack: 0.01, decay: 0.1, sustain: 0.8, release: 0.3 }, | ||
| outputGain: 1, | ||
| }, | ||
| }; | ||
|
|
||
| // ── Tests ────────────────────────────────────────────────────────────────── | ||
|
|
||
| describe('getEngineForInstrument', () => { | ||
| it('returns a SynthEngineAdapter for subtractive instruments', () => { | ||
| const engine = getEngineForInstrument(subtractiveInstrument); | ||
| expect(engine).toBeInstanceOf(SynthEngineAdapter); | ||
| }); | ||
|
|
||
| it('returns a SamplerEngineAdapter for sampler instruments', () => { | ||
| const engine = getEngineForInstrument(samplerInstrument); | ||
| expect(engine).toBeInstanceOf(SamplerEngineAdapter); | ||
| }); | ||
|
|
||
| it('returns a FmEngineAdapter for fm instruments', () => { | ||
| const engine = getEngineForInstrument(fmInstrument); | ||
| expect(engine).toBeInstanceOf(FmEngineAdapter); | ||
| }); | ||
|
|
||
| it('returns the same instance for repeated calls with the same kind', () => { | ||
| const a = getEngineForInstrument(subtractiveInstrument); | ||
| const b = getEngineForInstrument(subtractiveInstrument); | ||
| expect(a).toBe(b); | ||
| }); | ||
|
|
||
| it('returns different instances for different instrument kinds', () => { | ||
| const synth = getEngineForInstrument(subtractiveInstrument); | ||
| const sampler = getEngineForInstrument(samplerInstrument); | ||
| const fm = getEngineForInstrument(fmInstrument); | ||
| expect(synth).not.toBe(sampler); | ||
| expect(synth).not.toBe(fm); | ||
| expect(sampler).not.toBe(fm); | ||
| }); | ||
| }); | ||
|
|
||
| describe('InstrumentEngine interface conformance', () => { | ||
| const engines: Array<[string, InstrumentEngine]> = [ | ||
| ['SynthEngineAdapter', getEngineForInstrument(subtractiveInstrument)], | ||
| ['SamplerEngineAdapter', getEngineForInstrument(samplerInstrument)], | ||
| ['FmEngineAdapter', getEngineForInstrument(fmInstrument)], | ||
| ]; | ||
|
|
||
| it.each(engines)('%s implements all InstrumentEngine methods', (_name, engine) => { | ||
| expect(typeof engine.noteOn).toBe('function'); | ||
| expect(typeof engine.noteOff).toBe('function'); | ||
| expect(typeof engine.triggerAttackRelease).toBe('function'); | ||
| expect(typeof engine.setParameter).toBe('function'); | ||
| expect(typeof engine.releaseAll).toBe('function'); | ||
| expect(typeof engine.removeTrack).toBe('function'); | ||
| expect(typeof engine.dispose).toBe('function'); | ||
| }); | ||
|
|
||
| it.each(engines)('%s.setParameter does not throw for unknown params', (_name, engine) => { | ||
| expect(() => engine.setParameter('test-track', 'unknownParam', 42)).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstrumentEnginedoesn’t specify the expected velocity range (MIDI 0–127 vs normalized 0–1), but existing engines use both conventions (SynthEngine.noteOntreats velocity as 0–127, while sampler playback often uses 0–1). Please document a single convention in this interface and ensure all adapters/engines convert to it consistently to avoid very quiet notes or extremely loud sampler playback.