-
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
Avoid collisions in full tab layout #22860
Conversation
327409a
to
abda505
Compare
|
||
int Note::stringOrLine() const | ||
{ | ||
return staff()->staffType(tick())->isTabStaff() ? string() * 2 : 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.
Wait, why string() * 2
?
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 6 strings are numbered 1 to 6 whereas the 5 stave lines are numbered 1 to 10 including spaces. This method is used in layout methods which are built around lines and assume even numbers are spaces and odd are lines.
I could make this more clear - rename it staveLine()
or just leave a comment?
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.
Aha! Good point. No I think the name is fine, but definitely worth a comment :)
const bool isSimpleTab = !staff->staffType() || (!staff->staffType()->stemThrough() && !staff->staffType()->isCommonTabStaff()); | ||
if (staff && staff->isTabStaff(tick) && isSimpleTab) { | ||
// Reset position | ||
for (track_idx_t track = partStartTrack; track < partEndTrack; ++track) { |
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.
Could you explain why you need to do this reset here? Do chords in normal staves also perform this reset? If so, it would be better to perform it in the same place if we can
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 needed when switching from full/common TAB to simple TAB. It isn't needed for normal staves as these always have offsets calculated and applied later on in layoutChords1
. I'll leave a clearer comment.
double nominalWidth = ctx.conf().noteHeadWidth() * staff->staffMag(tick); | ||
// Fret width plus white background box for TAB | ||
double nominalWidth = !isTab ? ctx.conf().noteHeadWidth() * staff->staffMag(tick) | ||
: (ctx.conf().fretWidth(staffType) + 0.2 * staff->spatium(tick)) * staff->staffMag(tick); |
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.
What is 0.2? Is that the white background padding around the fret number? If so, it would be better to properly define/retrieve this value (I think I may have seen it defined somewhere, it definitely should be a style definition if it isn't already)
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.
It is. It isn't defined properly anywhere. I'll add it as a style value.
@@ -2155,7 +2186,8 @@ double ChordLayout::layoutChords2(std::vector<Note*>& notes, bool up, LayoutCont | |||
|
|||
// accumulate return value | |||
if (!mirror) { | |||
maxWidth = std::max(maxWidth, note->bboxRightPos()); | |||
const double noteWidth = isTab ? note->tabHeadWidth(tab) + 0.2 * note->spatium() : note->bboxRightPos(); |
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 0.2 as above, I think?
|
||
if (isTab) { | ||
// Need to lay out TAB note to know width | ||
TLayout::layoutNote(note, note->mutldata()); |
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.
Similar question as before: why do we need this here for TABs but not for normal staves? Ideally, this should be done in the same place as it's done for normal staves (that is, I would imagine, well before we get here)
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 layout in this area just uses the notehead symbol dimensions and doesn't need the whole note to be laid out.
TAB notes need to have m_fretString
set correctly (which happens during note layout) before tabHeadWidth
can return an accurate value. I can change tabHeadWidth
to set m_fretString
to avoid needing to call note layout.
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.
Ah ok I see, makes sense. Then I'd leave it as it is (perhaps, again, with a comment to give this context). Let's keep tabHeadWidth single-responsibility and do the layout call here
|
||
muse::draw::Font f = tab->fretFont(); | ||
f.setPointSizeF(tab->fretFontSize()); | ||
return muse::draw::FontMetrics::width(f, u"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.
This feels quite hacky though. Fret width is highly variable from one fret mark to another (a two-digit fret is about twice as wide) so it doesn't feel like something that should be in LayoutConfiguration, which is about styles and general constants. Ideally we should just use the individual width of the fret that we are working on. If you really need a "reference" value of some sort, than I'd call this function "referenceFretWidth" or something like that and put it probably in the fret class. Though I think it'd be better to avoid this if we can
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 process of centering notes in chords needs a reference value, though it isn't important as to what that value is. I'll remove this and we can just use ctx.conf().noteHeadWidth()
(which is the width of the noteheadBlack
symbol) for both staves.
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.
After another look, we don't need to centre frets at all. (See chordlayout.cpp line 1641)
b370da6
to
9a45e79
Compare
Resolves: #21394
![Screenshot 2024-07-15 at 09 53 17](https://private-user-images.githubusercontent.com/26510874/348648053-66eab6df-378c-4c2b-8930-acec2654dcf7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQzODY3MDcsIm5iZiI6MTcyNDM4NjQwNywicGF0aCI6Ii8yNjUxMDg3NC8zNDg2NDgwNTMtNjZlYWI2ZGYtMzc4Yy00YzJiLTg5MzAtYWNlYzI2NTRkY2Y3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIzVDA0MTMyN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTMxNzdkNWUyODFkNTIyM2RjZDYzOTRjMjBmYmRjZWRiMmUyNjcwYWU2NzgyNGU5NTBhNWQwOGZiNDJmN2VkODQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.erO5L5R0cNCsfFxmsYylXmLa_V9QOFaOMey0XItzJ4Y)
This PR will avoid collisions between up & down stemmed chords in full TAB staves in the same way as we do in standard staves. It will also offset fret markings in common tabs, so voices and stems can be distinguished