viewBox numerical errors caused by default.svg file

[Alvin Penner]

- running Windows XP, Inkscape rev 13632.

In rev 13632 some efforts were made to reduce conversion errors caused by using float variables. However some errors still remain, and appear to be caused by the default file default.svg. This has been attached here with the name default_bzr.svg for reference purposes. If I load Inkscape with no file, open Document Properties, and switch from Page Size A4 to Letter, then the viewbox height, in the XML editor, is 990.00002. It should be 990 exactly. This appears to be related to the default.svg file. This file has two deficiencies.

The first deficiency is that the file does not survive a round trip caused by switching from Page Size A4 to Letter and back again. During the original load, when the Page Size was A4, the document height is reported in pixels in the XML editor. After switching to Letter, the height is reported in inches as expected. When switching back to A4, the height is reported in mm, as expected. This means it should have been mm in the first place, during the initial load, to be consistent.

The second deficiciency is that there will be round-off error in the calculated viewBox, if it does not initially exist. If it does not exist, then it will be calculated from the document height. But the document height is a single-precision float variable, while the viewBox is declared to be, intended to be, a double-precision variable. It will therefore inherit the imprecision of the height variable, and once this error is committed, it can never be undone.

[Alvin Penner (apenner)]

These deficiencies have been fixed in the attached file default.svg.
Using this file to start Inkscape, we get the following viewBox in the XML editor: 0 0 744.09448819 1052.3622047.
This is an exact duplicate of the default.svg file, so no error has yet been committed.
After toggling page size back and forth between A4 and Letter, no new errors are introduced in ViewBox.
After toggling default units back and forth between px and mm, no new errors are introduced in ViewBox.

I would like to propose using the new version of default.svg.
My main problem is, I do not know exactly who will be affected by this, since there are so many versions of default.??.svg, none of which apparently affect my install. Therefore I thought I would post this here for comment.

[su_v (suv-lp)]

1) Please test the current experimental branch - Tav recently reworked most of the basic templates there... (the branch will be merged into trunk for 0.92). Note that the experimental branch has already switched internal resolution from 90 to 96dpi though.
2) How does the issue reported here differ from bug #1235279 (comment #3)?
3) in trunk (or experimental), with your proposed new template, are you able to snap precisely to page corners, grid intersections etc? Draw a rect (without border, integer size e.g. 50x50mm), drag and snap rect corner to page corner and inspect x, y in the XML Editor; adjust page grid to mm, and snap to grid intersections (e.g. the SVG origin in the upper left corner of the page),

[su_v (suv-lp)]

Sorry - forget about question 3 for now (that mostly applies to the new default template in the experimental branch which uses viewBox="0 0 210 297" for the A4 page, with units 'mm').

[su_v (suv-lp)]

Ignore question 2 in comment 3 too, please (I should have checked first - referenced bug is closed now as 'Fix committed' with milestone 0.91).

[su_v (suv-lp)]

New template tested successfully with current trunk r13632 on OS X 10.7.5 - tests with recent stable release branch 0.91.x r13628 though failed (viewBox of page switched to 'Letter' is still imprecise). This makes me wonder where to go from here: the proposed change can't be applied as is to the stable release branch for 0.91, and the default templates in trunk will change once the changes in the experimental branch have been merged back.

[Alvin Penner (apenner)]

in order to successfully use the new default.svg file, it will be necessary to first apply rev 13632, which is essential

[su_v (suv-lp)]

On 2014-10-24 24:56 (+0200), Alvin Penner wrote:
> in order to successfully use the new default.svg file, it will be
> necessary to first apply rev 13632, which is essential

I'm aware of that - but no one backported that revision to the stable release branch so far, or left a note about this as 'TODO' …

Would it be save to commit rev 13632 to the stable release branch? Does it depend on other changes committed to trunk after the stable release branch was created?

[Alvin Penner (apenner)]

personally, I think it is safe, the only reason I didn't commit it to the stable release was because I ran out of time....

[Alvin Penner (apenner)]

with respect to rev 13632, it might be interesting to see whether Johan or Matthew have a comment?

with respect to this particular Bug 1384915 report, perhaps, I will simply wait for the experimental branch to be merged and see what it looks like at that point.

[Alvin Penner (apenner)]

From the experimental branch, I have taken the template files default.svg and default.en_US.svg, and applied them both to my main branch. They both work flawlessly, I have not been able to generate any numerical error by toggling the default units or the page size.

Similarly, I also tried the file default_px.svg from experimental branch. This file caused problems of lack of precision, which I will report elsewhere, since it relates specifically only to experimental branch.

This particular bug report is going to become obsolete or invalid once the experimental branch is merged. I think my recommendation would be to commit both rev13632 and the current proposed default.svg to the stable 0.91 branch, since the two must go hand in hand, then wait for experimental to merge, then delete this bug report, and address the new bug report which I will issue shortly...

[su_v (suv-lp)]

On 2014-10-30 10:18 (+0100), Alvin Penner wrote:
> ** Changed in: inkscape
> Status: Confirmed => Invalid

Just curious - you no longer think it would be beneficial for users if the changes as discussed earlier would get backported to the stable release branch for 0.91 <lp:inkscape/0.91.x>? Why?

[Alvin Penner (apenner)]

sorry, that is probably a misunderstanding. what I meant is that this particular change is no longer relevant to trunk since trunk has changed. In the next few days or so I'll look at the new trunk to see if anything needs to be done.
      I would still like to see this particular bug report applied to 0.91 if anyone is interested in doing so.

[su_v (suv-lp)]

On 2014-10-30 16:52 (+0100), Alvin Penner wrote:
> I would still like to see this particular bug report applied to 0.91
> if anyone is interested in doing so.

Chances are less likely that this happens if you close the report as 'Invalid' ;-)

[su_v (suv-lp)]

AFAICT this needs to be done for the stable release branch:
1) backport of trunk rev 13632
   <http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13632>
2) update of all templates bundled with Inkscape in <lp:inkscape/0.91.x> to include a viewBox attribute (page size in px, based on 90dpi)

AFAIU there is no solution for custom user templates created with current stable 0.48 (those usually never have a viewBox attribute, and thus may trigger the same issue when used with 0.91 as the current templates shipping in the 0.91.x branch).

[su_v (suv-lp)]

It might be useful to coordinate the proposed changes with
- Bug #1387864 “Document units should not differ from user units for 0.91”
  https://bugs.launchpad.net/inkscape/0.91.x/+bug/1387864

[Tavmjong Bah (tavmjong-free)]

Actually, I think we could go with Alvin's solution but use 96dpi. As long as the viewBox matches it doesn't matter the dpi.

Where dpi does matter is in the guides/grids as they don't use the viewBox at the moment. It would be good to get that taken care of ASAP. In the mean time, DPI switcher can be used... but should only adjust guides and grids.

[su_v (suv-lp)]

On 2014-11-14 10:59 (+0100), Tavmjong Bah wrote:
> Actually, I think we could go with Alvin's solution but use 96dpi.
> As long as the viewBox matches it doesn't matter the dpi.

Not 100% sure what solution is referred to in this comment, but IIUC using 96dpi in a branch which hasn't switched to 96dpi internally yet triggers known bugs with units (e.g. mismatch of size reported in select and rectangle tool), since in 0.91.x the content would actually be scaled (if the viewBox size <-> svg width, height don't match the internal resolution of 90dpi). Thus default templates with viewBox sizes based on 96dpi would trigger documents with inconsistent sizes displayed in inkscape's GUI right from the beginning.

[Tavmjong Bah (tavmjong-free)]

Ah, after adding some more tests I recall the fundamental problem... inkscape:document-units doesn't allow for arbitrary ratios between width/height and the viewBox. I wonder how easy it would be to fix this. If it is not fixed, the files with mixed units created with 0.91 will have problems in 0.92. (One should be able to created files with arbitrary ratios.)

[Tavmjong Bah (tavmjong-free)]

The difference in size reported by the select tool and the rectangle tool is independent of the 90/96 dpi issue. The select tool reports the size relative to the document coordinate system while the rectangle tool reports the size in the current coordinate system which may include transforms.

[su_v (suv-lp)]

On 2014-11-19 11:20 (+0100), Tavmjong Bah wrote:
> The difference in size reported by the select tool and the rectangle
> tool is independent of the 90/96 dpi issue.

It triggers an existing (known) bug - tracked in
- Bug #307656 “rect toolbar width/height numbers do not consider parent transform.”
  https://bugs.launchpad.net/inkscape/+bug/307656
(related more recent reports are bug #1366434 and parts of bug #1239682)

[Tavmjong Bah (tavmjong-free)]

There is still a problem in trunk. Repeat the change in page size unit twice starting with default.svg

After mm->px, viewBox is (0, 0, 210, 296.99999)
After mm->px->mm->px->mm, there is a non-zero translation of the layer.

[Alvin Penner (apenner)]

thanks for testing. I think I have been able to reproduce the problem, and I'll take a look at it next week. This test is a bit different than what I had been doing. I had always been switching from A4 To US Letter and back again repetitively to confirm that all was well. That test is still being passed on my machine but the px to mm is not being passed so I'll check into it.

[su_v (suv-lp)]

On 2014-10-23 22:35 (+0100), Alvin Penner wrote:
> In rev 13632 some efforts were made to reduce conversion errors
> caused by using float variables.

It appears that r13632 plays a contributing role to bug #1395637.

[su_v (suv-lp)]

Reopening for trunk based on comments 24-25

[Alvin Penner (apenner)]

fix committed to rev 13776.
for this particular change, namely units of the page size, it is not actually necessary to recalculate the viewbox, so this recalculation has been avoided. The other two types of change, namely page type and page dimensions, will continue to be calculated as before.

[Alvin Penner (apenner)]

slightly improved fix committed to rev 13789. this is to deal with comment 24 above, namely:

>> After mm->px->mm->px->mm, there is a non-zero translation of the layer.

[ScislaC (scislac)]

Alvin: Would you be willing to produce a patch against the 0.91.x branch? There are a few commits referenced in the comments here and to my eye it appears 13632 isn't relevant. Since you have the last couple commits mentioned, I figured it was worth asking what is specifically needed for the soon-to-be stable branch.

[Alvin Penner (apenner)]

yes, I can take a look at that, over the next day or two. I don't have a working version of the build for that branch, so I won't be able to test it, but I can put together whatever seems to be relevant.

[Alvin Penner (apenner)]

@ScislaC - using 0.91pre3 I have not been able to reliably reproduce the problems reported in the original report. Since the release of the original bug report there have been three changes made, elsewhere, which all lead to a simplification of the problem, so the original report is no longer relevant to 0.91pre3.
1. the original default.svg file did not have a viewbox defined. The current one does, in 0.91pre3. This eliminates an unnecessary conversion from a single precision page size to a double precision viewbox, which was probably the source of most of the original problem.
2. the original default.svg file specified page size in px. The current one specifies it in mm, which is consistent with the fact that the page size is A4. This eliminates an unnecessary conversion from a non-integer px size to an integer mm size, where the resulting mm size quite possibly would not be an integer because these variables are single-precision not double.
3. since the original report, the live switching of document units has been disabled, so that it is no longer possible to change the units that the viewbox is expressed in, after the file is loaded. This eliminates another piece of conversion code which was possibly causing a problem with precision.

in any event I cannot reproduce the original problem. Was there a specific issue that you wanted to address?

[su_v (suv-lp)]

Inkscape 0.91pre4 r13712:
1) launch with default prefs, default new doc
2) open Document Properties
3) switch 'Custom size > Units' from 'mm' to 'px'
4) switch 'Custom size > Units' back to 'mm'

--> adds transform attribute to top-level layer (see comment #24):
    translate(0,2.2107223e-5)

Not reproduced with Inkscape 0.91+devel r13863.

[Alvin Penner (apenner)]

yes, you're right. Sorry about that, I was focusing so much on the size of the viewbox that I forgot about the possible added transform.
This can be fixed by implementing rev 13776, which effectively avoids a recalculation that is unnecessary.
However, rev 13776 requires that rev 13632 must first be implemented, at least the change involving document.cpp, not the change in units.xml.
And there was an intermediate change, rev 13753, which would also need to be included, since it was a bug introduced by rev 13632.

I can do this, over the next two or three days. I don't have a working copy of the 91x repository, so I'll need to download it for testing purposes. And I don't have the ability to produce diff files that involve more than one file, so I would just submit the original files in their entirety as a zip file after they have been tested.

[Alvin Penner (apenner)]

well, maybe I spoke too hastily. All that rev 13776 does is just to decide whether or not to call the routine root->viewBox.setMax. It does not actually care what is in this routine, it just decides not to call it. Therefore rev 13632 and rev 13753 are probably not necessary, since these revs refer only to what is actually inside setMax. This simplifies the problem significantly, but it will still need some testing to confirm.

[ScislaC (scislac)]

For 0.91.x r13776 alone didn't seem to do it for me, but r13776+r13789 seemed to do it. Confirmation would be appreciated.

[Alvin Penner (apenner)]

yes, I finally got it to work just now, attached is the patch I produced from rev 13776 and rev 13789

[ScislaC (scislac)]

Fix backported in 0.91.x r13713.