[MEI] add support for fingered tremolo#32855
[MEI] add support for fingered tremolo#32855rettinghaus wants to merge 7 commits intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds MEI fingered-tremolo (fTrem) support: new dur/unitdur conversion helpers, importer/exporter handling for TremoloTwoChord (fTrem read/write), test fixtures, and related API/signature updates. Changes
Sequence Diagram(s)sequenceDiagram
participant MEI_DOM as MEI DOM
participant Importer as MeiImporter
participant Converter as Convert
participant Tremolo as TremoloTwoChord
participant Score as Score/ObjectModel
MEI_DOM->>Importer: readFTrem(fTremNode)
Importer->>Converter: unitdurFromMEI(fTrem)
Converter-->>Importer: TremoloType
Importer->>Tremolo: create TremoloTwoChord (register UID)
Importer->>Importer: readElements() (children)
MEI_DOM->>Importer: readChord()/readNote()
Importer->>Score: attach chord -> setChord1/setChord2 and setTremoloTwoChord
Importer->>Importer: clear m_tremolo state
sequenceDiagram
participant Score as Score/ObjectModel
participant Exporter as MeiExporter
participant Converter as Convert
participant MEI_DOM as MEI DOM
Score->>Exporter: writeChord(chord)
Exporter->>Exporter: detect TremoloFirstChord
Exporter->>Converter: unitdurToMEI(tremolo)
Converter-->>Exporter: data_DURATION
Exporter->>MEI_DOM: create <fTrem> with `@unitdur` and `@xml:id`
Exporter->>MEI_DOM: write child notes
Exporter->>Exporter: detect TremoloSecondChord
Exporter->>MEI_DOM: close </fTrem>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca4f29a3-70be-40af-a23a-2d7c8427e7e5
📒 Files selected for processing (9)
src/importexport/mei/internal/meiconverter.cppsrc/importexport/mei/internal/meiconverter.hsrc/importexport/mei/internal/meiexporter.cppsrc/importexport/mei/internal/meiexporter.hsrc/importexport/mei/internal/meiimporter.cppsrc/importexport/mei/internal/meiimporter.hsrc/importexport/mei/tests/data/ftrem-01.meisrc/importexport/mei/tests/data/ftrem-01.mscxsrc/importexport/mei/tests/mei_tests.cpp
| if (isNode(chordNode.parent(), u"fTrem")) { | ||
| TremoloTwoChord* tremolo = Factory::createTremoloTwoChord(chord); | ||
| m_uids->reg(tremolo, m_tremoloId); | ||
| tremolo->setTremoloType(m_tremoloType); | ||
| chord->add(tremolo); |
There was a problem hiding this comment.
Build one shared TremoloTwoChord for the whole <fTrem>.
Both branches allocate a fresh TremoloTwoChord and attach it to the current chord. src/engraving/dom/tremolotwochord.h expects one object with chord1/chord2 set, and src/engraving/dom/chord.cpp:2361-2376 derives tremoloChordType() from that shared pairing. As written, imported fingered tremolos are left unpaired, so first/second chord classification—and therefore layout/re-export—will be wrong. Please keep a single tremolo instance for the <fTrem> and wire it with setChord1() / setChord2() (or setChords()), then feed its id through readXmlId() rather than m_uids->reg().
Also applies to: 2068-2072
| TEST_F(Mei_Tests, mei_ftrem_01) { | ||
| meiReadTest("ftrem-01"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one multi-note chord case here as well.
The new fixture only exercises <fTrem> with <note> children, so the <chord> path added in MeiImporter::readChord() and MeiExporter::writeChord() still isn't covered.
rettinghaus
force-pushed
the
mei/tremolo
branch
from
b94d642 to
0ff5753
Compare
rettinghaus
force-pushed
the
mei/tremolo
branch
from
0ff5753 to
afa28e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/importexport/mei/internal/meiimporter.cpp (2)
1842-1846:⚠️ Potential issue | 🔴 CriticalBuild one shared
TremoloTwoChordfor the whole<fTrem>.This still allocates a separate
TremoloTwoChordper chord and never wireschord1/chord2.Chord::tremoloChordType(), layout, and re-export expect one paired object. Please mirrorsrc/importexport/mnx/internal/import/mnximportpartmeasures.cpp:626-633andsrc/importexport/musicxml/internal/import/importmusicxmlpass2.cpp:6737-6747: create one tremolo, callsetChords(...), attach it only on the first chord, and route the MEI id throughreadXmlId().Also applies to: 2070-2074
1911-1914:⚠️ Potential issue | 🟡 MinorDon't drop unsupported
unitdurwarnings.
Convert::unitdurFromMEI()can already report invalidunitdurvalues, but this path ignoreswarning, so malformed<fTrem unitdur="...">input imports silently and then proceeds withINVALID_TREMOLO. Please calladdLog("tremolo unit duration", fTremNode)before recursing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 251fc559-9532-4491-a403-bd0b31da8d64
📒 Files selected for processing (2)
src/importexport/mei/internal/meiconverter.cppsrc/importexport/mei/internal/meiimporter.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ec43401-d2b3-43dc-9ee8-a878ab3595cb
📒 Files selected for processing (2)
src/importexport/mei/internal/meiimporter.cppsrc/importexport/mei/internal/meiimporter.h
rettinghaus
force-pushed
the
mei/tremolo
branch
from
9d3f204 to
8fcc01d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/importexport/mei/internal/meiimporter.cpp (1)
1896-1917:⚠️ Potential issue | 🔴 CriticalCritical:
readFTrem()will not compile and has logic errors.Multiple issues in this function:
Line 1908:
chordis undefined - There is nochordvariable in scope. This code will not compile.Line 1908: Local variable shadows member -
TremoloTwoChord* m_tremolo = ...declares a local variable that shadows the class memberm_tremolo. The child elements read viareadElements()will see the member (which is uninitialized/nullptr), not this local variable. The tremolo will never be attached to the chords.Lines 1907-1910: Warning not logged -
Convert::unitdurFromMEI()setswarning=truefor unsupported values, but it's never checked or logged.Line 1909: Inconsistent ID registration - Uses
m_uids->reg()instead ofthis->readXmlId()which is used elsewhere and handles MuseScore EIDs properly.🐛 Proposed fix
bool MeiImporter::readFTrem(pugi::xml_node fTremNode, Measure* measure, int track, Fraction& ticks) { IF_ASSERT_FAILED(measure) { return false; } bool success = true; libmei::FTrem meiFTrem; meiFTrem.Read(fTremNode); bool warning = false; - TremoloTwoChord* m_tremolo = Factory::createTremoloTwoChord(chord); - m_uids->reg(m_tremolo, meiFTrem.m_xmlId); - m_tremolo->setTremoloType(Convert::unitdurFromMEI(meiFTrem, warning)); + m_tremolo = Factory::createTremoloTwoChord(m_score->dummy()->chord()); + this->readXmlId(m_tremolo, meiFTrem.m_xmlId); + TremoloType tremoloType = Convert::unitdurFromMEI(meiFTrem, warning); + if (warning) { + this->addLog("tremolo unit duration", fTremNode); + } + m_tremolo->setTremoloType(tremoloType); success = readElements(fTremNode, measure, track, ticks); m_tremolo = nullptr; return success; }src/importexport/mei/internal/meiimporter.h (1)
255-256:⚠️ Potential issue | 🔴 CriticalMissing initialization of
m_tremolomember.The
m_tremolopointer is declared without a default initializer. InMeiImporter::read()(line 109 in the .cpp),m_tremoloId.clear()is called butm_tremolois never initialized tonullptr. This leaves the pointer with an indeterminate value, which will cause undefined behavior when checked inreadChord()/readNote()withif (m_tremolo).🐛 Proposed fix
- engraving::TremoloTwoChord* m_tremolo; + engraving::TremoloTwoChord* m_tremolo = nullptr;Additionally, in
meiimporter.cppread()function around line 109, add initialization:m_tremoloId.clear(); m_tremolo = nullptr; // Add this line
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b984829-9a5f-495a-b5be-0101a9e9b8ba
📒 Files selected for processing (2)
src/importexport/mei/internal/meiimporter.cppsrc/importexport/mei/internal/meiimporter.h
rettinghaus
force-pushed
the
mei/tremolo
branch
from
8fcc01d to
afa28e3
Compare
1 participant
This adds import/export for fingered tremolos to the MEI support.
Summary by CodeRabbit
New Features
Tests