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

Fret diagrams refactor #4785

Merged
merged 1 commit into from May 4, 2019

Conversation

Projects
None yet
6 participants
@jthistle
Copy link
Contributor

commented Mar 9, 2019

This is in response to the fretboard diagram EPIC. I realized when looking through some of the issues that to fix many of them (i.e. partial barres), a full refactor would be needed. So, this is what I've come up with. Code review, as always, is very much appreciated.

Todo: nothing. I would like to implement playback of fret diagrams, and chord names, and saving to palettes, but that is code for another pull request I think. So, this should be ready for merge.

This fixes:

#284966: Fretboard diagrams: barre lost on copying; and when saving to workspace
#283759: Fretboard diagrams: Barré property is redundant
#283682: Fretboard diagrams: allow multiple barrés (although not fully - multiple barres per fret is a very niche feature and relatively difficult to implement)
#283678: Fretboard diagrams: barré applied wrongly if there is another dot already at same position
#283676: Fretboard diagrams: barré deleted when separate dot is added afterwards at same position
#283674: Fretboard diagrams: barré deleted when you add a dot at another position
#283452: Editing Fretboard Diagrams
#275658: The ability to draw partial barre is missing for fretboard diagrams
#285324: Fretboard diagram anomalies with MS3 3.0.4.5763
#285616: Musescore Leaves Dots Under Barres in Chord Diagrams
#285613: Barre Deletes Dots on Higher Frets
#280575: Copy&Paste Fretboard diagram’s number is mirrored.
#283762: Fretboard diagrams: remove some "Set as style" buttons
#283679: Fretboard diagrams: symbols on top of diagram, and position number do not resize when scaling is changed

@jthistle jthistle force-pushed the jthistle:fret-refactor branch 2 times, most recently from 3ef6ca7 to 3cbc892 Mar 9, 2019

Show resolved Hide resolved libmscore/fret.cpp Outdated
Show resolved Hide resolved libmscore/fret.cpp Outdated
@Jojo-Schmitz

This comment has been minimized.

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from a13d79b to 0d817d1 Mar 12, 2019

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Regarding undo and linked elements, I would revert a18ef6f and apply mattmcclinch@9d17a98 instead.

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Thanks matt, I'll implement your change. What's the difference between links and linkList, by the way?

@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Quite a vew mtests fail, most (all but one) because of

  <fretMag>1</fretMag>

Which probably is a default and as such shouldn't be written out in the firrst place, or should it?

Edit: Ah, I see, the default changed from 1.0 to 1.2

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

What's the difference between links and linkList, by the way?

linkList() is guaranteed to be non-null. If _links is null, then linkList() returns a list that contains a single element. Since you don't need the functionality provided by the LinkedElements class, it is easier to use linkList() in this case.

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

aha, great. That will allow me for remove the checks I was using with links. Thanks for the help. Does the rest of the code look OK? Anything that sticks out as needing changing?

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@Jojo-Schmitz yes, I don't know where we stand with changing defaults, but fret diagrams are really too small at the current scale. So I changed the style default for a fret mag from 1 to 1.2, hence the failures.

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

I will take another look and let you know if I see anything else. I was serious about reverting
a18ef6f by the way. Aside from a few comments, I did not see anything worth keeping.

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I think it's worth it to keep the FretUndoData class and that method of undoing, since it's the easiest way of ensuring consistent undos with fret diagrams. The rest can go, yes.

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

I think it's worth it to keep the FretUndoData class

I disagree. I have shown you the easiest way of ensuring consistent undos. If you take my advice, you will find that the FretUndoData class is unnecessary and inefficient, since undo and redo are called on each linked fret diagram in turn anyway.

If you want, you can make undoFretDot(), undoFretMarker(), and undoFretBarre() members of the FretDiagram class so that they can be called outside of fretproperties.cpp if needed. Maybe call them undoSetFretDot(), undoSetFretMarker(), and undoSetFretBarre(). Now that I think about it, that's what I would probably do too. Like this: mattmcclinch@6be9358

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I'm talking about in undo.h. FretUndoData is the class I use for storing the state of a fret diagram before adding a dot or changing markers etc. It makes it easier to reset the fret diagram to its state before the action, because adding dots, barres and markers can also remove dots and markers in an often unpredictable way. So, by storing the state before the change, all that has to be done is a reset to this previous state, instead of complex logic that has to be updated with changes to setDot, setMarker etc. Make sense? There really doesn't seem to be a better way to do it, to me.

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

What was wrong with the way you did it before adding the FretUndoData class? I presume you added FretUndoData to keep linked fret diagrams in sync after undo/redo. But they will remain in sync anyway, as long as you use my FretDiagram::undoSetFret* functions from mattmcclinch@6be9358.

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

but fret diagrams are really too small at the current scale

That is your opinion. Which is why you can set your own style defaults, and your own default style. To my knowledge, that change has not been discussed, and it does not have to be part of this PR. Besides, there is something strange about setting a default magnification of 120%. If 100% is too small, then why not render it larger in the first place?

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

What was wrong with the way you did it before adding the FretUndoData class?

Well, for example, you'd add a dot, which would remove a string marker, and remove another dot. Or maybe it wouldn't if the dot was added in a multiple dot context and so on... Previously, adding a dot and undoing wouldn't put the marker back. So instead of copying the logic from setDot to the undo functions as well, I decided to use this class to just copy the state of a fret diagram so it could be reset on undo.

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

If 100% is too small, then why not render it larger in the first place?

Because that changes compatibility. If I import a score from 3.0.5 into 3.1, I expect everything to be the same size as it was. Then again, this may not be needed, it's just some of the symbols don't display clearly at the current default size. Maybe it would be simpler to leave defaults alone. Thoughts?

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from a18ef6f to a21efcc Mar 15, 2019

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@mattmcclinch - a21efcc has your implementation, in part. Does it look OK to you?

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from a21efcc to 87af2a6 Mar 15, 2019

@mattmcclinch

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Much better. Now that FretUndoData is simply a container for dots, markers, and barres, I am no longer worried about it, but I still do not see the advantage over simply storing all three in each UndoCommand like they had been before. As for fretMag, I think it would be best to leave the default alone, at least for now.

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

The benefit over storing each in UndoCommand is to prevent code duplication, mainly. It's much nicer imo to have the code that sets values and updates the diagram in one place rather than duplicated across many undo functions.

Show resolved Hide resolved libmscore/fret.cpp Outdated

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from d944d5f to c428a8f Mar 16, 2019

@RobFog

This comment has been minimized.

Copy link

commented Mar 27, 2019

Rebase needed, it seems?

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from 7ad7934 to 1ccfd63 Apr 12, 2019

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Rebased.

@jthistle jthistle force-pushed the jthistle:fret-refactor branch 3 times, most recently from cd589b1 to f40da8f Apr 14, 2019

@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Travis CI isn't happy, see https://travis-ci.org/musescore/MuseScore/jobs/520166192#L4221-L4233
Should be easy to fix I guess.

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from f40da8f to a0c0348 Apr 15, 2019

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Should be fixed now, we'll see.

@RobFog

This comment has been minimized.

Copy link

commented Apr 16, 2019

Seems to be fixed. :-)

Time for a final review?

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Yep, @dmitrio95 said he would some point this week.

@@ -37,6 +37,7 @@ vtest/META-INF
vtest/Thumbnails
vtest/Pictures
.vs
.vscode

This comment has been minimized.

Copy link
@jthistle

jthistle Apr 16, 2019

Author Contributor

evidently no one else uses vscode, and I just switched to it, so this line should be OK to add. We have .vs already, after all.

@dmitrio95
Copy link
Contributor

left a comment

I didn't review the whole patch closely yet but here are some brief remarks on it. More will still probably follow but overall the approach looks good to me. Still I have yet to review some its parts like the file format changes.

Also I left the comment in the issue concerning barre property removal, I believe it would be good to have any instantly visible way to change barre which that property could possibly be intended for.

/// Create diagram from string like "XO-123"
/// Always assume barre on the first visible fret
// Create diagram from string like "XO-123"
// Always assume barre on the first visible fret

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 18, 2019

Contributor

Actually it is better to leave three slashes. Then Doxygen would recognize this as a documentation comment. Of course, we don't use Doxygen for internal documentation (only for plugins, though I made a test page with the Doxygen-generated MuseScore codebase docs) but maybe some day it could be done again...

This comment has been minimized.

Copy link
@jthistle

jthistle Apr 19, 2019

Author Contributor

Yep, I'll change it back. I only changed it because the convention in the codebase seems to be using three spaces rather than 3 slashes.

struct Dot {
int fret { 0 };
FretDotType dtype { FretDotType::NORMAL };
int fingering; // NOTE:JT - possible future feature?

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 18, 2019

Contributor
  1. Do I understand correctly that it is not supported also in current master?
  2. If so, isn't it better to either remove this variable or initialize to something meaningful?

This comment has been minimized.

Copy link
@jthistle

jthistle Apr 18, 2019

Author Contributor

I was going to add fingering but didn't get round to it. I plan on doing it in a future PR, so rather than removing all the traces of fingering code, I decided to just leave it in for now. It doesn't have any negative effects, AFAIK...

But, if you're sure, I can remove it if you think that'd be better.

@@ -21,43 +21,145 @@ class StringData;
class Chord;
class Harmony;

// Keep this in order - used for comparisons

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 18, 2019

Contributor

Do you compare if square is less than triangle? :)

If this enumeration defines some ordering that is used somewhere else it is better to explicitly describe which ordering does it define.

This comment has been minimized.

Copy link
@jthistle

jthistle Apr 18, 2019

Author Contributor

Aha, yes it's a bit ambiguous :)

There's a multidot mode, which allows multiple dots to be added to a single string. But, it changes symbol based on how many dots are on a string - the first dot is normal, the second dot is a cross, the third is a square etc.

So, in that respect, square is actually more than triangle ;)

void removeMarker(int s);
void removeDotsMarkers(int ss, int es, int fret);

// Used by FretUndoData which is a friend

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 18, 2019

Contributor

If it is a friend then it can perfectly assign the corresponding fields without these setters. If this is the only their usage then maybe these functions are not needed.

This comment has been minimized.

Copy link
@jthistle

jthistle Apr 18, 2019

Author Contributor

Yes, I think it's their only usage. I'll remove them.

void FretDiagram::removeBarre(int f)
{
auto it = _barres.find(f);
_barres.erase(it);

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 18, 2019

Contributor

Removing directly by key should also work:

_barres.erase(f);
Show resolved Hide resolved libmscore/fret.cpp Outdated

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from a0c0348 to 65bb3a5 Apr 19, 2019

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

I've addressed your comments @dmitrio95

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from 65bb3a5 to 201bbd8 Apr 19, 2019

@dmitrio95
Copy link
Contributor

left a comment

I built and tested this branch a bit, and the new fretboard editor looks really great! There are, however, some issues that I noticed during my tests:

  1. All changes to fretboard diagrams seem to be undoable (almost all, see below) which is good, but undoing any change causes the fretboard diagram be immediately deselected (and, thus, lost from the Inspector view) so you have to select it again before making any further changes to this diagram. This is a minor issue but this still can make working with the fretboard editor less convenient.

  2. Barre created in the new editor is lost when opening the file in the "old" version of MuseScore (current master or 3.0.5). In the same time, if you create a barre in the old version of MuseScore, open in with the new editor and save the file, the barre will correctly appear when opening the file in older MuseScore versions.

  3. Barre and undo:
    3.1) Create a freatboard diagram and put there a barre on all strings (or all except one);
    3.2) Reduce number of strings;
    3.3) Undo
    Barre will not be restored. Still undoing once more time and redoing will restore that barre so it should still be saved in undo stack.

  4. Multiple dots mode, although I am not sure whether it is designed to behave so as I do not fully understand when this mode is intended to be used:
    4.1) Enter mutltiple dots mode
    4.2) Put four dots to one string
    4.3) Remove the first three dots by clicking on them
    4.4) Try to add new dots to that string
    Result: all new dots are triangles. I am not sure which meaning do these shapes have but if they are about the number of dots being already added then this behavior may be incorrect.

I'll post more issues here in case I find something new.

// If there is a barre with a set start and end, remove it.
//---------------------------------------------------------

void FretDiagram::setBarre(int string, int fret, bool add /*= false*/)

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 27, 2019

Contributor

add parameter is unused, should we keep it?

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Thanks for this review! 1 & 3 are fixable.

4 is also fixable, I realise that the logic for that is a bit confusing and I'll reimplement it so that it works how you expect, since I'd imagine that'd how most users would expect it. The purpose of the different shapes is for voicings, so that you play all the circles first, then the crosses etc. without needing separate fret diagrams. This was a feature that Trevor Hanson(?), who contacted me directly be email, really wanted, since it allows Ted Baker style diagrams.

2 is interesting... the barre capability in 3.0.5 is quite broken. Full backwards compatability is literally impossible. So, the barre can be exported, but only in certain situations. I have just realised that I've done one bit wrong, though, so I'll fix that.

Thanks again!

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from 201bbd8 to 85c8c19 May 2, 2019

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

2 and 4 are fixed. I'll have a look at 1, but 2 is a bit tricky. Is there are way to override the slots for the strings/frets selector and reset button while still having a set as style button?

Also, this should be fine for merge now @anatoly-os , 1 and 2 are minor issues.

Refactor fret diagrams
fix #284966
fix #283759
fix #283678
fix #283676
fix #283674
fix #283452
fix #275658
fix #265324
fix #285616
fix #285613
fix #280575
fix #283762
fix #283679

@jthistle jthistle force-pushed the jthistle:fret-refactor branch from 85c8c19 to 40cef95 May 2, 2019

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Ok, and 1 is now fixed. I'm still struggling with 3, but as said, this is still good for merge.

@@ -21,43 +21,139 @@ class StringData;
class Chord;
class Harmony;

// Keep this in order - not used directly for comparisons, but the dots will appear in
// this order in fret multidot mode. See fretproperties.cpp.
enum class FretDotType : signed char {

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz May 3, 2019

Contributor

Why signed char when we don't use negative numbers?
I'd use signed/unsigned char only if we really care, signed char if negative number are needed, unsigned char if numbers great than 128 are needed, else let the compiler use its defaults

This comment has been minimized.

Copy link
@jthistle

jthistle May 3, 2019

Author Contributor

Force of habit I guess... it really makes no difference

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz May 3, 2019

Contributor

other than documenting/indicating something (the need of negative values) that isn't true...

This comment has been minimized.

Copy link
@jthistle

jthistle May 3, 2019

Author Contributor

I mean, in the context of this it doesn't matter whether it's a signed enum or an unsigned enum. There's no performance improvement, and it doesn't make it more confusing making it signed.

};


enum class FretMarkerType : signed char {

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz May 3, 2019

Contributor

ditto

This comment has been minimized.

Copy link
@jthistle

jthistle May 3, 2019

Author Contributor

ditto response

@@ -234,7 +234,7 @@ class Element : public Ms::PluginAPI::ScoreElement {
API_PROPERTY( glissandoStyle, GLISSANDO_STYLE )
API_PROPERTY( fretStrings, FRET_STRINGS )
API_PROPERTY( fretFrets, FRET_FRETS )
API_PROPERTY( fretBarre, FRET_BARRE )
/*API_PROPERTY( fretBarre, FRET_BARRE )*/

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz May 3, 2019

Contributor

Why not API_PROPERTY( fretNut, FRET_NUT )?

This comment has been minimized.

Copy link
@jthistle

jthistle May 3, 2019

Author Contributor

Good point, I missed this line. I'll fix it.

@jthistle

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Oops, I meant I'm struggling with 3, the undoing string number and fret number changes. 2 is fixed.

@anatoly-os anatoly-os merged commit 004c93a into musescore:master May 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.