Add an option to separate layout of similarly stemmed voices #23001
Conversation
miiizen
force-pushed
the
chord-voice-layout
branch
4 times, most recently
from
c27c4f9
to
ca2a581
Compare
| @@ -2032,17 +2048,25 @@ double ChordLayout::layoutChords2(std::vector<Note*>& notes, bool up, LayoutCont | |||
| // to track mirror status of previous note | |||
| bool isLeft = notes[startIdx]->chord()->up(); // is notehead on left? | |||
| staff_idx_t lStaffIdx = notes[startIdx]->chord()->vStaffIdx(); // staff of last note (including staffMove) | |||
| track_idx_t lTrack = notes[startIdx]->chord()->track(); | |||
| bool lCombine = notes[startIdx]->chord()->combineVoice(); | |||
There was a problem hiding this comment.
I know you're following the naming style that was already there, but I didn't know what lTrack meant until I noticed the comment on the previous line. Always better to use readable names. Also, I find the naming lastSomething misleading, as it doesn't mean the actual last but the previous one in the loop, so perhaps prevSomething is clearer.
| Fraction tick = notes.front()->chord()->segment()->tick(); | ||
| double sp = staff->spatium(tick); | ||
| double stepDistance = sp * staff->lineDistance(tick) * .5; | ||
| int stepOffset = staff->staffType(tick)->stepOffset(); | ||
|
|
||
| double upDotPosX = 0.0; | ||
| double downDotPosX = 0.0; | ||
| std::array<double, 12> dotPos{}; |
There was a problem hiding this comment.
I can't quite understand why this has size 12. Is it because it's 3 * VOICES? Better express it in terms of VOICES and perhaps a comment to give it some context
| Note* above = (i < nNotes - 1) ? notes[i + 1] : nullptr; | ||
| if (above | ||
| && (!above->visible() || above->dotsHidden() || above->chord()->dots() == 0 || !chord->combineVoice() | ||
| || !above->chord()->combineVoice())) { |
There was a problem hiding this comment.
I see lots of instances where you're evaluating conditions like chord1->combineVoice() && chord2->combineVoice() or its negation. You could probably benefit from introducing a method in the Chord class for this. It could be something like
bool combineVoice(const Chord* otherChord) const { return combineVoice() && otherChord->combineVoice(); }.
Or actually even better as a static method:
static bool combineVoice(const Chord* chord1, const Chord* chord2) { return chord1->combineVoice() && chord2->combineVoice(); }
and maybe tag it as inline to make it zero-cost.
That would make these things a bit more readable.
There was a problem hiding this comment.
There are 2 && and 2 || comparisons so far - I'm tempted to leave them as is. I think static combineVoiceAnd and combineVoiceOr functions would be overkill
There was a problem hiding this comment.
You don't need two: the || comparison here is just the negation of the && comparison. In boolean logic !(a && b) == !a || !b. So if you define
static bool Chord::combineVoices(Chord* chord1, Chord* chord2) { return chord1->combineVoice() && chord2->combineVoice(); },
then above you will call combineVoices(chord1, chord2) and here you will call !combineVoices(chord1, chord2), which makes it clearer what this means and easier to read :)
There was a problem hiding this comment.
🤦 sorry, demorgen's rolling in his grave - sorted!
| continue; | ||
| } | ||
| chord->setDotPosX(maxPosX); | ||
| const double maxPosX = std::max(upDotPosX, downDotPosX); |
There was a problem hiding this comment.
Could this part be moved out into its own function, perhaps, or would that complicate your life? In general, all of these ChordLayout functions are extremely hard to read, but there's nothing we can do about it really, when our starting point was functions with 500 LOC and 25 levels of indentation.
A day may come when we rewrite this whole thing...
There was a problem hiding this comment.
Yes, sorted this section out and will return to tidy the other parts of the function another day...
miiizen
force-pushed
the
chord-voice-layout
branch
from
ca2a581
to
44cc797
Compare
miiizen
force-pushed
the
chord-voice-layout
branch
from
44cc797
to
5a0e2a0
Compare
This PR introduces a toggle to disable the current behaviour when laying out chords, which is to combine similarly stemmed chords into one large chord in order to calculate note placement, notehead mirroring and dot layout.
There is a score wide option in styles and a per-voice option in the properties panel.
Screen.Recording.2024-06-03.at.10.37.33.mov