Issue 124084

Please specify steps to reproduce.

Please attach the drawing as it was generated by the external program, without any changes. If you provide it already inserted and resaved, it is not possible to see what happens during import.

Which external program do you use? Or do you set the values manually using an editor?

Adding me to CC...

Created attachment 82512 [details]
ODG file generated by external program

The ODG file generated by external program.

Created attachment 82515 [details]
Example for misinterpretation of absolute M command

AOO gets the command M wrong, if it is the first command of the path and the coordinates are others than (0|0).

The upper example in the attached file has the sequence M2000 3000 and the drawing should start in point (2000|3000), but is does not.

The lower example has the sequence M0 0 M2000 3000 and there the drawing starts correctly in point (2000|3000).

Compared with the original example I have changed parts, to make sure, that the error is not in that parts: I have added units to x, y, width, height; and removed comma on path description; and reduced the drawing to a single curve segment.

The original document has a syntax error in attribute value of attribute preserveAspectRatio. It should be "xMidYMid meet". But is does not matter here, because "xMidYMid meet" is the default value for this attribute.

For my tests I have used AOO410m1(Build:9750)  -  Rev. 1563305

I have tested previous versions now. I see the error already in OOo1.1.5.

It might be connected to bug 37213.

I have reduced "Importance", because AOO do not write such files, and authors who work directly on the file format, can add an additional M0 0 command to workaround the problem.

Grepping, will have a look ASAP...

Checked the svg:d parser, all works well there. Problem is not the svg:d, but how it is used in our model.
A SdrPathObj cannot be smaller/bigger than the contained polygon, so the polygon *defines* the objects size.
In your correction you have 'achieved' that by adding (0,0) as a first, one-point polygon, so the SdrPathObj contains a PolyPolygon made up of two polys, the 1st with just one point.
The svg:d we write (and read) is the unified PolyPolygon plus scaling (due to the fromer core need that integers are used in-between, thus unit coordinates [0.0 .. 1.0] would have been lost).
This automatically means that it will always have a bounding box containing a minimum X and Y being 0.0.
That is the reason that our viewBoxes always start with two zeros and have the size of the boundrect of the contained polygon.

To be honest, I have no clue why ODF uses a viewBox at all; the only adaption that is made is at load time (see SdXMLPathShapeContext::StartElementline 1517) is to scale the polygon from the viewBox range to a range constructed by viewBox x,y (so the same) and the loaded object size (svg:width and svg:height). This would allow a scaling of the defined polygon (by svg:width/viewBox:width in x and svg:height/viewBox:height in y), but *no translation* of it, since minX and minY for both rabnges are defined equal, so even when using values different from zero in viewBox:x and y nothing would happen. This has been like this in all AOO versions and cannot be changed in the installed versions.

The true bound rect of the imported PolyPolygon is not used currently at all. Thus, the viewBox usage in ODF has nothing to do with the intention of this statement in svg, I would have not added it initially at all, it makes things just more complicated here.

So, whoever writes these files, please tell them
- the polygons have to be unified, but scaled (no mirror (aka no negative scale), no shear, no rotation, no translation)
- thus, the bound rect of this will have a minX/minY of 0.0
- the viewBox should have a x,y of 0.0 and a width/height identical to svg:width and svg:height
- the rest of the object transform should move to
 (a) when not rotated, mirrored sheared: svg:x, svg:y for translation
 (b) else, do not use svg:x, svg:y (or set to zero) and use the draw:transform to define the full transformation, starting from the unified, scaled polygon, so scale is already in the polygon, but mirror is not. Thus, when mirrored, the transformation has to start with a scale in x and/or y of -1.0.

Adding a 'M0 0' is *no* correction to be advised; as explained, this will create a PolyPolygon which looks nice, but 'artificially' expands the SdrPathObj shape. We had something similar with one of the CustomShapes, and this was leading to problems, e.g. in slideshow this single point polygons were painted and visible, do not do that please.

All in all there is no error in the svg:d reader, but there are limitations in the usage of viewBox in odf that the original inventors left behind for us, without clearly specifying what intentions they had for viewBox at all. As said, the viewBox intention of svg - to scale some content somewhere and leaving white space somehere between object and content - is not even possible with the SdrPathObj in our model implementation (and makes no sense - why should a object have a size different from the bounding box of its content?).

I do not find, where the bezier curve is shifted, so that its bound rectangle starts in svg:x, svg:y. I have looked through \xmloff\source\draw\ximpshap.cxx, but in SdXMLPathShapeContext::StartElement in line#1583 the variable aSourcePolyPolygon still has the absolute values in regard to the page as it would be for a svg-element.

I see the conflict between the way, that the bounding box of a path element is aligned with the svg:x, svg:y position (draw-interpretation), and the demand, that the M command in the path is honored (svg-interpretation).
But I see no reason, why not use the svg-interpretation on import and change position. This does not effect old documents, because the values were written in a way, that the svg-position interpretation results in the same position as the draw:path interpretation, and using the UI it is not possible to get such M command. It makes it possible to write paths directly, without the need to calculate bounding boxes manually. 

You are right, that the clipping properties of svg:viewBox in svg-graphics is not used in draw:path elements. But why not use the coordinate transformation given by svg:viewBox? I think, that this was the intention in the specification, to fit the unit-less svg:d values to the svg:x, svg:y, svg:width, and svg:height values by an implicit linear transformation. AOO core interprets it as 100th mm, but that is not mentioned in any place in the specification (as far as I know).

Created attachment 82599 [details]
Misinterpretation Bezier Curve (with "M 0 0 " in start of path) - "Content.xml" only.

Created attachment 82600 [details]
ODG file with problems with Bezier Curves

This file contains Bezier Curves misinterpretated.

A prefix "M 0 0 " at start of the path not has been omitted.

All lines with "00_Epures" uses control point coordinates of each Bezier arc, sames as Bezier curves, and are correctly intrerpreted.

Additionally, this ODG is successfully repaired during loading.

The good method for drawing Bezier uses the following steps:
// ViewBox must be used
   // Steps:
   // 1 - Calculate viewbox parameters
   // -- Find maxi et mini of the points control
   // --> obtaining ViewBoxX, ViewBoxY, ViewBoxL, ViewBoxH
   // 2 - Translate all points with tx = -ViewBoxX et ty = -ViewBoxY
   // 3 - Draw curve with avec svg:x="<ViewBoxX>",
   //                           svg:y="<ViewBoxY>",
   //                           svg:width="<ViewBoxL>",
   //                           svg:height="<ViewBoxH>",
   //                           svg:viewBox="0, 0, <ViewBoxL>, <ViewBoxH>",



See the following code
========================

procedure Todg.DessinPolyBezier(const QStyle: string; var BezierArcs: array of TBezierArc; const QClosed, QFilled: boolean);
const
   FM1 = '%.0f, %.0f ';
 var
   ViewBoxX, ViewBoxY, ViewBoxL, ViewBoxH: TScalaire;
   BB_X1, BB_Y1, BB_X2, BB_Y2: TScalaire;
   Qx1, Qy1, Qx2, Qy2: TScalaire;
   i:integer;
   coords: String;
   sname: String;
   EWE: TBezierArc;
   WU: String;
   // translater les arcs pour les mettre dans la ViewBox
   procedure TranslaterArcs;
   var
     BBCx1, BBCy1, BBCx2, BBCy2 : TScalaire;
     MonArc: TBezierArc;
     A: Integer;
     procedure Miou(const qx, qy: TScalaire);
     begin
       BBCx1 := min(qx, BBCx1);
       BBCx2 := max(qx, BBCx2);
       BBCy1 := min(qy, BBCy1);
       BBCy2 := max(qy, BBCy2);
     end;
   begin
     // Etape 1: Calcul des min/max
     BBCx1 := +BIGMAX;
     BBCy1 := +BIGMAX;
     BBCx2 := -BIGMAX;
     BBCy2 := -BIGMAX;
     for A := 0 to High(BezierArcs) do
     begin
       MonArc := BezierArcs[A];
       Miou(MonArc.P1.X, MonArc.P1.y);
       Miou(MonArc.PC1.X, MonArc.PC1.y);
       Miou(MonArc.PC2.X, MonArc.PC2.y);
       Miou(MonArc.P2.X, MonArc.P2.y);
     end;
     // Etape 2: Paramétrage des viexbox
     ViewBoxX := BBCx1;
     ViewBoxY := BBCy1;
     ViewBoxL := BBCx2 - BBCx1;
     ViewBoxH := BBCy2 - BBCy1;
     // Etape 3: Translation
     for A := 0 to High(BezierArcs) do
     begin
       MonArc := BezierArcs[A];
       MonArc.P1.X    := MonArc.P1.X - ViewBoxX;
       MonArc.P1.Y    := MonArc.P1.Y - ViewBoxY;
       MonArc.PC1.X   := MonArc.PC1.X - ViewBoxX;
       MonArc.PC1.Y   := MonArc.PC1.Y - ViewBoxY;
       MonArc.PC2.X   := MonArc.PC2.X - ViewBoxX;
       MonArc.PC2.Y   := MonArc.PC2.Y - ViewBoxY;
       MonArc.P2.X    := MonArc.P2.X - ViewBoxX;
       MonArc.P2.Y    := MonArc.P2.Y - ViewBoxY;
       BezierArcs[A] := MonArc;
     end;
   end;
   procedure DrawArc2(var QCoords: string; const BA: TBezierArc; const Idx: integer);
   begin
     if (Idx = 0) then QCoords += Format('M ' + FM1, [BA.P1.X, BA.P1.Y]);
     QCoords += Format('C ' + FM1, [BA.PC1.X, BA.PC1.Y]);
     QCoords += Format(FM1, [BA.PC2.X, BA.PC2.Y]);
     QCoords += Format(FMT_2V_CMM, [BA.P2.X, BA.P2.Y]);
   end;
Begin
   Inc(FNbCourbesBeziers);
   // Pour dessiner un Polybezier, il est nécessaire d'utiliser ViewBox
   // Etapes:
   // 1 - Calculer les paramètres de la viewbox
   // -- Rechercher les maxi et mini des points de controle
   // --> on obtient ViewBoxX, ViewBoxY, ViewBoxL, ViewBoxH
   // 2 - Translater les points avec tx = -ViewBoxX et ty = -ViewBoxY
   // 3 - Tracer la courbe avec svg:x="<ViewBoxX>",
   //                           svg:y="<ViewBoxY>",
   //                           svg:width="<ViewBoxL>",
   //                           svg:height="<ViewBoxH>",
   //                           svg:viewBox="0, 0, <ViewBoxL>, <ViewBoxH>",
   // calcul des translations
   TranslaterArcs;
   // composition du texte à passer en paramètre de path=""
   if (QFilled) then coords := 'M 0 0 Z '    // tout polygone (ou courbe) rempli est encadré de deux Z en début et en fin de chemin
                else coords := 'M 0 0 ';     // Le Z de tête indique que l'objet est rempli
                                             // Le Z de queue indique qu'il est fermé
                                             // L'objet n'est rempli que s'il y a un Z de queue
   for i := 0 to High(BezierArcs) do
   begin
     EWE := BezierArcs[i];
     DrawArc2(coords, EWE, i);
   end;
   // polygone fermé ?
   if (QClosed) then coords += 'z';
   // début de la chaine de paramètres
   WU := '';
   AddAttribute(WU, 'draw:style-name', QStyle, false);
   AddAttribute(WU, 'draw:name', Format('Polybezier%d', [FNbCourbesBeziers]),false);
   AddAttribute(WU, 'svg:x'          , Format(FMT_CMM, [ViewBoxX]), false);
   AddAttribute(WU, 'svg:y'          , Format(FMT_CMM, [ViewBoxY]), false);
   AddAttribute(WU, 'svg:width'      , Format(FMT_CMM, [ViewBoxL]), false);
   AddAttribute(WU, 'svg:height'     , Format(FMT_CMM, [ViewBoxH]), false);
   AddAttribute(WU, 'svg:viewBox'    , Format(FMT_4V_CMM, [0.0, 0.0, ViewBoxL, ViewBoxH]), false);
   AddAttribute(WU, 'svg:d', coords, false);
   BeginSection(FPointerToFileContent, 5, 'draw:path', WU);
     WriteLnUTF8(FPointerToFileContent, StringOfChar(' ', 6 * 2) + '<text:p/>');
   EndSection(FPointerToFileContent, 5, 'draw:path');
   // move to last point for further painting with lineto
   DessinMoveTo(EWE.P2.x, EWE.P2.y);
 end;

@Regina: You can find it starting at the four cases 'OWN_ATTR_BASE_GEOMETRY', two for bezier and two for non-bezier. These use SdrPathObj::TRGetBaseGeometry and SdrPathObj::TRSetBaseGeometry where on the latter you can read in the comment:

// sets the base geometry of the object using infos contained in the homogen 3x3 matrix. 
// If it's an SdrPathObj it will use the provided geometry information. The Polygon has 
// to use (0,0) as upper left and will be scaled to the given size in the matrix.

Thus, to write the polygon in the currently used form in ODF, you need two things:
(a) the normalized, but scaled PolyPolygon [0.0 .. fabs(scale)] in X and Y
(b) the complete transformation

The viewBox will then use the [0.0 .. fabs(scale)] range and the polygon will also use that form. This makes the PolyPolygon definition in the ODF independent of the transformation, except the scale which is there for historical reasons (already explained).

There is one more caveat with beziers: the BoundRect and thus the scale used for the bezier is without the control points (also due to what the original implementors did). This makes the BoundRect unfortunately non-trivial to calculate since the bezier needs to be split at it's exreama points to getthe correct BoundRect. See getRange and getRangeWithControlPoints in basegfx for more info.
History: When the integer polygons were used this was done by using a copy of the polygon converted to line segments, with the result that the BoundRect was always wrong by +-10 100thMM units since that adaptive subdivision is always only a approximation. Thus, the geometry changed slightly at each save/reload...

Again: Please do *not* add a 'M 0 0', this will add extra geometry to the polygon which leads to other problems later. This was already done in other circumstances and is known to have its own problems.
What you do when adding this is to make the polygon seemingly conform to the requirements by always giving it a minX and minY at (0,0), but thats not a good thing to do. Instead, the transformation and viewBox needs to be correctly adapted, e.g. for a polygon having a BoundRect with a MinX of 100 and a minY of 200 to really use that in the transformation ad viewBox. This is not done in the nitial examples and that is the problem there.

@JP CASSOU: Looks pretty good, but as desribed above: The BoundRect needs to be the BoundRect of the curve without the control points, but including the extremas. This makes it not easy to calculate, but is what the initial implementors defined. To see how to do that, plesae refer to the module 'basegfx' of AOO, the classes B2DPolygon, B2dPolyPolygonn and the methods getRange and getRangeWithControlPoints there. These use a temporary split of the bezier at the exreamas to ensure the correct calculation of the BoundRect without the control points.

HTH!

taking over for verification

Unfortunately, I am not experienced enough to verify a possible fix.

I am also not sure, if this issue has been fixed as I did not see any code changes.

Can somebody please clarify the status of this issue? Thx in advance.

No, it has not been fixed. It is not even clear where to fix it, because changes in SdrPathObj::TRGetBaseGeometry and SdrPathObj::TRSetBaseGeometry would have deep impacts and solutions might be different for AW080, and such cases only come from foreign application or manual manipulations. Please ask Armin.

There is nothing to fix currently, all works well in AOO (and others based on that code do probably still the same). This task is about clarification what exactly is currently done when representing polygon shapes in ODF to allow others more easy to also use this shape type. Of course aw080 will do some stuff differently, but will in principle keep the pair of normalized, scale dpolyfon and transformation. It cannot be changed in existing files and to keep compatible it should not. There is also no reason for a change it works as intended (by the original implementors, of course).

To make things more clear, another explanation (hopefully simpler):

The representation of a draw shape containing a path in ODF format uses basically two objects:

(a) The DrawObject itself containing the object transformation, either as x,y,width,height or as transformation if more aspects as translate and shear are used.
It has to be split to x,y and a transformation part when mirror x/y (negative scales), rotation or shear is used. This is because:
The object transformation is seen as the transformation from object unit coordinates (the object is at 0,0 and has size 1,1) to the target transformation; the x,y represents the translation, the width,height the scale. There is a logical order in which the aspects are applied:
- scale
- mirror
- shear
- rotate
- translate
This comes from simple considerations; scaling only works as long as the object is at (0,0); shear and rotate has to be done around top-left, so this has to be (0,0); shear defines the relation between the X and Y axes, thus is only useful before rotation.
In ODF when x,y, width, height and transformation would be used the order of applying to builtd the whole object transformation would be: scale width/height, move x,y, apply transformation. The problem here is that translation would be added *before* the transformation which contains evtl. rotation/shear. To solve this, x,y has to be 0 in those cases and the translation part has to be moved to ODFs transformation part to apply it after the roation/shear parts.

(b) the path geometry. This is in principle the path geometry in object unit coordinates (the object is at 0,0 and has size 1,1). For historical reasons (integer transport) the scale is included, so path data has a bound rect of (0, 0, width, height). This also defines that the path geometry always has a minX and minY of 0.
Imagine the path geometry being transformed by first removing it's own scaling again and then applying the complete object transformation (see (a)).
The viewBox part of this then also has a x,y of 0, thus this is practically not used in ODF. The only part of viewBox that is used is the width/height part. If this is different from the objects (a) width,height a scaling will be applied to the path geometry.
The BoundRect used from the path geometry is *without* control points for historical reasons; to calculate this see tipps in comments above. Do *not* simply build the BoundRect including the control points if a bezier is used (!)

I hope this makes the relationships clearer and why viewBox is a not needed part in ODF for path objects at all.

Some additions: (a) is pretty much the same for all draw objects represented in ODF. The need to add stuff of the complete object transformation happens in two cases, so with the 'normal' case that there is just scale and transformation (without mirroring) there are three:

(1) transformation has scale without mirror (positive in X and Y) and translation: AOO will use svg:x, svg:y, svg:width and svg:height, no transformation will be written

(2) transformation uses mirror (scale X and/or Y is negative; addon: when X and Y is negative, it's equal to a 180 deg rotation, rotation is preferred): AOO will use svg:width and svg:height for scale (in absolute values since svg does not allow negative values for these), svg:x and svg:y will not be used (or zero), a transformation containing mirror (scale -1 in X or Y) and the translation (which would have been in svg:x and svg:y)

(3) transformation uses mirror, and/or shearX (shearY also possible, but can be expressed as rotation 90 deg and shearX combination, so shearX is preferred) and/or rotation: AOO will use svg:width and svg:height for scale absolute values, svg:x and svg:y will not be used (or zero), a transformation containing mirror (scale -1 in X or Y), shearX, rotation and the translation (which would have been in svg:x and svg:y)

For paths, (b) is simply to be seen as an addon carrying the path data in normalized object coordinates (plus the exception with the positive scale). In normalized object coordinates to have this data as independent from the object as possible (added zoom for historical reasons is the exception here).

Reset the assignee to the default "issues@openoffice.apache.org".