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

01 16 svg fixes #2338

Merged
merged 3 commits into from
Jan 10, 2016
Merged

01 16 svg fixes #2338

merged 3 commits into from
Jan 10, 2016

Conversation

sidewayss
Copy link
Contributor

This fixes MuseScore issues #92901 and #92921.
See here: https://musescore.org/en/node/92901
and here: https://musescore.org/en/node/92921

fix #92901
SVG Export: Invisible measures not supported.  Changes affect one block
of code in mscore/file.cpp.  Now if there is an invisible measure within
a system/staff, that system/staff paints its staff lines by measure.
fix #92921
SVG Export: capture tool malfunction
1 new line of code plus
1 modified line of code in
1 file: fotomode.cpp
@lasconic
Copy link
Contributor

lasconic commented Jan 8, 2016

Thank you for the clean PR. I just tested the capture tool. There is one remaining issue with the musical glyphs resolution. See how the clef and the note heads are pixelated.

@lasconic
Copy link
Contributor

lasconic commented Jan 8, 2016

The cutaway and invisible looks ok. You have some problems in your code indentation. I will comment inline

+ lastSL->pagePos().x()
- firstSL->pagePos().x());
printer.setElement(firstSL);
paintElement(p, firstSL);
Copy link
Contributor

Choose a reason for hiding this comment

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

paintElement should be aligned with printer.setElement

Idem for the constabove.

@sidewayss
Copy link
Contributor Author

Re: capture tool - Interesting. My test score looks fine. OK, I didn't zoom in to the point where I could see pixelation. Now I see it. I don't even know how it's possible with a bezier curve to create pixelation like that. But I'll look into it.
Re: indentation - Yes, I like to vertically align things around things other then the left edge sometimes. Examples are in various places in the code I've submitted.This is one of those cases, I'm right-justifying the paintElement() line relative to the line above it. If that really bothers you I can realign it. It's only two lines I'm trying to group here, so it's borderline for me anyway.

@lasconic
Copy link
Contributor

lasconic commented Jan 8, 2016

Capture tool. I zoomed it in my browser to highlight the pixel effect.
Here is a mscz https://dl.dropboxusercontent.com/u/497455/My_First_Score.mscz
(My computer is a macbook pro retina btw. It could be linked)

Indentation. This file is special because we inherited it from Qt so it doesn't follow our Coding Style Guidelines (which are already pretty weird, but it's historical...). I would prefer if it doesn't deviate too much, so if you could align all the lines in a block, it would be great

@lasconic
Copy link
Contributor

lasconic commented Jan 8, 2016

I need to go. You can fix the pixelation with Mscore::pdfPrinting=true. If you have no time to look at it, I will merge this PR tomorrow and fix this small bug and the indentation. I see that the indentation is also bad in file.cpp.

@sidewayss
Copy link
Contributor Author

Good detective work on the pdfPrinting=true (don't forget to reset it to false when you're done). But that line of code was missing prior to any changes of mine. I don't see the pixelation problem in 2.0.2, but your fix works, I just tested it. Maybe a separate problem was introduced in between 2.0.2 but prior to my changes? I don't really understand what MScore::pdfPrinting does, so this mystifies me for the moment.

Yes, I have applied this fix, and normalized the code with the code it essentially duplicates in file.cpp/saveSvg() - so changes in both files. But I'm not sure what the correct procedure here is with this branch and this pull request. Do I need to undo, then redo the two commits? Or add a new commit with these updates?

I made the following additional changes as well:
I was bothered by the capture's default title for the file "Qt SVG Document", or something like that. And all the code calling SvgGenerator set the document description to the same thing. So I changed the defaults inside SvgGenerator and nows it's all MuseScore, not Qt, and all correct.

One more thing: We both missed yet another problem spot in fotomode.cpp, ScoreView::fotoDragDrop().
I'm not 100% sure how to test it. What exactly is the user functionality? It always generates an SVG file, never any other type.
I did make basic, consistent code changes there too, so you can certainly test my semi-blind fix once I confirm the correct way to commit it. Or you can tell me how to test it prior to me committing these changes. If it all works, then the duplicate code can be consolidated in the next round of changes.

On a related note: When I received notification of your post about pdfPrinting=true, I had been trying to get class=Element::TypeName to work in captures (as opposed to exports). It's a bit dicey the way it's setup right now because the capture uses generic ScoreView functionality to "draw" the SVG. This same code is used to draw various other formats. So I'm not confident on short notice to make that feature work for capture in this round of changes, but I'm sure it's doable. I need to be able to use this line of code somewhere in there:
SvgGenerator.setElement(Element* e);
And right now I'm not sure how or where to add it so that I don't screw up everything non-SVG.

On a final note: feel free to make any additional indentation changes you need to. I fixed the ones you pointed out. I do prefer an unusual style of indentation and alignment, you can see it a lot in svggenerator.cpp. But now that I look at it, it's mostly within the line that I add extra whitespace, the initial indents are generally consistent. I'll keep that in mind as something to avoid. Please let me know how to proceed with committing these new changes and this pull request. I'll be in front of my computer tomorrow morning, in my more Eastern time zone from yours.

@lasconic
Copy link
Contributor

lasconic commented Jan 9, 2016

Just commit your changes in the same branch in a new commit and it will be added to this branch. For the commit message, just describe in one line what you did (fix drawing of musical symbol in svg for screen capture, fix code alignment)

(fotomode.cpp) fix pixelation of zoomed svg, document title, normalize
duplicate code with same code in file.cpp (file.cpp) normalize code
duplicated elsewhere, normalize code indentation (svggenerator.cpp) fix
default document title/description, change raw literal 72 to Ms::DPI
@sidewayss
Copy link
Contributor Author

OK. What about the DragDrop() functionality? Is that simply not used? I have now added the commit to the branch with a single line commit message describing all the changes.

And what's the deal with MScore::pdfPrinting? How did that fix the problem? Why wasn't it a problem previously? Or was it an existing problem, somehow revealed by the recent 72dpi change (assuming it was previously hidden by some other factor, like scaling)?

@lasconic
Copy link
Contributor

lasconic commented Jan 9, 2016

pdfPrinting will export the symbol as path, without it they are exported as bitmap. Check sym.cpp for more info.

DragAndDrop is used. Enter the capture mode, select a region, then hold Shift + Ctrl + Click on the region and drag somewhere else. It should make an svg capture of the region and put it in the score where you drag it. It seems that the resulting SVG is not the same size than the capture, and that we don't support viewbox correctly but I hope you get the point.

@sidewayss
Copy link
Contributor Author

pdfPrinting - that makes sense.

Hold on, I just inspected the dragged/dropped svg and I can scale it in the inspector. The Image inspector provides both % and fixed units for scaling . It doesn't solve the initial size issue, but at least it provides a workaround until a proper solution can be found and submitted in a separate PR.

I am reproducing your drag/drop size issue. This seems to have more to do with displaying the SVG than capturing it, at least mechanically. If you're happy with the size of the captured .svg files, then this is something to fix in the display of SVG files. Though I see no way to import SVG, so this seems like the only way to place SVG into a score within MuseScore. Is that correct?

Part of what I'm saying is that if a fix for Drag/Drop, as well as class = element type, can wait until a separate pull request, I would feel more comfortable about implementing it.

@lasconic
Copy link
Contributor

lasconic commented Jan 9, 2016

Though I see no way to import SVG, so this seems like the only way to place SVG into a score within MuseScore. Is that correct?

No, it's not. You can add any svg to the score or a custom palette by drag and dropping it to the score (or a new palette) See https://musescore.org/en/handbook/image-0

Yes, the PR looks fine. Let me take a fresh look tomorrow and I will merge it.

@sidewayss
Copy link
Contributor Author

Now I think I understand your statement about not handling the SVG viewbox attribute properly.
Is the external source drag/drop the same drop code as the internal source fotomode.cpp drag/drop?
Is copy/paste via menu/keyboard another way to place an SVG inside a score in MuseScore?
Both methods require an external app to display the SVG and copy it (keyboard or mouse). Just clarifying that point. I'm simply looking to understand the scope of the issue here.

@lasconic
Copy link
Contributor

Is the external source drag/drop the same drop code as the internal source fotomode.cpp drag/drop?

Is copy/paste via menu/keyboard another way to place an SVG inside a score in MuseScore?
No. I don't think that would work. You need to drag and drop a file from the file explorer of your system. Eventually, in MuseScore, you can right click a frame (like the title frame) > Add > Image and pick an SVG file.

Both methods require an external app to display the SVG and copy it (keyboard or mouse).
Not you don't need any third party app to display the SVG. MuseScore does that using QSvgRenderer from Qt.
https://github.com/musescore/MuseScore/blob/master/libmscore/image.cpp#L256

lasconic added a commit that referenced this pull request Jan 10, 2016
@lasconic lasconic merged commit b8f9558 into musescore:master Jan 10, 2016
lasconic pushed a commit that referenced this pull request Jan 10, 2016
(fotomode.cpp) fix pixelation of zoomed svg, document title, normalize
duplicate code with same code in file.cpp (file.cpp) normalize code
duplicated elsewhere, normalize code indentation (svggenerator.cpp) fix
default document title/description, change raw literal 72 to Ms::DPI
@sidewayss
Copy link
Contributor Author

Re: external app for SVG - I meant that you cannot currently import external SVG directly from a file, you must open it in an external app, then drag it from the external app to MuseScore. But now I see that the external app can be the file explorer, which in it's own way opens a file directly. Does it only work from the file explorer, or can I drag/drop from Inkscape or other SVG editing apps? I'm assuming that from your answer it's only from the file explorer, and given cross-platform compatibility I can see how that might be the simplest solution.
I don't know about Mac or Linux, but in Windows, Drag/Drop uses the same internal clipboard functionality as Copy/Paste as it relates to the actual dropping/pasting of the file, including drag/drop from the file explorer. That's why I bring it up

@lasconic
Copy link
Contributor

I meant that you cannot currently import external SVG directly from a file
I'm not sure what you mean by that. To me, you are able to import external SVG from a file. Right click a frame > Add > Image. A "Open Dialog" opens and you can pick an SVG file. It's similar for an explorer, what get copy pasted it's the path of the file. MuseScore does the SVG reading and rendering in both cases. It's again similar for fotoDragDrop. In this case, MuseScore creates the SVG as a (external?) temp file and then the file is imported in MuseScore "by path".

@sidewayss sidewayss deleted the 01-16-SVG-fixes branch January 11, 2016 12:49
@sidewayss
Copy link
Contributor Author

One minor pseudo-bug to report: current master, file.cpp line 2569

It doesn't affect the user experience, it's just not efficient or as-intended. "continue" should be "break", to exit the for loop entirely. In case the file changes and the above link becomes invalid, here is the context:

if (!static_cast<Measure*>(mb)->visible(i)) {
    byMeasure = true;
    continue;
}

@lasconic
Copy link
Contributor

Consider it fixed.
For the line number and file change, see http://andrew.yurisich.com/work/2014/07/16/dont-link-that-line-number/

@sidewayss
Copy link
Contributor Author

Thanks for the link.
Re: viewBox / capture - do you need any help with that? If it relates to SVG I'm happy to contribute.
Re: size of displayed SVG file, is it too small? Should it match (or at least approximate) the MuseScore screen display size? I posted about this at musescore.com, but no reply. It's a simple change to the viewBox/width/height attributes in the svg element. I would think it's a change that would be desirable, but maybe not. It's another viewBox-related subject, so I'm checking here...

@lasconic
Copy link
Contributor

Sure! It would be awesome if you could take a look to the scale of the
exported svg in capture mode. Not entirely sure if the problem is on import
or export but I believe it would be better to have the size than the score
by default. Just create another branch and another PR .
Le 20 janv. 2016 21:54, "sidewayss" notifications@github.com a écrit :

Thanks for the link.
Re: viewBox / capture - do you need any help with that? If it relates to
SVG I'm happy to contribute.
Re: size of displayed SVG file, is it too small? Should it match (or at
least approximate) the MuseScore screen display size? I posted about this
at musescore.com, but no reply. It's a simple change to the
viewBox/width/height attributes in the svg element. I would think it's a
change that would be desirable, but maybe not. It's another viewBox-related
subject, so I'm checking here...


Reply to this email directly or view it on GitHub
#2338 (comment).

@sidewayss
Copy link
Contributor Author

Can you direct me to where the code currently scales from 72dpi to the screen-display resolution? I understand it to be a conversion factor derived from the OS. Is there a global variable for this conversion factor? That would make all of this a snap.

And can you point me to the capture import/display code?

For capture to achieve the screen-display size there is definitely an import issue, as the capture display size is not the same as either the SVG default display size for that file, or the MuseScore screen display size. But to achieve the screen-size scaling, the export must be changed. That way the SVG capture import/display can be neutral and display any SVG correctly. This will also "fix" the svg export in general to generate files that are, by default, scaled to the screen-size in MuseScore.

I'm not saying I'll fix this today, but I'll look into it, and by next week I should have something posted.

@lasconic
Copy link
Contributor

OK. Let me sum it up. MuseScore uses SVG in many places so let's try to not mix things up.

1/ MuseScore can export the full score in SVG. As you know, the code path starts in MuseScore::saveSvg(). This SVG export should generate exactly the same SVG file on any platform except if we add a scaling factor like the PNG DPI defined in Preferences > Export > PNG/SVG resolution.

2/ Via the capture tool, MuseScore lets the user selects a rectangle area in the score and save it to SVG. There are two modes: the screenshot mode and the printing mode. In screenshot mode, MuseScore will save to SVG exactly the same objects that are display on the screen. In printing mode, MuseScore will not export non printable elements such as Line Break symbols, Frames border, etc... The user can also save as PNG when using the capture tool. For PNG, scaling depends on the DPI value defined with right click > Resolution and it's 300dpi by default. You will see that this menu also mention that this DPI is used for SVG. If it's not the case right now, then maybe it should. This is all coded in fotomode.cpp

3/ As an add-on to the capture tool, it's also possible to export the printing mode SVG to a temporary file and embed this temporary file in the score or in a palette. It's possible via Ctrl + Shift + Drag. It's also in fotomode.cpp` but in another function. This fonction will export the svg to a temporary file and initiate a drag of this file. For the drop part, see 4.

4/ I believe the 3 points above are the 3 ways to export SVG out of MuseScore. Now the import. It's possible to import an SVG via drag and drop from the system file explorer to the score or a palette. It's also possible to insert an SVG in a frame via the contextual menu Add > Image. And in 3/ it's possible to import an SVG from the capture tool via temporary file. SVG in the score or in the palette are Image as defined image.cpp, this class also deal with PNG and other graphical format import. This class does the drawing, resizing etc... and use Qt capabilities to decode the graphical formats. Image is a class derived from Element. Element is the root class for all elements in the score (a note, a rest, a dynamic, everything is an Element). The element type of image is Element::Type::IMAGE. When dropped, an image is added to the list of child elements of the drop target (for example the elements attached to a rest, or the elements on frame/box)

Hope it answers some of your questions.

@sidewayss
Copy link
Contributor Author

That definitely answers some of my questions. Thanks for the complete MuseScore SVG inventory!
Do you know where the code is to scale from 72dpi to the OS-dependent-dpi? If not, I'll fish around for it. With that conversion factor I can match the screen size via the ratio between viewBox=(0,0,x,y) at 72dpi and width=x height=y at OS-dependent-dpi.

@sidewayss
Copy link
Contributor Author

Nothing to post yet, but I see where the import problem is. I'll try to get to a fix some time soon. I need to complete something else first. I'll post it as a separate issue on MuseScore.org.

liang-chen pushed a commit to liang-chen/MuseScore that referenced this pull request May 6, 2016
(fotomode.cpp) fix pixelation of zoomed svg, document title, normalize
duplicate code with same code in file.cpp (file.cpp) normalize code
duplicated elsewhere, normalize code indentation (svggenerator.cpp) fix
default document title/description, change raw literal 72 to Ms::DPI
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.

None yet

2 participants