Bug 442268 - SVG images not properly displayed
Summary: SVG images not properly displayed
Status: CLOSED FIXED
Alias: None
Product: Sirius
Classification: Modeling
Component: Diagram (show other bugs)
Version: 1.0.0   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.0.0   Edit
Assignee: Pierre-Charles David CLA
QA Contact: Florian Barbin CLA
URL:
Whiteboard:
Keywords: triaged
Depends on:
Blocks:
 
Reported: 2014-08-21 10:07 EDT by Emmanuel Billaud CLA
Modified: 2016-06-24 08:02 EDT (History)
4 users (show)

See Also:

Attachments
Project and patch (7.10 KB, application/zip)
2014-08-21 10:07 EDT, Emmanuel Billaud CLA 		no flags Details
Correctly formatted patch (2.65 KB, patch)
2014-08-25 10:32 EDT, Emmanuel Billaud CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Emmanuel Billaud CLA 2014-08-21 10:07:55 EDT 
Created attachment 246207 [details]
Project and patch

On the diagram, at zoom > 100%, the rendering of a SVG file is equivalent to a PNG file.
I'm working on eclipse Kepler.

To reproduce this bug, I used the get-started project, with the current head of git repository (e4ee76).
Attachments :
* man.svg : the svg to copy in the 'icons' folder of the plug-in 'org.eclipse.sirius.sample.basicfamily.design' ;
* basicfamily.odesign : the design modified to take into account the SVG ;
* SvgProject : a basic project to show the problem ;
* SVG.patch : a proposition of correction.


What happens ?
In the 'paintFigure' method of the 'AbstractCachedSVGFigure' class (package org.eclipse.sirius.diagram.ui.tools.api.figure), the image to display is retrieve from cached images. But, the 'getClientArea()' method returns always the same value (zoom is not taken into account) : the image is always the same.

By considering the scale (getContextKey and getCachedImage), the image is good.
By changing the drawImage to call on graphics, the image is correctly positionned on the diagram (the former one did the scaling, so the image had a good size, but a bad 'resolution').

For me, this is a hack, and I have the feeling that getClientArea shall return a Rectangle with scale taken into account. Am I wrong ?

Note :
When exporting diagram as SVG file, the SVG images are drawn like PNG. It seems that the method in charge of copying the image (CopyToImageUtil.copyToImage(DiagramEditPart, IPath, ImageFileFormat, IProgressMonitor)) uses the 'paintFigure' method instead of using the SVG file. But this is GMF code.
Comment 1 Laurent Redor CLA 2014-08-25 08:55:11 EDT 
The patch is not well formated (inverse patch). To facilitate the review process, I wanted to create a gerrit, with you as author but it's not possible: 
"error: The author does not have a Gerrit account.
All authors must either be a commiter on the project, or have a current CLA on file.
Please see http://wiki.eclipse.org/CLA"

Is it possible for you to create this gerrit commit? It's not mandatory, but it simplifies the "patch" application to test it.

The problem of your solution is that the memory footprint can be strongly impacted (one image cache by SVG by zoom level).
Comment 2 Emmanuel Billaud CLA 2014-08-25 10:32:23 EDT 
Created attachment 246315 [details]
Correctly formatted patch
Comment 3 Pierre-Charles David CLA 2014-12-11 07:57:02 EST 
Related to this, it seems that GMF Tooling has a figure here:

http://git.eclipse.org/c/gmf-tooling/org.eclipse.gmf-tooling.git/tree/plugins/org.eclipse.gmf.runtime.lite.svg/src/org/eclipse/gmf/runtime/lite/svg/SVGFigure.java

which, from a quick reading of the code (I have not tested it) re-renders the SVG itself on each Draw2D paintFigure() call. This would avoid having to deal with a cache, at the cost of runtime performance. Only actual tests and measures on concrete cases could tell us which approach is best.

It may be possible to test this without touching Sirius itself by using the existing extension points to provide a custom edit part which uses this specific figure implementation.
Comment 4 Jessy Mallet CLA 2015-01-21 11:49:30 EST 
currently working on it
Comment 5 Pierre-Charles David CLA 2015-09-22 04:19:50 EDT 
I've restarted work on this in the last few days, and have some preliminary results (not published anywhere yet). Unfortunately a real, complete solution which takes all the trade-offs into account (rendering quality, rendering time, memory consumption) is more impacting and risky that what we can push this late in the cycle. We may push a fix for an SWT resource leak detected while working on this code, but the actual improvement of SVG rendering will have to wait.
Comment 6 Eclipse Genie CLA 2016-01-20 03:27:40 EST 
New Gerrit change created: https://git.eclipse.org/r/64731
Comment 7 Eclipse Genie CLA 2016-01-20 03:40:52 EST 
New Gerrit change created: https://git.eclipse.org/r/64733
Comment 8 Eclipse Genie CLA 2016-02-08 10:23:40 EST 
New Gerrit change created: https://git.eclipse.org/r/66132
Comment 9 Eclipse Genie CLA 2016-02-08 10:23:43 EST 
New Gerrit change created: https://git.eclipse.org/r/66131
Comment 12 Eclipse Genie CLA 2016-02-10 11:10:17 EST 
New Gerrit change created: https://git.eclipse.org/r/66317
Comment 13 Eclipse Genie CLA 2016-02-10 11:10:19 EST 
New Gerrit change created: https://git.eclipse.org/r/66316
Comment 14 Eclipse Genie CLA 2016-02-10 11:10:20 EST 
New Gerrit change created: https://git.eclipse.org/r/66315
Comment 18 Eclipse Genie CLA 2016-02-19 05:36:57 EST 
New Gerrit change created: https://git.eclipse.org/r/66906
Comment 19 Eclipse Genie CLA 2016-02-19 05:37:01 EST 
New Gerrit change created: https://git.eclipse.org/r/66905
Comment 20 Eclipse Genie CLA 2016-02-19 05:37:03 EST 
New Gerrit change created: https://git.eclipse.org/r/66904
Comment 21 Eclipse Genie CLA 2016-02-19 05:37:05 EST 
New Gerrit change created: https://git.eclipse.org/r/66903
Comment 22 Eclipse Genie CLA 2016-02-19 09:12:15 EST 
New Gerrit change created: https://git.eclipse.org/r/66928
Comment 23 Eclipse Genie CLA 2016-02-19 09:12:17 EST 
New Gerrit change created: https://git.eclipse.org/r/66927
Comment 30 Eclipse Genie CLA 2016-02-22 10:13:47 EST 
New Gerrit change created: https://git.eclipse.org/r/67063
Comment 31 Eclipse Genie CLA 2016-02-22 10:13:51 EST 
New Gerrit change created: https://git.eclipse.org/r/67062
Comment 32 Eclipse Genie CLA 2016-02-22 10:13:52 EST 
New Gerrit change created: https://git.eclipse.org/r/67061
Comment 33 Eclipse Genie CLA 2016-02-22 10:20:09 EST 
New Gerrit change created: https://git.eclipse.org/r/67066
Comment 34 Eclipse Genie CLA 2016-02-22 10:20:12 EST 
New Gerrit change created: https://git.eclipse.org/r/67069
Comment 35 Eclipse Genie CLA 2016-02-22 10:20:13 EST 
New Gerrit change created: https://git.eclipse.org/r/67068
Comment 36 Eclipse Genie CLA 2016-02-22 10:20:15 EST 
New Gerrit change created: https://git.eclipse.org/r/67067
Comment 44 Eclipse Genie CLA 2016-03-10 09:03:40 EST 
New Gerrit change created: https://git.eclipse.org/r/68151
Comment 45 Eclipse Genie CLA 2016-03-10 09:03:46 EST 
New Gerrit change created: https://git.eclipse.org/r/68150
Comment 46 Eclipse Genie CLA 2016-03-10 09:03:54 EST 
New Gerrit change created: https://git.eclipse.org/r/68149
Comment 50 Eclipse Genie CLA 2016-04-11 05:47:30 EDT 
New Gerrit change created: https://git.eclipse.org/r/70349
Comment 51 Eclipse Genie CLA 2016-04-11 05:47:36 EDT 
New Gerrit change created: https://git.eclipse.org/r/70348
Comment 52 Eclipse Genie CLA 2016-04-11 05:47:38 EDT 
New Gerrit change created: https://git.eclipse.org/r/70350
Comment 53 Eclipse Genie CLA 2016-04-12 11:14:15 EDT 
New Gerrit change created: https://git.eclipse.org/r/70484
Comment 58 Pierre-Charles David CLA 2016-04-18 04:09:05 EDT 
Fixed.

There are some issues with Batik mis-handling some SVG files, but there are not introduced by this change, they are just more easily triggered once you use more SVGs. Not sure we can do anything about it though, except try to understand better what kind of SVG features make Batik crash and document it.

A new system property has been added, org.eclipse.sirius.diagram.ui.svg.maxCacheSizeMB, to configure the maximum size (in MB) to use to cache pre-rendered SVGs. The current default value is 50, which means 50MB. The default value and the configuration mechanism itself may change before the final release.
Comment 59 Pierre-Charles David CLA 2016-06-24 08:02:54 EDT 
Available in Sirius 4.0.0.