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

First attempt to correct kerning under Windows #3298

Merged
merged 1 commit into from
Dec 7, 2017

Conversation

AntonioBL
Copy link
Contributor

No description provided.

@AntonioBL
Copy link
Contributor Author

This is a Work In Progress.

I tried to increase the dpi for the texts.
It appears to solve the kerning problem of https://musescore.org/en/node/117191 and https://musescore.org/en/node/82021

I still have problems with some texts, for example the fretboard diagrams inside the palette. However I have no clue on how to solve this problem.

I didn't check if there are other texts problems introduced by this PR (staff/system texts, figured bass, tempo texts seem ok). I quickly tried pdf and svg export and they seem ok, but more tests are needed.

@@ -70,7 +70,12 @@ static const int MAX_TAGS = 32;
static constexpr qreal INCH = 25.4;
static constexpr qreal PPI = 72.0; // printer points per inch
static constexpr qreal SPATIUM20 = 5.0;
static constexpr qreal DPI = 72.0;
#ifdef Q_OS_WIN
static constexpr qreal DPIFACTOR = 10.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

in master at this spot we see DPI_F = 5 and for all platforms.
It is only used here and in sym.cpp, not in those other files you use DPI_FACTOR in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but in master also SPATIUM20 is modified by DPI_F.
See 4ac407d
I didn't want to change the code too much for fear of unexpected regressions.
If I change only mscore.h by adding a DPIFACTOR also to SPATIUM20, all score objects get reduced by a factor DPIFACTOR.

By the way, I noticed that the exported svg seems right, but the page is scaled up by a factor DPIFACTOR (the page size of the exported pdf on the other hand looks correct).

@AntonioBL
Copy link
Contributor Author

I think that the new implementation I pushed today solves the fretboard scaling in the palette, and also the svg export scaling.
Tests on other pc configurations are needed.

@AntonioBL
Copy link
Contributor Author

I already discovered a residual problem with fretboard diagrams (when adding one if the global scale was changed resulted in a different font size). Now it should be correct.

@Jojo-Schmitz
Copy link
Contributor

For some strange reason Travis CI didn't run against your latest commit?!?

@Jojo-Schmitz
Copy link
Contributor

Maybe @sidewayss wants to check the SVG stuff here?

@sidewayss
Copy link
Contributor

SVG scales. That's the whole point. So initial size is relatively unimportant, it's the relative sizes of all the elements that matters. That said, it's also important to have a default size that looks reasonable when you double-click on an SVG file and open it in your browser - or however you open/embed the file.

I assume that this change is intended to provide a better default size for the exported SVG files. Is that the case? If that is so, I suggest the primary concern should be that size itself - how does it compare to the size of the same score inside MuseScore?

I had made an attempt to neutralize all the different unit conversions, but that PR never got merged, and then Werner added an additional scaling factor, and now it seems that this PR is adding another scaling factor that is 10 for Windows OS. I still think there are too many unit conversions going on in MuseScore, and there are efficiencies to be gained. But if all this does to the existing code is create a more acceptable initial size for the SVG exports, then I have no objections.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Oct 1, 2017

@wschweer's added scaling is in master only, this here for 2.2 only, and the purpose is to get kerning on PDFs right for Windows only. It only touches SVG, to compensate for that scaling

@sidewayss
Copy link
Contributor

OK, I understand. So this is for PDF but it's touching SVG code? Regardless, I understand the change to file.cpp, and I would simply validate that the exports look good. The result is to make the SVG 10 times smaller on Windows, right? I have no idea why that's necessary, but if the result is good, then go with it.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Oct 1, 2017

My understand is that this doesn't change the size of SVGs at all, in the saveSVG it just compensates for an increase does earlier. But I'd better let @AntonioBL comment on this

@AntonioBL
Copy link
Contributor Author

This PR is trying to correct kerning problems appearing only under Windows in all graphical representations of scores in MuseScore (main program window, pdf, svg, png, etc...). This problem appears much more evident for small font sizes since it is caused by an integer- vs real-value representation of character dimensions. Under Windows, Qt QFontMetricsF gives only integer values (unless DirectWrite is used), see http://doc.qt.io/qt-5/qfont.html#HintingPreference-enum (the note at the end of the paragraph, after the two tables).
By upscaling the font and then manually re-adjusting the size of the text it is possible to have true real values, almost identical to the values given by that function under Linux.
The problem is that svgexport reads the pixel position values and uses them to save the svg, and scaling the DPI scales proportionally the pixels. That's why I was also scaling the svg output: even without this further scaling the relative scale was correct, but when opening the svg file under Inkscape I was obtaining a huge page size (because of the DPI scaling).

However, I found additional problems with this high DPI patch, for example a wrong horizontal spacing in the case of an end repeat barline at the end of a system, or a crash with the snapshot tool.
Therefore I reverted the changes and I applied some changes only to text.cpp, isolating the changes to text drawing only. I compared the parameters given by Qt font metrics funcitons under Windows and Linux and I rounded back to integer the values which were integers also under Linux.
By doing so, most kernings and spacings are now almost identical under Linux and Windows. There is still a difference in the vertical positioning of spatium-independent texts (e.g. title, composer, lyricist, footnote, ...), but I think that it comes from a 1-pixel shift resulting from the QFontMetrics differences between the two operating systems (the height of the text is given by the bounding rectangle and/or the line spacing, which are both integer pixel values also under Linux). I don't know how to corred it further.

I will push the updated PR in a few minutes.

As an additional note: I tried the same trick of scaling the font also under Linux (i.e. using a DPIFACTOR larger than 1 also under Linux), but for small values of zoom (i.e. score zoomed out) I was obtaining drawing artifacts: the text was appearing huge, but then when zooming in, after a certain zoom value (depending on text size) the text was appearing correct, also for larger zoom values.
This does not happen under Windows.

@@ -198,8 +198,14 @@ bool TextFragment::operator ==(const TextFragment& f) const

void TextFragment::draw(QPainter* p, const Text* t) const
{
p->setFont(font(t));
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 I will add a #ifdef Q_OS_WIN here. It seems a pity to do all the following if DPIFACTOR is 1.0. Otherwise, I don't see any artefact on MacOS. So I'll merge this so we can get more testers.

@lasconic lasconic merged commit 7f6892d into musescore:2.2 Dec 7, 2017
lasconic added a commit that referenced this pull request Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants