fix: preserve stem position attributes on round-trip

.gitignore

@@ -1,4 +1,5 @@
 build/
+build-*/
 cmake-build-*
 src/private/mxtest/file/PathRoot.h
 *.DS_Store

Makefile

@@ -435,9 +435,15 @@ discover-api-roundtrip: $(DOCKER_STAMP) docker-volume
 
 dump-api-roundtrip: $(DOCKER_STAMP) docker-volume
 	$(DOCKER_RUN) make dump-api-roundtrip BUILD_TYPE=$(BUILD_TYPE)
+	@mkdir -p $(BUILD_ROOT)/api/roundtrip-dump
+	$(DOCKER) run --rm -v $(DOCKER_VOLUME):/vol:ro -v $(CURDIR)/$(BUILD_ROOT)/api/roundtrip-dump:/out \
+		alpine sh -c 'cp -a /vol/api/roundtrip-dump/. /out/'
 
 classify-api-roundtrip: $(DOCKER_STAMP) docker-volume
 	$(DOCKER_RUN) make classify-api-roundtrip BUILD_TYPE=$(BUILD_TYPE)
+	@mkdir -p $(BUILD_ROOT)/api
+	$(DOCKER) run --rm -v $(DOCKER_VOLUME):/vol:ro -v $(CURDIR)/$(BUILD_ROOT)/api:/out \
+		alpine cp /vol/api/classified.json /out/classified.json
 
 coverage-api: $(DOCKER_STAMP) docker-volume
 	@rm -rf $(COV_DIR)/api

data/synthetic/double.3.0.xml

@@ -13,6 +13,7 @@
     <measure number="x">
       <attributes>
         <transpose>
+          <diatonic>1</diatonic>
           <chromatic>1</chromatic>
           <double />
         </transpose>

data/synthetic/double.4.0.xml

@@ -13,6 +13,7 @@
     <measure number="x">
       <attributes>
         <transpose>
+          <diatonic>1</diatonic>
           <chromatic>1</chromatic>
           <double above="yes" />
         </transpose>

src/include/mx/api/ClefData.h

@@ -45,6 +45,7 @@ class ClefData
     // had no <line> element so the round-trip does not inject an implied default (#228).
     bool isLineSpecified;
     int octaveChange;
+    bool isOctaveChangeSpecified;
     int tickTimePosition;
     ClefLocation location;
     // Visibility of the clef via the MusicXML print-object attribute.
@@ -77,6 +78,7 @@ MXAPI_EQUALS_MEMBER(symbol)
 MXAPI_EQUALS_MEMBER(line)
 MXAPI_EQUALS_MEMBER(isLineSpecified)
 MXAPI_EQUALS_MEMBER(octaveChange)
+MXAPI_EQUALS_MEMBER(isOctaveChangeSpecified)
 MXAPI_EQUALS_MEMBER(tickTimePosition)
 MXAPI_EQUALS_MEMBER(location)
 MXAPI_EQUALS_MEMBER(printObject)

src/include/mx/api/NoteData.h

@@ -103,6 +103,7 @@ class NoteData
     PitchData pitchData; // step, alter, octave, accidental, etc
     int userRequestedVoiceNumber;
     Stem stem;
+    PositionData stemPositionData;
 
     // the time location of the note, within the measure, denominated in ticksPerQuarter which
     // is defined in ScoreData. in each measure, the note with tickTimePosition 0 is located at
@@ -143,6 +144,7 @@ MXAPI_EQUALS_MEMBER(noteType)
 MXAPI_EQUALS_MEMBER(pitchData)
 MXAPI_EQUALS_MEMBER(userRequestedVoiceNumber)
 MXAPI_EQUALS_MEMBER(stem)
+MXAPI_EQUALS_MEMBER(stemPositionData)
 MXAPI_EQUALS_MEMBER(tickTimePosition)
 MXAPI_EQUALS_MEMBER(durationData)
 MXAPI_EQUALS_MEMBER(beams)

src/private/mx/api/ClefData.cpp

@@ -11,8 +11,8 @@ namespace api
 {
 ClefData::ClefData()
     : symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, isLineSpecified{true},
-      octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, tickTimePosition{0}, location{ClefLocation::unspecified},
-      printObject{Bool::unspecified}
+      octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, isOctaveChangeSpecified{true}, tickTimePosition{0},
+      location{ClefLocation::unspecified}, printObject{Bool::unspecified}
 {
 }
 

src/private/mx/impl/Converter.cpp

@@ -1708,18 +1708,12 @@ mx::core::Transpose Converter::convertToTranspose(const mx::api::TransposeData &
         group.setChromatic(mx::core::Semitones{mx::core::Decimal{static_cast<double>(chromatic)}});
     }
 
-    if (inTransposeData.diatonic != 0)
+    auto harmonic = inTransposeData.diatonic;
+    if (octave != 0)
     {
-        auto harmonic = inTransposeData.diatonic;
-        if (octave != 0)
-        {
-            harmonic += octave * -7;
-        }
-        if (harmonic != 0)
-        {
-            group.setDiatonic(harmonic);
-        }
+        harmonic += octave * -7;
     }
+    group.setDiatonic(harmonic);
 
     transpose.setTranspose(std::move(group));
     return transpose;

src/private/mx/impl/MeasureReader.cpp

@@ -809,10 +809,12 @@ void MeasureReader::importClef(const core::Clef &inClef) const
     if (inClef.clef().clefOctaveChange().has_value())
     {
         clefData.octaveChange = *inClef.clef().clefOctaveChange();
+        clefData.isOctaveChangeSpecified = true;
     }
     else
     {
         clefData.octaveChange = 0;
+        clefData.isOctaveChangeSpecified = false;
     }
 
     if (inClef.printObject().has_value())

src/private/mx/impl/NoteFunctions.cpp

@@ -135,6 +135,7 @@ api::NoteData NoteFunctions::parseNote() const
     if (reader.getIsStemSpecified())
     {
         myOutNoteData.stem = converter.convert(reader.getStem());
+        myOutNoteData.stemPositionData = impl::getPositionData(*myNote.stem());
     }
     myOutNoteData.isTieStart = reader.getIsTieStart();
     myOutNoteData.isTieStop = reader.getIsTieStop();

src/private/mx/impl/NoteWriter.cpp

@@ -422,6 +422,7 @@ void NoteWriter::setStemDirection() const
 
     core::Stem stem;
     stem.setValue(myConverter.convert(myNoteData.stem));
+    impl::setAttributesFromPositionData(myNoteData.stemPositionData, stem);
     myOutNote.setStem(std::move(stem));
 }
 

src/private/mx/impl/PropertiesWriter.cpp

@@ -216,7 +216,7 @@ void PropertiesWriter::writeClef(int staffIndex, const api::ClefData &inClefData
         cg.setLine(core::StaffLinePosition{inClefData.line});
     }
 
-    if (inClefData.octaveChange != 0)
+    if (inClefData.isOctaveChangeSpecified)
     {
         cg.setClefOctaveChange(inClefData.octaveChange);
     }

src/private/mxtest/api/TranspositionTest.cpp

@@ -489,7 +489,7 @@ TEST(Conversion01, Transposition)
     const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic};
     const auto converted = mx::impl::Converter::convertToTranspose(original);
     CHECK_EQUAL(expectedChromatic, static_cast<int>(converted.transpose().chromatic().value().value()));
-    CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0));
+    CHECK(converted.transpose().diatonic().has_value());
     CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0));
     CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0));
     CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0));
@@ -509,7 +509,7 @@ TEST(Conversion02, Transposition)
     const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic};
     const auto converted = mx::impl::Converter::convertToTranspose(original);
     CHECK_EQUAL(expectedChromatic, static_cast<int>(converted.transpose().chromatic().value().value()));
-    CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0));
+    CHECK(converted.transpose().diatonic().has_value());
     CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0));
     CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0));
     CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0));
@@ -529,7 +529,7 @@ TEST(Conversion03, Transposition)
     const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic};
     const auto converted = mx::impl::Converter::convertToTranspose(original);
     CHECK_EQUAL(expectedChromatic, static_cast<int>(converted.transpose().chromatic().value().value()));
-    CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0));
+    CHECK(converted.transpose().diatonic().has_value());
     CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0));
     CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0));
     CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0));
@@ -549,7 +549,7 @@ TEST(Conversion04, Transposition)
     const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic};
     const auto converted = mx::impl::Converter::convertToTranspose(original);
     CHECK_EQUAL(expectedChromatic, static_cast<int>(converted.transpose().chromatic().value().value()));
-    CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0));
+    CHECK(converted.transpose().diatonic().has_value());
     CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0));
     CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0));
     CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0));

src/private/mxtest/api/roundtrip-baseline.txt

@@ -149,3 +149,9 @@ musuite/testVoiceMapper3_ref.xml
 musuite/testVoicePiano1.xml
 musuite/testVolta1.xml
 musuite/testWords1.xml
+
+# Unblocked by #263 (clef-octave-change zero), #264 (diatonic zero), #265 (stem
+# default-y zero): the zero-as-unset guard bugs in PropertiesWriter, Converter,
+# and NoteWriter dropped valid zero values on round-trip.
+custom/transposition.musicxml
+ksuite/k002a_Fermatas.xml

src/private/mxtest/impl/PositionFunctionsTest.cpp

@@ -6,10 +6,14 @@
 #ifdef MX_COMPILE_IMPL_TESTS
 
 #include "cpul/cpulTestHarness.h"
+#include "mx/api/DocumentManager.h"
 #include "mx/core/generated/Bracket.h"
 #include "mx/core/generated/Direction.h"
+#include "mx/core/generated/Stem.h"
 #include "mx/impl/PositionFunctions.h"
 
+#include <sstream>
+
 using namespace mx;
 using namespace mx::impl;
 
@@ -76,4 +80,135 @@ TEST(setAttributesFromPositionDataDirectionAttributes, PositionFunctions)
 
 T_END
 
+TEST(setAttributesFromPositionDataStem, PositionFunctions)
+{
+    core::Stem stem;
+    api::PositionData positionData;
+
+    positionData.isDefaultYSpecified = true;
+    positionData.defaultY = 5.0;
+    positionData.isRelativeYSpecified = true;
+    positionData.relativeY = -3.0;
+
+    impl::setAttributesFromPositionData(positionData, stem);
+
+    CHECK(stem.defaultY().has_value());
+    CHECK_DOUBLES_EQUAL(5.0, stem.defaultY()->value().value(), 0.0001);
+    CHECK(stem.relativeY().has_value());
+    CHECK_DOUBLES_EQUAL(-3.0, stem.relativeY()->value().value(), 0.0001);
+    CHECK(!stem.defaultX().has_value());
+    CHECK(!stem.relativeX().has_value());
+}
+
+T_END
+
+TEST(setAttributesFromPositionDataStemZero, PositionFunctions)
+{
+    core::Stem stem;
+    api::PositionData positionData;
+
+    positionData.isDefaultYSpecified = true;
+    positionData.defaultY = 0.0;
+
+    impl::setAttributesFromPositionData(positionData, stem);
+
+    CHECK(stem.defaultY().has_value());
+    CHECK_DOUBLES_EQUAL(0.0, stem.defaultY()->value().value(), 0.0001);
+}
+
+T_END
+
+TEST(getPositionDataStem, PositionFunctions)
+{
+    core::Stem stem;
+    stem.setDefaultY(core::Tenths{core::Decimal{0.0}});
+    stem.setRelativeY(core::Tenths{core::Decimal{-2.5}});
+
+    auto positionData = impl::getPositionData(stem);
+
+    CHECK(positionData.isDefaultYSpecified);
+    CHECK_DOUBLES_EQUAL(0.0, positionData.defaultY, 0.0001);
+    CHECK(positionData.isRelativeYSpecified);
+    CHECK_DOUBLES_EQUAL(-2.5, positionData.relativeY, 0.0001);
+    CHECK(!positionData.isDefaultXSpecified);
+    CHECK(!positionData.isRelativeXSpecified);
+}
+
+T_END
+
+TEST(stemPositionRoundTrip, PositionFunctions)
+{
+    core::Stem original;
+    original.setDefaultY(core::Tenths{core::Decimal{0.0}});
+
+    auto positionData = impl::getPositionData(original);
+    CHECK(positionData.isDefaultYSpecified);
+
+    core::Stem roundTripped;
+    impl::setAttributesFromPositionData(positionData, roundTripped);
+
+    CHECK(roundTripped.defaultY().has_value());
+    CHECK_DOUBLES_EQUAL(0.0, roundTripped.defaultY()->value().value(), 0.0001);
+}
+
+T_END
+
+TEST(stemDefaultYZeroApiRoundTrip, PositionFunctions)
+{
+    const char *xml = R"(<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 4.0 Partwise//EN"
+    "http://www.musicxml.org/dtds/partwise.dtd">
+<score-partwise version="4.0">
+  <part-list>
+    <score-part id="P1"><part-name/></score-part>
+  </part-list>
+  <part id="P1">
+    <measure number="1">
+      <attributes>
+        <divisions>1</divisions>
+        <time><beats>4</beats><beat-type>4</beat-type></time>
+        <clef><sign>G</sign><line>2</line></clef>
+      </attributes>
+      <note>
+        <pitch><step>C</step><octave>4</octave></pitch>
+        <duration>4</duration>
+        <type>whole</type>
+        <stem default-y="0">up</stem>
+      </note>
+    </measure>
+  </part>
+</score-partwise>
+)";
+
+    auto &mgr = api::DocumentManager::getInstance();
+    std::istringstream iss{xml};
+    auto docIdResult = mgr.createFromStream(iss);
+    CHECK(docIdResult.ok());
+    auto docId = docIdResult.value();
+
+    auto scoreResult = mgr.getData(docId);
+    CHECK(scoreResult.ok());
+    auto scoreData = scoreResult.value();
+    mgr.destroyDocument(docId);
+
+    // Verify the reader populated stemPositionData
+    const auto &note = scoreData.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0);
+    CHECK(note.stem == api::Stem::up);
+    CHECK(note.stemPositionData.isDefaultYSpecified);
+    CHECK_DOUBLES_EQUAL(0.0, note.stemPositionData.defaultY, 0.0001);
+
+    // Round-trip back to XML
+    auto docId2Result = mgr.createFromScore(scoreData);
+    CHECK(docId2Result.ok());
+    auto docId2 = docId2Result.value();
+    std::stringstream ss;
+    mgr.writeToStream(docId2, ss);
+    mgr.destroyDocument(docId2);
+
+    const auto output = ss.str();
+    CHECK(output.find("default-y") != std::string::npos);
+}
+
+T_END
+
 #endif