Skip to content

Conversation

@mike-spa
Copy link
Contributor

Currently, our layout system (meaning all the collision checks) looks at accidentals like this:
image

With this PR, we look at accidentals like this:
image

In score very heavy of accidentals, this may have a small performance cost, because every accidental now is made of more than a single rectangle. However, that cost is basically offset by the gain made with this optimization. Besides, it is exactly in the situation with many accidentals that using cutouts like this can dramatically improve the layout.

This allows smufl cutouts to be used also by any other item that needs them in future.

@mike-spa mike-spa requested a review from its-not-nice January 11, 2024 16:36
@its-not-nice its-not-nice added the vtests This PR produces approved changes to vtest results label Jan 11, 2024
return score()->engravingFont()->bbox(symbols, magS());
}

Shape EngravingItem::symShape(SymId id) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better move this method to IEngravingFont

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And when doing that, we might want to cache the Shape calculated by this method, just like we do for the bbox actually

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better move this method to IEngravingFont

Done!

And when doing that, we might want to cache the Shape calculated by this method, just like we do for the bbox actually

I'm not sure I know where or how we would cache that Shape. How would you do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I know where or how we would cache that Shape. How would you do this?

You would add a new shapeWithCutouts field to the EngravingFont::Sym struct. This will probably need to be mutable.
Then, in EngravingFont::shapeWithCutouts, use const Sym& s = sym(id) to obtain the Sym for that symbol. Check if the shapeWithCutouts is empty; if not, just return it; if yes, then calculate it and store it for the next time, and return it.
You might also need to do some stuff with useFallbackFont(id).

To some extent, EngravingFont::bbox is a good example, except that all bboxes are calculated in advance when the font is loaded, instead of just at the moment that they are needed.

}

for (RectF& rect : rects) {
shape.add(rect, this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add method like addAll(const std::vector<RectF>& rects, EngravingItem* item)
then we can do m_elements.reserve to avoid relocated several times

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we can add method reserve to Shape class :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a new Shape constructor for this case :)

@MarcSabatella
Copy link
Contributor

MarcSabatella commented Jan 11, 2024

You probably realize this already, but just in case: we do already use cutouts in the stacking algorithm for accidentals within a chord:

PointF bboxNE = ac->symBbox(ac->symId()).topRight();
PointF bboxSW = ac->symBbox(ac->symId()).bottomLeft();
PointF cutOutNE = ac->symSmuflAnchor(ac->symId(), SmuflAnchorId::cutOutNE);
PointF cutOutSW = ac->symSmuflAnchor(ac->symId(), SmuflAnchorId::cutOutSW);
if (!cutOutNE.isNull()) {
acel.ascent = cutOutNE.y() - bboxNE.y();
acel.rightClear = bboxNE.x() - cutOutNE.x();
} else {
acel.ascent = 0.0;
acel.rightClear = 0.0;
}
if (!cutOutSW.isNull()) {
acel.descent = bboxSW.y() - cutOutSW.y();
acel.leftClear = cutOutSW.x() - bboxSW.x();
} else {
acel.descent = 0.0;
acel.leftClear = 0.0;
}

Also for key signatures:
SmuflAnchorId currentCutout = isAscending ? SmuflAnchorId::cutOutSW : SmuflAnchorId::cutOutNW;
SmuflAnchorId previousCutout = isAscending ? SmuflAnchorId::cutOutNE : SmuflAnchorId::cutOutSE;
PointF cutout = item->symSmuflAnchor(sym, currentCutout);
double currentCutoutY = line * step + cutout.y();
double previousCutoutY = previous.line * step + item->symSmuflAnchor(previous.sym, previousCutout).y();
if ((isAscending && currentCutoutY < previousCutoutY) || (!isAscending && currentCutoutY > previousCutoutY)) {
x -= cutout.x() / _spatium;

These are the "dev" versions, but the code has been in place a long time - predates autoplace. Perhaps the new shape system could be used here instead of this special casing.

@mike-spa
Copy link
Contributor Author

@igorkorsukov @cbjeukendrup I've implemented your suggestions. If you think this is good to go, it would be good to merge it before Igor's work on the Shape class, otherwise we get conflicts :)

@mike-spa mike-spa merged commit 56cf692 into musescore:master Jan 12, 2024
@its-not-nice its-not-nice added the engraving Shape, position, or ability to notate musical symbols & text in the score label Feb 19, 2024
@its-not-nice its-not-nice removed the request for review from RomanPudashkin February 19, 2024 13:07
@Eism Eism mentioned this pull request Feb 26, 2024
@mike-spa mike-spa deleted the accidentalCutouts branch February 17, 2025 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engraving Shape, position, or ability to notate musical symbols & text in the score vtests This PR produces approved changes to vtest results

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

5 participants