computeBBox() from simpletransform.py calculates non-drawing elements

[Thomas Weber]

I provided a fix here:

http://bazaar.launchpad.net/~inkscape+th-we/inkscape/inkscape/revision/15298

Please advice on how to actually go about when fixing things like this. Is it better to *first* create a bug report and *then* push the fix, pointing to the bug report? (I'd highly welcome the move to Github.)

[Thomas Weber (th-we)]

BTW, why can't Inkscape's already existing functionality concerning bounding box calculation be exposed to Python? Re-writing a good bounding box algorithm is no simple feat. It would prevent both a lot of duplicate work and inconsistencies.

I realize that the Python code operates on a separate DOM, so one would probably have to serialize the DOM when passing it from Python to Inkscape and back. Not ideal, but much better than having to implement the same functionality a second time.

[Alvin Penner (apenner)]

yes, in terms of visibility, this is the best place to expose the problem. The normal procedure would be to provide an svg file illustrating the problem, then wait for someone to independently confirm it, and then propose a fix, preferably in the form of a diff file.

[jazzynico (jazzynico)]

Thanks for the patch!

Did you notice some cases where the issue affected the result of the extension? At first sight, your patch is optimizing the code, but doesn't correct any calculation errors.

[Thomas Weber (th-we)]

Sorry - accidentally hit Tab and Space, which resulted in posting while I was in the midst of composing the comment with some rubbish content. Anyway, here I go again:

Yes, I ran into problems (that's why I tried to fix them). Say you have this document:

    <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="100" height="100">
      <g>
        <defs>
          <rect width="50" height="50" id="rect"/>
        </defs>
      <use xlink:href="#rect" transform="scale(.5)"/>
      </g>
    </svg>

Something like

  for group in document.xpath("//svg:g", namespaces=inkex.NSS):
    computeBBox(group)

would compute "0, 50, 0, 50", but in reality it's "0, 25, 0, 25".

[jazzynico (jazzynico)]

Thanks for your explanation Thomas.
Did you run into that issue with an official Inkscape extension?

The patch looks good, but we're dramatically missing some test cases for the simple transform extension. I'm going to write some minimal tests in order to detect regressions.

[Thomas Weber (th-we)]

Not with an official extension. I modified the tar_layers export heavily to - among other things - add an option to crop each exported image. I didn't contribute it as it feels a little hacky in some regard:

* For precise cropping, `computeBBox()` isn't good enough, so I resorted to a system call to Inkscape with `--query-all` option and only used `computeBBox()` as a fallback.
* The GUI actually relies on which tab of the dialogue the user has selected, more like with a radio button with additional per-button sub-choices.
* It has an input field for XPath expressions to choose which elements to export and how to name it. You can easily break thing here by inputting an invalid XPath expression.

I attached a screenshot of the GUI to give an impression.

[jazzynico (jazzynico)]

> For precise cropping, `computeBBox()` isn't good enough

If I understand correctly, it's good when using a geometric bounding box. But it's not when using a (default) visual bounding box.
The Dimension extension also uses both methods (computeBBox and Inkscape --query) depending on the bounding box.

A very basic test file for simpletransform.py is ready (test for the computeBBox function with the test case attached comment #5). Now I need to find some other cases to improve it a bit.
The patch should be ready to commit to lp:inkscape (trunk) later this week.

Thanks again for your help!