Skip to content

Mnx import/export changes for schema version 15 #33236

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.

Sign up for GitHub

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

Open
rpatters1 wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Mnx import/export changes for schema version 15 #33236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8bbc01d
update to latest mnxdom with latest schema (15)
rpatters1 Apr 30, 2026
d7606ed
refactored existing code to latest mnx schema changes
rpatters1 Apr 30, 2026
bc3d7e5
implement bow direction (with tests)
rpatters1 Apr 30, 2026
1f56823
import fermatas
rpatters1 May 1, 2026
d904056
export working (with test)
rpatters1 May 1, 2026
cfc84ce
fix breath marks
rpatters1 May 1, 2026
414201b
code formatting issues
rpatters1 May 1, 2026
b260154
coderabbit review changes
rpatters1 May 1, 2026
b82f951
add clarifying comment about barline fermata exports
rpatters1 May 1, 2026
3410eed
update comment about solitary sequences to match revised code behavior
rpatters1 May 2, 2026
567eeb6
improve test coverage for breath marks, with fixes
rpatters1 May 3, 2026
64a0cd5
better fermata test coverage and code cleanup
rpatters1 May 3, 2026
3b9e219
coderabbit fixes:
rpatters1 May 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/importexport/mnx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ else()
FetchContent_Declare(
mnxdom
GIT_REPOSITORY https://github.com/rpatters1/mnxdom.git
GIT_TAG e7c947bf768caccf315426dcae0dfac02caf738b
GIT_TAG 1d9822f8fc5ee568c14f1c699d4f10565908dc6b
)
FetchContent_MakeAvailable(mnxdom)
set(MNXDOM_LIB mnxdom)
Expand Down
14 changes: 14 additions & 0 deletions src/importexport/mnx/internal/export/mnxexporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/
#include "mnxexporter.h"
#include "internal/shared/mnxtypesconv.h"

#include <stdexcept>

Expand Down Expand Up @@ -85,6 +86,19 @@ std::pair<size_t, int> MnxExporter::mnxPartStaffFromStaffIdx(engraving::staff_id
return it->second;
}

//---------------------------------------------------------
// mnxFermataFromFermata
//---------------------------------------------------------

mnx::Fermata MnxExporter::mnxFermataFromFermata(const Fermata* fermata)
{
mnx::Fermata result;
result.set_or_clear_duration(toMnxFermataDuration(fermata->fermataType()));
result.set_or_clear_symbol(toMnxFermataSymbol(fermata->symId()));
result.set_or_clear_orient(toMnxOrientation(fermata->placement()));
return result;
}

//---------------------------------------------------------
// exportMnx
//---------------------------------------------------------
Expand Down
3 changes: 3 additions & 0 deletions src/importexport/mnx/internal/export/mnxexporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class ChordRest;
class EID;
class EngravingItem;
class EngravingObject;
class Fermata;
class GraceNotesGroup;
class Instrument;
class Measure;
Expand Down Expand Up @@ -68,6 +69,7 @@ class MnxExporter
std::optional<mnx::sequence::Event> mnxEventFromCR(const engraving::ChordRest* cr);
size_t mnxMeasureIndexFromMeasure(const engraving::Measure* measure) const;
std::pair<size_t, int> mnxPartStaffFromStaffIdx(engraving::staff_idx_t staffIdx) const;
static mnx::Fermata mnxFermataFromFermata(const engraving::Fermata* fermata);

private:
enum class ContentContext {
Expand Down Expand Up @@ -119,6 +121,7 @@ class MnxExporter
void createTies(mnx::sequence::NoteBase& mnxNote, engraving::Note* note);
const engraving::Tuplet* findTopTuplet(engraving::ChordRest* chordRest, const ExportContext& ctx) const;
void exportDrumsetKit(const engraving::Part* part, const engraving::Instrument* instrument, mnx::Part& mnxPart);
void createMarkings(mnx::sequence::Event& mnxEvent, engraving::ChordRest* cr);

void exportSpanners();

Expand Down
21 changes: 19 additions & 2 deletions src/importexport/mnx/internal/export/mnxexportglobal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,8 @@ static void exportMeasureElements(mnx::global::Measure& mnxMeasure, const Measur
}
}

for (Segment* segment = measure->first(); segment; segment = segment->next()) {
for (EngravingItem* item : segment->annotations()) {
for (const Segment* segment = measure->first(); segment; segment = segment->next()) {
for (const EngravingItem* item : segment->annotations()) {
IF_ASSERT_FAILED(item) {
continue;
}
Expand All @@ -302,6 +302,23 @@ static void exportMeasureElements(mnx::global::Measure& mnxMeasure, const Measur
}
}
}

if (const BarLine* barLine = measure->endBarLine()) {
if (const Segment* seg = barLine->segment()) {
for (const EngravingItem* item : seg->annotations()) {
IF_ASSERT_FAILED(item) {
continue;
}
if (item->isFermata()) {
const Fermata* fermata = toFermata(item);
DO_ASSERT(fermata);
if (fermata) {
mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata));
}
}
}
Comment on lines +308 to +319
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

set_fermata is called in a loop — only the last fermata survives.

set_fermata is a setter, not an appender. If the end-barline segment carries more than one Fermata annotation (e.g. one per track), every iteration overwrites the previous call and all but the last are silently discarded.

The import side confirms that MNX v15 supports exactly one global-measure fermata (std::optional<mnx::Fermata> in mnximportmarkings.cpp:844), and the importer always assigns track(0) with a /// @todo`` for multiples. The export should be consistent: prefer the first fermata found (or filter for track 0 to match import semantics), then break.

🛡️ Proposed fix — export the first fermata, consistent with import track-0 semantics 
         for (const EngravingItem* item : seg->annotations()) {
             IF_ASSERT_FAILED(item) {
                 continue;
             }
             if (item->isFermata()) {
                 const Fermata* fermata = toFermata(item);
-                DO_ASSERT(fermata);
-                if (fermata) {
+                IF_ASSERT_FAILED(fermata) {
+                    break;
+                }
+                if (fermata->track() == 0) {   // match importer's track-0 assumption
                     mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata));
+                    break;  // MNX global measure supports only one fermata
                 }
             }
         }

If keeping the loop track-agnostic is intentional (e.g. always take whatever is first), a plain break after set_fermata is the minimal fix to make the "first wins" policy explicit.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/importexport/mnx/internal/export/mnxexportglobal.cpp` around lines 308 -
319, The loop over seg->annotations() calls mnxMeasure.set_fermata(...)
repeatedly so only the last Fermata survives; change the export to pick the
first fermata (or explicitly prefer track 0) and stop iterating once it's set.
Locate the loop in mnxexportglobal.cpp where it checks item->isFermata(),
convert the behavior to: when you find the first valid Fermata (toFermata(item)
with DO_ASSERT/IF_ASSERT checks), call
mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata)) and then
break out of the loop (or alternatively filter items by track == 0 before
selecting the first), ensuring only the first fermata is exported.

Copy link
Copy Markdown
Contributor Author

@rpatters1 rpatters1 May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really matter if we export the first found or the last found. The current code stays clean in case we need to add other types of annotations. But I will add a note about it.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpatters1, that makes sense — keeping the loop open for future annotation types is a reasonable design choice, and a comment clarifying the "last wins" behaviour (along with a note that MNX v15 supports only one global-measure fermata) will be sufficient for readers. Thanks for the context!

(ᵔ◡ᵔ)🐰

🧠 Learnings used 
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32874
File: src/appshell/widgets/splashscreen/loadingscreenview.cpp:32-32
Timestamp: 2026-04-01T07:34:44.168Z
Learning: In the MuseScore C++ codebase, `IApplication::fullVersion()` (non-static virtual instance method in `src/framework/global/iapplication.h`) must only be called after application/services initialization. Do not use it in code paths that run before services are ready (e.g., splash/loading screens). If you need the app version via service-free static access in early UI/boot code, call `muse::BaseApplication::appFullVersion()` instead (static method declared in `src/framework/global/internal/baseapplication.h`), which reads compile-time constants.

Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32950
File: src/framework/testing/environment.cpp:65-65
Timestamp: 2026-04-09T08:32:46.263Z
Learning: When using MuseScore’s IoC container (e.g., `muse::modularity::globalIoc()->registerExport<T>(name, impl)`), treat the `name` argument as the *registering module* identifier used for informational/tracing purposes. It does not have to match the key that consumers use to resolve the service later. In reviews, do not flag cases where the registration key (module name) differs from the resolution/lookup key as a bug, as this mismatch can be intentional (e.g., `IApplication` registered under `"testing"` in `src/framework/testing/environment.cpp`).

Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33000
File: src/framework/audio/main/audiomodule.cpp:154-154
Timestamp: 2026-04-14T09:30:22.267Z
Learning: When registering exports via MuseScore’s IoC container (`muse::modularity`), it is acceptable to pass a raw pointer created with `new` to `ioc()->registerExport<T>(name, ...)`. The container will wrap it internally into a `std::shared_ptr`, so calls like `ioc()->registerExport<T>(name, new Impl(...))` are not a bug and should not be flagged for ownership/style issues. Prefer clarity, but do not treat raw `new` in `registerExport` arguments as an error; `ioc()->registerExport<T>(name, std::make_shared<Impl>(...))` is also valid.

Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33028
File: src/project/internal/opensaveprojectscenario.cpp:477-477
Timestamp: 2026-04-16T10:04:41.682Z
Learning: In the MuseScore C++ codebase, `configuration()->supportForumUrl()` intentionally returns a MuseScore Studio/app support URL on `musescore.org` (e.g., `/forum/...`, language-specific). When adding user-facing help text or help-button links for cloud-related errors (issues involving saving/uploading via `musescore.com`), ensure the help URL points to `musescore.com` (not `musescore.org`). Use `musescore.org` for Studio/app support problems and `musescore.com` for cloud-saving/upload issues.

Learnt from: saintmatthieu
Repo: musescore/MuseScore PR: 33036
File: src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cpp:101-105
Timestamp: 2026-04-21T09:36:52.350Z
Learning: This repository guarantees Qt 6.10 as the minimum supported version. Therefore, when reviewing C++ code, do not require compatibility guards (e.g., QT_VERSION_CHECK) or fallback paths for Qt 6.10+ APIs/classes/enums. For example, do not flag uses of QSortFilterProxyModel::endFilterChange or QSortFilterProxyModel::Direction::Rows as needing guards for older Qt versions; such guards are unnecessary in this codebase.

}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

//---------------------------------------------------------
Expand Down
190 changes: 104 additions & 86 deletions src/importexport/mnx/internal/export/mnxexportsequences.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,92 +167,114 @@ static void createLyrics(mnx::sequence::Event& mnxEvent, const ChordRest* cr,
}
}

//---------------------------------------------------------
// createMarking
// export a single marking from an Articulation instance
//---------------------------------------------------------

static std::optional<mnx::sequence::EventMarkingBase> createMarking(const Articulation* a,
mnx::sequence::EventMarkings mnxMarkings)
{
const SymId sym = a->symId();

switch (sym) {
case SymId::articAccentAbove:
case SymId::articAccentBelow:
return mnxMarkings.ensure_accent();
case SymId::stringsDownBow:
case SymId::stringsDownBowTurned:
return mnxMarkings.ensure_bowDirection(mnx::MarkingUpDown::Down);
case SymId::stringsUpBow:
case SymId::stringsUpBowTurned:
return mnxMarkings.ensure_bowDirection(mnx::MarkingUpDown::Up);
case SymId::articSoftAccentAbove:
case SymId::articSoftAccentBelow:
return mnxMarkings.ensure_softAccent();
case SymId::articStaccatoAbove:
case SymId::articStaccatoBelow:
return mnxMarkings.ensure_staccato();
case SymId::articStaccatissimoAbove:
case SymId::articStaccatissimoBelow:
return mnxMarkings.ensure_staccatissimo();
case SymId::articStaccatissimoStrokeAbove:
case SymId::articStaccatissimoStrokeBelow:
return mnxMarkings.ensure_spiccato();
case SymId::articStressAbove:
case SymId::articStressBelow:
return mnxMarkings.ensure_stress();
case SymId::articUnstressAbove:
case SymId::articUnstressBelow:
return mnxMarkings.ensure_unstress();
case SymId::articMarcatoAbove:
case SymId::articMarcatoBelow:
return mnxMarkings.ensure_strongAccent();
case SymId::articTenutoAbove:
case SymId::articTenutoBelow:
return mnxMarkings.ensure_tenuto();
default:
break;
}

return std::nullopt;
}

//---------------------------------------------------------
// processAnnotations
// template function to process annotations for both
// events and full measure rests
//---------------------------------------------------------

template<typename EventType>
void static processAnnotations(EventType& mnxEvent, ChordRest* cr)
{
// Grace notes are parented to their main chord, not directly to a segment.
// Segment-anchored annotations such as fermatas belong to the main chord's
// segment and must not be re-exported from each grace note event.
if (cr->isGrace()) {
return;
}

if (Segment* seg = cr->segment()) {
for (EngravingItem* ann : seg->annotations()) {
if (!ann || ann->track() != cr->track()) {
continue;
}
if (ann->isFermata()) {
const Fermata* fermata = toFermata(ann);
IF_ASSERT_FAILED(fermata) {
continue;
}
mnxEvent.set_fermata(MnxExporter::mnxFermataFromFermata(fermata));
/// @note MNX currently can only store one barline fermata per measure stack.
/// To keep the loop clean for future expansion, this code deliberately
/// exports the last one found. We can revisit this if/when MNX provides
/// more clarity around barline fermatas.
}
}
}
}

//---------------------------------------------------------
// createMarkings
// export articulations, breath marks, single-note
// tremolo
//---------------------------------------------------------

static void createMarkings(mnx::sequence::Event& mnxEvent, ChordRest* cr)
void MnxExporter::createMarkings(mnx::sequence::Event& mnxEvent, ChordRest* cr)
{
IF_ASSERT_FAILED(cr) {
return;
}

auto pointingFromAnchor = [](ArticulationAnchor anchor) -> std::optional<mnx::MarkingUpDown> {
switch (anchor) {
case ArticulationAnchor::TOP: return mnx::MarkingUpDown::Up;
case ArticulationAnchor::BOTTOM: return mnx::MarkingUpDown::Down;
case ArticulationAnchor::AUTO:
default:
return std::nullopt;
}
};

if (cr->isChord()) {
const Chord* chord = toChord(cr);
for (Articulation* a : chord->articulations()) {
auto mnxMarkings = mnxEvent.ensure_markings();
IF_ASSERT_FAILED(a) {
continue;
}
const SymId sym = a->symId();
const auto pointing = pointingFromAnchor(a->anchor());

switch (sym) {
case SymId::articAccentAbove:
case SymId::articAccentBelow: {
auto acc = mnxMarkings.ensure_accent();
if (pointing) {
acc.set_pointing(*pointing);
}
break;
}
case SymId::articSoftAccentAbove:
case SymId::articSoftAccentBelow: {
auto sa = mnxMarkings.ensure_softAccent();
break;
}
case SymId::articStaccatoAbove:
case SymId::articStaccatoBelow: {
auto st = mnxMarkings.ensure_staccato();
break;
}
case SymId::articStaccatissimoAbove:
case SymId::articStaccatissimoBelow: {
auto st = mnxMarkings.ensure_staccatissimo();
break;
}
case SymId::articStaccatissimoStrokeAbove:
case SymId::articStaccatissimoStrokeBelow: {
auto sp = mnxMarkings.ensure_spiccato();
break;
}
case SymId::articStressAbove:
case SymId::articStressBelow: {
auto st = mnxMarkings.ensure_stress();
break;
}
case SymId::articUnstressAbove:
case SymId::articUnstressBelow: {
auto un = mnxMarkings.ensure_unstress();
break;
}
case SymId::articMarcatoAbove:
case SymId::articMarcatoBelow: {
auto sa = mnxMarkings.ensure_strongAccent();
if (pointing) {
sa.set_pointing(*pointing);
}
break;
}
case SymId::articTenutoAbove:
case SymId::articTenutoBelow: {
auto tn = mnxMarkings.ensure_tenuto();
break;
}
default:
break;
auto mnxMarkings = mnxEvent.ensure_markings();
if (auto marking = createMarking(a, mnxMarkings)) {
marking->set_or_clear_orient(toMnxOrientation(a->anchor()));
}
}

Expand All @@ -266,22 +288,16 @@ static void createMarkings(mnx::sequence::Event& mnxEvent, ChordRest* cr)
}
}

if (Segment* seg = cr->segment()) {
for (EngravingItem* ann : seg->annotations()) {
if (!ann || ann->type() != ElementType::BREATH || ann->track() != cr->track()) {
continue;
}
const Breath* breath = toBreath(ann);
if (!breath) {
continue;
}
auto mnxMarkings = mnxEvent.ensure_markings();
auto mnxBreath = mnxMarkings.ensure_breath();
if (const auto breathSym = toMnxBreathMarkSym(breath->symId())) {
mnxBreath.set_symbol(breathSym.value());
}
if (Breath* breath = cr->hasBreathMark()) {
auto mnxMarkings = mnxEvent.ensure_markings();
auto mnxBreath = mnxMarkings.ensure_breath();
mnxBreath.set_or_clear_orient(toMnxOrientation(breath->placement()));
if (const auto breathSym = toMnxBreathMarkSym(breath->symId())) {
mnxBreath.set_symbol(breathSym.value());
}
}

processAnnotations(mnxEvent, cr);
}

//---------------------------------------------------------
Expand Down Expand Up @@ -1074,6 +1090,7 @@ void MnxExporter::createSequences(const Part* part, const Measure* measure, mnx:
LOGW() << "Skipping MNX fullMeasure staffPosition export; invalid staff step.";
}
}
processAnnotations(fullMeasure, rest);
} else {
/// @todo If MNX adds explicit rest visibility, export hidden (non-gap) full-measure rests; keep omitting gap rests.
// Hidden/gap measure rests should not generate a sequence.
Expand All @@ -1089,12 +1106,13 @@ void MnxExporter::createSequences(const Part* part, const Measure* measure, mnx:
}
}

// Avoid cluttering output with unnecessary full-measure rests.
// Keep a solitary full-measure sequence only when it carries explicit placement data.
// Avoid cluttering the output with unnecessary full-measure rests.
// Keep a solitary full-measure sequence only when the full measure rest
// carries additional information, such as a staff position or fermata.
if (mnxSequences.size() == 1) {
auto onlySequence = mnxSequences.at(0);
const auto fullMeasure = onlySequence.fullMeasure();
if (fullMeasure && onlySequence.content().empty() && !fullMeasure->staffPosition()) {
if (fullMeasure && onlySequence.content().empty() && fullMeasure->empty()) {
mnxSequences.erase(0);
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/importexport/mnx/internal/import/mnximporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,10 @@ void MnxImporter::importGlobalMeasures()
createTempoMark(measure, tempo);
}
}
if (const std::optional<mnx::Fermata>& mnxFermata = mnxMeasure.fermata()) {
Segment* const segment = measure->getSegment(SegmentType::EndBarLine, measure->endTick());
addFermata(segment, mnxFermata.value(), 0);
}

/// @todo MNX currently offers no way to exclude a measure from having
/// a measure number.
Expand Down
Loading
Loading