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 #292387 : [MusicXML] Drumset of MusicXML sounds pitch #6955

Merged
merged 2 commits into from
Nov 29, 2020

Conversation

AntonioBL
Copy link
Contributor

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

This PR sets the channel bank to 128 before setting the first instrument, which is then added to the masterScore midiMapping (and the channel is cloned to midiMapping articulation channel).
Bank 128 is thus set for both the masterChannel and the articulation channel inside midiMapping

  • 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
  • [N/A] I created the test (mtest, vtest, script test) to verify the changes I made

@AntonioBL
Copy link
Contributor Author

@dmitrio95 : Sorry to disturb you, but you are the expert of the midiMapping; can you please have a look at the related comment https://musescore.org/en/node/292387#comment-1042795 ?

Additional note: I think the line

part->instrument()->channel(0)->setBank(128);
becomes redundant with the change of this PR, but I am not 100% sure.

// note: setMidiProgram() does more than simply setting the MIDI program
if (mxmlInstr.midiProgram >= 0) part->setMidiProgram(mxmlInstr.midiProgram);
if (mxmlInstr.midiProgram >= 0)
part->setMidiProgram(mxmlInstr.midiProgram);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be an issue for this pull request, but part->setMidiProgram() also sets bank to a value which is 0 by default:

void setMidiProgram(int, int bank = 0);

So with this PR updatePartWithInstrument effectively sets the channel's bank to 128, adds the channel to MIDI mapping (via setMidiChannel()) and then sets the channel's bank value back to 0. Here it doesn't seem to cause any issues (although it doesn't seem right anyway), but in other places (for example, in OVE import) it might cause undesirable results. So maybe setMidiProgram() behavior should be somehow reconsidered, although this certainly doesn't belong to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for your comment.

So line part->instrument()->channel(0)->setBank(128); is not actually redundant, in the end.

@AntonioBL AntonioBL deleted the musicxmldrumset branch December 2, 2020 19:25
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.

3 participants