Merged
Conversation
Add complete MIDI export feature per spec /specs/MIDI-EXPORT.md: - Add midi-writer-js library for SMF Type 1 file generation - Create midiExport.ts with comprehensive export logic: - 480 ticks/quarter note resolution - Tempo track + instrument tracks structure - Drum mapping to GM channel 10 with correct note numbers - Synth mapping with GM program changes - Support for parameter locks (pitch/velocity) - Swing timing offsets - Polyrhythm support via LCM pattern length calculation - Solo mode support (matches audio scheduler behavior) - Add download button (↓) to header UI - Add CSS styles for download button hover state - Include 54 comprehensive unit tests covering all edge cases Export behavior (matches audio scheduler): - If any track is soloed, only soloed tracks are exported - Solo wins over mute (soloed+muted track is exported) - Otherwise, all unmuted tracks are exported
Key additions based on implementation lessons: 1. Core Principle: "What You Hear Is What You Export" - Explicit WYSIWYG requirement for export features - Users should experience no surprises in DAW 2. Track Selection Logic section - Documents the canonical implementation (scheduler.ts:249-251) - Explains solo wins over mute behavior - Shows anti-pattern to avoid (checking muted without soloed) - 8-case behavioral parity test matrix 3. Cross-References section - Links to canonical implementation in scheduler.ts - Lists dependent features that share this logic - RFC-style normative vs informative references 4. Updated Implementation section - Reference code now shows correct filtering pattern - Comments mark the critical integration point 5. Updated Validation section - Behavioral parity tests as Priority: Critical - Golden master testing approach - Added success criterion for mute/solo parity Lessons learned: Specs for integrated features must identify cross-cutting concerns and reference canonical implementations rather than re-specifying shared logic.
Spec changes: - Replace brittle line number references with @SPEC: code markers - Mark reference implementation as "conceptual" with link to actual code - Remove phantom parseMidiTrackIds function from test examples - Complete synth preset table (9 → 17 mappings) - Fix test helper name (createSession → createState) - Add note range validation requirement (clamp 0-127) - Add DAW compatibility section with known issues per DAW - Add MIDI validation tools section - Add Browser Considerations section (mobile support, memory) - Mark success criteria with verified/untested status - Add "Why Type 1?" and "Why 480 PPQN?" explanations - Add note-off timing explanation (why TICKS - 1) - Add MIDI Import Considerations for future feature - Add program change limitations warning (never use GM RESET SysEx) Implementation changes: - Add @SPEC: track-selection marker to scheduler.ts - Add note clamping to getSynthNotePitch (0-127) - Add note clamping to drum pitch with p-locks Research incorporated from sub-agent: - DAW-specific compatibility notes - Channel 10 drum quirks - PPQN conversion formulas for future import - Common "file corrupted" causes
- Add visual diagrams showing Type 0 (single merged track) vs Type 1 (separate tracks) - Explain why Type 1 is essential for DAW editing workflows - Add note clamping to implementation feature list - Add ArrayBuffer workaround to implementation notes - Complete internal consistency audit (all cross-references verified)
- Add "Adding New Instruments" section with step-by-step instructions - Document silent fallback behavior (unknown presets → piano) - Add cross-references to preset definition files (synth.ts, toneSynths.ts, advancedSynth.ts) - Recommend coverage test pattern to prevent silent fallbacks - Note that coverage test would fail today (17 of 32 presets mapped)
Add 34 fidelity tests that parse actual MIDI output using midi-file package: - Verify file structure (SMF Type 1, 128 PPQN, track count) - Verify tempo meta events - Verify note timing and swing offsets - Verify pitch (drums, synths, transpose, p-locks, clamping) - Verify velocity scaling (percentage to MIDI) - Verify channel assignment (drums on 10, synths skip 10) - Verify program changes (0-indexed GM programs) - Verify polyrhythm expansion - Verify track selection (mute/solo) Fix PPQN and timing constants: - Use 128 PPQN (midi-writer-js default) not 480 - Update TICKS_PER_STEP from 120 to 32 - Update NOTE_DURATION_TICKS from 119 to 31 Fix velocity handling: - midi-writer-js treats velocity as percentage (0-100) - Updated getVelocity to return percentage, not MIDI value Fix program change numbering: - Subtract 1 to convert 1-indexed GM to 0-indexed MIDI Update spec with: - Fidelity testing documentation and examples - midi-writer-js quirks (velocity, program, PPQN) - Corrected timing constants throughout - Updated success criteria
The Velocity Mapping section incorrectly showed: Math.round(volumePLock * 127) // MIDI velocity But midi-writer-js treats velocity as percentage, so it should be: Math.round(volumePLock * 100) // percentage (library scales to MIDI) This was already correctly documented in the midi-writer-js Quirks section, but the Velocity Mapping section had stale information. Added cross-reference link to the quirks section for clarity.
This PR contains only the MIDI Export specification document. The implementation will be done separately. Reverted: - app/src/audio/midiExport.ts (export logic) - app/src/audio/midiExport.test.ts (unit tests) - app/src/audio/midiExport.fidelity.test.ts (fidelity tests) - app/src/App.tsx (download button) - app/src/App.css (button styles) - app/package.json (midi-writer-js dependency) - app/src/audio/scheduler.ts (@SPEC marker comment) Kept: - specs/MIDI-EXPORT.md (complete specification)
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
2 participants
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.
No description provided.