After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762039 - Extract accurate positions and dimensions
Extract accurate positions and dimensions
Status: RESOLVED OBSOLETE
Product: librsvg
Classification: Core
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: Federico Mena Quintero
librsvg maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-14 17:42 UTC by Jehan
Modified: 2017-12-13 18:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix em/ex dimensions in RsvgDimensionData. (3.42 KB, patch)
2016-02-14 17:42 UTC, Jehan
reviewed Details | Review
RsvgPositionData x/y values as double. (1.99 KB, patch)
2016-02-14 17:48 UTC, Jehan
none Details | Review
New double fields in RsvgPositionData as alternative. (3.02 KB, patch)
2016-02-14 18:03 UTC, Jehan
none Details | Review
Example of extracted vectorial with inaccurate position/dimensions. (15.00 KB, image/svg+xml)
2016-02-14 18:06 UTC, Jehan
  Details
Extracted icon after patching librsvg (15.03 KB, image/svg+xml)
2016-02-14 18:10 UTC, Jehan
  Details
New double fields in RsvgPositionData as alternative (use double instead of gdouble). (3.02 KB, patch)
2016-02-14 19:24 UTC, Jehan
none Details | Review
Int position/dimensions are properly rounded. (2.57 KB, patch)
2016-02-14 19:27 UTC, Jehan
none Details | Review

Description Jehan 2016-02-14 17:42:43 UTC
Created attachment 321138 [details] [review]
Fix em/ex dimensions in RsvgDimensionData.

RsvgDimensionData has both int (width, height) and double (em, ex) dimensions, but in practice, em/ex are created from width/height, which makes them useless. It should be the other way around, especially since we compute double values (either through cairo or _rsvg_css_hand_normalize_length).

As for RsvgPositionData, it does not even have any double value.

The point is that Inkscape (and probably any other vectorial editor) heavily uses double value so we need accurate positions and dimensions.
In GIMP, we are switching to vector icons, and I want to use librsvg to extract the icons from a single source SVG. We cannot afford 1 pixel off errors for icons which will be displayed on 12 or 16 pixels.

The attached patch fixes rsvg_handle_get_dimensions/rsvg_handle_get_dimensions_sub() by now filling accurate double values in dimension_data->em/ex.
Comment 1 Jehan 2016-02-14 17:48:02 UTC
Created attachment 321139 [details] [review]
RsvgPositionData x/y values as double.

This second patch is maybe more a problem since it breaks compatibility. I am proposing to have x/y values on RsvgPositionData as double instead of int.

I don't think it makes sense to use int values, and anyway double to int conversion should work ok in most cases so existing code would likely not break. Instead software will now output warning at build and they can update their code.

If that is absolutely not an acceptable thing, I will propose as an alternative to add another field to RsvgPositionData, similarly to RsvgDimensionData which has both int and a double fields.
Just tell me and I update this second patch.
Comment 2 Jehan 2016-02-14 18:03:14 UTC
Created attachment 321140 [details] [review]
New double fields in RsvgPositionData as alternative.

Well actually let me upload directly the alternative patch to attachment 321139 [details] [review].

I called the new field in RsvgPositionData abscissa/ordinate (which is basically similar to x/y, but these are taken).
Comment 3 Jehan 2016-02-14 18:06:13 UTC
Created attachment 321141 [details]
Example of extracted vectorial with inaccurate position/dimensions.

To emphasize how much this is important for the usage in GIMP, for instance, attached is an icon (basically a square) which I extracted from our source file, and updated the viewBox based on the position/dimension information returned by rsvg. As you can see, this is completely wrong because the information is just too inaccurate.
Comment 4 Jehan 2016-02-14 18:10:04 UTC
Created attachment 321142 [details]
Extracted icon after patching librsvg

And now the same icon extracted after having patched librsvg with my patches, i.e. I have accurate position and dimension values.
The positionning of the viewBox is just perfect.
Comment 5 Jasper St. Pierre (not reading bugmail) 2016-02-14 18:27:16 UTC
Review of attachment 321139 [details] [review]:

Wouldn't this be a major API and ABI break?
Comment 6 Emmanuele Bassi (:ebassi) 2016-02-14 18:40:15 UTC
Review of attachment 321138 [details] [review]:

::: rsvg-base.c
@@ +1442,3 @@
     }
+    dimension_data->width = (int) dimension_data->em;
+    dimension_data->height = (int) dimension_data->ex;

You likely don't want to use C integer rounding rules, here; if you want to ensure that the sizes are big enough to hold the surface, you should use ceil() instead.

@@ +1531,2 @@
+    dimension_data.width = (int) dimension_data.em;
+    dimension_data.height = (int) dimension_data.ex;

Same as above.
Comment 7 Emmanuele Bassi (:ebassi) 2016-02-14 18:42:42 UTC
Review of attachment 321139 [details] [review]:

::: rsvg.h
@@ +123,3 @@
 struct _RsvgPositionData {
+    gdouble x;
+    gdouble y;

You're breaking the ABI of this public structure by changing the size from int to double.

Not sure if we care that much, but now people using RsvgPositionData will require to be recompiled against librsvg.
Comment 8 Emmanuele Bassi (:ebassi) 2016-02-14 18:43:36 UTC
Review of attachment 321140 [details] [review]:

::: rsvg.h
@@ +127,3 @@
     int y;
+    gdouble abscissa;
+    gdouble ordinate;

Once again, you're breaking ABI by adding fields.

Also, don't use `gdouble`: just use `double`.
Comment 9 Jehan 2016-02-14 18:57:22 UTC
> Wouldn't this be a major API and ABI break?

Yes I guess the changes proposed by attachment 321139 [details] [review] would be API/ABI breaks and 321140 and ABI break. But having int pos/dimensions is anyway so broken that I would not be against having a major release which changes these if that is possible with the maintainers of librsvg.
Without any possibility to get a double position, librsvg is just useless for many things.

> You likely don't want to use C integer rounding rules, here; if you want to ensure that the sizes are big enough to hold the surface, you should use ceil() instead.

I agree. I had a first version where I was using ceil (and floor for position), which was ensuring that at least the full icon would be in the extracted SVG. This worked (at least the viewBox was not cropping the icons anymore) but this resulted in very ugly icons (with added spaces on sides, not at all what the icon designer went for). So I just dropped this and kept as is, because anyway I don't see *at all* the point for any version of the int positions and dimensions. Whether they are ceil or floor rounding actually does not matter much: these are wrong positions and dimensions anyway.
But yeah I can change this, no problem. :-)

> Also, don't use `gdouble`: just use `double`.

Ok. I can change this no problem.
Comment 10 Jehan 2016-02-14 19:24:35 UTC
Created attachment 321155 [details] [review]
New double fields in RsvgPositionData as alternative (use double instead of gdouble).

Update attachment 321140 [details] [review]. The only difference is that I use double instead of gdouble for abscissa/ordinate.
Comment 11 Jehan 2016-02-14 19:27:23 UTC
Created attachment 321156 [details] [review]
Int position/dimensions are properly rounded.

A new patch which also changes the current logics with floor() rounding for the positions and ceil() rounding for the dimensions.

This is complementary but definitely does not replace having accurate double values. Even with this, extracting objects renders ugly icons (instead of being cropped, spaces are added on sides).
Comment 12 Jehan 2017-01-06 13:42:42 UTC
I see that there is revival of development on librsvg. Could my patches be taken into account?
Comment 13 Federico Mena Quintero 2017-08-30 02:13:54 UTC
I'm going to base a patch on the one to ceil()/floor() the coordinates properly.  Good eye on that, thank you.

Unfortunately we can't change the public structs, as that's an ABI change.  Instead, I'll add new functions to get the logical rect / ink rect.
Comment 14 GNOME Infrastructure Team 2017-12-13 18:16:29 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/librsvg/issues/134.