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 #97416 - toggle between pitched and unpitched instruments #6398

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

elerouxx
Copy link
Contributor

@elerouxx elerouxx commented Jul 31, 2020

Resolves: https://musescore.org/en/node/97416
Resolves https://musescore.org/en/node/307659

Changes:
The main changes in the code are:

  • Most "unticked" queries for part()->instrument() were properly "ticked" (part()->instrument(tick)), for staff changes
  • If the staff is not a tablature, queries for the staff "StaffGroup" (i.e. "Percussion Standard" vs "pitched standard" etc) are overridden by queries to the part()->instrument(tick)->isDrumSet, so now it is the instrument who determines if the staff behavior - valid clefs, transposition, entry mode, clicking, etc. - is suited for pitched or unpitched.

As consequence, the following has been fixed:

  • Allows pitched and unpitched (drumset) instrument changes on the same staff.

  • Fixes Drumset Edits not having effect after instrument changes

  • Different note entry modes are now activated under different instrument changes without exiting Note Entry
    (User can just navigate left or right and MuseScore will activate or deactivate the proper note entry mode/tools seamlessly)

  • The DrumTools panel will open, show and edit the correct drumset in note entry mode depending on the current instrument change, if it has a drumset.

  • Dragging up/down will only transpose pitched notes (if both pitched and unpitched are selected)

  • Using up/down keys will give the expected results for either pitched notes (transposing) or unpitched notes (cycle thru percussion hits) even if both pitched and unpitched parts of the staff are selected. Of course this is not recommended.

  • Clef compatibility now checks for instrument instead of staff, so the right clefs are allowed for the instrument changes - and automatically generated if needed.

  • Fixes string data changes along with instrument changes in tablatures.

  • Also fixes editing (custom) instrument string data on an instrument change.

Known issue: when editing string data on linked staves, the wrong string data is saved to the file. Will try to fix this too, if possible in this PR. this issue is fixed in this PR: #6491

  • TO DO: Key signatures are still showing after changing to unpitched instrument.

image

image

(History...)

The first approach was to set StaffType StaffGroup every time an instrument changed, but that should be not required (Except for tabs of different strings, a visually different staff type is not mandatory for every instrument). Then I implemented this second approach in which most checks for staffType->group are overriden by instrument->useDrumset (except on tablatures). This way, all normal edit/entry operations are allowed for standard or percussion depending on the instrument change of current tick, and the staff type change is optional.

The plan was to make smaller commits, but as I added the 'Ticks' to part()->instrument(tick) queries, all related issues ended up being fixed with just a bit more of code.

Note about clefs and instrument/staff changes (for a later fix):
- Generated clefs before measures with a staffType change will need a special fix since they are computed from stafftype in previous measure. A solution is needed allow staff type changes to visually start mid measure, or the user will have to decide if she/he adds a stc one measure before the instrument change (so the clef displays correctly) or adding the clef after the barline, which is not as elegant on an instrument change.(See G clefs on 3rd staff on first image).

  • 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

@elerouxx elerouxx force-pushed the 3x-pitched-vs-unpitched branch 2 times, most recently from 5b3e9ca to 80a1e2d Compare August 2, 2020 14:12
@elerouxx elerouxx changed the title [WIP] Fix #97416 - toggle between pitched and unpitched percussion Fix #97416 - toggle between pitched and unpitched percussion Aug 2, 2020
@Jojo-Schmitz
Copy link
Contributor

Probably the same linker problem on AppVeyor as for thw underlying PR

@elerouxx elerouxx changed the title Fix #97416 - toggle between pitched and unpitched percussion Fix #97416 - toggle between pitched and unpitched instruments Aug 2, 2020
@elerouxx elerouxx force-pushed the 3x-pitched-vs-unpitched branch from 80a1e2d to 1b74dba Compare August 20, 2020 17:23
@MarcSabatella
Copy link
Contributor

I wonder if the issues addressed here relate to the reasons why it doesn't work so well to have a chord symbol playback channel (thus using piano sound) within a drumset instrument? @elerouxx would you mind checking out #6584? The basic issue is, if you add a chord symbol to drum staff, we try to use program 0 for the playback thinking it will be piano, which for drum staves it won't be. @Jojo-Schmitz 's PR forces the bank to 0 which kind of works but not totally, and I'm thinking it's for similar reasons as the things you've been looking at.

@elerouxx
Copy link
Contributor Author

I took a quick look but the relation is not clear to me. The harmony code is new terrain for me so I might take some time to understand how things work there, as I look further.

@MarcSabatella
Copy link
Contributor

My point is is that I believe this has nothing to do with the harmony code at all. It's a more general question: what should happen if you have a subchannel on a drum staff - like one you might get from adding an instrument change - then try to use the Mixer to explicitly set that subchannel only to use a non-percussion playback sound?

@@ -1,13 +1,13 @@
:: set platform-dependent variables
IF "%PLATFORM%" == "x64" (
SET "QTURL=https://utils.musescore.org.s3.amazonaws.com/qt598_msvc2017_64.7z"
Copy link
Member

Choose a reason for hiding this comment

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

It's not really necessary now because we've moved all the CI into GitHub Actions

You need to rebase your branch on the latest 3.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

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.

4 participants