Fix #34451: Add rest position in musicxml export #9955

julienripoche · 2021-12-02T11:39:31Z

Resolves: https://musescore.org/en/node/34451

Export rest position attributes (default-x, default-y) in the same way it has already been done for notes.

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

julienripoche · 2021-12-02T11:43:59Z

Hi ! I added some code to export rest positional attributes in the same way as for the notes.
I originally tested it on the v3.6.2 branch and it worked as expected.
But I couldn't manage to install the latest version of Qt in order to build the master branch on my machine.

Jojo-Schmitz · 2021-12-02T11:44:30Z

@lvinken Mind to double-check this?

Jojo-Schmitz · 2021-12-02T13:39:16Z

src/importexport/musicxml/internal/musicxml/exportxml.cpp

@@ -3757,6 +3781,7 @@ void ExportMusicXml::rest(Rest* rest, int staff)

    QString noteTag = QString("note");
    noteTag += color2xml(rest);
+    noteTag += restPosition(this, rest);


I wonder if we could simply use noteTag += notePosition(this, dynamic_cast<Note*>(rest)); here, i.e. fully reuse the existing code (which except for that type is 100% identical) rather than duplicating it? Makes the entire fix a one-liner.
Or is this to 'hacky'?

Or, probably much cleaner, change notePosition to this:

//--------------------------------------------------------- // noteRestPosition //--------------------------------------------------------- QString ExportMusicXml::noteRestPosition(const ExportMusicXml* const expMxml, const ChordRest* const cr) { QString res; IF_ASSERT_FAILED(!cr->isNote() && !cr->isRest()) { return res; } if (configuration()->musicxmlExportLayout()) { const double pageHeight = expMxml->getTenthsFromInches(expMxml->score()->styleD(Sid::pageHeight)); double measureX = expMxml->getTenthsFromDots(cr->measure()->pagePos().x()); double measureY = pageHeight - expMxml->getTenthsFromDots(cr->measure()->pagePos().y()); double noteX = expMxml->getTenthsFromDots(cr->pagePos().x()); double noteY = pageHeight - expMxml->getTenthsFromDots(cr->pagePos().y()); res += QString(" default-x=\"%1\"").arg(QString::number(noteX - measureX, 'f', 2)); res += QString(" default-y=\"%1\"").arg(QString::number(noteY - measureY, 'f', 2)); } return res; }

(not 100% sure whether I got the assertion right)

Maybe even call it chordRestPosition() and drop the IF_ASSERT_FAILED part entirely, as it should be legal for chords too.

Actually just forget about it, it just doesn't work. And the code isn't 100% identical either, notePosition() picks on the chord a note belongs to, restPosition()(of course) does not.
So back to your initial approach, 2 methods for 2 types, which similar but not identical code.
Sorry for the mess...

julienripoche · 2021-12-02T13:56:32Z

I actually thought it was possible but I'm not very familliar with c++ and thus did not know the correct syntax.
I tried what you proposed.

Jojo-Schmitz · 2021-12-02T14:05:58Z

Well, yeah, I'm not sure about that casting trick either, it certainly does look quite hacky, but maybe my alternative approach is better, no casting needed and inside making sure the single method for both is reall only used on notes and rests, not also on chords.

Jojo-Schmitz · 2021-12-02T14:07:06Z

src/importexport/musicxml/internal/musicxml/exportxml.cpp

@@ -3757,6 +3757,7 @@ void ExportMusicXml::rest(Rest* rest, int staff)

    QString noteTag = QString("note");
    noteTag += color2xml(rest);
+    noteTag += notePosition(this, dynamic_cast<Note*>(rest)),


semicolon, not colon ;-)

julienripoche · 2021-12-02T14:10:22Z

Oups sorry...

julienripoche · 2021-12-02T14:15:12Z

Should I try with your cleaner approach ?

Jojo-Schmitz · 2021-12-02T14:17:20Z

I'd like input from others on that

julienripoche · 2021-12-02T14:59:40Z

Oh yes I forgot to mention this slight difference between the notePosition and restPosition methods.
So I rolled back to the 2 methods version.

Jojo-Schmitz · 2021-12-02T16:13:38Z

Actually the issue this fixes is https://musescore.org/en/node/34451, https://musescore.org/en/node/34346 is just a forum thread that triggered the creation of that issue.
You may want to start the PR and/or commit title with "Fix #34451: "

julienripoche · 2021-12-02T16:25:35Z

Done for PR and commit titles

@julienripoche

Backport of musescore#9955 Todo: Attribute this commit to @julienripoche

lvinken · 2021-12-04T09:11:43Z

Checked, thanks for looking into this. In its current state (commit 30dd4e2) the code does exactly what it is supposed to do.

I do agree with Jojo that the duplication of code is not desired and also think the mechanism to generate default-x and default-y should be generalised (and extended to other elements).

Wrt to this change specifically, as the code (both for Note and Rest) depends the element's postion and its containing measure's position, in master both Note and Rest inherit from EngravingItem and EngravingItem defines the measure() function, the solution for now could be to create a single function supporting both Note and Rest with an EngravingItem as parameter.

Jojo-Schmitz · 2021-12-04T10:09:20Z

So something like this?

//---------------------------------------------------------
//   elementPosition
//---------------------------------------------------------

QString ExportMusicXml::elementPosition(const ExportMusicXml* const expMxml, const EngravingItem* const elm)
{
    QString res;

    if (configuration()->musicxmlExportLayout()) {
        const double pageHeight  = expMxml->getTenthsFromInches(expMxml->score()->styleD(Sid::pageHeight));

        const auto meas = elm->findMeasure();
        IF_ASSERT_FAILED(meas) {
            return res;
        }

        double measureX = expMxml->getTenthsFromDots(meas->pagePos().x());
        double measureY = pageHeight - expMxml->getTenthsFromDots(meas->pagePos().y());
        double noteX = expMxml->getTenthsFromDots(elm->pagePos().x());
        double noteY = pageHeight - expMxml->getTenthsFromDots(elm->pagePos().y());

        res += QString(" default-x=\"%1\"").arg(QString::number(noteX - measureX, 'f', 2));
        res += QString(" default-y=\"%1\"").arg(QString::number(noteY - measureY, 'f', 2));
    }

    return res;
}

(Now tested: It does build!)

@lvinken : it apparently is findMeasure(), not measure(), just like in 3.x too

Jojo-Schmitz · 2021-12-04T11:55:59Z

For 3.x this here does seem work:

//---------------------------------------------------------
//   elementPosition
//---------------------------------------------------------

static QString elementPosition(const ExportMusicXml* const expMxml, const Element* const elm)
      {
      QString res;

      if (preferences.getBool(PREF_EXPORT_MUSICXML_EXPORTLAYOUT)) {
            const double pageHeight  = expMxml->getTenthsFromInches(expMxml->score()->styleD(Sid::pageHeight));

            const auto meas = elm->findMeasure();
            Q_ASSERT(!meas);
            if (!meas)
                  return res;

            double measureX = expMxml->getTenthsFromDots(meas->pagePos().x());
            double measureY = pageHeight - expMxml->getTenthsFromDots(meas->pagePos().y());
            double noteX = expMxml->getTenthsFromDots(elm->pagePos().x());
            double noteY = pageHeight - expMxml->getTenthsFromDots(elm->pagePos().y());

            res += QString(" default-x=\"%1\"").arg(QString::number(noteX - measureX,'f',2));
            res += QString(" default-y=\"%1\"").arg(QString::number(noteY - measureY,'f',2));
            }

      return res;
      }

lvinken · 2021-12-04T16:32:43Z

Both Jojo's 3.x and master solution look OK to me.

@julienripoche

Backport of musescore#9955 Todo: Attribute this commit to @julienripoche

@julienripoche

Backport of musescore#9955 Todo: Attribute this commit to @julienripoche

Backport of musescore#9955

julienripoche · 2022-01-10T10:44:32Z

Hi @Jojo-Schmitz and happy new year !
Should I commit your elementPosition method for use in the master branch ?
I understand you take care of the 3.x branch ?

Jojo-Schmitz · 2022-01-10T10:47:56Z

Not sure, either way is apparently OK. Mine seems a bit more generic though, and @lvinken seems to agree on making it more generic.
And yes, that's what 've done to the 3.x branch (rather to #9000)

Jojo-Schmitz · 2022-02-04T08:13:10Z

Note that the Q_ASSERT(!meas) im my version for 3.x (#9955 (comment)) is just wrong and bad.
But then the IF_ASSERT_FAILED(meas) for master (#9955 (comment)) is probably equally bad and might better just be if (!meas).

Backport of musescore#9955

lvinken · 2022-02-04T20:46:04Z

I think an assertion is kind of useful here: if the code is generalised it should be robust against being called for unexpected element types (such as elements that are not attached to measures). In that case for a production build simply returning an empty string should be OK. For a development build either a message or a failed assertion would be OK.

This could be achieved by
Q_ASSERT(meas); if (!meas) return res;

Backport of musescore#9955

Jojo-Schmitz · 2022-02-05T09:44:12Z

OK, so for master is should be IF_ASSERT_FAILED(!meas)

lvinken · 2022-02-05T12:24:02Z

Yes, but (I assume) without the exclamation mark.

…

On Sat, Feb 5, 2022 at 10:44 AM Joachim Schmitz ***@***.***> wrote: OK, so for master is should be IF_ASSERT_FAILED(!meas) — Reply to this email directly, view it on GitHub <#9955 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAK4YKSXNVO2LWTOWJTNLSDUZTWPPANCNFSM5JG4SG2A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Jojo-Schmitz · 2022-02-05T12:41:44Z

No, it is like a normal if, but logs/abends when the condition is true:

#define IF_ASSERT_FAILED_X(cond, msg) if (!(cond)) { \
        LOGE() << "\"ASSERT FAILED!\":" << msg << __FILE__ << __LINE__; \
        Q_ASSERT(cond); \
} \
    if (!(cond)) \

#define IF_ASSERT_FAILED(cond) IF_ASSERT_FAILED_X(cond, #cond)

bit of a screwed up logic that threw me off several time already (and here too)

@julienripoche are you going to implement it that way, or should I?

I just did it, see PR #10456. It ran all the tests and passed them too.

Backport of musescore#9955, resp. musescore#10456

cbjeukendrup · 2022-06-21T22:12:22Z

Closing, since this has been replaced by #10456.

Backport of musescore#9955, resp. musescore#10456

Jojo-Schmitz reviewed Dec 2, 2021

View reviewed changes

julienripoche force-pushed the musicxml_export_rest_position branch from 7a10f23 to 218a460 Compare December 2, 2021 14:57

julienripoche changed the title ~~Add rest position in musicxml export~~ Fix #34451: Add rest position in musicxml export Dec 2, 2021

Fix #34451: Add rest position in musicxml export

30dd4e2

julienripoche force-pushed the musicxml_export_rest_position branch from 218a460 to 30dd4e2 Compare December 2, 2021 16:23

Jojo-Schmitz mentioned this pull request Dec 3, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 3, 2021

Fix #34451: Add rest position in musicxml export

284c2f5

Backport of musescore#9955 Todo: Attribute this commit to @julienripoche

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 4, 2021

Fix #34451: Add rest position in musicxml export

3156ffa

Backport of musescore#9955 Todo: Attribute this commit to @julienripoche

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 7, 2021

Fix #34451: Add rest position in musicxml export

bdfaf9c

Backport of musescore#9955 Todo: Attribute this commit to @julienripoche

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 8, 2021

Fix #34451: Add rest position in musicxml export

ab6eea4

Backport of musescore#9955

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 13, 2021

Fix #34451: Add rest position in musicxml export

f5cedd1

Backport of musescore#9955

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 13, 2021

Fix #34451: Add rest position in musicxml export

9401bcb

Backport of musescore#9955

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 4, 2022

Fix #34451: Add rest position in musicxml export

7d51cb8

Backport of musescore#9955

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 5, 2022

Fix #34451: Add rest position in musicxml export

cdfe6e2

Backport of musescore#9955

Jojo-Schmitz mentioned this pull request Feb 5, 2022

[MU4] Fix #34451: Add rest position in musicxml export #10456

Merged

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 7, 2022

Fix #34451: Add rest position in musicxml export

3cd84e7

Backport of musescore#9955, resp. musescore#10456

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 12, 2022

Fix #34451: Add rest position in musicxml export

ef1a2cf

Backport of musescore#9955, resp. musescore#10456

cbjeukendrup closed this Jun 21, 2022

Jojo-Schmitz mentioned this pull request Jun 22, 2022

Fix assert in MusicXML Export #12122

Merged

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023

Fix #34451: Add rest position in musicxml export

d18c00c

Backport of musescore#9955, resp. musescore#10456

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #34451: Add rest position in musicxml export #9955

Fix #34451: Add rest position in musicxml export #9955

julienripoche commented Dec 2, 2021 •

edited

Loading

julienripoche commented Dec 2, 2021

Jojo-Schmitz commented Dec 2, 2021

Jojo-Schmitz Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021

julienripoche commented Dec 2, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021

julienripoche commented Dec 2, 2021

julienripoche commented Dec 2, 2021

Jojo-Schmitz commented Dec 2, 2021

julienripoche commented Dec 2, 2021

Jojo-Schmitz commented Dec 2, 2021

julienripoche commented Dec 2, 2021

lvinken commented Dec 4, 2021

Jojo-Schmitz commented Dec 4, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 4, 2021 •

edited

Loading

lvinken commented Dec 4, 2021

julienripoche commented Jan 10, 2022

Jojo-Schmitz commented Jan 10, 2022 •

edited

Loading

Jojo-Schmitz commented Feb 4, 2022 •

edited

Loading

lvinken commented Feb 4, 2022

Jojo-Schmitz commented Feb 5, 2022

lvinken commented Feb 5, 2022 via email

Jojo-Schmitz commented Feb 5, 2022 •

edited

Loading

cbjeukendrup commented Jun 21, 2022

Fix #34451: Add rest position in musicxml export #9955

Fix #34451: Add rest position in musicxml export #9955

Conversation

julienripoche commented Dec 2, 2021 • edited Loading

julienripoche commented Dec 2, 2021

Jojo-Schmitz commented Dec 2, 2021

Jojo-Schmitz Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Dec 2, 2021

Choose a reason for hiding this comment

julienripoche commented Dec 2, 2021 • edited Loading

Jojo-Schmitz commented Dec 2, 2021 • edited Loading

Jojo-Schmitz Dec 2, 2021

Choose a reason for hiding this comment

julienripoche commented Dec 2, 2021

julienripoche commented Dec 2, 2021

Jojo-Schmitz commented Dec 2, 2021

julienripoche commented Dec 2, 2021

Jojo-Schmitz commented Dec 2, 2021

julienripoche commented Dec 2, 2021

lvinken commented Dec 4, 2021

Jojo-Schmitz commented Dec 4, 2021 • edited Loading

Jojo-Schmitz commented Dec 4, 2021 • edited Loading

lvinken commented Dec 4, 2021

julienripoche commented Jan 10, 2022

Jojo-Schmitz commented Jan 10, 2022 • edited Loading

Jojo-Schmitz commented Feb 4, 2022 • edited Loading

lvinken commented Feb 4, 2022

Jojo-Schmitz commented Feb 5, 2022

lvinken commented Feb 5, 2022 via email

Jojo-Schmitz commented Feb 5, 2022 • edited Loading

cbjeukendrup commented Jun 21, 2022

julienripoche commented Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021 •

edited

Loading

Jojo-Schmitz Dec 2, 2021 •

edited

Loading

julienripoche commented Dec 2, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 2, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 4, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 4, 2021 •

edited

Loading

Jojo-Schmitz commented Jan 10, 2022 •

edited

Loading

Jojo-Schmitz commented Feb 4, 2022 •

edited

Loading

Jojo-Schmitz commented Feb 5, 2022 •

edited

Loading