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
[WIP] Instrument change improvements #5128
[WIP] Instrument change improvements #5128
Conversation
d210d5a
to
7c82f41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of notes, but generally seems good. Any 👍s are just for bits that we've agreed on in other conversations, as a reminder.
6424ed4
to
0d3c0a9
Compare
0d3c0a9
to
429b54d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this looks like a good basis for adding the warning text. Nothing stands out as a major problem, but there are just a few smaller issues. Keep at it :)
Travis is not happy, see https://travis-ci.org/musescore/MuseScore/jobs/555267917#L2108-L2112: /home/travis/build/musescore/MuseScore/libmscore/key.cpp: In function ‘Ms::Interval Ms::calculateInterval(Ms::Key, Ms::Key)’: |
Thank you, I've fixed that now. |
429b54d
to
e09317c
Compare
2 mtest failures actually crashes: https://travis-ci.org/musescore/MuseScore/jobs/556958555#L4166-L4189 and https://travis-ci.org/musescore/MuseScore/jobs/556958555#L4270-L4348 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments :)
e09317c
to
a61b376
Compare
Thank you for this, it's very helpful to have another set of eyes on it. Some of the things I'm aware of, like the cutting and pasting and the various segfaults, and I've been working on fixing them this week, but some of them are things I've overlooked, and will have a look at fixing, so thank you. |
b4c0929
to
e2b0799
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have 'a few' comments :)
Overall, it's looking good. Just the last few bugs to iron out, and now that everything is separated into functions, it should be easier to fix. At least, that's what I'd hope...
Bugs found during testing
-
adding an instrument change before an entry, and then another after that but before the entry, then deleting the second instrument change leads to the warning text disappearing until I click on the first instrument change.
-
There's some weird stuff going on with key sigs. I'm not entirely sure how I got it into this state, sorry:
libmscore/chordrest.cpp
Outdated
@@ -556,6 +550,24 @@ Element* ChordRest::drop(EditData& data) | |||
score()->undoAddElement(e); | |||
return e; | |||
} | |||
case ElementType::INSTRUMENT_CHANGE: | |||
if (e->isInstrumentChange() && part()->instruments()->find(tick().ticks()) != part()->instruments()->end()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e->isInstrumentChange()
is redundant AFAIK, because we already switch e->type()
and are in a block where the type is ElementType::INSTRUMENT_CHANGE
.
Also, not tested, but if I try to add an instrument change to the first bar of a score, won't this block run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I'll remove the isInstrumentChange(). I've tested it, and it doesn't block if you add to the first bar of the score.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, this was an existing piece of code, that had a fall-through in it, hence the check. I rearranged it so that the above items didn't fall-through to it, so I could add some more specific code for instrument changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, when you say it 'doesn't block', you mean that block doesn't run, right?
@@ -286,6 +286,8 @@ void Clef::read(XmlReader& e) | |||
_clefTypes._transposingClef = Clef::clefType(e.readElementText()); | |||
else if (tag == "showCourtesyClef") | |||
_showCourtesy = e.readInt(); | |||
else if (tag == "forInstrumentChange") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always wary about adding new tags, but I think this can be justified 👍
libmscore/edit.cpp
Outdated
deleteItem(clef); | ||
InstrumentChangeWarning* warning = nextICWarning(ic->part(), ic->segment()); | ||
if (warning) | ||
undoRemoveElement(warning); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a solution here (for the first bug described) would be to somehow check if we need to add a new warning for any previous instrument change. i.e., if we search backwards for ICs and IC warnings, and hit and IC before an IC warning, we need to add a new warning for that IC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll investigate this further, but I think that sounds like the right sort of thing.
{ | ||
InstrumentChangeWarning* warning = toInstrumentChangeWarning(el); | ||
InstrumentChange* ic = prevInstrumentChange(warning->segment(), warning->part(), false); | ||
ic->setShowWarning(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
continue; | ||
if (!annotation->systemFlag() && annotation->track() == track) | ||
undoRemoveElement(annotation); | ||
deleteItem(annotation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code to delete an instrument change warning when an instrument change is is deleted is kept in deleteItem(), as is the same for special cases for all objects. So if an instrument change is deleted as part of a selection, without that change it wouldn't delete the warning associated with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And deleteItem makes use of undoable functions?
@@ -118,6 +220,8 @@ void InstrumentChange::read(XmlReader& e) | |||
const QStringRef& tag(e.name()); | |||
if (tag == "Instrument") | |||
_instrument->read(e, part()); | |||
else if (tag == "init") | |||
_init = e.readBool(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above - do we really need to write it?
InstrumentChangeWarning* _warning = nullptr; | ||
bool _init = false; | ||
bool _showWarning = true; | ||
Q_DECLARE_TR_FUNCTIONS(setupInstrument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I had forgotten completely about translations.
|| !(*i.stringData() == *stringData()) | ||
|| i._singleNoteDynamics != _singleNoteDynamics; | ||
//return !operator==(i); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just make this whole function return !(*this == i)
? (That syntax may not be exactly correct off the top of my head).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see now that that's been commented out. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was something I was unsure about. Because the equals function compares the channels vector, and creating a new instrument in an instrument change creates a new channel, it would not return equal, even if they were the same instrument, which is what I was wanting to check. However, thinking about it, this should probably be in a new function, as it's quite confusing to have these work slightly differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that makes sense. As you say, I'd consider putting the conditions that are common between them into a new function and using that to reduce duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And make sure to comment why it's like that as well for confused people like me!
} | ||
score->setLayoutAll(); | ||
score->endCmd(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it's good to have options for this stuff in the inspector.
ic->setPlainText(tr("To %1").arg(it->trackName)); | ||
Instrument instr = Instrument::fromTemplate(it); | ||
ic->setInit(true); | ||
ic->setupInstrument(&instr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! It's so much more simplified.
cc7c997
to
fcfe0aa
Compare
af47c61
to
328b76b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good! Just a few comments.
libmscore/instrument.h
Outdated
StaffGroup staffGroup() { return _staffGroup; } | ||
void setLines(int lines) { _lines = lines; } | ||
const int lines() const { return _lines; } | ||
int lines() { return _lines; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be int lines() const
, since we're not modifying any values of Instrument
.
libmscore/instrument.h
Outdated
@@ -292,6 +294,12 @@ class Instrument { | |||
QString instrumentId() { return _instrumentId; } | |||
void setInstrumentId(const QString& instrumentId) { _instrumentId = instrumentId; } | |||
|
|||
void setStaffGroup(const StaffGroup staffGroup) { _staffGroup = staffGroup; } | |||
const StaffGroup staffGroup() const { return _staffGroup; } | |||
StaffGroup staffGroup() { return _staffGroup; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, StaffGroup staffGroup() const
is better.
Also, const StaffGroup staffGroup() const
and the similar one for lines
cause compiler warnings for me. When the keyword const
comes after a declaration, it means that the function itself is const
- as in, the function cannot change the value of any members of the class. This is good, because for a simple getter, we don't want anything to be modified.
When const
comes before, however, it's referring to the return type. So, saying const StaffGroup
tells the compiler that this function will return a StaffGroup
with an unchangeable value. Except, it doesn't.
IIRC, the const
keyword for types can only be applied to pointers or references. For a copyable type like StaffGroup
, const StaffGroup
doesn't make sense - there are no side effects if you change the value of a StaffGroup
, so why care if it's const
? However, with references and pointers, there is the side effect of changing the original value that the pointer/reference was created from. So, const StaffGroup*
or const StaffGroup&
both make sense and would be fine.
That said, you have a couple of options to avoid the compiler warning:
-
make both the functions (
const
and non-const
) returnconst StaffGroup&
. This speeds up execution, since you're not copying the value of_staffGroup
around (references are just pointers but better), and_staffGroup
cannot be changed by changing that object, because it has theconst
qualifier. -
remove the first
const
qualifier fromconst StaffGroup staffGroup() const
Everything I've just said about _staffGroup
also applies for _lines
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, that makes a lot more sense. I think at that point I was just copying the format from other getter/setters, but I must have been copying from a pointer one. I've changed them all to making them non-const. I think returning const StaffGroup* wouldn't work, as you shouldn't return pointers to non-heap allocated objects or local variables, as they may be deleted before accessing them, leading to use after free errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, a pointer wouldn't be suitable. AFAIK a reference would though, although in this case it's pretty much negligable because it's such a small object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the reference would work either, as under the hood references are just pointers, just const and non-null, and with automatically dereferencing
libmscore/instrument.cpp
Outdated
switch (_staffGroup) { | ||
case StaffGroup::PERCUSSION: xml.tag("staffGroup", 1); break; | ||
case StaffGroup::TAB: xml.tag("staffGroup", 2); break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although there are exceptions, we prefer words, e.g.:
case StaffGroup::PERCUSSION: xml.tag("staffGroup", "percussion"); break
case StaffGroup::PERCUSSION: xml.tag("staffGroup", "tab"); break
For this, you could invest in a couple of helper functions similar to Dynamic::speedToName and Dynamic::nameToSpeed. Or you could do the quicker thing, which is hardcoding it. Your choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will do that instead. I'll probably stick with hardcoding for now, as there would only need to be 2 options (with standard being the default), and it's unlikely that there'll ever be more.
@@ -1051,7 +1051,7 @@ Chord* Score::nextChord(Segment* seg, const Part* part, bool lookForIc) | |||
} | |||
} | |||
seg = seg->next1(); | |||
if (lookForIc) { | |||
if (lookForIc && seg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix for the crash, I guess? Nice 👍
Bugs and thingsI don't know if this is quite right... should the key change come before or after the clef change?
No change to treble clef here, where there should be. I can't add notes when change instrument after being in a drumset. Repro:
Expected: I can add notes Actual: I cannot add notes (this seems to be related to the fact that, with drumsets, you must select notes from a kind of Deleting an instrument change that changes key does not make another one ahead of it, which should change key, change the key instead. See this video: That's what I've found so far. The general operation of instrument changes (that is, without staff changes) seems to be working great! Undo is working well, I can copy and paste stuff everywhere without corruption (well, mostly), and the sounds change perfectly. |
Thank you for this. I'm aware of a lot of the bugs related to percussion and tab changes, and I'll try and get them fixed today. I'm already aware of the problem with clefs and key signatures halfway through a bar, although that's not a problem with instrument changes specifically (try adding a clef and key change to the same point, in any order), but I'll see if I can work out why that happens if I have time. That fix could probably go in a different PR though, since it is an independent problem. |
…t change warning have the same style as instrument changes
…nters between them and instrument changes
…the instrument change warning should be shown or not
…oshwd36/MuseScore into instrument-change-improvements
That last commit should not be there, don't merge, but rebase. |
I tried to do a rebase, but there were merge conflicts between master and my branch. What would be the correct way to resolve those without a merge? |
Here's how OI usually do it: |
@joshwd36 could you please do a rebase of your branch? Currently Github shows changes in 423 files and 438 commits in this PR which makes it not very convenient to review the actual changes. |
So not just rebase, but also squash? Oh, I see, there are many commits in here that just don't belong there, apparently the result of a merge commit (cdf03ae) rather than a rebase. |
@joshwd36, best make another copy of this branch as backup before you do anything else. git checkout instrument-change-improvements
git checkout -b instrument-change-improvements-backup
git push origin instrument-change-improvements-backup Then ideally you would undo that merge commit somehow, get everything back how it was before, and then do a rebase instead of a merge. If that's not possible then I would start again creating a new branch from the latest git checkout master
git pull upstream master
git checkout -b try-again
git cherry-pick 1db0960ac # Adding instrument changes to a score automatically shows…
git cherry-pick 525e17887 # Fix adding objects to multiple staves through selection
git cherry-pick d6c3c36d3 # etc.
... Lesson for the future: never use |
I have also tested this PR some time ago, and while most of changes are really useful improvements, the "warning text" feature (which places a special warning element when the changed instrument actually starts having any notes) doesn't seem to perform well with linked parts and has a number of other issues (for example, deleting the warning text is not synchronized with "show warning text" property, changes to warning text properties get reset when user clicks on the corresponding instrument change element). If this feature could be split out to a separate PR to make it possible to fix the issues with it separately it would be especially good as the rest of the project could be merged much sooner. Otherwise I could try picking and merging the relevant changes from this branch manually. However this might make further rebasing more difficult so please let me know if you are planning to split or rebase this PR. |
See #5782 with certain features from this pull request rebased to current master. |
Replaced by #5782 |
The aim of this pull request is to make a number of improvements to instrument changes, to reduce the amount of work users need to do, and to introduce some new functionality. The currently implemented functionality is:
Much of this work was completed during the Google Summer of Code 2019. The last commit of the GSoC period was 34a1b27.