SVG export in 3.0-dev incorrectly leaves out many measures of staff lines
On armv7 arch linux. SVG export of attached .mscz seems to work fine on 2.0.3 AppImage, however on 3.0 dev AppImage e8a2d43 it incorrectly leaves out many staff lines.
Disregard many of the comments below...I got distracted with something I did wrong in 2.2 in my own branch which was my own fault and was apparently unrelated the svg export bug in 3.0-dev.
| Attachment | Size |
|---|---|
| Willow_Weep_for_Me_3.0.0-AppImage.svg | 760.95 KB |
| Willow_Weep_for_Me.mscz | 25.13 KB |
| Willow_Weep_for_Me_2pt0pt3-AppImage.svg | 782.32 KB |
Comments
interestingly, 3.0 export has some of the staves endined too soon, but there are no lines that are out of place.
So just an update...strangely things work fine in 2.2 when I build musescore via command line via:
make && sudo make install
but they don't work with qtcreator debug build...
update: so it works via make && sudo make install, but doesn't work with just plain make (and running mscore from ./build.release/mscore/mscore)
So apparently installing somehow fixes the glitch.
You should never be attempting to run MuseScore without installing. Quite a few things will be known not to work correctly, and probably plenty of others won't work either. Not really a bug. Although since this isn't something already known not to work, it could be worth investigating a bit just to be sure it's to be expected if a user fails to install and that we understand why.
In reply to You should never be… by Marc Sabatella
ok. I do understand that should be installing. However the qtcreator debug build does install it but still fails. And the AppImages were failing. So the issue is not specific to not installing, but is somehow closely related to that...haven't nailed it down yet.
I got distracted with something I was doing in 2.2 on my own branch, so any of my comments about 2.2 are irrelevant. BUT there is still a bug in 3.0-dev, so I've updated the title accordingly.
I think it comes from the fact that in MS 3.0 svg the page size is 5 times larger (and everything is scaled up) than that of the svg exported by 2.0.3. This is a consequence of the dpi multiplicative factor:
https://github.com/musescore/MuseScore/blob/4ce82112ae9c6b4d0e1e33004ce…
I think that the measure lines are drawn only for the measures starting inside the page 1/5 of the svg effective page (i.e. inside the MS 2.0.3 page size).
For example in the attached examples:
MS 3.0 svg size (opened in Inkscape): 787.7mm x 1113.5mm
MS 2.0.3 svg size (opened in Inkscape): 157.5mm x 222.7mm
Maybe a solution (if the source of the bug is this one) could be scaling the pixel of the QPainter
https://github.com/musescore/MuseScore/blob/4ce82112ae9c6b4d0e1e33004ce…
by a factor DPI_F, or the printer page size or viewbox (I can't check at the moment if these are set accordingly to the pixel size).
EDIT: DISREGARD my comment here...it was mis-informed...I tried returning that scaling factor and it didn't fix it:
Possibly (but proably not) related is the fact I get a lot of "Unhandled Metric 12" error messages from:
https://github.com/musescore/MuseScore/blob/5507d256b32c0a7aa4d758bcc94…
Which doesn't handle case 12 "QPaintDevice::PdmDevicePixelRatioScaled" which http://doc.qt.io/qt-5/qpaintdevice.html says is:
"The scaled device pixel ratio for the device. This is identical to PdmDevicePixelRatio, except that the value is scaled by a constant factor in order to support paint devices with fractional scale factors. The constant scaling factor used is devicePixelRatioFScale(). This enum value has been introduced in Qt 5.6."
https://github.com/musescore/MuseScore/pull/3299 could be relevant.
In reply to I think it comes from the… by ABL
As the author of the original zero-scaling code here, let me add a few points:
- The last time this code was touched was prior to the 5x change, well over a year ago now. The intention was to do zero scaling for SVG, none whatsoever. I had another PR which completed this effort, but which was rejected. It added pixels as a unit of measurement in the Format:Page dialog too, for complete integration of pixels as a unit (pixels are the base unit of SVG).
- Then the 5x got added. Does it matter? There's no clear answer to that, as there is no clear best implementation of a general initial size for an SVG. By size I mean the scaling relative to the viewBox, the width and/or height attributes on the svg element itself. My intention was to make everything the same and let the .msc(z) file determine the size. The 5x changed that, thankfully through a simple multiplication factor.
- Since then I have been occupied with other aspects of my own project, and have not kept up-to-date with 3.0 changes. My own version of 3.0 uses a pre-5x master for now, and is thus totally out-of-date.
- The issue of initial size, the value for the width/height attributes relative to the viewBox, is certainly worthy of discussion, and there is no single correct answer or implementation.
I hope that adds some useful context to this thread. If you want to multiply by the 5x factor, that's certainly a reasonable idea. There are many other conversions going on between units, so be careful. I was trying to eliminate all that converting with my zero-scaling and px as units PR, but even that could be integrated easily with a simple 5x. Just be sure you're getting what you want out of it. It's just initial size. The display container for the SVG is what will actually determine the display size on screen. Printing is a whole other matter. The absolute size of the SVG really needs to be determined by the final output, there is no magic bullet, perfect initial size, IMO.
well I just spent a long time trying to unify the SVG & PDF line printing code...although I'm not sure if I've made the code better. Might be simplest to just remove the SVG optimization instead.
In reply to well I just spent a long… by ericfontainejazz
Please don't remove the SVG optimization, it makes a big difference to the file size and readability.
In reply to Please don't remove the SVG… by sideways
Ok. I have a question, do you think the optimization would also benefit PDF export (due to fewer line elements in the PDF). If so, I might continue on with my attempt to unify the two printing functions.
In reply to Ok. I have a question, do… by ericfontainejazz
It certainly won't hurt the PDF export. It is especially helpful for wide scores, and essential for piano-roll-style horizontal scrolling. I assume you understand the basic idea: MuseScore draws staff lines by measure, the optimization draws them by system.
It's probably more important for SVG because they are text files and are often modified in a text editor. Thus readability and ease of manipulation are factors. They are also interpreted by web browsers and other clients, so file size is probably more critical than for PDF.
Well I almost have a fix (Work-in-progress) https://github.com/musescore/MuseScore/pull/3512
"printing a page follows same code strucutre regardless of SVG, PDF,
thumbnail, etc. This unifies the code between these print modes a
bit, and that helps out with keeping consistent behavior in the output.
Previously there was a bug with SVG output because it was trying to use
an optimization whereby staff lines were extended if all systems in a
measure were visible and weren't broken by frames. However, that
optimization had a mistake, whereby only the first measure of staff
lines was drawn. By unifying the print function, then also PDF can
benefit from the optimization. I've also improved the optimizatino a
bit to also handle all continuous measures even if the entire staff is
unbroken. I still need to investigate a bug with MMRests, though."
It works with the following attached test case, correctly merging any contingous staff lines for pdf & svg, although not when I have MMRests.
In reply to Well I almost have a fix … by ericfontainejazz
"staff lines were extended if all systems in a measure were visible and weren't broken by frames"
Not accurate, and I'm not sure if it even makes sense. Maybe you typed it wrong. "all systems in a measure"? - I have no idea what that means.
Here's what the optimization does: It attempts to draw one set of staff lines for each system, as opposed to drawing them by measure. If there is an invisible measure or invisible staff lines for a measure, then it abandons the optimization for that system, and draws the staff lines for that system by measure. Staff lines by system versus staff lines by measure.
That's a bit wordy and repetitive, but it's accurate.
In reply to "staff lines were extended… by sideways
Regareding MMRests, are you using the MM measure functions to iterate over the measures in each system? I just peeked at your file.cpp changes and you're not using them. Try using MeasureBase::nextMeasureMM(). Though the problem is that because it's printing it's iterating over measures by system, not by segment, which is much more sensible. But you still might find a way to integrate MeasureBase::nextMeasureMM() within the System::nextMeasure() loop somehow, maybe as yet another check, like checking for visibility. You could check measure numbers or something. Sorry I don't have a clear solution, but I don't understand the details of the failure either.
Gosh, yes the existing master version of the optimization was as you describe. I meant to write that my new variant of the optimization is what I wrote: "staff lines were extended if all systems in a measure are visible and aren't broken by frames". I'll fix that message...I wrote it quickly as I was saving my progress.
Regarding nextMeasureMM, thanks for reminding me about that function...that is actually the type of function I wanted to use...since clearly systems->nextMeasure() is not doing that. (I'm also going to rename of System::nextMeasure to System::nextMeasureBase since that is what it more clearly does).
I'm raising this priority to critical simply because the current master produces a SegFault with my MM test mscx in the middle of writing the SVG file.
So I've fixed the MM rest bug I was experiencing. Basically by iterating with nextMeasureMM() in the for loop instead of just next().
I'll mark as ready for review, although to be honest I need to test this some more, maybe in a couple days. But at least my somewhat complicated test case works (attached).
In reply to So I've fixed the MM rest… by ericfontainejazz
Could you please add the reference with the related PR?
See https://github.com/musescore/MuseScore/pull/3512
I have a fix for this original issue; it does not include any unifying of page printing functions.
The issue here is that something changed and caused SvgPaintEngine::drawPolygon() to receive the points array argument differently:
https://github.com/musescore/MuseScore/blob/4c5e1e96a43bb0fdd866f9ce340…
This is only an issue for the cloned StaffLines object that is used to simulate one extra wide set of staff lines. The current/original file.cpp code that asynchronously calls drawPolygon() modifies the right edge of the StaffLines->bbox():
https://github.com/musescore/MuseScore/blob/4c5e1e96a43bb0fdd866f9ce340…
The resulting points[] in drawPolygon() used to contain lines that were automatically the width of the bbox QRectF. I don't know where that was happening, but calling bbox().setRight() updated the lines to be the width of the rect, at least as far as the paint engine is concerned. And yes, maybe it was the inheritance from QPaintEngine that created this auto-sizing of the lines inside the StaffLines object. I don't know, and at this points it's moot. It's not necessary to regress to that inheritance.
The fix is to set the width of all the lines inside the StaffLines object, and leave the bbox alone:
https://github.com/musescore/MuseScore/blob/fadcd86267251a9a7313ddb5ab7…
Because the lines member of StaffLines is private, I had to expose it via a new StaffLines::getLines() function in stafflines.h:
https://github.com/musescore/MuseScore/blob/fadcd86267251a9a7313ddb5ab7…
Yes, exposing the QVector of lines as a writable object is not best practice. But it's better than dealing with friend classes, and this is only being used on cloned StaffLines objects, nothing that is attached to a score. It's called getLines(), not lines() as many other modules do, because the member var is not named _lines. Also note that _lines is used as a local variable name in stafflines.cpp. Thus, the public function is called getLines(), and it returns a writable version of the lines member var.
The branch containing the above links is fully up-to-date as of this moment. I changed the status of this issue to Active just to change the status. I have attached current and fixed versions of sample exports, along with the .mscz file that exported them. This is the diff page for the branch versus the master, which I just updated:
https://github.com/musescore/MuseScore/commit/fadcd86267251a9a7313ddb5a…
Unfortunately my master has some unwanted commits that I am having trouble eliminating. I have done a git reset --hard upstream/master, but it hasn't eliminated this history. Once that is cleared up I can file the PR. (I have fixed this now, but will wait until it's time to make the PR to fix the branch or create a new one)
One important note: There is a big difference in the SVG file if I export from the debugger, or from a launched debug .exe. Running inside the debugger produces the correct results. Running outside the debugger produces the last attached file, z-1.svg. Everything but the lines (staff and bar lines) is way too big. Maybe 5x too big, but it's hard to do that math on svg path coordinates. This is a separate issue, but why is it happening? What is different about running inside the debugger versus running the debug .exe? This is an issue in the current 3.0 master, my changes do not impact this. I am running on Windows 10, if that matters.
I'm changing the status back to patch (code needs review), because that is the accurate status. I felt the need to churn this over one time to indicate that it's a new patch.
I will take a look at the patch. Thank you @sideways!
Relates to #274863: [EPIC] Importing <= 1.14 version scores to 3.0
@sideways, could you please make a PR with the mentioned changes?
Like https://github.com/musescore/MuseScore/pull/3512 ?
In reply to Like https://github.com… by Jojo-Schmitz
Sideways references different changes, afaics
In reply to Sideways references… by Anatoly-os
3 months later and I've almost completely forgotten what this is about. I have not been inside my MuseScore C++ code in all that time, so I need to fiddle a bit to make this PR happen. Let me look into this today. It shouldn't be too hard to set myself to the current master branch and copy/paste this bit of code in there.
Re: https://github.com/musescore/MuseScore/pull/3512
That's someone else's code, and attempts to "unify the page print functions", which I do not attempt.
In reply to 3 months later and I've… by sideways
Thank you! Looking forward to seeing your PR :)
PR is here: https://github.com/musescore/MuseScore/pull/3853
I haven't tested the functionality, but I did build the branch. The changes are tiny and I'm busy, so I hope that's OK.
I had to delete my previous branch and add a new one with what I thought would be the same name, but the name changed because github client won't let me put spaces in the branch name, so it's FIX#269870, all one word. I hope that's OK too.
Github is taking a long time to do the checks, so I'll take the risk in posting this comment, and assume that it all passes. That usually backfires, but this is such a simple pair of changes...
Fixed in branch master, commit c859eae4ce
Fix #269870: SVG export in 3.0-dev incorrectly leaves out many measures of staff lines
Automatically closed -- issue fixed for 2 weeks with no activity.