-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
New cross staff beam positions #23529
Conversation
a621dc1
to
f99523c
Compare
Pointer & dictator must reflect a beam's flatness in offsetBeamToRemoveCollisions. Calculate the middle line on the correct stave, and get this in the correct number of quarter spaces.
src/engraving/types/types.h
Outdated
@@ -1133,6 +1140,45 @@ enum class LayoutFlag : char { | |||
}; | |||
|
|||
typedef muse::Flags<LayoutFlag> LayoutFlags; | |||
|
|||
struct ChordPosition { |
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.
Would NotePosition be a more descriptive name here? It seems to be mostly connected to notes. Also, given that this type is very specific of the Beam code and isn't used anywhere else, it probably doesn't need to be defined here. I think you could move this definition to beam.h or beambase.h, or if you like even put it inside the Beam (or BeamBase) class. Then, methods of the Beam class can refer to it as ChordPosition, while methods outside the Beam class (like BeamLayout) can refer to it as Beam::ChordPosition, which I think is elegant and also makes its meaning clearer (otherwise the meaning of this class is not super clear being here out of context).
src/engraving/types/types.h
Outdated
@@ -356,6 +356,13 @@ enum class BeamMode : signed char { | |||
END | |||
}; | |||
|
|||
enum class CrossStaffBeamPosition : signed char { |
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.
Same reasoning here: this could probably be defined inside BeamBase or even inside BeamBase::LayoutData
bool secondToLastNoteSameHeightAsLowerEnd = endNote > startNote && ldata->elements[chordCount - 2]->isChord() | ||
&& toChord( | ||
ldata->elements[chordCount - 2])->downLine() == lowerEnd; | ||
Chord* secondNote = ldata->elements[1]->isChord() ? toChord(ldata->elements[1]) : nullptr; |
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.
Would be better to define a variable earlier to avoid so many repeated calls to ldata->elements
std::set<int> noteSet(item->notes().begin(), item->notes().end()); | ||
std::vector<int> notes(noteSet.begin(), noteSet.end()); | ||
std::set<ChordPosition> noteSet(item->notes().begin(), item->notes().end()); | ||
std::vector<int> notes; |
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've seen this elsewhere (and I think it's due to legacy), but it looks very weird to define a vector of ints and call it notes. Perhaps noteLines?
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.
Also good to do here notes.reserve(noteSet.size)
since you already know the size you'll need
std::vector<int> notes; | ||
std::transform(std::begin(noteSet), std::end(noteSet), std::back_inserter(notes), [] (const ChordPosition& pos) { | ||
return pos.line; | ||
}); |
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 an old skool for loop is simpler to read (and shorter, too)
for (ChordPosition pos : noteSet) {
notes.pushBack(pos.line)
}
|| ((topSlantDir != 0 && topSlantDir != overallDirection) | ||
|| (bottomSlantDir != 0 && bottomSlantDir != overallDirection) | ||
|| (beamSideSwitchDirection != 0 && beamSideSwitchDirection != overallDirection) | ||
|| (staffSwitchDirection != 0 && staffSwitchDirection != overallDirection)); |
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.
Try to avoid these big lists of conditions if you can. Sometimes breaking these down into intermediate bool variables helps make it more readable and simpler to debug
|
||
// Calculate slant between notes on stave closest to the beam | ||
int closestChordsSize = closestChordsToBeam.size(); | ||
ChordPosition closestStartPos(closestChordsSize > 0 && closestChordsToBeam.front() ? closestChordsToBeam.front()->line() : 0, |
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 null-checking closestChordsToBeam.front() and .back() is redundant here
Resolves: #21901
This PR implements correct cross staff beam placement for beams between staves and also above & below staves.
![Screenshot 2024-07-11 at 11 41 21](https://private-user-images.githubusercontent.com/26510874/347786351-8622e98b-bb14-4fb4-a395-7b3436d75454.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQzODcxNTUsIm5iZiI6MTcyNDM4Njg1NSwicGF0aCI6Ii8yNjUxMDg3NC8zNDc3ODYzNTEtODYyMmU5OGItYmIxNC00ZmI0LWEzOTUtN2IzNDM2ZDc1NDU0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIzVDA0MjA1NVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWQxOTM4OGU3N2QyODViM2Y1ZmYxNTExZmQxN2RmZGNjZjcxMDFjNmQ3YjNjMjc4N2I4ZWFjOWQ1YjE5OWM0NTImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.gEfzUX3MTwASGhrQJtgAYVI8RBWlXteGGihZc14aZOE)