From 4b75588b4a5ee9000aa2c3e19ec64c1ef1435d15 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Tue, 16 Jun 2026 23:10:41 +0000 Subject: [PATCH] Add read+write support for the element (#188) Summary: Model the commonly-used scalar attributes of the MusicXML element in the api layer and wire them through impl so they round-trip. Previously MeasureReader::parseSound was a no-op and DirectionData carried only a "TODO - sound element", so any tempo/dynamics/segno/coda/etc. encoded on a (standalone or as a direction child) was dropped on read. - New api type api::SoundData (src/include/mx/api/SoundData.h) holding: tempo, dynamics (doubles, <0 = unspecified); dacapo, forward-repeat, pizzicato (Bool); segno, dalsegno, coda, tocoda, fine (strings, empty = unspecified). Carried on DirectionData via isSoundDataSpecified + soundData. - impl read: shared impl::readSoundData helper (SoundFunctions). A standalone is read by MeasureReader::parseSound into a sound-only DirectionData on staff 0 (isStaffValueSpecified=false); a child is read by DirectionReader into the same DirectionData. - impl write: impl::writeSoundData rebuilds core::Sound. DirectionWriter emits a standalone MusicDataChoice when the direction is sound-only, or attaches the as a child of the when other content exists. Test plan: - New src/private/mxtest/api/SoundApiTest.cpp: round-trips standalone tempo, standalone dynamics, the navigation booleans/strings, asserts the serialized XML emits a standalone , and round-trips a direction-child alongside . - MX_RUNNING_IN_DOCKER=1 make test (3944 assertions, 227 cases all pass), make fmt, make check (fmt-check passed), make test-api-roundtrip (1/1 pinned). Scope note: Nested child elements of (, , , , ) and the less-common scalar attributes (divisions, time-only, pan, elevation, damper/soft/sostenuto-pedal, id) are intentionally not modeled; the focus is the high-value playback/navigation attributes. No new corpus roundtrip-baseline entry: discovery shows the ~141 wild files still fail the strict DOM compare on unrelated subset gaps, so unit tests are the coverage. --- docs/ai/api-feature-audit.md | 2 +- src/include/mx/api/DirectionData.h | 19 ++- src/include/mx/api/SoundData.h | 83 ++++++++++++ src/private/mx/impl/DirectionReader.cpp | 12 ++ src/private/mx/impl/DirectionWriter.cpp | 26 ++++ src/private/mx/impl/MeasureReader.cpp | 24 +++- src/private/mx/impl/SoundFunctions.cpp | 127 +++++++++++++++++ src/private/mx/impl/SoundFunctions.h | 24 ++++ src/private/mxtest/api/SoundApiTest.cpp | 173 ++++++++++++++++++++++++ 9 files changed, 482 insertions(+), 8 deletions(-) create mode 100644 src/include/mx/api/SoundData.h create mode 100644 src/private/mx/impl/SoundFunctions.cpp create mode 100644 src/private/mx/impl/SoundFunctions.h create mode 100644 src/private/mxtest/api/SoundApiTest.cpp diff --git a/docs/ai/api-feature-audit.md b/docs/ai/api-feature-audit.md index e8bea525a..c82d0e018 100644 --- a/docs/ai/api-feature-audit.md +++ b/docs/ai/api-feature-audit.md @@ -178,7 +178,7 @@ so these boxes are status markers, not clickable; the live checklist is the pare | [ ] | technical marks with payloads (`fingering`, `pluck`, `bend`, ...) | 1c | #185 | | [x] | read `` per-measure layout | 2(1) | #186 | | [x] | `` gaps (credit-image, no-words credits, multiple credit-type) | 2(2) | #187 | -| [ ] | read and write `` | 2(3) | #188 | +| [x] | read and write `` | 2(3) | #188 | | [x] | defaults fonts (`word-font`, `lyric-font`, `music-font`) | 2(4) | #189 | | [ ] | round-trip `` | 2(5) | #190 | | [ ] | harmony `inversion`, `function`, `numeral` | 2(6) | #191 | diff --git a/src/include/mx/api/DirectionData.h b/src/include/mx/api/DirectionData.h index aceb54b4e..3bd246e45 100644 --- a/src/include/mx/api/DirectionData.h +++ b/src/include/mx/api/DirectionData.h @@ -11,6 +11,7 @@ #include "mx/api/OttavaData.h" #include "mx/api/RehearsalData.h" #include "mx/api/SegnoData.h" +#include "mx/api/SoundData.h" #include "mx/api/TempoData.h" #include "mx/api/WedgeData.h" #include "mx/api/WordsData.h" @@ -93,7 +94,12 @@ struct DirectionData // only relevant for Directions placed on staff zero, it is otherwise ignored. bool isStaffValueSpecified; - // TODO - sound element + // The element carried by this direction. It is only meaningful when + // isSoundDataSpecified is true. A direction whose only content is a sound round-trips as a + // standalone within the measure; a direction with other content writes the sound as a + // child of the element. + bool isSoundDataSpecified; + SoundData soundData; std::vector tempos; std::vector marks; @@ -115,9 +121,10 @@ struct DirectionData std::vector orderedComponents; DirectionData() - : tickTimePosition{0}, placement{Placement::unspecified}, voice{-1}, isStaffValueSpecified{true}, marks{}, - wedgeStarts{}, wedgeStops{}, ottavaStarts{}, ottavaStops{}, bracketStarts{}, bracketStops{}, dashesStarts{}, - dashesStops{}, pedalStarts{}, pedalStops{}, words{}, chords{}, segnos{} + : tickTimePosition{0}, placement{Placement::unspecified}, voice{-1}, isStaffValueSpecified{true}, + isSoundDataSpecified{false}, soundData{}, marks{}, wedgeStarts{}, wedgeStops{}, ottavaStarts{}, ottavaStops{}, + bracketStarts{}, bracketStops{}, dashesStarts{}, dashesStops{}, pedalStarts{}, pedalStops{}, words{}, + chords{}, segnos{} { } }; @@ -131,7 +138,7 @@ inline bool isDirectionDataEmpty(const DirectionData &directionData) directionData.pedalStarts.size() == 0 && directionData.pedalStops.size() == 0 && directionData.tempos.size() == 0 && directionData.ottavaStarts.size() == 0 && directionData.ottavaStops.size() == 0 && directionData.words.size() == 0 && - directionData.segnos.size() == 0 && directionData.codas.size() == 0 && + directionData.segnos.size() == 0 && directionData.codas.size() == 0 && !directionData.isSoundDataSpecified && directionData.orderedComponents.size() == 0; } @@ -140,6 +147,8 @@ MXAPI_EQUALS_MEMBER(tickTimePosition) MXAPI_EQUALS_MEMBER(placement) MXAPI_EQUALS_MEMBER(voice) MXAPI_EQUALS_MEMBER(isStaffValueSpecified) +MXAPI_EQUALS_MEMBER(isSoundDataSpecified) +MXAPI_EQUALS_MEMBER(soundData) MXAPI_EQUALS_MEMBER(tempos) MXAPI_EQUALS_MEMBER(marks) MXAPI_EQUALS_MEMBER(wedgeStarts) diff --git a/src/include/mx/api/SoundData.h b/src/include/mx/api/SoundData.h new file mode 100644 index 000000000..acfb83040 --- /dev/null +++ b/src/include/mx/api/SoundData.h @@ -0,0 +1,83 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#pragma once + +#include "mx/api/ApiCommon.h" + +#include + +namespace mx +{ +namespace api +{ +// MusicXML Documentation: The sound element contains general playback parameters. They can stand +// alone within a part/measure, or be a component element within a direction. +// +// mx::api models the commonly-used scalar attributes of . The nested child elements +// (, , , , ) are intentionally not modeled. +// +// A SoundData is carried on DirectionData. When a DirectionData holds a SoundData but no other +// direction content, it round-trips as a standalone element within the measure. When a +// DirectionData holds other direction content in addition to the SoundData, the is written +// as a child of the element. +struct SoundData +{ + // tempo in quarter notes per minute. A value less than 0 means 'unspecified'. + double tempo; + + // dynamics (MIDI velocity) as a percentage of the default forte value. A value less than 0 + // means 'unspecified'. + double dynamics; + + // Dacapo indicates to go back to the beginning of the movement. When used it always has the + // value 'yes'. + Bool dacapo; + + // forward-repeat indicates that a forward repeat sign is implied but not displayed. When used + // it always has the value 'yes'. + Bool forwardRepeat; + + // Pizzicato in a sound element effects all following notes. 'yes' indicates pizzicato, 'no' + // indicates arco. + Bool pizzicato; + + // Segno and dalsegno are used for backwards jumps to a segno sign; coda and tocoda are used for + // forward jumps to a coda sign. The fine attribute follows the final note or rest in a movement + // with a da capo or dal segno direction. These are strings; an empty string means 'unspecified'. + std::string segno; + std::string dalsegno; + std::string coda; + std::string tocoda; + std::string fine; + + SoundData() + : tempo{-1.0}, dynamics{-1.0}, dacapo{Bool::unspecified}, forwardRepeat{Bool::unspecified}, + pizzicato{Bool::unspecified}, segno{}, dalsegno{}, coda{}, tocoda{}, fine{} + { + } + + bool isSpecified() const + { + return tempo >= 0.0 || dynamics >= 0.0 || dacapo != Bool::unspecified || forwardRepeat != Bool::unspecified || + pizzicato != Bool::unspecified || !segno.empty() || !dalsegno.empty() || !coda.empty() || + !tocoda.empty() || !fine.empty(); + } +}; + +MXAPI_EQUALS_BEGIN(SoundData) +MXAPI_DOUBLES_EQUALS_MEMBER(tempo) +MXAPI_DOUBLES_EQUALS_MEMBER(dynamics) +MXAPI_EQUALS_MEMBER(dacapo) +MXAPI_EQUALS_MEMBER(forwardRepeat) +MXAPI_EQUALS_MEMBER(pizzicato) +MXAPI_EQUALS_MEMBER(segno) +MXAPI_EQUALS_MEMBER(dalsegno) +MXAPI_EQUALS_MEMBER(coda) +MXAPI_EQUALS_MEMBER(tocoda) +MXAPI_EQUALS_MEMBER(fine) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(SoundData); +} // namespace api +} // namespace mx diff --git a/src/private/mx/impl/DirectionReader.cpp b/src/private/mx/impl/DirectionReader.cpp index 1438e9dc6..bb69c58d0 100644 --- a/src/private/mx/impl/DirectionReader.cpp +++ b/src/private/mx/impl/DirectionReader.cpp @@ -44,6 +44,7 @@ #include "mx/core/generated/RootStep.h" #include "mx/core/generated/Scordatura.h" #include "mx/core/generated/Segno.h" +#include "mx/core/generated/Sound.h" #include "mx/core/generated/StartStop.h" #include "mx/core/generated/StartStopContinue.h" #include "mx/core/generated/Step.h" @@ -57,6 +58,7 @@ #include "mx/impl/MarkDataFunctions.h" #include "mx/impl/MetronomeReader.h" #include "mx/impl/PrintFunctions.h" +#include "mx/impl/SoundFunctions.h" #include "mx/impl/SpannerFunctions.h" #include "mx/utility/Round.h" #include "mx/utility/Unused.h" @@ -152,6 +154,16 @@ void DirectionReader::parseValues() { parseDirectionType(dt); } + + if (myDirection->sound().has_value()) + { + auto soundData = readSoundData(*myDirection->sound()); + if (soundData.isSpecified()) + { + myOutDirectionData.isSoundDataSpecified = true; + myOutDirectionData.soundData = std::move(soundData); + } + } } else if (myHarmony) { diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index c9cf29c66..e68d0bb03 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/src/private/mx/impl/DirectionWriter.cpp @@ -44,6 +44,7 @@ #include "mx/core/generated/Root.h" #include "mx/core/generated/RootStep.h" #include "mx/core/generated/Semitones.h" +#include "mx/core/generated/Sound.h" #include "mx/core/generated/StartStop.h" #include "mx/core/generated/StartStopContinue.h" #include "mx/core/generated/String.h" @@ -56,6 +57,7 @@ #include "mx/impl/LineFunctions.h" #include "mx/impl/MarkDataFunctions.h" #include "mx/impl/PrintFunctions.h" +#include "mx/impl/SoundFunctions.h" #include "mx/impl/SpannerFunctions.h" #include "mx/utility/Throw.h" #include "mx/utility/Unused.h" @@ -393,8 +395,32 @@ std::vector DirectionWriter::getDirectionLikeThings() if (myIsFirstDirectionTypeAdded) { + // The direction has other content; attach the as a child of the . + if (myDirectionData.isSoundDataSpecified && myDirectionData.soundData.isSpecified()) + { + core::Sound sound{}; + writeSoundData(myDirectionData.soundData, sound); + direction.setSound(std::move(sound)); + } + output.push_back(core::MusicDataChoice::direction(direction)); } + else if (myDirectionData.isSoundDataSpecified && myDirectionData.soundData.isSpecified()) + { + // The direction has no other content; emit a standalone element. + core::Sound sound{}; + writeSoundData(myDirectionData.soundData, sound); + + if (offset != 0) + { + core::Offset coreOffset{}; + coreOffset.setValue(core::Divisions{core::Decimal{static_cast(offset)}}); + coreOffset.setSound(core::YesNo::yes()); + sound.setOffset(coreOffset); + } + + output.push_back(core::MusicDataChoice::sound(std::move(sound))); + } auto harmonyMdcs = createHarmonyElements(offset); addMusicDataChoices(harmonyMdcs, output); diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index 1ad368d5c..9afd74f2c 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -53,6 +53,7 @@ #include "mx/impl/DirectionReader.h" #include "mx/impl/NoteFunctions.h" #include "mx/impl/NoteReader.h" +#include "mx/impl/SoundFunctions.h" #include "mx/impl/TimeReader.h" #include "mx/utility/Throw.h" #include "mx/utility/Unused.h" @@ -622,8 +623,27 @@ void MeasureReader::parsePrint(const core::Print &inMxPrint) const void MeasureReader::parseSound(const core::Sound &inMxSound) const { - MX_UNUSED(inMxSound); - // std::cout << "sound is not supported" << std::endl; + auto soundData = readSoundData(inMxSound); + + if (!soundData.isSpecified()) + { + return; + } + + if (myOutMeasureData.staves.empty()) + { + return; + } + + // A standalone has no ; place it on staff 0 with isStaffValueSpecified = false + // and no other direction content, so it round-trips as a standalone element. + auto directionData = api::DirectionData{}; + directionData.tickTimePosition = myCurrentCursor.tickTimePosition; + directionData.isStaffValueSpecified = false; + directionData.isSoundDataSpecified = true; + directionData.soundData = std::move(soundData); + + myOutMeasureData.staves.at(0).directions.emplace_back(std::move(directionData)); } void MeasureReader::parseBarline(const core::Barline &inMxBarline) const diff --git a/src/private/mx/impl/SoundFunctions.cpp b/src/private/mx/impl/SoundFunctions.cpp new file mode 100644 index 000000000..14807aeda --- /dev/null +++ b/src/private/mx/impl/SoundFunctions.cpp @@ -0,0 +1,127 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mx/impl/SoundFunctions.h" +#include "mx/core/Decimal.h" +#include "mx/core/generated/NonNegativeDecimal.h" +#include "mx/core/generated/Sound.h" +#include "mx/core/generated/YesNo.h" + +namespace mx +{ +namespace impl +{ +namespace +{ +api::Bool toApiBool(const std::optional &value) +{ + if (!value.has_value()) + { + return api::Bool::unspecified; + } + + return value->tag() == core::YesNo::Tag::yes ? api::Bool::yes : api::Bool::no; +} +} // namespace + +api::SoundData readSoundData(const core::Sound &inSound) +{ + api::SoundData out{}; + + if (inSound.tempo().has_value()) + { + out.tempo = inSound.tempo()->value().value(); + } + + if (inSound.dynamics().has_value()) + { + out.dynamics = inSound.dynamics()->value().value(); + } + + out.dacapo = toApiBool(inSound.dacapo()); + out.forwardRepeat = toApiBool(inSound.forwardRepeat()); + out.pizzicato = toApiBool(inSound.pizzicato()); + + if (inSound.segno().has_value()) + { + out.segno = *inSound.segno(); + } + + if (inSound.dalsegno().has_value()) + { + out.dalsegno = *inSound.dalsegno(); + } + + if (inSound.coda().has_value()) + { + out.coda = *inSound.coda(); + } + + if (inSound.tocoda().has_value()) + { + out.tocoda = *inSound.tocoda(); + } + + if (inSound.fine().has_value()) + { + out.fine = *inSound.fine(); + } + + return out; +} + +void writeSoundData(const api::SoundData &inSoundData, core::Sound &outSound) +{ + if (inSoundData.tempo >= 0.0) + { + outSound.setTempo(core::NonNegativeDecimal{core::Decimal{inSoundData.tempo}}); + } + + if (inSoundData.dynamics >= 0.0) + { + outSound.setDynamics(core::NonNegativeDecimal{core::Decimal{inSoundData.dynamics}}); + } + + if (inSoundData.dacapo != api::Bool::unspecified) + { + outSound.setDacapo(inSoundData.dacapo == api::Bool::yes ? core::YesNo::yes() : core::YesNo::no()); + } + + if (inSoundData.forwardRepeat != api::Bool::unspecified) + { + outSound.setForwardRepeat(inSoundData.forwardRepeat == api::Bool::yes ? core::YesNo::yes() : core::YesNo::no()); + } + + if (inSoundData.pizzicato != api::Bool::unspecified) + { + outSound.setPizzicato(inSoundData.pizzicato == api::Bool::yes ? core::YesNo::yes() : core::YesNo::no()); + } + + if (!inSoundData.segno.empty()) + { + outSound.setSegno(inSoundData.segno); + } + + if (!inSoundData.dalsegno.empty()) + { + outSound.setDalsegno(inSoundData.dalsegno); + } + + if (!inSoundData.coda.empty()) + { + outSound.setCoda(inSoundData.coda); + } + + if (!inSoundData.tocoda.empty()) + { + outSound.setTocoda(inSoundData.tocoda); + } + + if (!inSoundData.fine.empty()) + { + outSound.setFine(inSoundData.fine); + } +} +} // namespace impl +} // namespace mx diff --git a/src/private/mx/impl/SoundFunctions.h b/src/private/mx/impl/SoundFunctions.h new file mode 100644 index 000000000..5fd385d1b --- /dev/null +++ b/src/private/mx/impl/SoundFunctions.h @@ -0,0 +1,24 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#pragma once + +#include "mx/api/SoundData.h" + +namespace mx +{ +namespace core +{ +class Sound; +} + +namespace impl +{ +// core -> api: read the commonly-used scalar attributes of a element into api::SoundData. +api::SoundData readSoundData(const core::Sound &inSound); + +// api -> core: write the api::SoundData scalar attributes onto a core::Sound element. +void writeSoundData(const api::SoundData &inSoundData, core::Sound &outSound); +} // namespace impl +} // namespace mx diff --git a/src/private/mxtest/api/SoundApiTest.cpp b/src/private/mxtest/api/SoundApiTest.cpp new file mode 100644 index 000000000..4dc20728e --- /dev/null +++ b/src/private/mxtest/api/SoundApiTest.cpp @@ -0,0 +1,173 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mxtest/control/CompileControl.h" +#ifdef MX_COMPILE_API_TESTS + +#include "cpul/cpulTestHarness.h" +#include "mx/api/DocumentManager.h" +#include "mx/api/ScoreData.h" +#include "mxtest/api/RoundTrip.h" +#include "mxtest/api/TestHelpers.h" + +using namespace std; +using namespace mx::api; +using namespace mxtest; + +namespace +{ +// Build a minimal one-note, one-measure, one-staff score with a single direction carrying the +// given (already-populated) SoundData on staff 0. +ScoreData makeScoreWithSound(const SoundData &inSound) +{ + ScoreData score; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + auto &staff = measure.staves.back(); + auto &voice = staff.voices[0]; + voice.notes.emplace_back(); + + DirectionData direction; + direction.isStaffValueSpecified = false; + direction.isSoundDataSpecified = true; + direction.soundData = inSound; + staff.directions.emplace_back(std::move(direction)); + return score; +} + +const DirectionData *findSoundDirection(const ScoreData &score) +{ + if (score.parts.empty() || score.parts.back().measures.empty()) + { + return nullptr; + } + const auto &measure = score.parts.back().measures.back(); + if (measure.staves.empty()) + { + return nullptr; + } + for (const auto &direction : measure.staves.front().directions) + { + if (direction.isSoundDataSpecified) + { + return &direction; + } + } + return nullptr; +} +} // namespace + +TEST(standaloneSoundTempoRoundTrips, SoundApi) +{ + SoundData sound; + sound.tempo = 120.0; + + const auto score = makeScoreWithSound(sound); + const auto out = roundTrip(score); + + const auto *direction = findSoundDirection(out); + REQUIRE(direction != nullptr); + CHECK(direction->isSoundDataSpecified); + CHECK_DOUBLES_EQUAL(120.0, direction->soundData.tempo, MX_API_EQUALITY_EPSILON); +} + +T_END; + +TEST(standaloneSoundEmitsSoundElement, SoundApi) +{ + SoundData sound; + sound.tempo = 120.0; + + const auto score = makeScoreWithSound(sound); + const auto xml = toXml(score); + + // The standalone must be present with a tempo attribute (not wrapped in a ). + CHECK(xml.find("soundData.dynamics, MX_API_EQUALITY_EPSILON); +} + +T_END; + +TEST(standaloneSoundNavigationRoundTrips, SoundApi) +{ + SoundData sound; + sound.dacapo = Bool::yes; + sound.forwardRepeat = Bool::yes; + sound.pizzicato = Bool::no; + sound.segno = "1"; + sound.dalsegno = "2"; + sound.coda = "3"; + sound.tocoda = "4"; + sound.fine = "yes"; + + const auto score = makeScoreWithSound(sound); + const auto out = roundTrip(score); + + const auto *direction = findSoundDirection(out); + REQUIRE(direction != nullptr); + const auto &rt = direction->soundData; + CHECK(Bool::yes == rt.dacapo); + CHECK(Bool::yes == rt.forwardRepeat); + CHECK(Bool::no == rt.pizzicato); + CHECK_EQUAL("1", rt.segno); + CHECK_EQUAL("2", rt.dalsegno); + CHECK_EQUAL("3", rt.coda); + CHECK_EQUAL("4", rt.tocoda); + CHECK_EQUAL("yes", rt.fine); +} + +T_END; + +TEST(directionChildSoundRoundTrips, SoundApi) +{ + // A direction that has other content (words) plus a sound: the is written as a child of + // the . + ScoreData score; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + auto &staff = measure.staves.back(); + auto &voice = staff.voices[0]; + voice.notes.emplace_back(); + + DirectionData direction; + WordsData words; + words.text = "Allegro"; + direction.words.emplace_back(std::move(words)); + direction.isSoundDataSpecified = true; + direction.soundData.tempo = 144.0; + staff.directions.emplace_back(std::move(direction)); + + const auto out = roundTrip(score); + + const auto *outDirection = findSoundDirection(out); + REQUIRE(outDirection != nullptr); + REQUIRE(outDirection->words.size() == 1u); + CHECK_EQUAL("Allegro", outDirection->words.front().text); + CHECK_DOUBLES_EQUAL(144.0, outDirection->soundData.tempo, MX_API_EQUALITY_EPSILON); +} + +T_END; + +#endif