Added fretboard diagram legend box #26225
Conversation
Eism
force-pushed
the
fret_diagram_legend
branch
from
30fca22 to
b25a74f
Compare
Eism
force-pushed
the
fret_diagram_legend
branch
from
b25a74f to
b920ca6
Compare
Eism
force-pushed
the
fret_diagram_legend
branch
from
b920ca6 to
eb17522
Compare
| void FBox::setContentHorizontallAlignment(AlignH alignment) | ||
| { | ||
| m_contentAlignmentH = alignment; | ||
| } |
There was a problem hiding this comment.
Personally I find it cleaner to put the getter/setters in the header when they are 1-line. But that's just personal preference :)
| @@ -1640,6 +1642,10 @@ bool EngravingItem::isPrintable() const | |||
|
|
|||
| bool EngravingItem::isPlayable() const | |||
| { | |||
| if (m_isPlayable.has_value()) { | |||
| return m_isPlayable.value(); | |||
There was a problem hiding this comment.
Is it really necessary to add a data member for this? If I understand correctly, you're using this to make sure that the harmonies inside the legend aren't played. For that, I think it would simpler to add a case below with some logic like:
case ElementType::HARMONY:
return explicitParent() && explicitParent()->isSegment();
I think we can make it a general principle that if the Harmony doesn't have a parent Segment then it can't be played (because it means it's not "in the score")?
|
|
||
| FBox* fbox = toFBox(measure); | ||
| fbox->init(); | ||
| fbox->triggerLayout(); |
There was a problem hiding this comment.
Can there be more than one legend in the same score? If not, perhaps you could break after this, so we avoid looping the entire score.
| @@ -1027,6 +1027,83 @@ void TDraw::draw(const VBox* item, Painter* painter) | |||
| void TDraw::draw(const FBox* item, Painter* painter) | |||
| { | |||
| draw(static_cast<const Box*>(item), painter); | |||
|
|
|||
| const std::vector<Harmony*> harmonies = item->harmonies(); | |||
| const std::vector<FretDiagram*> fretDiagrams = item->fretDiagrams(); | |||
There was a problem hiding this comment.
const std::vector<Harmony*>&
const std::vector<FretDiagram*>&
| PropertyValue propertyDefault(Pid propertyId) const override; | ||
|
|
||
| const std::vector<Harmony*> harmonies() const { return m_harmonies; } | ||
| const std::vector<FretDiagram*> fretDiagrams() const { return m_fretDiagrams; } |
There was a problem hiding this comment.
const std::vector<Harmony*>&
const std::vector<FretDiagram*>&
I think?
| draw(fretDiagram, painter); | ||
| painter->translate(-pos); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ok I think this should be done a bit differently.
- All of these calculations should be moved in
TLayout::layoutFBox(), otherwise we repeat this at every frame refresh, which is unnecessary. InsidelayoutFBoxyou should calculatex, yin the same way as you're doing here, and then doharmony->mutldata()->setPos(x, y). - The
drawmethod should not be responsible for drawing the children. Instead, you should override thescanElements()method for FBox (see for exampleTBox::scanElements()) and make sure to scan all your harmonies and fretdiagrams. This will collect them for drawing and, if you've set the correctx,yposition for each of them during layout, they will be drawn in the correct place without you needing to do anything here. - Then you can basically eliminate this function, because all that's left is drawing the borders of the box, and that you can do via the generic
draw(Box*, ...)function.
| layoutBaseBox(item, ldata, ctx); | ||
|
|
||
| const std::vector<Harmony*> harmonies = item->harmonies(); | ||
| const std::vector<FretDiagram*> fretDiagrams = item->fretDiagrams(); |
| double maxFretDiagramWidth = 0.0; | ||
| double maxHarmonyHeight = 0.0; | ||
|
|
||
| for (int i = 0; i < totalHarmonies; ++i) { |
There was a problem hiding this comment.
Inside this loop, you can also calculate the x,y position for this Harmony and do harmony->mutldata()->setPos(x, y) (see my previuos comment)
| xml.tag("columnGap", item->columnGap().val()); | ||
| xml.tag("rowGap", item->rowGap().val()); | ||
| xml.tag("chordsPerRow", item->chordsPerRow()); | ||
| xml.tag("horizontalAlign", static_cast<int>(item->contentHorizontallAlignment())); |
There was a problem hiding this comment.
Since you've created new properties for all of these, it would be bette to use writeProperty(item, xml, Pid::FRET_FRAME_H_ALIGN)
| } else if (tag == "chordsPerRow") { | ||
| b->setChordsPerRow(xml.readInt()); | ||
| } else if (tag == "horizontalAlign") { | ||
| b->setContentHorizontallAlignment(static_cast<AlignH>(xml.readInt())); |
There was a problem hiding this comment.
Same here, it would be better to use the readProperty function using the Pid. That guarantees that we don't make mistakes in the tags
|
Also (this is an engraving comment, not a coding comment 😄) it would be good to set the default bottom padding to 2sp or something like that, in order to avoid "touching" between diagrams in the legend and in the score.
|
The first part of #26042
Screen.Recording.2025-01-27.at.2.22.26.PM.mov