Cleanup several aspects of tech debt around graphics and font management #30665
+1,061
−1,277
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
from
534179c to
11f8dd6
Compare
XiaoMigros
reviewed
Oct 22, 2025
I think this should go hand in hand with removing As a solution, I would propose:
|
|
Another option might be to figure out what's the difference between QFont::pixelSize and QFont::pointSize, and whether that can somehow help us |
miiizen
reviewed
Oct 23, 2025
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
from
11f8dd6 to
6725ed0
Compare
miiizen
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
from
1e1476a to
b63abe1
Compare
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
2 times, most recently
from
ca2dc51 to
b57a77b
Compare
src/importexport/musicxml/internal/musicxml/import/importmusicxmlpass1.cpp
Outdated
Show resolved
Hide resolved
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
2 times, most recently
from
6b41e91 to
425f805
Compare
cbjeukendrup
approved these changes
Oct 31, 2025
8 tasks
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
3 times, most recently
from
8d2f6fa to
54715be
Compare
The reason why that code exist is that we handle text via Qt font metrics but music symbols via FreeType metrics. That is now managed by FontProviderDispatcher so there is no reason to keep this code hidden behind a compiler flag. In fact it doesn't even compile if the flag is removed.
I have never really understood what this quantity represents, other than it has a value of 5 and was multiplied by 72 to obtain the DPI. From what I can gather, it seems that the idea is to start from 72 points per inch (which comes from the definition of the typographical point, defined as 1/72 of an inch) and to set resolution of the program to 5 times that. In that sense DPI_F is probably intended as a "fractional" resolution with respect to the typographical point. That results in a DPI of 360. It now seems that this DPI_F variable is effectively unused (all the functions that take it as parameter end up ignoring it). And the DPI can be just set to 360, I don't see why it has to be constructed as an integer multiple of a typographical unit (in fact most printers have resolutions of 600 or 1200 DPI, which are not multiples of 72).
The necessity of using Qt for some font metrics and FreeType for others came from musescore#22189, which was caused by a Qt bug. Qt has now resolved that bug by introducing the flag PreferTypoLineMetrics. So there is no reason now for using two different source of metrics for text. In fact, I see also no reason to not use Qt for the music symbols fonts. I don't know the historical reason for that, but Qt should be able to handle them like any other font. To me this just feels like historical tech debt. Additionally, given that all of our drawing uses QPainter, using Qt for the metrics that define such drawing feels more logical to me and more likely to work consistently in all situations. Incidentally, this also happens to fix musescore#30456. I don't know why, but FreeType returns wrong values for the line spacing, while Qt seems to give the correct values.
After switching the symbol metrics to Qt, I found that Qt wasn't returning very precise bounding boxes. They seemed to all have some (small, but noticeable) rounding errors. By contrast, the bounding boxes that we were getting before were pixel-perfect. This may well be the historical reason why Qt metrics were not used for music symbol. It turns out though that Qt wasn't imprecise, it was just limited by the resolution of 360 DPI that we were setting ourselves. I assume that QFontMetricsF, despite returning double precision values, internally still rounds to whatever resolution is set. Increasing the resolution to 1200 DPI makes Qt's metrics on music symbols also pixel-perfect. As far as I understand, this resolution value only affects internal calculations, not the actual rendering resolution (plus music font metrics are cached anyway), so I don't think this should have any impact on performance. This also requires updating the value of default spatium (having finally understood what that 24.8 was). That number implicitely assumed a DPI of 360. We should obviously just write the spatium with the proper conversion factor from millimetres to pixels/points. Luckily this doesn't affect existing files, where it's already written in millimitres.
I have no idea what that magic number of 100 was about nor why it was even working before the change of resolution. It definitely wasn't working anymore with the new resolution.
Music symbols are just characters in a font, so there is no reason why their metrics should need special functions.
Error was always there but somehow was made visible by the change of resolution.
This is another variable that has never made any sense to me whatsoever. I can only infer from the way it was used that it was meant to represent the default value of spatium in pixel units. But if that's the case, the way in which it was defined made no sense at all. DPI / 72 used to just be equal to DPI_F, so SPATIUM20 was equal to 5 * DPI_F = 25. To me, the only reason why this seemed to work correctly as the default spatium value is that it just happened to be close to 24.8, which was the actual value. [Spoiler: see commit 577929d41b1416e1918dd9fa8159b091826f8598 for the explanation of what SPATIUM_20 meant] This commit removes SPATIUM20 and replaces it with appropriate calls to the style defaults. In a couple of places (namely StaffType and EngravingFont) the Score and the corresponding Style haven't been created yet, so they need to access the StyleDef class directly. In fact I don't see what StyleDef should make its members private. I think they can be public cause they are const so they can't be changed.
No idea about the exact history of this. It seems to have been introduced as a Mac-specific hack ages ago to solve some Qt-related problem. Pretty sure it's not necessary anymore (also produces no difference in vtests).
I think this was the only DOM class that still had its own draw method. I have moved it to the respective draw module where it belongs.
Cause that will work with both 16bit and 32bit (i.e. ucs4) character maps
MuseScore's internal coordinates are expressed in units determined by mu::engraving::DPI. The only quantity that is not expressed in this unit is text size, which is expressed in points and ignores the DPI. In our score model, doubling the DPI means for example that point (10, 10) becomes (20, 20), but a text of 10pt size does not become 20pt, it remains 10pt. When we draw on screen we apply a transform that maps our coordinates to the screen's physical coordinates, but because the text size is expressed in different units we need to apply a different scaling factor. On top of that, QFont::pointSizeF is also DPI-aware, meaning QPainter will apply its own scaling factor based on the painter's DPI to determine the resulting pixel size. QFont::setPixelSize is not a viable solution because it only accepts integer values. Another solution would be applying an equivalent scaling transform to the painter, but that results awkward because we need to scale only the text size but not the position. In the end the better solution is using QFont::setPointSizeF. What is bad about the current implementation is that the entire program has to be aware of the painter's DPI via MScore::pixelRatio. This is not necessary because any painter implementation is aware of its own logical DPI, so the scaling factor can be applied in the Painter class and no other part of code needs to know about it.
In theory, this unit is entirely arbitrary, so we could also set it to 1mm (that would make debugging nicer, cause every number we see represents millimetres). However, that corresponds to a very low DPI value (25.4), which doesn't work nicely with QFontMetrics, and very small scaled point sizes when drawing, which doesn't work nicely with QPainter. This seems the better compromise.
It has somehow become common knowledge that the Smufl font size corresponding to a 7 mm staff (i.e. a spatium of 1.75 mm) is 20 pt. I too thought that that was a definition. Turns out it isn't. Smufl just prescribes that the glyphs are scaled in a way that the em size of the font corresponds to a 5-line staff height, and it just so happens that 7 mm are close to 20 typographical points (7mm / 25.4 * 72 = 19.84 pt). Instead of computing all the font metrics using the correct point size of 19.84 pt, corresponding to our 7mm default, the metrics were being computed at 20 pt, and the SPATIUM_20 constant (which means "the value of spatium corresponding to 20 pt", different from the actual spatium) was being used throughout the codebase as a correction factor to account for the difference between 19.84 and 20.0, most notably in EngravingItem::magS. In other words we were scaling down the size of all items by a correction factor instead of computing it correctly from the start. Having removed SPATIUM_20, it was now necessary to compute the Smufl font metrics at the correct point size.
Turns out that the resolution does have a performance impact on QFontMetrics, but it's only noticeable in pieces with tons of text (typically lyrics) because unlike music symbol fonts those values can't be cached. It also seems to only start scaling badly for values beyond 1200 DPI, while going below seems to make no difference, so we can settle on 1200 DPI.
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
from
54715be to
5e2e768
Compare
Having eliminated SPATIUM_20 and replaced it with calls to defaultSpatium(), this function gets called a lot now. No need to check old defaults: the default spatium was always 1.75mm. Saves around 3% layout time on my test file.
mike-spa
force-pushed
the
cleanupTechDebtAroundGraphicsAndFonts
branch
from
be68845 to
cc211b6
Compare
cbjeukendrup
approved these changes
Nov 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #30456
Resolves: #14435
Resolves: #12714
Please refer to the commit descriptions for explanations about the specific changes. Should be reviewed one commit at a time so that what I'm doing makes more sense.
All the vtests will fail by rounding errors.