Skip to content
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.

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

Fix #23977: Export to MusicXml wavy line starts in second voices #25811

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pacebes
Copy link
Contributor

@pacebes pacebes commented Dec 12, 2024

Resolves: #23977

This PR changes the export and import procedure to MusicXML for Trills

  • Export procedure takes into account the voice Trill is attached to
  • Import procedure imports the Trills' voice
  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@pacebes pacebes force-pushed the 23977-missingWavyLineStartSecondVoiceExportToMusicXml branch from 4d29922 to bf05986 Compare December 12, 2024 10:58
Comment on lines 8516 to 8517
// To be sure the right voice is set as createTrill does not copy the right voice from cr->score
trill->setVoice(cr->voice());
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not accurate. createTrill does nothing with the voice at all, and that is by design.

setVoice actually just calls setTrack. Given the definition of trk, it looks like it was intentional that the trill is assigned to the first voice (0). If that is not desirable, just replace trk with track (see above) and remove trk if it becomes unused because of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. It works that way
I don't know why I think trills has a voice property which I think is linked to the chord it is attached to. I think the voice is used to know where it ends and it depends on the voice's chords.

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Dec 14, 2024

Choose a reason for hiding this comment

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

Looking at @lvinken's sample score shows that this trill is attached to voice 2: it is green and below staff
image

Before your PR the entire trill gets lost on export already. This happens with master and 4.4.4 (forced to open that 4.5 score) at least, also with 3.7, but not with 3.6.2 (on the extracted mscx with the LastEID line deleted). So this part is a Mu3 regression.

With your PR a) the trill exports that voice assignment is lost (the trill is blue and shows above staff rather than below).
Same as without your PR

With that

                trill->setVoice(cr->voice());

added, the voice assignment gets kept on import (it is green), but it still shows above score, so there's still something amiss

Copy link
Contributor

Choose a reason for hiding this comment

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

However: trying to access the trill's properties leads to a crash!

MuseScore4.exe!mu::inspector::AbstractInspectorModel::styleIdByPropertyId(const mu::engraving::Pid pid) Line 419
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\abstractinspectormodel.cpp(419)
MuseScore4.exe!mu::inspector::AbstractInspectorModel::loadPropertyItem(mu::inspector::PropertyItem * propertyItem, const QList<mu::engraving::EngravingItem *> & elements, std::function<QVariant __cdecl(QVariant const &)> convertElementPropertyValueFunc) Line 640
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\abstractinspectormodel.cpp(640)
MuseScore4.exe!mu::inspector::AbstractInspectorModel::loadPropertyItem(mu::inspector::PropertyItem * propertyItem, std::function<QVariant __cdecl(QVariant const &)> convertElementPropertyValueFunc) Line 623
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\abstractinspectormodel.cpp(623)
MuseScore4.exe!mu::inspector::OrnamentSettingsModel::loadProperties() Line 124
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\notation\ornaments\ornamentsettingsmodel.cpp(124)
MuseScore4.exe!mu::inspector::AbstractInspectorModel::updateProperties() Line 403
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\abstractinspectormodel.cpp(403)
MuseScore4.exe!mu::inspector::AbstractInspectorModel::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 328
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\inspector\inspector_autogen\include_Debug\IVK4DJNV27\moc_abstractinspectormodel.cpp(328)
[External Code]
MuseScore4.exe!mu::inspector::ElementRepositoryService::elementsUpdated(const QList<mu::engraving::EngravingItem *> & _t1) Line 140
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\inspector\inspector_autogen\include_Debug\KA2SHC4NR2\moc_elementrepositoryservice.cpp(140)
MuseScore4.exe!mu::inspector::ElementRepositoryService::updateElementList(const QList<mu::engraving::EngravingItem *> & newRawElementList, mu::engraving::SelState selectionState) Line 77
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\internal\services\elementrepositoryservice.cpp(77)
MuseScore4.exe!mu::inspector::InspectorListModel::setElementList(const QList<mu::engraving::EngravingItem *> & selectedElementList, mu::engraving::SelState selectionState) Line 110
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\inspectorlistmodel.cpp(110)
MuseScore4.exe!mu::inspector::InspectorListModel::updateElementList() Line 340
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\inspectorlistmodel.cpp(340)
MuseScore4.exe!mu::inspector::InspectorListModel::setInspectorVisible(bool visible) Line 152
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\inspector\models\inspectorlistmodel.cpp(152)
MuseScore4.exe!mu::inspector::InspectorListModel::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 70
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\inspector\inspector_autogen\include_Debug\IVK4DJNV27\moc_inspectorlistmodel.cpp(70)
MuseScore4.exe!mu::inspector::InspectorListModel::qt_metacall(QMetaObject::Call _c, int _id, void * * _a) Line 114
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\inspector\inspector_autogen\include_Debug\IVK4DJNV27\moc_inspectorlistmodel.cpp(114)
[External Code]
MuseScore4.exe!muse::dock::DockFrameModel::currentDockChanged() Line 367
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\framework\dockwindow\muse_dockwindow_autogen\include_Debug\EGDWYGFDBT\moc_dockframemodel.cpp(367)
MuseScore4.exe!muse::dock::DockFrameModel::listenChangesInFrame::__l2::<lambda_2>::operator()() Line 158
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\framework\dockwindow\internal\dockframemodel.cpp(158)
MuseScore4.exe!QtPrivate::FunctorCall<QtPrivate::IndexesList<>,QtPrivate::List<>,void,muse::dock::DockFrameModel::listenChangesInFrame'::2'::<lambda_2>>::call(muse::dock::DockFrameModel::listenChangesInFrame::__l2::<lambda_2> & f, void * * arg) Line 146
at C:\Qt\6.2.9\include\QtCore\qobjectdefs_impl.h(146)
MuseScore4.exe!QtPrivate::Functor<muse::dock::DockFrameModel::listenChangesInFrame'::2'::<lambda_2>,0>::call<QtPrivate::List<>,void>(muse::dock::DockFrameModel::listenChangesInFrame::__l2::<lambda_2> & f, void * _formal, void * * arg) Line 256
at C:\Qt\6.2.9\include\QtCore\qobjectdefs_impl.h(256)
MuseScore4.exe!QtPrivate::QFunctorSlotObject<muse::dock::DockFrameModel::listenChangesInFrame'::2'::<lambda_2>,0,QtPrivate::List<>,void>::impl(int which, QtPrivate::QSlotObjectBase * this
, QObject * r, void * * a, bool * ret) Line 420
at C:\Qt\6.2.9\include\QtCore\qobjectdefs_impl.h(420)
[External Code]
MuseScore4.exe!KDDockWidgets::Frame::currentDockWidgetChanged(KDDockWidgets::DockWidgetBase * _t1) Line 296
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\framework\dockwindow\thirdparty\KDDockWidgets\src\kddockwidgets_autogen\include_Debug\PGVKHOT7RA\moc_Frame_p.cpp(296)
MuseScore4.exe!KDDockWidgets::Frame::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 126
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\framework\dockwindow\thirdparty\KDDockWidgets\src\kddockwidgets_autogen\include_Debug\PGVKHOT7RA\moc_Frame_p.cpp(126)
[External Code]
MuseScore4.exe!KDDockWidgets::TabWidgetQuick::currentDockWidgetChanged(KDDockWidgets::DockWidgetBase * _t1) Line 229
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\framework\dockwindow\thirdparty\KDDockWidgets\src\kddockwidgets_autogen\include_Debug\Q7DMEJDZ4X\moc_TabWidgetQuick_p.cpp(229)
MuseScore4.exe!KDDockWidgets::TabWidgetQuick::setCurrentDockWidget(int index) Line 73
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\framework\dockwindow\thirdparty\KDDockWidgets\src\private\quick\TabWidgetQuick.cpp(73)
MuseScore4.exe!KDDockWidgets::TabWidgetQuick::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 100
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\framework\dockwindow\thirdparty\KDDockWidgets\src\kddockwidgets_autogen\include_Debug\Q7DMEJDZ4X\moc_TabWidgetQuick_p.cpp(100)
MuseScore4.exe!KDDockWidgets::TabWidgetQuick::qt_metacall(QMetaObject::Call _c, int _id, void * * _a) Line 200
at C:\Users\Jojo\Documents\GitHub\MuseScore\msvc.build\x64-debug\src\framework\dockwindow\thirdparty\KDDockWidgets\src\kddockwidgets_autogen\include_Debug\Q7DMEJDZ4X\moc_TabWidgetQuick_p.cpp(200)
[External Code]
MuseScore4.exe!muse::dock::DockTabBar::event(QEvent * event) Line 49
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\framework\dockwindow\internal\docktabbar.cpp(49)
[External Code]
MuseScore4.exe!KDDockWidgets::MouseEventRedirector::eventFilter(QObject * source, QEvent * ev) Line 90
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\framework\dockwindow\thirdparty\KDDockWidgets\src\private\quick\QWidgetAdapter_quick.cpp(90)
[External Code]
MuseScore4.exe!main(int argc, char * * argv) Line 206
at C:\Users\Jojo\Documents\GitHub\MuseScore\src\app\main.cpp(206)
[External Code]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is no related to the patch:

  • If you compile a version without the patch (OS: Windows 11 Version 2009 or later, Arch.: x86_64, MuseScore Studio version (64-bit): 4.5.0-, revision: github-musescore-musescore) and then:
  1. Create a file in MuseScore with a trill as simple as: Sample score.zip
  2. Export the file to a .musicxml file
  3. Import this .musicxml
  4. Try to access the Trill properties.... you also get a crash.
  • However with this version with the patch:
  1. Export the example MuseScore file to musicxml
  2. Imports the MuseScore file and saves it as ".mscz"
  3. Opens the ".mscz" file: you can access to the Trill properties

So, I think the problems is related with the version we are dealing with, but not with the patch itself. The problem is unrelated to the patch. With or without the patch:

  • If you try to access to any Trill property when you open a .musicxml file... you get a crash
  • If you save this .musicxml to .mscz the file and you open the ".mscz" file you can access to Trill properties.

It seems that there is a problem editing the Trill properties when you open a .musicxml file. Please, could you double check it @Jojo-Schmitz ?

Regarding

added, the voice assignment gets kept on import (it is green), but it still shows above score, so there's still something amiss

It is shown above the score unless you save it as ".mscz" and then opens the ".mscz". In that case you can see the trill below the score.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pandora's Box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have found the problem and I have uploaded the latest changes. I have copied the "Ornaments" values from the linked Chord to the Trill.
I hope it works now as it should.

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Dec 15, 2024

Choose a reason for hiding this comment

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

Works fine, except for the glitch that after the import the trill shows above staff, but below after saving as mscz and reopening.
After the import its placement property shows above (which is where it is, wrongly), after save as mscz and reopen its placement property shows as Auto, and the trill shows below (which is what it should).
When after the import using the "Reset to default", that corrects this too.
Seems that is an Ornament setting, inherited by the trill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made a new change to set this property to Auto.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yessss, that's it!

@pacebes pacebes force-pushed the 23977-missingWavyLineStartSecondVoiceExportToMusicXml branch 4 times, most recently from a139301 to d1b6c98 Compare December 15, 2024 13:40
@pacebes pacebes force-pushed the 23977-missingWavyLineStartSecondVoiceExportToMusicXml branch from d1b6c98 to 740fece Compare December 15, 2024 16:23
@pacebes pacebes force-pushed the 23977-missingWavyLineStartSecondVoiceExportToMusicXml branch from 740fece to a351256 Compare December 15, 2024 16:31
@cbjeukendrup cbjeukendrup requested a review from miiizen December 15, 2024 17:23
@miiizen miiizen self-assigned this Dec 18, 2024
@@ -416,6 +416,9 @@ mu::engraving::Sid AbstractInspectorModel::styleIdByPropertyId(const mu::engravi
mu::engraving::Sid result = mu::engraving::Sid::NOSTYLE;

for (const mu::engraving::EngravingItem* element : m_elementList) {
IF_ASSERT_FAILED(element) {

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can reproduce the crash, but I think it might be better to fix the issue further upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this lines before I solved the problem within importmusicxmlpass2.cpp. Therefore I can delete them.

About:

I can reproduce the crash, but I think it might be better to fix the issue further upstream.

I haven't been able to reproduce now the crash. Could yo provide me with an example to test it and solve it please ?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry to bother @miiizen , could you please tell me how you can reproduce the crash ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Musicxml Export] - Missing wavy line start
4 participants