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 #298887: generic wind.flutes.whistle.tin is ambiguous #5556

Merged
merged 1 commit into from
Jan 9, 2020
Merged

fix #298887: generic wind.flutes.whistle.tin is ambiguous #5556

merged 1 commit into from
Jan 9, 2020

Conversation

riaanvn
Copy link
Contributor

@riaanvn riaanvn commented Dec 25, 2019

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

As discussed in Improve Tin Whistle instrument name by adding key the Tin Whistle as an instrument does not have a default key/pitch (like trumpet in Bb flute in C, where the keys for these instruments are implied if left out). Not specifying a key on the tin whistle is open to interpretation and will cause confusion as it can be interpreted as either C or D.

After adding key of .c, changed order (moved c to below bflat to make the instrument stanzas alphabetical, winds.whistle.tin.bflat/c/d
This inflates the change line count, and rieviewing what has changed in the diff tricky, but getting the order right is the "right thing to do"

Use "x" letter to fill the checkboxes below like [x]

  • 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

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 25, 2019

And as discussed in https://musescore.org/en/node/298887, there is no such instrument (<musicXMLid>wind.flutes.whistle.tin.c</musicXMLid>) in MusicXML yet, so we'd need to wait for that to happen.

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 25, 2019

Before and after
winds tin whistle
winds tin whistle c

@dmitrio95
Copy link
Contributor

Am I correct this PR shouldn't be merged before the new instrument gets added to MusicXML standard? If so, I'll put a "work in progress" label for now, please ping me when this is ready to merge.

@dmitrio95 dmitrio95 added the work in progress not finished work or not addressed review label Dec 26, 2019
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 26, 2019

Yes, I think so

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 27, 2019

@DLLarson , @jgadsden : FYI

@jgadsden
Copy link

From my point of view, as a user of MuseScore who uses Tin Whistle staves, it would be good to add the C tin whistle explicitly.
Having the C tin whistle as the default 'tin whistle' is misleading, because by far the most common tin whistle is tuned in D - with C and B-flat as the unusual ones. So this PR would go some way to helping us Tin Whistle players.

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 27, 2019

I have expanded this PR to include a generic / common wind.whistle.tin , equivalent in pitch to wind.whistle.tin.d / Tin Whistle, which aligns it with
a) https://github.com/w3c/musicxml/blob/v3.1/schema/sounds.xml once PR w3c/musicxml#299 is merged
b) https://github.com/jgadsden/musescore-tin-whistle
c) https://en.wikipedia.org/wiki/Tin_whistle#Tuning

with_common_tin whistle

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 27, 2019

If, for the time being, you'd leave out the .c from the musicXMLid, we could merge this PR now, and add the .c once Musicxml has it too

@Harmoniker1
Copy link
Contributor

Harmoniker1 commented Dec 27, 2019

I don't understand why the .c was added anyway. Unless different tunings cause great differences in timbre, like different kinds of saxophones, the ids of MusicXML shouldn't contain tuning information at all.

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 27, 2019

@Jojo-Schmitz
Thanks for the offer. I am happy to wait for upstream to add .c, and us merge both, if that is the only way we can get this whole commit merged. This issue has been around for a long time, so waiting a couple days (even a week or 2), is not a problem for me.

(personally I don't understand what will happen if we have a tin.c when W3C MusicXML does not have it. It is up to the program that imports the Mu export to know what to do with tin.c, and AFAIK, the instrument definition travels with the score? Once it is merged in W3C MusicXML, will it change things in any way other than being able to say the instrument officially exists there now? - Genuinely curious tbh)

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 27, 2019

Updating MusicXML surely won't be a matter of days or a few weeeks, more likely several months, if not more. The last change to MusicXML was mach in March this year. MusicXML 3.1 was released 2 years ago, no idea when the next version is supposed to come out, but https://github.com/w3c/musicxml/milestone/2 suggests Dec 31, 2020.

The issue with us using that not-yet existing musicXMLid is that we'd then export invalid MusicXML.

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 27, 2019

@Howard-C

The IDs do contain key/pitch info, because
a) whistle.tin is a diatonic instrument (there is a key for almost every key)
b) whistle.tin.c is not the most common pitch (Where it makes sense to omit the .c)

Please see the discussions at
https://musescore.org/en/node/292977
and
w3c/musicxml#289

If you are still not sure or convinced after that, please ask away.

@Jojo-Schmitz
They are pretty responsive on the W3C MusicXML side (they have gone quiet over the festive season). mdgood with whom I have been having the discussion on w3c/musicxml#289 is the actual inventor of MusicXML and the guy who will be merging the PR.

@Harmoniker1
Copy link
Contributor

OK, but the most common clarinet is also not in C, it's in B-flat, then why don't I see xx.clarinet.bflat for the genetic clarinet?

@Jojo-Schmitz
Copy link
Contributor

Your PR being merged in MusicXML is not sufficient, as long as no updated version of it got published, and that is, as mentioned, scheduled for December 31, 2020.

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 27, 2019

@Jojo-Schmitz

Thanks, and well spotted - that poses a big problem for this improvement:
a) if we merge the whistle.tin change only as you suggested, we have 2 ways to get a whistle in key of D (whistle.tin and whistle.tin.d) and 0 ways to get a whistle in key of C.
b) if we wait for MusicXML 3.2, it perpetuates the problem.
c) if we merge both, it solves the problem fully but we are not in sync with MusicXML for the next year.

(I personally would go with (c), but it is not up to me)

Any suggestions?

@dmitrio95
Copy link
Contributor

Maybe add C tin whistle but with wind.flutes.whistle.tin MusicXML id? Then it would be unambiguously marked at least within MuseScore, and after the new version of MusicXML gets published we could just assign the new wind.flutes.whistle.tin.c ID to it. I believe @Jojo-Schmitz's proposal was about something similar.

@Jojo-Schmitz
Copy link
Contributor

Exactly

@riaanvn
Copy link
Contributor Author

riaanvn commented Dec 28, 2019

Sorry guys, I completely misread "leave out the .c from the musicXMLid " (I was registering removing the whole stanza, not just the two chars from the C Tin Whistle musicXMLid)
I updated the PR:
a) removing .c from wind.flutes.whistle.tin.c, but adding/keeping C or equivalent to all other entries for this instrument stanza
b) removing the wind.flutes.whistle.tin stanza (until upstream W3C MusicXML have published the version that includes wind.flutes.whistle.tin.c

I am happy for it to be merged, if you are satisfied.

@riaanvn
Copy link
Contributor Author

riaanvn commented Jan 1, 2020

Diff showing the before and after better (3 lines changed) than the above "Files Changed" tab (since the instrument stanzas change order, it makes comparison difficult):

tin-whistle_before_after

Screenshot of 3 tin whistles

tin-whistle_BbCD

PR is ready for merge.

@dmitrio95 dmitrio95 removed the work in progress not finished work or not addressed review label Jan 7, 2020
@dmitrio95 dmitrio95 merged commit 245cd3f into musescore:master Jan 9, 2020
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.

None yet

5 participants