Page Settings: storage units
Here is the PR with the current code: https://github.com/musescore/MuseScore/pull/4456
I have chosen Points x 10,000 as the units for storing all of the page settings' values, including page width/height, the margins, and the spatium itself. The available units are: Millimeters, Inches, Points, Picas, Didot and Cicero.
The reason for the 10,000 multiplier is so that the values can be stored as integral numbers, no floating point mess (thanks to Peter Jonas for that suggestion).
Currently the spatium is stored in Millimeters and all the other values are stored in Inches, all floating point values.
The reasons for choosing Points are:
1) The spatium and all internal units are based on Points. The default spatium value is 5pt, multiplied by a factor of 5 to achieve higher internal resolution (see the constant DPI_F
in the code). This means simpler unit conversions everywhere in the code because of the alignment around Points. Less conversion is better, IMO. It means simpler mathematical formulas, less numeric manipulation, etc.
2) Points are the most granular unit available, the most precise at whole number values. This is not really a critical reason, but it's not a bad one either. The multiplier (10,000) can be adjusted if a different unit is chosen.
I'll let others comment with their opposition or support for Points x 10,000 as the units. I understand that there are concerns about using Points, but I'll let others state them - I don't want to put words in anyone's mouth. Thanks in advance for sharing your point of view!
Comments
I have looked through the issues listed in #280363: [EPIC] Page Settings: fixes and changes as well as through the proposed changes and I still don't believe these changes are necessary. My objections are of two kinds, I'll try to describe them below.
The file format is not a problem
Or, strictly speaking, it has some problems but not in the context of the discussed issues.
As far as I understand, a large part of the issues listed in #280363: [EPIC] Page Settings: fixes and changes (probably most of them if to exclude various UI feature requests) are different variations of the same problem: when one switches from inches to millimeters or back and applies the page settings without changing anything the settings are actually getting changed due to rounding errors. And talking about this sort of problems, the file format has nothing to do with it at all. The rounding error gets introduced exactly at pressing "OK" in the page settings dialog, since it always reads the values listed in the checkboxes (which have limited precision) and applies them as the new page settings for the current score, regardless of whether the user has changed anything about those values or not. The solution can be similar to what is done in LibreOffice (and is a good practice in any case): we can actually change the given page parameter only in case that parameter's value got changed by a user. Regardless of the solution, this is purely a user interface issue and it should be fixed by altering the user interface behavior, not by introducing drastic file format changes.
Points are absolutely not natural units
I believe I have written it somewhere before but I'll duplicate it here. Units like millimeters and inches are well-defined beyond the scope of MuseScore and they will not change in any time. The internal DPI (or that
DPI_F
parameter) are purely internal implementation-specific parameters and making the file format be dependent on such details without a very good reason doesn't look like a good decision. It is natural to assume thatDPI
,DPI_F
or something like that can be changed in any time without breaking anything to the end user but making a file format be dependent on it effectively makes us fix those parameters forever or introduce extra compatibility layers in case of such changes. This problem simply does not exist with inches or millimeters.For the listed reasons, I believe that changing units for page settings storage is unneeded and may introduce some extra problems so it is better to avoid making such changes.
In reply to I have just looked through… by dmitrio95
SPATIUM20 is defined in points today. Do you want to switch to defining it in Millimeters? That is the core issue that drives all of this. The default spatium value of SPATIUM20 must be preserved correctly, without rounding errors, otherwise unnecessary scaling occurs. Converting SPATIUM 20 to Millimeters, as is done today, causes rounding errors, unless other code is in place to correct those rounding errors.
It would be possible to simply correct the rounding errors, with hard-coded, rounded values, as I have done for reading older file formats. That seems like a half-measure, a kludgy solution, to me, but I can implement it for everything.
I disagree that Points are not as well defined as Millimeters or Inches, though their definition does depend on Inches. PostScript Points are a completely standard unit, absolutely defined (relative to inches at 72dpi). I can revert to Inches for Page Settings styles; but the spatium is another matter. I believe it's a good idea to use the same units for storing all of these values, but it's not essential. Using the 10,000 multiplier is also an improvement over storing floating-point values.
As for DPI_F, there will always be some manner of determining the internal units, and there will always be unit conversions. Do you have a specific plan for the future, or are you guessing as to how it might be? For now, until SPATIUM20 is defined in something other than points and DPI_F goes away, points are the best units because they create the fewest, simplest unit conversions.
I agree that the rounding inside the pagesettings.ui dialog is unavoidable at a certain point. The dialog rounds to 2 decimals, and that is the issue. The file format changes are definitely separate from that. I decided it was a good idea to take a holistic approach and solve a bunch of related problems in the process.
In summary, I believe that for the spatium, Points are by far the best units of storage. That is the core of the change. Everything else follows. I can revert the page settings styles to Inches, but that seems messy to me.
Thanks for expressing your views so clearly. I hope others can chime in and we can come to an agreement.
In reply to SPATIUM20 is defined in… by sideways
...and to be absolutely clear:
In version 3.0.5, today, the spatium value is rounded when converted to Millimeters to store in the file. You do not have to open the page settings dialog box to cause that rounding. It happens 100% of the time to 100% of the scores.
In reply to SPATIUM20 is defined in… by sideways
...and if you want to understand just how standard Points are, look at the QPageLayout and QPageSize classes in Qt. They both provide special methods just for Points, not for any other standard unit. Not only is it a standard unit, it is the "universal" unit for page settings values in Qt itself.
https://doc.qt.io/qt-5/qpagesize.html#rectPoints
https://doc.qt.io/qt-5/qpagelayout.html#fullRectPoints
https://doc.qt.io/qt-5/qpagelayout.html#marginsPoints
https://doc.qt.io/qt-5/qpagelayout.html#paintRectPoints
Plus it is the global standard unit for font sizing. Points may not be used in the hard, physical world outside of a computer, but we're talking about software here. In the soft world, and with software-driven printers, Points are absolute, and as standard as Inches or Millimeters, if not more so. Where do Inches and Millimeters exist inside a running instance of the MuseScore executable? Nowhere, except in the way that Pixels and Points are defined relative to Inches. The fact that you cannot go to the hardware store and buy a ruler or tape measure in Points is not relevant here.
In reply to SPATIUM20 is defined in… by sideways
> SPATIUM20 is defined in points today. Do you want to switch to defining it in Millimeters? That is the core issue that drives all of this.
Are there any user-visible issues that are caused exactly by this?
> I agree that the rounding inside the pagesettings.ui dialog is unavoidable at a certain point. The dialog rounds to 2 decimals, and that is the issue.
My point is that maybe MuseScore should avoid rounding page settings values if user didn't change them. It would probably help to avoid most of the critical issues mentioned in #280363: [EPIC] Page Settings: fixes and changes.
> if you want to understand just how standard Points are, look at the QPageLayout and QPageSize classes in Qt
So you use PostScript points in your approach? Sorry, I didn't understand that from your initial description. Then my arguments about the volatility of the measurement units indeed don't apply here, at least as long as we express this clearly in the code.
Still there shouldn't be a need to change something if the current approach works. If you are aware of the user-visible issues that originate exactly from the file format, could you please link them here? Looking through the #280363: [EPIC] Page Settings: fixes and changes page I couldn't find such issues (though I could probably miss something).
In reply to Block quoted > SPATIUM20 is… by dmitrio95
The main user-visible issue that I have seen is bloated SVG files (very bloated and overly complex - every single element has a full matrix transformation). The other user visible issues are more subtle scaling problems, and mainly happen when you use Inches, because at 3 decimals Inches is still very coarse. Using Millimeters, the rounding is smaller.
I understand your "user-visible" qualification for the issues. There are definitely several workarounds that I could implement for this core problem. I proposed some of them initially on the forums, but they were shot down by others. So I looked for a "clean" solution.
I understand that the spatium rounding is something that affects SVG and internals more than anything else, but I care a lot about SVG :)
In reply to Block quoted > SPATIUM20 is… by dmitrio95
> My point is that maybe MuseScore should avoid rounding page settings values if user didn't change them. It would probably help to avoid most of the critical issues mentioned in #280363: [EPIC] Page Settings: fixes and changes.
My code uses QPageLayout and QPageSize to store those values, and those classes also round to 2 decimals, so it all aligns with the dialog. I think that for margins and page width/height there is no issue anymore. It's the spatium rounding to 3 decimals that is the only "critical" issue with the dialog, and it's only an issue relative to SPATIUM20 and extra scaling. I have sub-classed QPageLayout as MPageLayout in style.h, and that handles various conversions without rounding too, for the style values.
In reply to Block quoted > SPATIUM20 is… by dmitrio95
The topic of file storage units is one thing. Regarding the topic of the dialog's rounding and of page settings unit conversions (regardless of what unit is stored in the file), this PDF is up-to-date: MS_PageSettings.pdf
The basic idea is that MPageLayout stores the values in user units. Any other units are converted from there on demand. The page settings dialog operates in these same user units, so the user is directly connected to the source values. Any other values are derived. Here is a link to MPageLayout itself:
https://github.com/sidewayss/MuseScore/blob/8f5ae6b2ba9dc20c99e96b0939f…
A few lines above is
struct PageUnit
, which is modeled afterstruct StyleType
, which is at the top of style.cpp....your question has caused me to review and find a change I need to make:
QPageLayout stores Points and Didot as whole numbers. I need to change the resolution of the width/height and margin spinboxes in the dialog to zero decimals when those units are selected. Otherwise the user might enter values that are too precise, only to find them ignored later.
Since your initial discussion of the rationale for these changes is kind of ancient history at this point and many of the details may be different, it might help to see if you can succinctly state the actual real-world problem, ideally with a specific example we can follow step-by-step. I recall it has something to do with SVG export but so there has been much water under the bridge since you first brought this up, it's worth a re-explanation as if you were explaining it for the first. Otherwise, it's going to be hard for anyone to comment meaningfully.
The one thing I do definitely get is that the rounding when we store the spatium value is not great. And I have no problem with points as a unit. But when you do produce the specific real world example, it would be good if it illustrates why it can't be fixed with 2-line change - one line to convert to points before writing, another to convert from points when reading. I think maybe I also have an extremely vague concept about this, but I would be willing to bet most others here will have no idea whatsoever.
In reply to Since your initial… by Marc Sabatella
The SVG Export problem is based on the premise of SPATIUM20 as the base size/resolution, relative to which the document scales. SVG scales fluidly. The "S" stands for "Scalable".
An SVG document has a size and a resolution, in pixels. For MuseScore SVG exports, these are in MuseScore internal pixels, at 360dpi. You have to pick a resolution, and it's simplest to use the same one as MuseScore does already (that's the whole issue, using anything else is a nightmare).
If an element's size is scaled larger or smaller, the SVG export code recognizes that and applies the appropriate
transform
to the element.If the spatium is different from the default of SPATIUM20, the entire document is scaled, using SPATIUM20 as the base. That's not just SVG exports, but all of MuseScore. When you use a non-default spatium value, rendered font sizes are not the numeric size displayed in the dialog boxes. Distances between things are scaled. Everything is scaled relative to the spatium value. Only the values in Staff Space units (sp) are displayed accurately (that's almost everything but font sizes).
In MuseScore everything is relative to the spatium, so it's always scaling things. That's not the case in SVG. SVG document scaling is handled by the container, not the document itself.
In the current SVG export code, each element gets scaled, one by one. That could be adjusted, but a better fix would be to ignore any non-default spatium value when exporting SVG - always export as if spatium == SPATIUM20. Unfortunately that is messy because MuseScore has already applied the spatium scaling factor and Qt has it in memory that way. So SVG export would have to scale everything back to the default.
If SVG exports are all at the default spatium value of SPATIUM20, they come out clean. I can even create a .css style file with font sizes and stroke-widths that matches that specific, default resolution. The numbers are nice and round too. The MuseScore music font size is 100pt at that resolution, for example.
If the spatium is rounded, non-default by only 0.001mm, scaling occurs. Storing SPATIUM20 in millimeters is impossible without some kind of rounding. The decimals are endless. SPATIUM20 is defined at the lowest level as 5pt, a whole number.
I hope that's clear. It's not a simple thing to explain.
In reply to The SVG Export problem is… by sideways
Sorry for the extra words, but this just became clear to me (again):
Font sizes are what they are in MuseScore, including the music font that is used for all the note heads, accidentals, etc. If the spatium changes, the font-sizes do not. Instead, MuseScore scales the text at the specified font size.
SVG export code receives from Qt: a font-size, a character, and an x/y position on the canvas. Any scaling must be applied separately. The x/y positions are already modified, so the document itself cannot be scaled. The scaling must happen on an element-by-element basis, not at the document level. Scaling the document would screw up the x/y position values.
If the spatium is non-default, then the SVG code must scale all the elements, including notes, accidentals, etc. one element at a time. Each element gets a
transform
attribute set.In reply to The SVG Export problem is… by sideways
Sorry, still not clear to me. I am aware of how scaling works in general in MsueScore. What I still don't get is why something that works for everywhere else somehow doesn't for SVG. Eg, why can't SVG export simply set reset the scaling (and round as needed) during export? Can you explain with a specific example? As in, post a score, give us step by step instructions to follow, explains what you would like to see happen and what happens instead. Then we begin to comment on the proposed changes.
In reply to Sorry, still not clear to me… by Marc Sabatella
SVG Export does exactly that, and the file size goes up noticeably, the file readability goes down the toilet, and the container must interpret thousands of scaling transformations, which slows performance at read-time. All because of rounding that cannot be controlled fully.
And all completely unnecessary because the scaling is handled by SVG itself.
In reply to SVG Export does exactly that… by sideways
If the code could scale the whole document at once, by scaling the document element, that would be one thing. But each and every note, accidental, etc. must be scaled individually.
In reply to SVG Export does exactly that… by sideways
Again, in order to have any idea what you mean, we'd want to see a score and precise steps to reproduce the problem. Offhand I can't imagine why if, the scaling & rounding are happening correctly, the file size would go up.
In reply to Again, in order to have any… by Marc Sabatella
I missed this comment in spite of repeatedly refreshing the page... sorry about that.
The SVG file size goes up because, as explained above, every note, accidental, etc. gets stuck with an unnecessary transform attribute value. SVG files are text files. It's a flavor of XML. If you have a score with 1,000 elements (that's not a large score), the SVG file will include 1,000 instances of this text:
transform="matrix(1.11 2.22 3.33 4.44 5.55 6.66)"
those being demonstration numeric values with no meaning.
The container has to interpret every one of those transforms too at render-time.
In reply to Sorry, still not clear to me… by Marc Sabatella
Alternate spatium values exist as a way to fit things on a printed page. The page size/margins are fixed, everything else scales. You are creating a specific, hard size for everything.
SVG is not for paper, it is for pixels. There are no fixed page sizes, there are no fixed element sizes. Everything sizes to the container, which can be any size at all.
A non-default spatium value is just noise as far as SVG is concerned.
Keep in mind that I'm creating scores specifically for SVG, and I ran into this problem. I could not guarantee that my spatium value was equal to SPATIUM20. It is not possible to do so in the current code. As soon as you save the file or change the page settings, it has a rounding error. That was my original motivation. I have gone through various solutions and arrived here.
That doesn't mean that you cannot or should not export scores that have an alternate spatium; but it does mean that if you don't want an alternate spatium, you're stuck with one anyway in the current code.
The default value only exists during the application session in which the score is created, and then only until you apply any page settings changes. After that, or the next time you open it, you will be using a value with a rounding error - the size of that error depends on your page settings.
Marc - please take notice of how I've avoided the use of plural spatium :)
In reply to Alternate spatium values… by sideways
So again I'm back to not understanding why this isn't a two-line change. Again, without a specific example and precise steps, you might as well be speaking Greek to most of us.
In reply to So again I'm back to not… by Marc Sabatella
How would this be a 2 line change?
Steps are easy. Open any existing score from a file and export it to SVG. Open the SVG file in a text editor. You'll see all the scaling transforms.
If you want to reproduce the rounding you have to debug and check the live Score::spatium() value. Simply create a new score, check the spatium value - it will be 25.0. Now open page settings and press OK. Check the spatium value, it will not be 25.0 anymore.
Instead of applying page settings changes, simply save, then reopen the score. Check the Score::spatium() value again and it will not be 25.0.
In reply to How would this be a 2 line… by sideways
And yes, there are less radical ways to do a fuzzy match and restore the default value. I proposed some of those back at the start of this, and they were shot down. The clean solution is to store a clean value.
But if you all want to revisit the shortcut, workaround-y solutions, then we can do so.
In reply to How would this be a 2 line… by sideways
You're assuming I know more about SVG than I do. I recognize those letters are used in that order, but I have no idea what a scaling transform is.
The type of 2-line change I mean is something like, at the start of SVG export, set spatium to exactly 25, layout the score, do your thing then restore the old spatium and re-layout the score. OK, four lines to do it that way. I could imagine other ways too, like maybe calculating a different pixelRatio like we do for PDF. Maybe there is some reason that wouldn't work, but you have to realize, we are not intimately familiar with the details of SVG export as you are. So it's really hard to comment without some really good explanations.
The other type of 2-line change I refer to has to do with how the spatium value is stored in the file. So, maybe you store it in point, but why not continue to hold it in memory we always have? So, a couple of lines to convert to points on write, a couple of lines to convert back on read (and if you want, detect a value sufficiently close to 25 and force it there).
Again, without knowing more about the problem, it's really hard for me to see why these simple fixes don't cut it. I'm sure there are reasons, but there is pretty much no way someone who doesn't already understand the problem the way you do is going to understand without more explanation.
In reply to You're assuming I know more… by Marc Sabatella
Your second type of 2-line change is definitely plausible, and is one of the available shortcuts.
I am trying to build the master and give you example SVG files with and with the scaling, but I'm having trouble building the master right now...
Also, this discussion was initially targeted at Dmitri, who I know had objections/questions, and a few others that I invited via the developers' chat. So you're an unexpected, but not unwelcome addition.
In reply to Your second type of 2-line… by sideways
Fair enough. I figured since you were asking in the forum you were looking for broader input.
In reply to So again I'm back to not… by Marc Sabatella
Here is a shortcut solution I proposed almost a year ago, and that you (Marc) shot down in the next comment (see the bottom of the linked comment).
https://musescore.org/en/node/108196#comment-835327
The ability to restore the default is important too, not just the ability to prevent it from changing.
Is this "reset button" implementation more agreeable to you now than a year ago?
In reply to Here is a shortcut solution… by sideways
I wouldn't say the phrase "I'm not totally comfortable with the idea" counts as "shot down". It's an opportunity to explain further, but instead, you said "I don't disagree". And as I said then, "this sort of stuff is outside my area of expertise". So again, it is an opportunity for you to explain clearly and concisely what the problem is. A year ago was a lifetime ago in MuseScore terms.
One thing that is different now than a year ago - we now have reset buttons on virtually every style setting in Format / Style, that returns things to either baseStyle (the built-in style defaults) or defaultStyle (whatever you might have set as default in Edit / Preferences / Score). If your proposed reset button behaves similarly, then to me it's a no-brainer.
In reply to I wouldn't say the phrase "I… by Marc Sabatella
The proposed reset button would set the spatium to the default value, in the user's units. Internally there is code that knows the converted default values in every unit and forces SPATIUM20 instead of trying to convert them.
A lot has changed too in that I built a mini infrastructure here to deal with unit conversions. That part of the PR allows it to use any units and operate as cleanly as possible within the constraints that exist.
Backing out the file format changes leaves the rest of the PR intact, though it's not a clean solution for the future. But you all are the gatekeepers, so I defer to your judgements.
In reply to Sorry, still not clear to me… by Marc Sabatella
>why can't SVG export simply set reset the scaling (and round as needed) during export?
That would require doing two full layouts of the entire score, not just the part visible on the screen. 1 layout to revert to SPATIUM20. 2nd layout operation to revert to user spatium value. That does not sound like a good idea to me.
In reply to >why can't SVG export simply… by sideways
Also, if you are exporting a score with a non-default spatium value, then your page size is set relative to your modified spatium. If your score is setup for paper, and all you want to do is create an SVG version of it, then the SVG export cannot use the default spatium value, it must use your custom value. That is a common way to use SVG Export, or so I would imagine.
My request is non-intrusive in this regard: Just let me maintain a clean default value, somehow, some way. I will leave custom spatium values untouched. In general, scores created specifically for SVG will use a custom page size and Points as their units (until MuseScore changes its internal resolution calculation...).
In reply to >why can't SVG export simply… by sideways
The double layout wouldn't bother me in the slightest, I believe we already do this for several other export operations. Maybe even normal saves in some cases (continuous view?)
In reply to Wouldn't bother me in the… by Marc Sabatella
OK, that's interesting to know, but as mentioned in another reply of mine, ignoring the user spatium value is not a viable solution. That value must be maintained for scores that set it intentionally. This is only about scores that deviate from the default spatium unintentionally.
As far as I understood, the issues mentioned in the recent discussion are the following:
1) Lots of
transform
attributes in SVG files in case spatium has a non-default value;2) Rounding spatium and page layout parameters in case the page dialog was just opened and closed with OK button;
3) Rounding spatium in case of usual file save (though I didn't get the point here).
If I didn't miss some more topics, I'll try to answer to those I mentioned above.
Transform attributes in SVG
First, if the main problem is in SVG export, then it would be natural to look for the solution that affects only SVG export alone without changing all other things without a need. Concerning the transforms themselves, as far as I can see, all or almost all of them are placed in the elements (only
<path>
?) that do not have children so the transform applies only to the current element itself. Could we perhaps instead of writing thetransform
attribute apply the needed transform to that path element and write down the transformed coordinates instead? That should solve the issue of extratransform
tags, if it is so critical for SVG size and reading performance. Or am I missing something here?Rounding spatium and page layout parameters in case the page dialog was just opened and closed with OK button
That is what I have already mentioned previously, this is purely a UI issue. Just make the dialog not change the unchanged values and the problem should be solved. If the available precision for inches is not enough we can just add another digit to the dialog fields.
Rounding spatium in case of usual file save
Could you please explain which kind of rounding happens here? It seems to write the stored values with the precision available for
qreal
(that is, double precision on the targeted platrofms), maybe with rounding at some smaller digits, but errors of such kind are probably negligible. Or do you mean something else, or have some examples where such rounding causes issues?In reply to The discussion seems to have… by dmitrio95
Rounding caused by file save
Yes, the rounding might be negligible, except that this is a binary == situation, not a fuzzy match or "close enough". 5 / 72 x 25.4 has endless decimals, and Qt rounds it to 5 decimal places when converting to text for XML. I can make it a pseudo-fuzzy match and we can continue to store the spatium in millimeters. That is not the clean solution, but it avoids changing the file format at this time.
Transform attributes in SVG
SVG Export should not have to work around something that is a perpetual error by the rest of MuseScore. Any such workaround would be a major change to the SVG code for something that should be corrected elsewhere. The better solution is to maintain a clean default spatium value, if the user so desires.
Again, this is critical for scores specifically laid out for SVG export. Not a serious issue for a paper-printed score that wants to export to SVG. And SVG must maintain the user spatium in scores that intentionally set a non-default value. The scaling is relative to the page size in those cases.
Page Settings Dialog rounding
As I stated in a previous response, this is a separate issue that the PR improves, but does not fully fix. The only critical part is the rounding of the spatium itself when the user wants the default value of SPATIUM20. Separate from what units are stored, this code is in pagesettings.cpp in the PR and can stay there. It's now a non-issue as far as the file format is concerned.
In reply to The discussion seems to have… by dmitrio95
On a side note: given that layout is a performance issue in MuseScore 3.0, has anyone considered optimizations around the default spatium value? For the vast majority of users that use the default spatium value, the layout calculations might be simplified by not scaling. Non-default spatium values would run the not-optimized version of the mathematical formula.
It might not be a significant improvement, seeing that it's just one multiplier gone, but there are a lot of times that multiplier is applied in laying out a full score.
I'm simply looking for other reasons why maintaining a default spatium value might be useful in MuseScore code.
In reply to The discussion seems to have… by dmitrio95
> Could we perhaps instead of writing the transform attribute apply the needed transform to that path element and write down the transformed coordinates instead?
MuseScore + Qt uses
translate()
to position every element on the page. Each element's default coordinates are generally 0,0. Then there is atranslate(x, y)
to position the element on the page. That's howSvgPaintEngine
receives the data, and it matches the way MuseScore/Qt handles it in all thedraw()
andlayout()
functions. I believe that the element's coordinates are style-based and modified by the user in the inspector. The translations are for the position on the page, separate from user adjustments. That's my understanding of the "division of labor".Because every element is translated, the current code does exactly what you suggest with addition, not multiplication, in order to eliminate these
translate()
transforms. See the bottom half ofSvgPaintEngine::updateState()
starting here:https://github.com/musescore/MuseScore/blob/6860f9ef97816308a2a5dbaf764…
That is some older code of mine and I'm now seeing the incorrect indentation, sorry for that! The original code is pure Qt source, and I inherited a modified version of that.
Whenever an another transform is required, in addition to the
translate()
, the code switches to a fullmatrix()
transform in order to make the multiple transforms straightforward. Qt already stores this data as a 6 element matrix, so it lines up perfectly. Qt and SVG are set up almost identically for transforms.When there is a non-default spatium value,
SvgPaintEngine
receives a scaling transform for every element, in addition to the translate. So somewhere in the MuseScore code there is ascore->spatium() / SPATIUM20
calculation that is passed into this matrix. I didn't create that setup, I just made it work for SVG exports.I don't want to completely rewrite this code just to deal with an alternate spatium value, especially when that alternate value is created by a MuseScore rounding error, not a user choice. Yes, for alt spatium values there might be a way to integrate the scaling into the path's points, but that's certainly not critical. I'm also less inclined to multiply the x/y values than simply add or subtract from them. Doing both addition and multiplication is potentially problematic.
The other issue is dealing with path commands that are not straight lines, like arcs and bezier curves. I don't believe that straight multiplication would scale those in a straight manner. Some of the arguments to those commands might have to be divided, not multiplied, or might need other special treatment.
In case you are not aware of the history: Ever since v2.0 MuseScore exports each font glyph as a path. The reason is so that the viewer is not required to have the MuseScore (or whichever SMuFL font) installed. My own code exports these as text elements, not paths, and uses an external .css file to style them. That's why I might get confused here and refer to fonts and font sizes in the SVG file itself.
In reply to > Could we perhaps instead… by sideways
Another solution you might be considering, after reviewing the code in my previous comment's link:
This SVG Export code is already rounding, why not fix this issue there? specifically here:
https://github.com/musescore/MuseScore/blob/6860f9ef97816308a2a5dbaf764…
Because this rounds to the same 3 decimals as the spatium input in the page settings dialog box. If the users changes their spatium from 1.764mm to 1.765mm, that is a scaling factor of 1.001. The rounding in this code approximates the rounding in the user input widget, so it eliminates only extraneous floating point garbage.
The unintentional rounding of the spatium can exceed this 0.001 rounding. For example, the 1.753mm value that results when you switch to Inches in the page settings dialog. That creates a 0.994 scaling factor.
In summary, fixing this on the SVG Export side is not the solution. The solution is maintaining a clean default spatium value when appropriate. That's not a big deal, that part of the code. The issue of file format change is a big deal, and that's the core of this debate. You asked about solving this in SVG Export, and we're getting deep into the code now, so I'm communicating in detail now. Let me know if I've gone too far :)
In reply to Another solution you might… by sideways
> The unintentional rounding of the spatium can exceed this 0.001 rounding. For example, the 1.753mm value that results when you switch to Inches in the page settings dialog.
So, as far as I understand, the
transform
attributes issue is again the result of the problem of the changing values on inch-mm switch in the page settings dialog? If so, as mentioned above, it should be useful to fix the dialog so that itThis should be able to fix the problem for the default spatium, and this does not require changing a file format in any way. When the time comes to making more substantial changes to the file format (maybe indeed for MuseScore 4 or so?) using integer or some real values with well-defined fixed precision should certainly be considered but I don't see why it should be necessary to do it right now. Moreover, such a file format change alone seems to be unable to fix the issues that you have mentioned. In the same time, making such changes along with the latter file format changes should make the transition much clearer (for example, we wouldn't need to introduce duplicating tags for a compatibility).
So my proposal is to fix the page settings dialog to make it behave more predictably and not spoil the existing settings and to postpone the file format changes to the time when such changes will be both more appropriate and safer to implement. If my understanding of the problem is correct, changing the file format right now is not necessary to fix the existing issues and still, as any changes of such a scale, is somewhat risky to do.
> On a side note: given that layout is a performance issue in MuseScore 3.0, has anyone considered optimizations around the default spatium value? For the vast majority of users that use the default spatium value
Concerning this note, I believe that a large part of scores is created with non-default spatium values although I didn't do any assessments on that. Any score that contains relatively large number of instruments should alter its spatium value just to fit all instruments to the page. For this reason many of the default score templates contain altered spatium values.
In reply to > The unintentional rounding… by dmitrio95
> So, as far as I understand, the transform attributes issue is again the result of the problem of the changing values on inch-mm switch in the page settings dialog?
No, the rounding happens when you save the file, because of the conversion to Millimeters and the rounding to 5 decimals. The rounding error gets worse when you use the dialog and round further, but the rounding caused by saving the file is enough to cause scaling.
On the other hand, as I mention in my comment below here, the file format change can be postponed, and the duplicate tags avoided.
In reply to > So, as far as I understand… by sideways
my mistake: The rounding to 5 decimals when initially saving the file is not enough to cause scaling with the current code that rounds the scaling factor to 3 decimals.
In reply to > The unintentional rounding… by dmitrio95
Maybe now you can understand why I was upset when I realized I had missed the v3.0 release deadline. This would be so much cleaner without having to duplicate the tags or wait to change the file format. This issue arose 3 years go, and I have tried a few different ways to solve it in the master branch. I gave up for a while, and that's when 3.0 was being finalized. Oh well, that's how it goes sometimes.
In summary, on the core topic of file storage units:
Simply changing the file format to store integer values would be an improvement, regardless of which units are used. I think the core question is: Do we make that improvement now? Or are the file format changes too much trouble in the middle of file format version 3.01, and it should wait for version 4.00?
If the answer is No, then I'll adjust my code and remove my commit with all the mtest changes. If the answer is Yes, or if you want to plan for that future change, then we can discuss Points versus Millimeters and Inches.
Regardless, my PR solves the spatium rounding issue(s) and improves the page settings in general. No matter how this is resolved, the PR can move forward.
In reply to In summary: Simply changing… by sideways
Regarding making the file format changes in 4.00:
In case I am not available to make the changes when the time comes, I can leave comments and commented out code that can be uncommented at that time, or I can just leave it to be coded in the future.
What is the preferred way to go?
So far today I'm moving all the 3.01 code into read301.cpp, and setting up style.cpp for 4.00, but commented out.
In reply to Regarding making the file… by sideways
I believe having a commented out code is not a good solution in general. I believe it would be better to leave your code available in the current PR or some separate branch so we could refer to it later. Other issues (page settings dialog, maybe the spatium default value handling and others) can be addressed within separate pull requests which can be reviewed and merged separately. That looks like the most efficient way to go for me.
In reply to I believe having a commented… by dmitrio95
I'm working on a solution to this. I should have my current PR without the file format changes working later today.
In reply to I'm working on a solution to… by sideways
So am I correct that you will make a pull request that contains only the page settings dialog fix? And will your all code be available somewhere?
Also it might be better to make those smaller changes in new separate PRs so it would be more convenient to review and merge them.
In reply to So am I correct that you… by dmitrio95
The page settings dialog changes are intertwined with preferences dialog changes, and require several enhancements. It's hard to break this into pieces. I will look into breaking things out, but I don't expect to find much that is worth isolating.
It is also one big lousy hack if I don't add the new pageSize, pageUnits, and pageOrientation tags to the file. It makes no sense to create a PR without adding those tags, IMO. So the PR will continue to have a separate mtest commit and over 500 files changed.
This is a non-trivial set of changes that go together.
For now I'm sticking with the same branch in the same PR. If I find issues I can break out sensibly, I'll do so.
As I said on the chat, if not writing the spatium to the file for default values is critical to separate, I can do that one. But I don't see it as a big deal of a change. The read side of the code has no issue at all, it will simply stay with the default value. The MStyle::save() code is 2 lines, and very similar to the old code.
In reply to So am I correct that you… by dmitrio95
My understanding of the result of this forum topic's discussion are:
1) I will remove my duplicate, forward-compatible-only tags and styles.
2) Until file format 4.00, there will be no change in the way any of the existing page settings Style tags are stored.
It's a significant set of changes to my code, but it doesn't change the whole PR. The page settings dialog has several new features that coincide with adding Page Units to the preferences dialog. That has global repercussions that make it harder to break into pieces. It also solves a bunch of problems with one design change. Approaching this from the fix-one-bug-at-a-time perspective ignores the possibility of a design change and enhancements that solve a group of problems a different way.
See here for the original enhancement request. This comment has been there since at least the beginning of 2.0, probably earlier, but I can't find the v1 code anymore:
https://github.com/sidewayss/MuseScore/blob/380cf50f45dd22bf79466a57db3…
I can break out the changes to the edit style dialog, the new buttons. But all that does is remove 2 or 3 files from the PR. None of those changes overlap with anything else. It's not helpful for the history, though it reduces the quantity of stuff to review in one single PR, if that's really helpful now. I'll keep thinking about things to break out, but I still don't expect much.
In reply to My understanding of the… by sideways
OK, I think I can break out something significant, at least 2 PRs, the first one without mtest changes.
It's going to be hassle for me, but if it makes it easier for you then maybe it will go faster in the long run. I probably won't have it ready before tomorrow morning. When I've pushed it I'll post some details.
Will I be able to create the second PR before the first one is merged, if it depends on the code in the first one? I'm trying to figure out how to stage these changes. It's not a matter of just removing/adding lines of code. Each PR will require some custom code to make it work.
In reply to OK, I think I can break out… by sideways
> Approaching this from the fix-one-bug-at-a-time perspective ignores the possibility of a design change and enhancements that solve a group of problems a different way.
Sure, but the current PR version contains a lot of changes that are loosely related or unrelated at all, like both changing UI and fileformat, adding something to preferences and building some page format tables into libmscore. Even if they are parts of some more global redesign and cannot really be considered completely separately, they should at least be in separate commits. Right now it is hard to understand why some of these changes are needed at all, just because we see a one large patch that changes all at once without seeing what was actually meant to be done and which logical steps were taken to do that. If some part of your changes depends on another one, you can try making it a separate commit within the same PR, it will anyway be a good step towards the possibility to understand and discuss your changes, both now and in the future. Of course, if some of the changes can be harmlessly extracted completely out of the single PR, it is still better to do so.
> It is also one big lousy hack if I don't add the new pageSize, pageUnits, and pageOrientation tags to the file. It makes no sense to create a PR without adding those tags, IMO. So the PR will continue to have a separate mtest commit and over 500 files changed.
As far as I understand, most of mtest files contain just
<spatium>
tag changes, and I wasn't able to find any<pageUnits>
tags within that PR's changes (although I only tried to find them by searching through the changes shown by Github). Did I miss some changes? And why are these new tags needed for the UI redesign?In reply to > Approaching this from the… by dmitrio95
Be happy: I have split out a branch without mtest changes. It builds and I have done some testing, but I want to test further before I create a PR. It's getting late and I want to look at it with fresh eyes in the morning too. I should be able to create the PR by tomorrow afternoon, my time (GMT-6).
>Sure, but the current PR version contains a lot of changes that are loosely related or unrelated at all, like both changing UI and fileformat, adding something to preferences and building some page format tables into libmscore.
These changes are very much related. You just haven't taken a deep enough look at the code. Hopefully this simpler PR will make that a better experience for you, and we can move on.
In reply to > Approaching this from the… by dmitrio95
> As far as I understand, most of mtest files contain just
<spatium>
tag changes, and I wasn't able to find any<pageUnits>
tags within that PR's changesThat's because mtest doesn't change any of these 3 settings from the default, so they don't show up in the mtest files. I'll put these in a second PR that is still 3.01 compatible, but solves a couple more issues and is generally a good thing.
darn it's hard to get greater than and less than signs, aka angle brackets, to show up here.
In reply to > As far as I understand,… by sideways
You can enclose XML code in
<xml>
tags to get it displayed:<xml><somexml>< /xml>
(without spaces, of course)In reply to Hint: you can enclose XML… by dmitrio95
OK, thanks! I had resorted to the
<code>
tag combined xml entities like<
. That's what has worked for me when quoting code that uses pointers, likescore->spatium()
. But this is easier.Also, I was mistaken yesterday:
Even if I add these styles to the code, with the current set of mtests, there will be no changes to the -ref files because of exactly the reason I mentioned two comments ago: mtest doesn't change these settings from the default. So mtest changes are out of this round of PRs, and will only happen if/when the v4.00 file format changes are made. Nice, huh?
The first PR will not include these 3 styles. After sleeping on it, and after accepting the extra work it involves, I think I can break this original PR into 4 or 5 smaller PRs for v3.01. v4.00 would obviously be a separate PR. I'll post here when the first PR is ready, hopefully later today.
In reply to OK, thanks! I had resorted… by sideways
Oh no, yet another mistaken statement...
When I change the spatium to be stored only if it's the non-default value, that will change all the mtest files.
I will make that the final PR.
Here's a novel idea:
Treat the spatium like any other style and don't write it to the file if it equals the default value.
Why is the spatium treated differently and always written to the file? There is even a comment in the code about special handling, but no explanation of why:
https://github.com/sidewayss/MuseScore/blob/380cf50f45dd22bf79466a57db3…
In reply to Here's a novel idea: Treat… by sideways
Maybe special handling refers to the fact that we shouldn't just write the raw spatium value but convert it to millimeters first. Other than that, I don't see any reasons to handle it differently.
In reply to Maybe special handling… by dmitrio95
This is something that should be changed and normalized. Unfortunately it also requires regenerating all the mtest -ref files. It is something that I will try to make part of the file format changes piece of my code that is archived or however I sort that out.
In reply to Here's a novel idea: Treat… by sideways
In my opinion you should forget the idea of a default spatium value.
Real live examples:
A value about 1.6 - 1.7 mm is used for piano music;
Henle - Mozart Sonatas slightly larger than
Schirmer - Mendelssohn Songs without word or
Peters - Beethoven Sonatas.
For choral music often smaller are used, e.g.:
Breitkopf - Elias about 1.4 mm
Bärenreiter - Schütz Musikalische Exequien about 1.4 mm
Peters - Handel Messiah about 1.25 mm.
In reply to In my opinion you should… by Nikolaus Hold
Software is not paper. It does not natively measure things in millimeters. Default values are necessary for most everything. There are also several specific software issues discussed in the comments above.
I didn't quite make it to a PR yet, but I'm very close.
If you want to look at the branch as it is, here is a comparison to master:
https://github.com/musescore/MuseScore/compare/master...sidewayss:units
There will be mtest -ref file changes. You can see what those changes will be by reviewing diff errors at the top of the Travis log here (I have Travis setup on my repo):
https://travis-ci.org/sidewayss/MuseScore/jobs/518557424
It's not nearly as many mtest -ref files as before. All the mtest failures are diff failures, and all but three of those are justifiable as improvements. They fall into these categories, most common first:
1) Spatium values of 1.76389 versus 1.764. The new values are more precise. This is not a problem.
2) Missing values in the new files. This is a good thing. It is because default values are being recognized better and thus not saved to the file.
3) In two tst_split files for pagePrintableWidth: more precise numbers due to less rounding. I see this when I run ctest locally, but I don't see it in the Travis log. Otherwise Travis and local ctest seem to line up.
There are three other single-line diff failures that I want to look into further. They are in compat114 and compat206, so it's only the old file formats. They look simple enough to figure out, but it's getting late, and I'll be back in the morning.
In reply to I didn't quite make it to a… by sideways
The first compat114 failure is in textstyles.mscx. The new code adds:
<pageTwoSided>0</pageTwosided
This is correct, and a small bug fix. The current master is mistaken. The source file has this entry indicating that two-sided is false:
https://github.com/musescore/MuseScore/blob/3232ff32a8899d6715dd3c21d00…
The second compat114 failure is in chord_symbol.mscx. The new code adds:
The width/height are correct, and another small bug fix. The source file is in Letter size:
https://github.com/musescore/MuseScore/blob/3232ff32a8899d6715dd3c21d00…
However on Travis, the pagePrintableWidth is set as if the margins are zero, which they are not. When I run mtest locally this is correct, so the issue is only in Travis, unfortunately. Zero margins are my other Travis bug/issue, so it's probably related, and I can work around it similarly. This margins are stored correctly in the file, it's just the pagePrintableWidth, which is calculated using left/right margins.
The third failure is in compat206, in hairpin-test. Exactly the same issue as the second one, in the previous paragraph. Zero margins used to calculate the pagePrintableWidth, and only on Travis.
This is generally good news. My code is working as it should, except on Travis. I'll make my mtest -ref file changes in a separate commit in the PR.
In reply to The first compat114 failure… by sideways
That is mtests run in test mode: there paper format it ignored
In reply to That is mtests run in test… by Jojo-Schmitz
No it is not. Show me the code where you think this is happening.
On the other hand, Travis is doing at least 2 things differently:
1) An apparent bug I have to workaround where a QPageLayout's margins refuse to be set to anything other than zero.
2) Calculated floating point values are sometimes different. That might just be normal, and my response is to do my own fuzzy comparisons of the floats.
In reply to No it is not. Show me the… by sideways
I did show you, in the developers chat.
In reply to I did show you, in the… by Jojo-Schmitz
That's not relevant for opening and saving files.
In reply to I did show you, in the… by Jojo-Schmitz
This is an issue because of diffs between reference .mscx files and freshly generated .mscx files. Those files are generated by opening and saving another set of source files. It's what happens between opening and saving that is the issue. The 3.05 master does not have any testMode code for that. I have put code in my branch that checks for testMode and works around these Travis issues.
In reply to I did show you, in the… by Jojo-Schmitz
The code you pointed to is for ignoring user default styles that are different from the base style. It forces the testMode user into a 100% baseStyle session.
My branch passed Travis last night, right before my internet connection went down, so I left this message until this morning. It took me all of yesterday to re-workaround Travis issues for this branch, which has some additional quirks in compat114 and compat206 mtests. It took so long largely because for each attempt I have build and run mtest locally and then run Travis. Each test cycle consumes an hour.
I'll be testing, and then creating the PR later today - definitely later today :)
I'll post the PR link here, and in the original PR's comments.
In reply to My branch passed Travis last… by sideways
The PR is here: https://github.com/musescore/MuseScore/pull/4906