[MU3 Backend] ENG-8: Pedal Change Element #8275

iveshenry18 · 2021-06-08T03:39:17Z

Resolves: ENG-8: Pedal elements are ignored

This PR implements the pedal type="change" on MusicXML import and export, including a slightly (though minimally) hacky fix to accommodate the difference between the MusicXML representation of the pedal change (a single event), and the MuseScore representation (a HookType::HOOK_45 to end a line and a new line with a HookType::HOOK_45).

Additionally, this PR changes the behavior when a pedal type="start" that comes before the previous pedal has been stopped (a case which shouldn't occur, but nevertheless seems to in many scores). The previous behavior was to essentially ignore this new "start" and just continue the existing line; this change elects to delete the line that was started but never stopped, which is observed to be the more common intention.

Finally, this PR changes the default behavior of the sign attribute of pedals; where the MusicXML specification dictates an omitted sign attribute should be "yes" if line is "no" and vice versa, Dolet 6.6 for Sibelius erroneously omits this attribute when the sign is present (even when the line "yes"), so I have added an override set the default to "yes". Since this is technically against the spec, this portion (commit 148c9a6) may be omitted (or amended) when eventually porting to master.

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

This commit implements import and export of pedal change elements. There are a couple points worth noting about the implementation, due to the difference between the MusicXML representation and the internal data model: - Upon export, pedal changes are written based on the location of HookType::HOOK_45 *beginnings*, and HookType::HOOK_45 endings are ignored. This is because taking both results in redundancy, and the location of line beginnings as they related to notes is more reliable. - On the topic of unreliable endings, there is a slight hack to make the pedal change line endings (endHookType() == HookType::HOOK_45) render correctly. Namely, the duration of the note upon which the pedal change occurs is added to the tick2 position of the line. In other words, pedal lines that end in a change (HOOK_45) are extended to sustain *through* (rather than to) their last note.

Rather than ignore a "start" pedal if one has already been started, this deletes the unterminated line, then starts a new pedal normally.

Though the MusicXML specification dictates that the omission of a sign attribute should result in "yes" when line="no" and "no" when line="yes", it appears that this is not correctly observed by Dolet, so this commit overrides the default sign attribute to be "yes". It also specifies the sign attribute upon export. In some cases this makes the round-trip "gainy" (i.e. an unspecified sign becomes sign="yes").

RobFog · 2021-06-08T17:46:20Z

cc @lvinken

lvinken · 2021-06-08T18:14:40Z

Thanks for reminding me. I am unable to do an extensive review right now, but a quick glance at the conversation and the code changes suggests this is OK. Wrt the incorrect Dolet export, I'm afraid that due to lack of clear specification and/or formal test suite many applications will produce (somewhat) incorrect MusicXML in the foreseeable future. We'll have to try to make the best out of that, the only discussion IMHO is whether to do that automatically or involve the end user in some way (such as setting import options).

This commit implements import and export of pedal change elements. There are a couple points worth noting about the implementation, due to the difference between the MusicXML representation and the internal data model: - Upon export, pedal changes are written based on the location of HookType::HOOK_45 *beginnings*, and HookType::HOOK_45 endings are ignored. This is because taking both results in redundancy, and the location of line beginnings as they related to notes is more reliable. - On the topic of unreliable endings, there is a slight hack to make the pedal change line endings (endHookType() == HookType::HOOK_45) render correctly. Namely, the duration of the note upon which the pedal change occurs is added to the tick2 position of the line. In other words, pedal lines that end in a change (HOOK_45) are extended to sustain *through* (rather than to) their last note. Duplicate of musescore#8275, part 1

Rather than ignore a "start" pedal if one has already been started, this deletes the unterminated line, then starts a new pedal normally. Duplicate pf musescore#8275, part 2

Though the MusicXML specification dictates that the omission of a sign attribute should result in "yes" when line="no" and "no" when line="yes", it appears that this is not correctly observed by Dolet, so this commit overrides the default sign attribute to be "yes". It also specifies the sign attribute upon export. In some cases this makes the round-trip "gainy" (i.e. an unspecified sign becomes sign="yes"). Duplicate of musescore#8275, part 3

importexport/musicxml/exportxml.cpp

Though the MusicXML specification dictates that the omission of a sign attribute should result in "yes" when line="no" and "no" when line="yes", it appears that this is not correctly observed by Dolet, so this commit overrides the default sign attribute to be "yes". It also specifies the sign attribute upon export. In some cases this makes the round-trip "gainy" (i.e. an unspecified sign becomes sign="yes"). Duplicate of musescore#8275, part 3

Though the MusicXML specification dictates that the omission of a sign attribute should result in "yes" when line="no" and "no" when line="yes", it appears that this is not correctly observed by Dolet, so this commit overrides the default sign attribute to be "yes". It also specifies the sign attribute upon export. In some cases this makes the round-trip "gainy" (i.e. an unspecified sign becomes sign="yes"). Duplicate of musescore#8275, part 3, plus a fixed compiler warning

This commit implements import and export of pedal change elements. There are a couple points worth noting about the implementation, due to the difference between the MusicXML representation and the internal data model: - Upon export, pedal changes are written based on the location of HookType::HOOK_45 *beginnings*, and HookType::HOOK_45 endings are ignored. This is because taking both results in redundancy, and the location of line beginnings as they related to notes is more reliable. - On the topic of unreliable endings, there is a slight hack to make the pedal change line endings (endHookType() == HookType::HOOK_45) render correctly. Namely, the duration of the note upon which the pedal change occurs is added to the tick2 position of the line. In other words, pedal lines that end in a change (HOOK_45) are extended to sustain *through* (rather than to) their last note. Duplicate of musescore#8275, part 1

Rather than ignore a "start" pedal if one has already been started, this deletes the unterminated line, then starts a new pedal normally. Duplicate pf musescore#8275, part 2

Though the MusicXML specification dictates that the omission of a sign attribute should result in "yes" when line="no" and "no" when line="yes", it appears that this is not correctly observed by Dolet, so this commit overrides the default sign attribute to be "yes". It also specifies the sign attribute upon export. In some cases this makes the round-trip "gainy" (i.e. an unspecified sign becomes sign="yes"). Duplicate of musescore#8275, part 3, plus a fixed compiler warning, similar to musescore#8481