There is a class Geom::ViewBox recently added to the 2Geom tree (2geom/viewbox.h), I think it should be used in preference to a new base class. It doesn't feel completely right to have a common base class for every object that has a viewbox.

Regards, Krzysztof

2014-02-09 21:07 GMT+01:00 Johan Engelen <jbc.engelen@...2592...>:

On 9-2-2014 20:36, Johan Engelen wrote:

...

On 6-2-2014 15:44, Tavmjong Bah wrote:

...

Hi,

A heads-up...

While mucking about the code I noticed that the same basic code to handle viewBox and preserveAspectRatio appeared in multiple files (8 times?) having originally been pasted directly or copied with some changes. Overtime the code was tweaked here and there.

I have created a new base class to handle viewBox and preserveAspectRatio and modified sp-root, sp-symbol, sp-image, sp-pattern, and marker to use the new class, resulting in a reduction of over 400 lines of code (yeah!). In the process, I've fixed a number of issues with using percentage based x, y, width, and height values.

Great that you are working on this part! The new viewbox handling code needs some love.

I have only glanced at it. Comment: don't multiple inherit in this case (SPSymbol, possibly others). Just make viewBox a _member_ of SPSymbol.

...

Please let me know if you find any problems.

(fixed build on Mac, it's a 2Geom issue)

I understand it is mostly a copy-paste of code? The code certainly needs cleanup... :(

Did some simple clean-up just now.

• Johan

---

Managing the Performance of Cloud-Based Applications Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. Read the Whitepaper. http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.cl... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel

On Tue, 2014-02-11 at 03:02 +0100, su_v wrote:

On 2014-02-06 15:44 +0100, Tavmjong Bah wrote:

...

While mucking about the code I noticed that the same basic code to handle viewBox and preserveAspectRatio appeared in multiple files (8 times?) having originally been pasted directly or copied with some changes. Overtime the code was tweaked here and there.

I have created a new base class to handle viewBox and preserveAspectRatio and modified sp-root, sp-symbol, sp-image, sp-pattern, and marker to use the new class, resulting in a reduction of over 400 lines of code (yeah!). In the process, I've fixed a number of issues with using percentage based x, y, width, and height values.

Please let me know if you find any problems.

Stretched or squeezed bitmap images now (rev >= 13002) render according to the SVG 1.1 spec, but not to how Inkscape users expect it: the bounding box matches the expected dimensions, but the image itself appears to be displayed with 'preserveAspectRatio="xMidYMid"' (default for images in SVG 1.1 if 'preserveAspectRatio' is not specified).

Steps to reproduce:

1. launch current trunk (rev >= 13002)
2. import bitmap image (embed or link, doesn't matter)
3. stretch the image horizontally or vertically by dragging one of the

scale handles 4) release the mouse button

Expected result in Inkscape: The embedded or linked bitmap image is scaled to match the new dimensions.

Actual result with rev >= 13002: The bounding box of the selected image has the expected dimensions, but the actual image content reverts to a preserved aspect ratio, as large as fits into the transformed bbox.

Changes of r13002 affecting other parts of Inkscape:

• Bug #1278645 “image handling in sp-image corrupts EMF/WMF output” https://bugs.launchpad.net/inkscape/+bug/1278645

Related reports about the old (non-conformant) behavior:

• Bug #924377 “wrong default value for image preserveAspectRatio” https://bugs.launchpad.net/inkscape/+bug/924377
• Bug #461467 in Inkscape: “Image scale saved incorrectly” https://bugs.launchpad.net/inkscape/+bug/461467
• Bug #616717 “Resized bitmap images are rendered differently by Firefox

or Batik ('preserveAspectRatio' attribute not set)” https://bugs.launchpad.net/inkscape/+bug/616717

Note that adding an explicit 'preserveAspectRatio="none"' to the bitmap image currently doesn't help either - Inkscape trunk appears to lack support for this attribute yet (?). The only workaround with rev >= 13002 is to group the image (Ctrl+G), and then scale the group instead of the imported image, or to use preserved transforms (see preferences).

Johan introduced a bug in his code cleanup that prevented 'preserveAspectRatio' from being read. Fixed in r13019.

I've set 'preserveAspectRatio' to 'none' in importing an image, r13020.

If compatibility with old Inkscape files is important, we could set 'preserveAspectRatio' to 'none' on opening a file (if it is not already set). This would break non-Inkscape generated files.

Does the EMF/WMF bug still exist after r13020?

Tav

2014-02-11 12:56 GMT+01:00 Krzysztof Kosiński <tweenk.pl@...400...>:

2014-02-11 12:49 GMT+01:00 Tavmjong Bah <tavmjong@...8...>:

...

If compatibility with old Inkscape files is important, we could set 'preserveAspectRatio' to 'none' on opening a file (if it is not already set). This would break non-Inkscape generated files.

We should set this attribute if there's Inkscape-related markup in the document, such as sodipodi:namedview.

PS this is yet another case where creating our XML tree with a SAX parser would be very useful.

Regards, Krzysztof

On 2014-02-11 12:49 +0100, Tavmjong Bah wrote:

On Tue, 2014-02-11 at 03:02 +0100, su_v wrote:

...

On 2014-02-06 15:44 +0100, Tavmjong Bah wrote:

...

While mucking about the code I noticed that the same basic code to handle viewBox and preserveAspectRatio appeared in multiple files (8 times?) having originally been pasted directly or copied with some changes. Overtime the code was tweaked here and there.

I have created a new base class to handle viewBox and preserveAspectRatio and modified sp-root, sp-symbol, sp-image, sp-pattern, and marker to use the new class, resulting in a reduction of over 400 lines of code (yeah!). In the process, I've fixed a number of issues with using percentage based x, y, width, and height values.

Please let me know if you find any problems.

Stretched or squeezed bitmap images now (rev >= 13002) render according to the SVG 1.1 spec, but not to how Inkscape users expect it: the bounding box matches the expected dimensions, but the image itself appears to be displayed with 'preserveAspectRatio="xMidYMid"' (default for images in SVG 1.1 if 'preserveAspectRatio' is not specified).

Steps to reproduce:

1. launch current trunk (rev >= 13002)
2. import bitmap image (embed or link, doesn't matter)
3. stretch the image horizontally or vertically by dragging one of the

scale handles 4) release the mouse button

Expected result in Inkscape: The embedded or linked bitmap image is scaled to match the new dimensions.

Actual result with rev >= 13002: The bounding box of the selected image has the expected dimensions, but the actual image content reverts to a preserved aspect ratio, as large as fits into the transformed bbox.

...

Related reports about the old (non-conformant) behavior:

• Bug #924377 “wrong default value for image preserveAspectRatio” https://bugs.launchpad.net/inkscape/+bug/924377
• Bug #461467 in Inkscape: “Image scale saved incorrectly” https://bugs.launchpad.net/inkscape/+bug/461467
• Bug #616717 “Resized bitmap images are rendered differently by Firefox

or Batik ('preserveAspectRatio' attribute not set)” https://bugs.launchpad.net/inkscape/+bug/616717

I've set 'preserveAspectRatio' to 'none' in importing an image, r13020.

If compatibility with old Inkscape files is important, we could set 'preserveAspectRatio' to 'none' on opening a file (if it is not already set). This would break non-Inkscape generated files.

Bitmap images in PDF files don't render correctly if they are non-uniformly scaled - they don't have the attribute 'preserveAspectRatio="none"' added either (confirmed with r13020).

Tested among others with sample PDF file from (duplicate) report - Bug #878984 “pdf import problem” https://bugs.launchpad.net/inkscape/+bug/878984 https://launchpadlibrarian.net/83376324/example.pdf

On 11-Feb-2014 06:00, Tavmjong Bah wrote:

'preserveAspectRatio' is now set to 'none'; r13023.

Just tried 13024 and on import of images from EMF preserveAspectRatio is not set automatically. When you say it is set to 'none' now in what context does that apply? It isn't a problem adding

preserveAspectRatio="none"

to the SVG <image> creation in EMF and WMF, but before doing so, I want to double check that it is not supposed to be happening elsewhere automatically. Presumably a similar line will need to be added to every other file format's image input conversion if it is not to be handled in some central location.

Also, regarding bug 1278645, setting the above line in the <image> fixes the EMF/WMF image output. Which tells us that the default value for preserveAspectRatio is doing something wrong for the one image in the test case. That test is from start to finish just a (10,10) bitmap, but for some reason it gets trimmed down to a (5,5) in SPImage::print in sp-image.cpp in the default case, but stays (10,10) for "none".

Putting that preserveAspectRatio in the <svg> at the top of the file does NOT resolve the EMF/WMF image output issue, it has to be in (each) <image>. Is that right - or should the <image> inherit its preserveAspectRatio from the <svg> within which it resides?

preserveAspectRatio applies to:

<svg> <symbol> <image> <feimage> <marker> <pattern> <view>

are we going to have similar issues with any of these?

Regards,

David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech

On 11-Feb-2014 10:26, Tavmjong Bah wrote:

On Tue, 2014-02-11 at 09:57 -0800, mathog wrote:

...

Also, regarding bug 1278645, setting the above line in the <image> fixes the EMF/WMF image output. Which tells us that the default value for preserveAspectRatio is doing something wrong for the one image in the test case. That test is from start to finish just a (10,10) bitmap, but for some reason it gets trimmed down to a (5,5) in SPImage::print in sp-image.cpp in the default case, but stays (10,10) for "none".

There is an "if" statement in SPImage::print which I believe handles non-Cairo output that special cases 'none' from the rest. This is where to look for the fix.

That tests only

if (this->aspect_align == SP_ASPECT_NONE) {

there is nothing Cairo specific about that. The value for aspect_align at that point is 5 == SP_ASPECT_XMID_YMID. Hmm, "save as" operations that go through cairo never enter SPImage::print, and do output correctly. Now to find where those are going and compare the code in the two.

I don't have an easy way to view wmf and emf files...

Use inkscape trunk on any platform?

Thanks,

David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech

On Tue, 2014-02-11 at 11:19 -0800, mathog wrote:

On 11-Feb-2014 10:44, mathog wrote:

...

Hmm, "save as" operations that go through cairo never enter SPImage::print, and do output correctly. Now to find where those are going and compare the code in the two.

The only other file type that goes through SPImage::print is LaTEX.

And LaTeX doesn't handle images at all... so I can't use LaTeX output to test changes to SPImage::print.

Cairo seems to ignore this aspect stuff totally.

? I don't understand what you mean here. AFAICT Cairo (both interactive and cairo-renderer) is doing the correct thing.

I think what is happening here is that before the recent work on aspect the value of this->aspect_align in this method was always 0, so no attempt was made to handle the aspect issue. Now that that value is no longer 0 EMF,WMF, and LaTEX output is going through the other section, which does try to handle aspect, but apparently never worked. Inkscape's current handling of preserveAspectRatio (interactively) is that it forces the _entire_ image to be inscribed in one manner or another into a rectangle. Depending on the mode, the alignment within that rectangle changes.

Yes, this is correct and is according to the SVG 1.1 spec.

The current code in SPImage::print is not doing that at all, it is moving a subpixel array around inside the full image.

Oh well, at least I see where and what the problem is now.

Out file output system is a complete mess. It's hard to figure out what is outputting what. And the lack of comments in the code isn't very helpful.

Tav

On Wed, 2014-02-12 at 12:54 -0800, mathog wrote:

On 12-Feb-2014 11:42, Tavmjong Bah wrote:

...

On Tue, 2014-02-11 at 11:19 -0800, mathog wrote:

...

On 11-Feb-2014 10:44, mathog wrote:

...

And LaTeX doesn't handle images at all... so I can't use LaTeX output to test changes to SPImage::print.

Which in this case has the upside that my changes could not have broken it!

...

...

Cairo seems to ignore this aspect stuff totally.

? I don't understand what you mean here. AFAICT Cairo (both interactive and cairo-renderer) is doing the correct thing.

I didn't say that well.

Class SPImage carries these 4 values around:

 double sx, sy;
 double ox, oy;

These values already reflect the preserveAspectRatio settings when the image gets into SPImage::Print. So there is no need to put in cases for all the preserveAspectRatio settings in the print output because they have already been handled appropriately. Cairo doesn't use these 4 values, instead it calls calculatePreserveAspectRatio() during the output phase in sp_image_render() in cairo-renderer.cpp. In other words, it reinvents the wheel and repeats the aspect calculation. (I just checked this in the debugger for PDF output.)

The on-screen Cairo renderer does use sx, sy, ox, oy... via sp_image_update_canvas_image() which calls sp_image_update_arenaitem()

I think these 4 values are being set in SPViewBox::apply_viewbox(). Unless there is some execution path that bypasses that method, a possibility that at this point I have no way of excluding, no other section of the code should need to explicitly look at this->aspect_align.

They are being set from c2p (Geom::affine) which is calculated in apply_viewbox().

...

Our file output system is a complete mess. It's hard to figure out what is outputting what. And the lack of comments in the code isn't very helpful.

Yup.

There are two major kinds of lengths in SVG, those based on the bounding box of an object and those based on user units. The calculation of lengths that are defined as a percentage of the viewport (i.e user units) is not well supported in Inkscape. The easiest place to calculate them is in the sp-xxx classes. Bounding box percentages seemed to be calculated at a later point. I would like to eventually get all the lengths calculated in the sp-xxx classes. I think this makes sense as then we are not reinventing the wheel for each new renderer.

Tav

On 12-Feb-2014 15:03, Krzysztof Kosiński wrote:

The situation could be improved by using this solution, especially since the SP classes are now C++: http://en.wikipedia.org/wiki/Visitor_pattern

This would require some rewriting in the LaTeX and EMF renderers. On the other hand, adding new output formats would become much much easier after that.

Definitely wait until after the current release cycle.

The only complication I see is that EMF/WMF text output performs some strange processing in Layout-TNG-Output in Layout::print(). Depending on output settings, it may convert text from other fonts to Symbol or Wingdings font there (because, for instance, some programs that read EMF/WMF know how to process Greek characters in Symbol font, but not in Arial font), and since EMF text records demand x,y kerning information, that is extracted from the glyph array and smuggled via a bletcherous hack to the final text output. (The information is converted to a text string and stored after the normal output text's null byte string terminator.) Cairo output does not go through this routine. Instead it passes through the somewhat similar Layout::showGlyphs().

What happens below these is quite different. The downward paths are:

sp_print_text(ctx, smuggle_string, g_pos, text_source->style);

vs.

ctx->renderGlyphtext(span.font->pFont, font_matrix, glyphtext, style);

The first passes the position (g_pos) and the text (UTF plus the smuggled kerning information), whereas the second passes a bunch of font information along with an array of glyphs. Basically one is for "text string goes here" output, and the other for "draw these glyphs" output. I suppose in the fused output the common downward routine could just pass all of this information, and the called routine would use whichever was appropriate. That would be better in at least one way - the kerning information could be extracted in (new) sp_print_text, which would eliminate the need to smuggle it.

Regards,

David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech

2014-02-13 4:10 GMT+01:00 Martin Owens <doctormo@...400...>:

On Thu, 2014-02-13 at 00:03 +0100, Krzysztof Kosiński wrote:

...

This would require some rewriting in the LaTeX and EMF renderers. On the other hand, adding new output formats would become much much

If we were to go ideal, would we not hack every export but Inkscape SVG out of inkscape proper and force them into extensions? That would at least give plenty of impetus to have a decent extensions system that could interact with the inkscape codebase fundamentally.

Moving the exporters to extensions does not conflict with introducing the visitor pattern, in fact it makes doing that a lot simpler.

Regards, Krzysztof

2014-02-13 15:01 GMT+01:00 Tavmjong Bah <tavmjong@...8...>:

On Thu, 2014-02-13 at 14:41 +0100, Krzysztof Kosiński wrote:

...

2014-02-13 4:10 GMT+01:00 Martin Owens <doctormo@...400...>:

...

On Thu, 2014-02-13 at 00:03 +0100, Krzysztof Kosiński wrote:

...

This would require some rewriting in the LaTeX and EMF renderers. On the other hand, adding new output formats would become much much

If we were to go ideal, would we not hack every export but Inkscape SVG out of inkscape proper and force them into extensions? That would at least give plenty of impetus to have a decent extensions system that could interact with the inkscape codebase fundamentally.

Moving the exporters to extensions does not conflict with introducing the visitor pattern, in fact it makes doing that a lot simpler.

I've looked up the visitor pattern and wrote a little toy program using the pattern but I am having a little trouble seeing how it would work with the SPxxx structure? Could you elaborate a little bit?

- There would be a class called e.g. SPTreeVisitor, with an accept method for each of the SP tree node types. The default implementation would be to simply call the accept method for the supertype with the same parameter.

- SP tree nodes would expose a "visit" method taking SPTreeVisitor as a parameter, which would simply call the relevant accept method on the visitor.

- The exporters and the offline Cairo renderer (used for PDF and PS export) would derive from SPTreeVisitor, possibly via an intermediate base class which would provide default implementation of the accept methods in terms of simpler primitives (e.g. translating a spiral to a path).

Regards, Krzysztof