Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonioBL
Copy link
Contributor

@AntonioBL AntonioBL commented Jun 15, 2023

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.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

Copy link
Contributor

@cbjeukendrup cbjeukendrup left a 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.

@oktophonie oktophonie changed the title fix for #14435 and possibly #12714 : use a different workaround for a… Use a different workaround for artificially emboldened text Jun 15, 2023
@DmitryArefiev
Copy link
Contributor

DmitryArefiev commented Jun 16, 2023

@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

@AntonioBL
Copy link
Contributor Author

AntonioBL commented Jun 19, 2023

Actually, the problem comes from Qt itself.
Since Qt 5.15.1 there is a size limit over which the fonts without a bold variant (like MuseJazzText) are not synthetically emboldened.
See this Qt commit: qt/qtbase@f0637fb
and related Qt bug: https://bugreports.qt.io/browse/QTBUG-84570 if the bold text is drawn with fontsize larger than 64 points, then it is not artificially emboldened during the painting routine.

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
See the related commit: qt/qtbase@7d9329d
So if and when there will be a switch to Qt 5.15.9 (and higher), we could set this env variable as well and have an artificially emboldened for every size dimension.

Ciao,
ABL

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.
I'd like (a lot) to work on this, but I cannot guarantee a short deadline, since I have very little free time at the moment. 😞

@DmitryArefiev
Copy link
Contributor

DmitryArefiev commented Jun 27, 2023

@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?

@AntonioBL
Copy link
Contributor Author

@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.

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)

@cbjeukendrup
Copy link
Contributor

@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!

@AntonioBL
Copy link
Contributor Author

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.
I had to split compilation into three different jobs 😅 and make extensive use of the cache.
I compiled it within an Ubuntu Bionic Docker image, so that there should be no problems of possibly required newer libraries during runtime.
Here is the amd64 artifact: https://github.com/AntonioBL/Qt5Binaries/suites/14001144253/artifacts/781475003
I am not yet sure that all the needed modules were compiled, and I have doubts about file permissions: Docker creates binaries belonging to root user, but I tried to add rwx permissions to all.
Now I will try for an i386 build.
I shudder to think about the compilation under Windows...

@DmitryArefiev
Copy link
Contributor

@AntonioBL One test is failed. Can you rebase please?

…t workaround for artificially emboldened text
@AntonioBL
Copy link
Contributor Author

@DmitryArefiev : done 😉
But I think the test will still fail since the failure is vtests: text redering slightly changed because the workaround was apparently applied in most of text redering.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jul 3, 2023

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.

@DmitryArefiev
Copy link
Contributor

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?

@oktophonie
Copy link
Contributor

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.

@Jojo-Schmitz
Copy link
Contributor

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

@cbjeukendrup
Copy link
Contributor

Of course not, because this will only affect the drawing, and not the actual layout computations.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jul 3, 2023

Ah, I see, that 'skinnyness' doesn't seem to affect the overall width, doesn't accumulate due to the distance between elements

@DmitryArefiev
Copy link
Contributor

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.

@AntonioBL The changes which vtests found are only on display? PDF output, physical print output should not have those differences comparing with master?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jul 3, 2023

They are visible on PNG export, no reason to assume PDF export or print would be different?

@DmitryArefiev
Copy link
Contributor

Oh yeah, you're right

@AntonioBL
Copy link
Contributor Author

In principle, the function drawTextWorkaround was used only in drawing Harmony texts (i.e. chord texts) and Textfragment texts (i.e. other texts, such as Staff Text, Tempo Text, etc.).
Maybe musical symbols are drawn as Textfragments, so maybe that's the reason why they very slightly changed.
In MuseScore 3.x PDF drawing was using a different drawing function than the one for the displayed score; if evrything was unified, I would expect the pdf output to slightly change as well.
The workaround was originally thought to be used only for particular cases, while all other cases relied on Qt text drawing routines, but I think that in the rewriting for MuseScore 4 the original workaround was used even for normal text drawing. Qt routines are also the one used by MuseScore in MacOS.
[By the way, if you take pdfs of the same score exported by Linux, Windows and Mac they will probably look slightly different, even without this PR]

@cbjeukendrup
Copy link
Contributor

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?

@oktophonie
Copy link
Contributor

I'd still to prefer to know whether the before or after is more accurate.

@Jojo-Schmitz

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
Copy link
Contributor

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?

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 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)

@AntonioBL
Copy link
Contributor Author

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.
Here is the artifact:
https://github.com/AntonioBL/Qt5Binaries/suites/14163460885/artifacts/794305575
@cbjeukendrup : I shamelessly based the MacOS compilation on your branch https://github.com/cbjeukendrup/musescore_build_qt but I switched to the KDE-patched version because there was a problem during the compilation (and I found that I had to revert one of those patches for a successful compilation, since it was a workaround for an XCode bug already fixed upstream).

@cbjeukendrup
Copy link
Contributor

Great work, @AntonioBL! (And fortunately you found my musescore_build_qt repository, I should really have told you about that...)

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?
And, since we use the 5.15 branch, which version is this? Is it the latest available open source version, i.e. 5.15.10? Or is it Qt 5.15.10 with Qt WebEngine 5.15.14?

@AntonioBL
Copy link
Contributor Author

@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 think some of the patches were included in the 5.15 pacth releases (5.15.3, 5.15.4, etc...)

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:
https://invent.kde.org/qt/qt/qt5/-/tree/kde/5.15?ref_type=heads
Here is the list of the patches applied: https://invent.kde.org/qt/qt/qtbase/-/compare/v5.15.10-lts-lgpl...05406c3f5f516d3148254c8294e8883c28a2c95a?from_project_id=1216&straight=false
To me it seems that they are including patches not only related to Linux, but also other operating systems, for example patches for MinGW, and MacOS (such as the patch for compilation with new XCode, that I had to revert to make it compile with the even newer XCode). But I think that the patches related to non-Linux OSs are probably not their priority.

@cbjeukendrup
Copy link
Contributor

@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?

@AntonioBL
Copy link
Contributor Author

@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. 😅
I was thinking about using tuxliketimeout to split Qt WebEngine compilation, if needed, since the timeout function under Windows is not the same as under Linux.

@AntonioBL
Copy link
Contributor Author

And here is the Windows build: https://github.com/AntonioBL/Qt5Binaries/suites/14488802861/artifacts/819378767
Unfortunately, QtWebEngine requires at least Windows SDK 10.0.19041.0, so I don't think it will run in Windows 8 and 7 (MuseScore 3.x, based on Qt 5.9, could run even under Windows 7).

@musescore musescore deleted a comment from RobFog May 15, 2024
@musescore musescore deleted a comment from Jojo-Schmitz May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants