Initial TablEdit importer #29242
Initial TablEdit importer #29242
Conversation
|
Will fix the codestyle issues (but not tonight :-) ). |
| LOGD("tbed %d wOldNum %d wFormat %d", tefHeader.tbed, tefHeader.wOldNum, tefHeader.wFormat); | ||
| if ((tefHeader.wFormat >> 8) < 10) { | ||
| return Err::FileBadFormat; | ||
| //return Err::FileTooOld; // TODO: message is too specific for MuseScore format |
There was a problem hiding this comment.
Perhaps we should indeed somehow refactor the error handling so that engraving::Err no longer has to be used outside engraving (this also applies to other importers). But that's probably something for a next PR.
| void TablEdit::createTitleFrame() | ||
| { | ||
| VBox* vbox = Factory::createTitleVBox(score->dummy()->system()); | ||
| vbox->setTick(mu::engraving::Fraction(0, 1)); |
There was a problem hiding this comment.
Perhaps you need to call vbox->manageExclusionFromParts(/*exclude*/ false);, like in MasterNotation::applyOptions for new scores.
| else { | ||
| LOGD("measure %p", measure); | ||
| } | ||
| Segment* segment { measure->getSegment(mu::engraving::SegmentType::ChordRest, tick) }; |
There was a problem hiding this comment.
Shouldn't this be Segment::CHORD_REST_OR_TIME_TICK_TYPE, for the situation that the text doesn't coincide with a Chord? Or will it always do so?
| instrString str { pitch }; | ||
| stringData.stringList().push_back(str); | ||
| } | ||
| part->instrument()->setStringData(stringData); |
There was a problem hiding this comment.
For playback to work correctly, we'll also need to set an instrument ID. See perhaps searchTemplateForInstrNameList, but maybe there are better ways in this case.
| void TablEdit::createNotesFrame() | ||
| { | ||
| if (!tefHeader.notes.empty()) { | ||
| VBox* vbox = Factory::createTitleVBox(score->dummy()->system()); |
There was a problem hiding this comment.
Or would TBox perhaps be appropriate here?
There was a problem hiding this comment.
GitHub displays a couple of compiler warnings in this file
|
Regarding timing/releases: given that the feature freeze phase for 4.6 has been entered last week, and that this import is still a work in progress, this will likely have to be a 4.7 feature. Would that be okay with you? |
|
Hi Casper, thanks for the quick feedback. Will fix the issues, which will take a few days due to other (higher priority) personal obligations. With respect to timing, of course I'd prefer to have this in 4.6 but I understand the business need for feature freeze and respect any decision by the MuseScore team. I know a few people who are anxiously waiting for TablEdit import, who would be somewhat disappointed but I expect they can probably use the nightly builds whenever new features are merged into the master branch again. Regards, Leon. |
|
Hi Leon, |
|
Well, first of all this needs to build properly (on GitHub CI) ;-) |
|
Casper, Jojo, fixing the build issues shouldn't be that hard, I would guess that probably fits within a week. Making the code release-ready depends a bit on a judgement call, as the TablEdit application and its file format are quite feature rich. With the amount of time I currently have available, to support most of the features will probably take me more than a year. Question would be "what would you consider a minimum viable product". I'll share a list of features supported and main missing features plus some realistic examples of imported TablEdit files. Regards, Leon. |
|
Build fixed, root cause of the allocation error was my mistake of putting const pointers in a std::vector, which is not allowed but is not reported by my compiler (Apple clang version 15.0.0 (clang-1500.3.9.4)) and causes no ill effect in my environment. Unit tests fail on one file (TablEdit_Tests.tef_grace_1.tef) with a heap-buffer-overflow when calling mu::engraving::Part::setPlainLongName(muse::String const&), am looking into that. Once all tests run OK, I'll look into the remaining comments. |
|
Here's a fix for the MSVC compiler warnings
diff --git a/src/importexport/tabledit/internal/importtef.cpp b/src/importexport/tabledit/internal/importtef.cpp
index 4787031648..c640f4bb70 100644
--- a/src/importexport/tabledit/internal/importtef.cpp
+++ b/src/importexport/tabledit/internal/importtef.cpp
@@ -152,7 +152,7 @@ engraving::part_idx_t TablEdit::partIdx(size_t stringIdx, bool& ok) const
// return total number of strings in previous parts
-int TablEdit::stringNumberPreviousParts(part_idx_t partIdx) const
+part_idx_t TablEdit::stringNumberPreviousParts(part_idx_t partIdx) const
{
part_idx_t result { 0 };
for (part_idx_t i = 0; i < partIdx; ++i) {
@@ -318,7 +318,7 @@ void TablEdit::createContents()
for (size_t part = 0; part < tefInstruments.size(); ++part) {
LOGD("part %zu", part);
- for (size_t voice = 0; voice < mu::engraving::VOICES; ++voice) {
+ for (int voice = 0; voice < mu::engraving::VOICES; ++voice) {
LOGD("- voice %zu", voice);
auto& voiceContent { voiceAllocators.at(part).voiceContent(voice) };
TupletHandler tupletHandler;
@@ -865,13 +865,13 @@ void TablEdit::readTefInstruments()
instrument.fClef = readUInt8();
instrument.output = readUInt16();
instrument.options = readUInt16();
- for (uint16_t i = 0; i < 12; ++i) {
+ for (uint16_t j = 0; j < 12; ++j) {
auto n = readUInt8();
- instrument.tuning[i] = n;
+ instrument.tuning[j] = n;
}
// name is a zero-terminated utf8 string
bool atEnd = false;
- for (uint16_t i = 0; i < 36; ++i) {
+ for (uint16_t j = 0; j < 36; ++j) {
auto c = readUInt8();
if (c == 0) {
// stop appending, but continue reading
diff --git a/src/importexport/tabledit/internal/importtef.h b/src/importexport/tabledit/internal/importtef.h
index 39ff2dd2e8..0dd45a1d69 100644
--- a/src/importexport/tabledit/internal/importtef.h
+++ b/src/importexport/tabledit/internal/importtef.h
@@ -153,7 +153,7 @@ class TablEdit
void createTitleFrame();
void initializeVoiceAllocators(std::vector<VoiceAllocator>& allocators);
engraving::part_idx_t partIdx(size_t stringIdx, bool& ok) const;
- int stringNumberPreviousParts(engraving::part_idx_t partIdx) const;
+ engraving::part_idx_t stringNumberPreviousParts(engraving::part_idx_t partIdx) const;
void readTefContents();
void readTefHeader();
void readTefInstruments();
diff --git a/src/importexport/tabledit/internal/voiceallocator.cpp b/src/importexport/tabledit/internal/voiceallocator.cpp
index 6b1e2100d0..0a4c0605b0 100644
--- a/src/importexport/tabledit/internal/voiceallocator.cpp
+++ b/src/importexport/tabledit/internal/voiceallocator.cpp
@@ -87,7 +87,6 @@ static int durationToInt(uint8_t duration) // TODO duplicated code ?
case 28: return 7; //"16th double dotted";
default: return 0; //"undefined";
}
- return 0; //"undefined";
}
int VoiceAllocator::stopPosition(const size_t voice) |
|
Re: Question would be "what would you consider a minimum viable product". Maybe you could post that question here: There are some layout features in TablEdit that cannot be supported by MuseScore but most guitar notation should be OK even if playback effects do not work. |
|
Regarding that question, I had the following thought: whether we can include it in a release depends more on the nature of the incompleteness than on its size. If files are imported without errors, generating valid MSCZ files, but just some amount of information is ignored, then it's perhaps already fine. |
|
That makes sense and might allow inclusion in 4.6. Basic score import without errors would get the ball rolling and provide a basis for forum users to comment on the next steps. |
|
Replying to Jojo's fixes for the MSVC compiler warnings (thanks for looking into that, hadn't noticed it myself !):
Will push a new commit, let's see how that works out |
|
Replying to Casper's message:
Currently I am not aware of crashes or invalid MSCZ files being generated. The exception is the unit test in the Github environment. This issue does not reproduce for me (can only test on MacOS), so fixing it is not trivial. It does hint at some kind of memory corruption I have not yet been able to find. TablEdit files containing guitar-like stringed instruments import correctly but some musical data will be missing as itis not yet imported (e.g. fingering, volume, lyrics, voltas). |
diff --git a/src/importexport/tabledit/internal/importtef.cpp b/src/importexport/tabledit/internal/importtef.cpp
index 9b770b5122..c18722f203 100644
--- a/src/importexport/tabledit/internal/importtef.cpp
+++ b/src/importexport/tabledit/internal/importtef.cpp
@@ -320,7 +320,7 @@ void TablEdit::createContents()
LOGD("part %zu", part);
for (voice_idx_t voice = 0; voice < mu::engraving::VOICES; ++voice) {
LOGD("- voice %zu", voice);
- auto& voiceContent { voiceAllocators.at(part).voiceContent(voice) };
+ auto& voiceContent { voiceAllocators.at(part).voiceContent(static_cast<int>(voice)) };
TupletHandler tupletHandler;
for (size_t k = 0; k < voiceContent.size(); ++k) {
LOGD(" - chord %zu", k);
@@ -374,7 +374,7 @@ void TablEdit::createContents()
if (firstNote->rest) {
LOGD(" - rest position %d string %d fret %d", firstNote->position, firstNote->string, firstNote->fret);
- addRest(segment, track, tDuration, length, toColor(voice));
+ addRest(segment, track, tDuration, length, toColor((static_cast<int>(voice)));
} else {
LOGD(" - note(s) position %d string %d fret %d", firstNote->position, firstNote->string, firstNote->fret);
mu::engraving::Chord* chord { Factory::createChord(segment) };
@@ -396,11 +396,11 @@ void TablEdit::createContents()
int pitch = 96 - instrument.tuning.at(note->string - stringOffset - 1) + note->fret;
LOGD(" -> string %d fret %d pitch %d", note->string, note->fret, pitch);
// note TableEdit's strings start at 1, MuseScore's at 0
- addNoteToChord(chord, track, pitch, note->fret, note->string - 1, note->tie, toColor(voice));
+ addNoteToChord(chord, track, pitch, note->fret, note->string - 1, note->tie, toColor((static_cast<int>(voice)));
if (note->hasGrace) {
// todo fix magical constant 96 and code duplication
int gracePitch = 96 - instrument.tuning.at(/* todo */ note->string - stringOffset - 1) + note->graceFret;
- addGraceNotesToChord(chord, gracePitch, note->graceFret, /* todo */ note->string - 1, toColor(voice));
+ addGraceNotesToChord(chord, gracePitch, note->graceFret, /* todo */ note->string - 1, toColor((static_cast<int>(voice)));
}
}
tupletHandler.addCr(measure, chord);Using an static muse::draw::Color toColor(const int)
{
// no debug: color notes black
return muse::draw::Color::BLACK;
}You could change const std::vector<std::vector<const TefNote*> >& voiceContent(int voice) const { return voiceContents.at(voice); }to const std::vector<std::vector<const TefNote*> >& voiceContent(voice_idx_t voice) const { return voiceContents.at(voice); }Doing the same fot BTW: a rebase would get rid of 3 other compiler warnings (outside your code). |
|
Rebased to current master (commit 7fc2f52 of Sun Aug 17 02:57:53 2025 +0200) |
|
Still crashes in the unit tests, seems to be about |
|
Another crash, in TablEdit_Tests_tef_guitar_bass_Test::TestBody() ../src/importexport/tabledit/tests/tabledit_tests.cpp, line 80 and |
|
@lvinken In order to reproduce the crash from CI, enable Address Sanitizer by passing the following option to the CMake configuration command: Then rebuild the project. |
|
Rebuilt with Address Sanitizer enabled, crash reproduces, will investigate. |
…creating measures
|
My previous commit results in iex_tabledit_tests running correctly on macOS with Address Sanitizer enabled, but apparently not in the Github CI environment :-(. Will require some thinking to find out how to fix that ... |
Resolves: #26144
Initial (and still incomplete) TablEdit File Format 3.00+ importer. Merged into a single commit, full history still available in my import_tef_baseline_master_20250812_ae66071 branch.
Will continue working on completing the implementation, but want to prevent a complete big-bang pull request. As is, the import is already usable.