-
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
Fix #17891: Instrument names follow largest stave spatium in part #19161
Fix #17891: Instrument names follow largest stave spatium in part #19161
Conversation
380dbea
to
1b2b529
Compare
1b2b529
to
a5ef18a
Compare
src/engraving/dom/instrumentname.h
Outdated
@@ -64,6 +64,8 @@ class InstrumentName final : public TextBase | |||
SysStaff* sysStaff() const { return _sysStaff; } | |||
void setSysStaff(SysStaff* s) { _sysStaff = s; } | |||
|
|||
double spatium() const; |
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 implicitly overrides EngravingItem::spatium(). You should make EngravingItem::spatium() virtual and override it explicitly. This type of solution may not be perfect either, but at least it is more obvious, and you no longer need that cast inside TextFragment::font
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.
Making EngravingItem::spatium()
virtual was my first approach, but after Michele asked me to do a performance test we found that layout took noticeably longer (3-4%). I can change the name of the method to avoid confusion, though we'd keep the cast.
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.
Yeah, making spatium() virtual is very elegant codewise, but spatium() is an extremely hot function, and my concern was that resolving the vtable at every call would carry a noticeable runtime cost (which seems to be indeed the case), so I advised against making it virtual. I agree that overriding the method implicitely may cause confusion though, so it's probably better to just call it a different name
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.
Yes, you are right. We just should rename InstrumentName::spatium()
a5ef18a
to
0bbf173
Compare
Resolves: #17891
Resolves: #19577