[MU3] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch #6494

Jojo-Schmitz · 2020-08-25T16:59:31Z

resolves https://musescore.org/en/node/309371

mirabilos · 2020-10-31T19:51:36Z

libmscore/score.cpp

+                  for (Element* e : segment->elist()) {
+                        if (e->isAmbitus()) {
+                              Ambitus* a = toAmbitus(e);
+                              a->updateRange();


This is the wrong approach.

Ambiti may be hand-edited in inspector (which I often do if there’s an optional extra note that can be left out that is rather out of a normal range). Toggling concert pitch should not update the range from the score; it must rather transpose the top and bottom notes the same as it does to regular notes in that stave.

Hmm, I see. But a) how to and b) how to get it to work for octave transposing instruments too?

The problem is in the break after the a->updateRange();. Now the iterations stops at the first Ambitus found but the segment might contain more Ambitus's.

Segment, of course, more Ambiti in the same measure, but different staves! Bad thinko on my side I guess.

I think it's better the move the Ambitus loop outside the for (Staff* staff : _staves) loop since you end up doing the same thing nstaves() times.
This solves your first problem but still leaves the issue as mentioned by @mirabilos . I indeed do similar things, especially when newer instruments have a larger range (e.g. Baritone Saxophones, even Bb Clarinets which can be extended in the lower range)

Still needs the check for whether the staff is transposing or not, at least as an optimization. Would solve (a good part of) @mirabilos' issue too, as there it is not about transposing instruments but human singing voices IIRC ;-).
So I added that check.
@mirabilos you can avoid the manual changes of the Ambitus by disabling "Play" for those extra notes.

Hmm, bad, that change to skip non-transposing instruments/segments causes a crash :-(

mirabilos · 2020-11-02T16:34:53Z

Joachim Schmitz dixit:

Hmm, I see. But a) how to and b) how to get it to work for octave transposing instruments too?

Probably by duplicating (or extracting and calling) the logic used for these cases elsewhere in the code. Sorry, I didn’t look at this from the code level, but I saw the change which, from a user PoV, I think was incorrect.

njvdberg · 2020-11-04T06:52:19Z

... except that it does not for octave transposing instruments, anyone any idea why?

I didn't check but that's related to "transposing" clefs? E.g. treble clef 8va bassa?

Jojo-Schmitz · 2020-11-04T06:57:03Z

Yes (at least I think it is)

njvdberg · 2020-11-04T07:18:44Z

Did a quick check, it seems to works for the first staff only. My example uses a Bb Clarinet and a Bb Bass Clarinet. Now the Bb clarinet staff works fine but the Bass clarinet remains it originals ambitus. Next, switch the staves (in the Instrument Form). Now the Bass clarinet is fine but the the ambitus of the Bb Clarinet is wrong.

njvdberg · 2020-11-04T07:22:15Z

libmscore/score.cpp

+                  for (Element* e : segment->elist()) {
+                        if (e->isAmbitus()) {
+                              Ambitus* a = toAmbitus(e);
+                              a->updateRange();


The problem is in the break after the a->updateRange();. Now the iterations stops at the first Ambitus found but the segment might contain more Ambitus's.

libmscore/score.cpp

Jojo-Schmitz · 2020-11-04T07:58:02Z

BTW: attach here is my test score (rename the .zip away)
ambitus.mscz.zip

Jojo-Schmitz · 2020-11-04T08:31:41Z

OK, left to do: check for and obey manually changed ambitus

Jojo-Schmitz · 2020-11-04T18:10:57Z

libmscore/ambitus.cpp

-      int   tpcTop      = 0;  // Initialized to prevent warning
-      int   tpcBottom   = 0;  // Initialized to prevent warning
+      int   pitchTop    = SCHAR_MIN-1; // just outside Interval::{chromatic,diatonic}
+      int   pitchBottom = SCHAR_MAX+1; // just outside Interval::{chromatic,diatonic}


Better than some 'magic numbers' IMHO

Jojo-Schmitz · 2020-11-04T18:11:49Z

libmscore/ambitus.cpp

+      int   pitchTop    = SCHAR_MIN-1; // just outside Interval::{chromatic,diatonic}
+      int   pitchBottom = SCHAR_MAX+1; // just outside Interval::{chromatic,diatonic}
+      int   tpcTop      = Tpc::TPC_INVALID;  // Initialized to prevent warning
+      int   tpcBottom   = Tpc::TPC_INVALID;  // Initialized to prevent warning


Doesn't really matter here, but looks cleaner to me

Jojo-Schmitz · 2020-11-04T18:16:19Z

libmscore/ambitus.cpp

@@ -621,7 +621,8 @@ void Ambitus::updateRange()
            segm = segm->nextCR();
            }

-      if (pitchTop > -1000) {             // if something has been found, update this
+      if (pitchTop > SCHAR_MIN-1/* || pitchBottom < SCHAR_MAX+1 || tpcTop != Tpc::TPC_INVALID || tpc_Bottom != Tpc::TPC_INVALID*/) {


Getting rid if the 'magic numbers'. Not sure whether to uncomment that commented part, or whether it matters at all, tcp_Top always gets changed along with pitchTop, tpc_Bottom along with pitchBottom, and I don't see how pitchBottom could ever get changed without also changing pitchTop or vice versa, so it seems save to assume that checking for one is sufficient.

Jojo-Schmitz · 2020-11-04T18:18:36Z

libmscore/score.cpp

+            for (Element* e : segment->elist()) {
+                  if (e && e->isAmbitus()) {
+                        Ambitus* a = toAmbitus(e);
+                        a->updateRange(); // TODO: honor manually modified ambitus


Here indeed we'd need to transpose rather than update to not loose 'hand-made' settings

Jojo-Schmitz · 2020-11-04T18:24:32Z

libmscore/score.cpp

@@ -2692,6 +2693,15 @@ void Score::cmdConcertPitchChanged(bool flag, bool /*useDoubleSharpsFlats*/)
      {
      undoChangeStyleVal(Sid::concertPitch, flag);       // change style flag

+      for (Segment* segment = firstSegment(SegmentType::Ambitus); segment; segment = segment->next1(SegmentType::Ambitus)) {


I'd rather leave this loop early for all non-transposing instruments (including percussion), but attempts to do so, like using code similar to the one in the below loop, leads to crashes when calculating the Interval.
We'd get all that, if moving this block (back) into that loop, but at a cost: we'd now traverse the segments nstaves() times, which really doesn't make much sense. The upside is that we gather the interval, needed for transposing.
The problem with this is though, that while we can update multiple times, that'd just be a waste, we can't transpose multiple times...

Can we just add two Note objects into the innards of an Ambitus object, recreated upon reading from MSCX or creating the ambitus (i.e. transient, not stored by themselves), and set their properties to the respective highest and lowest note’s on update, and then use the usual transposition functions (epitch(), ppitch(), tpc1(), tpc2(), etc.) internally in the Ambitus’ functions?

Code sample?
I won't mind you to PR on top of mine ;-)

Wish I had any… but that’s going to be something major, and I’m behind on billable hours as is. Sorry.

libmscore/ambitus.cpp

…toggling concert pitch

RomanPudashkin · 2022-12-20T08:56:59Z

3.x is closed for any changes

Jojo-Schmitz marked this pull request as draft August 25, 2020 16:59

Jojo-Schmitz changed the title ~~[WIP] fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch~~ [MU3] [WIP] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch Oct 20, 2020

mirabilos suggested changes Oct 31, 2020

View reviewed changes

njvdberg suggested changes Nov 4, 2020

View reviewed changes

libmscore/score.cpp Outdated Show resolved Hide resolved

Jojo-Schmitz mentioned this pull request Nov 4, 2020

MuseScore 3.6 Alpha. PRs to be merged and issues to be fixed. #6505

Closed

74 tasks

Jojo-Schmitz marked this pull request as ready for review November 4, 2020 08:30

Jojo-Schmitz force-pushed the ambitus-transposing branch from e6d9864 to f7a1a2e Compare November 4, 2020 08:31

Jojo-Schmitz force-pushed the ambitus-transposing branch 6 times, most recently from 642fa14 to d4615e5 Compare November 4, 2020 16:23

Jojo-Schmitz commented Nov 4, 2020

View reviewed changes

Jojo-Schmitz force-pushed the ambitus-transposing branch from d4615e5 to e82ce14 Compare November 16, 2020 20:19

mirabilos reviewed Nov 16, 2020

View reviewed changes

libmscore/ambitus.cpp Outdated Show resolved Hide resolved

Jojo-Schmitz force-pushed the ambitus-transposing branch from e82ce14 to 94f64de Compare November 16, 2020 21:20

Jojo-Schmitz mentioned this pull request Nov 17, 2020

MuseScore 3.6 Beta/RC/Release. PRs to be merged and issues to be fixed. #6887

Closed

Jojo-Schmitz mentioned this pull request Jan 15, 2021

MuseScore 3.6.1. PRs to be merged and issues to be fixed. #7244

Closed

49 tasks

Jojo-Schmitz mentioned this pull request Jan 28, 2021

MuseScore 3.6.2. PRs to be merged and issues to be fixed. #7347

Closed

53 tasks

Jojo-Schmitz changed the title ~~[MU3] [WIP] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch~~ [MU3] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch Jan 29, 2021

Jojo-Schmitz marked this pull request as draft January 29, 2021 07:23

Jojo-Schmitz mentioned this pull request Feb 8, 2021

[MU3 Issue] MuseScore 3.6.3 or 3.7.0, PRs to be merged and issues to be fixed. #7449

Closed

Jojo-Schmitz mentioned this pull request Sep 4, 2021

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

Closed

fix #309371: Ambitus for transposing instruments doesn't update when …

fd62d12

…toggling concert pitch

Jojo-Schmitz force-pushed the ambitus-transposing branch from 94f64de to fd62d12 Compare September 14, 2021 13:10

RomanPudashkin closed this Dec 20, 2022

Jojo-Schmitz deleted the ambitus-transposing branch December 20, 2022 10:13

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MU3] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch #6494

[MU3] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch #6494

Jojo-Schmitz commented Aug 25, 2020 •

edited

Loading

mirabilos Oct 31, 2020

Jojo-Schmitz Nov 2, 2020

njvdberg Nov 4, 2020

Jojo-Schmitz Nov 4, 2020

njvdberg Nov 4, 2020

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020

mirabilos commented Nov 2, 2020 via email

njvdberg commented Nov 4, 2020

Jojo-Schmitz commented Nov 4, 2020 •

edited

Loading

njvdberg commented Nov 4, 2020

njvdberg Nov 4, 2020

Jojo-Schmitz commented Nov 4, 2020

Jojo-Schmitz commented Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020

Jojo-Schmitz Nov 4, 2020

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

mirabilos Nov 9, 2020

Jojo-Schmitz Nov 10, 2020 •

edited

Loading

mirabilos Nov 16, 2020

RomanPudashkin commented Dec 20, 2022

[MU3] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch #6494

[MU3] Fix #309371: Ambitus for transposing instruments doesn't update when toggling concert pitch #6494

Conversation

Jojo-Schmitz commented Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirabilos commented Nov 2, 2020 via email

njvdberg commented Nov 4, 2020

Jojo-Schmitz commented Nov 4, 2020 • edited Loading

njvdberg commented Nov 4, 2020

Choose a reason for hiding this comment

Jojo-Schmitz commented Nov 4, 2020

Jojo-Schmitz commented Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanPudashkin commented Dec 20, 2022

Jojo-Schmitz commented Aug 25, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz commented Nov 4, 2020 •

edited

Loading

Jojo-Schmitz commented Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 4, 2020 •

edited

Loading

Jojo-Schmitz Nov 10, 2020 •

edited

Loading