-
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
Use a different workaround for artificially emboldened text #17997
base: master
Are you sure you want to change the base?
Conversation
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.
Looks good from code point of view, but will indeed need testing to assess the effects on performance.
@AntonioBL Tested on Windows10 #14435 looks good But now I can't make text bold at least in some way on Windows comparing with master (when testing #12717) bandicam.2023-06-16.17-36-20-767.mp4 |
Actually, the problem comes from Qt itself. Now, in the workaround that was used, the size of the drawn text actually depended on the zoom level: internally, when changing the zoom, the painter coordinates (diagonal element of the transformation matrix) are modified, but the workaround drew the glyph for painter matrix with 1.0 diagonal elements to avoid the ugly cached glyphs; the text fontsize was changed accordingly to appear at the right (apparent) size (the workaround was a little more comples, but for what concerns the drawing of the single glyphs, that was the behavior). That's why by changing the level of zoom we can see the text emboldened (when zoomed out, so that the drawn fontsize is small) or not (when zoomed in, so that the drawn fontsize is large) in current master. With this new workaround there is no need for glyph scaling during drawing, so the behavior should now only depend on the font size: when the size is below a certain value (I found it to be roughly 12.9, that corresponds to the "64" limit, by probably considering the factors due to DPI etc.) the bold text appears artificially emboldened, while it is not emboldened for size >= 12.9. Since Qt 5.15.9 there is an additional environment variable that overrides this limit: QT_NO_SYNTHESIZED_BOLD_LIMIT Ciao, P.S: we could think of compiling Qt by ourselves, maybe inside Github actions for the different operating systems so that Qt updates should require little effort. Maybe even including KDE patch collection https://community.kde.org/Qt5PatchCollection By the way, @cbjeukendrup already introduced Qt 5.15.9 for Mac. |
@AntonioBL OK! I've tested PDF and SVG output on Windows and didn't see a difference between PR's build and master build, probable we can merge it. @cbjeukendrup Do I need to test something else before merging? |
Most probably because PDF and SVG export did not use the workaround (at least, in 3.x it was so), because there was no scaling (and export DPI, linked to scaling, set exactly equal to the internal DPI) |
@DmitryArefiev If the performance is okay when scrolling around in a large score with many MuseJazz bold texts, then I think this can be merged! |
After a long battle against QtWebEngine, I think I managed to make Qt 5.15.10 (with KDE patches) compile for Linux in a Github Action. |
@AntonioBL One test is failed. Can you rebase please? |
…t workaround for artificially emboldened text
@DmitryArefiev : done 😉 |
Seems it isn't just text, and not just MuseJazz, but also many other fonts' musical glyphs, like accidentals, but most/all other gylphs too, clefs, noteheads, etc. All very small changes. Seems every glyph became a tiny bit thinner. |
Yeah, I see a little difference too. @oktophonie Can you review vests please? |
The only question for me is whether the 'before' or 'after' versions are more accurate, and I don't know how best to determine that. |
As the differences are a) very small and b) everting seems a tiny bit smaller, so won't result in less measures to fit a system, IMHO this is OK |
Of course not, because this will only affect the drawing, and not the actual layout computations. |
Ah, I see, that 'skinnyness' doesn't seem to affect the overall width, doesn't accumulate due to the distance between elements |
@AntonioBL The changes which vtests found are only on display? PDF output, physical print output should not have those differences comparing with master? |
They are visible on PNG export, no reason to assume PDF export or print would be different? |
Oh yeah, you're right |
In principle, the function |
Since the differences seem too tiny to be spotted in real life, I think it's safe to merge this to master (but let's maybe not merge it to 4.1.0 yet). And the problem that bold does not work can be fixed later by updating Qt. Does anyone disagree? |
I'd still to prefer to know whether the before or after is more accurate. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@@ -64,6 +64,16 @@ int App::run(int argc, char** argv) | |||
qputenv("QT_STYLE_OVERRIDE", "Fusion"); | |||
qputenv("QML_DISABLE_DISK_CACHE", "true"); | |||
|
|||
//! NOTE workaround for https://musescore.org/en/node/284218 |
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.
Do we still need this workaround?
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.
I think so, otherwise we would have again the bug with emboldened fonts shown here: https://musescore.org/en/node/284218 and https://musescore.org/en/node/281601
This new workaround should address the issue with bold+underline and the issue in the Home tab preview.
Unfortunately, there is still a little problem left for large font size, see #17997 (comment)
And after a second long battle against Qt WebEngine, I managed to obtain a universal build of Qt 5.15.10 (KDE patched)+ Qt WebEngine for MacOS. |
Great work, @AntonioBL! (And fortunately you found my I'm not very familiar with the KDE-patched version of Qt. What kind of patches are merged into this? Only very basic build fixes and obviously-correct bug fixes, or also disputable fixes and whole new features? And intuitively, I associate KDE more with Linux than with for example macOS, but are these patches also regularly tested on macOS? |
@cbjeukendrup : From here they say that they are commits cherry-picked from upstream Qt (probably > 5.15) that patches security issues, or crashes or "functional defects", and relevant towards Open Source products and their viability. So I think they should be mainly fixes already present in Qt 6 of bug found also in Qt 5.15.x, that the Qt group does not intend to backport (yet). I took the code from the git repository, which at the moment should be 5.15.10 for Qt with 5.15.14 for Qt WebEngine: |
@AntonioBL Thanks, that sounds good! Let's go ahead with the patched version then. Next step is getting a build for Windows... do you want to look into this, and do you have time for it, or shall I give it a try? |
If you want, I will try for a Windows build, but I cannot state at the moment how much time it will require. 😅 |
And here is the Windows build: https://github.com/AntonioBL/Qt5Binaries/suites/14488802861/artifacts/819378767 |
fa1f8d3
to
525a11a
Compare
Resolves: #14435
Resolves: #12714 (only the rendering part, not the "strange behavior" of bold switch for the title)
The text drawing routines under Windows and Linux, that are using Freetype font engine, were patched with a rough hack to prevent a QT bug due to the artificial emboldening of a font, while at the same time preserve kerning of the text. It was not working for bold+underline, because the "trick" of rescaling the font was rescaling the size of the underline and this could not be overwritten. By setting this env variable, QT does not used cached font glyphs and the bug is gone.
I don't know if this change could introduce performance issues (since glyphs are not loaded from the cache).
Most probably some of the vtests are changed by this PR.