Ungrouping loses Transformation

[GoogleCodeExporter]

What steps will reproduce the problem?
1. draw 2 rectangles
2. group them together
3. rotate the group
4. ungroup
5. move either rectangle - the rectangle is no longer rotated

What is the expected output? What do you see instead?
The transforms should retain for each entity
The rectangles should remain rotated - even though you ungroup them.

In what browser did you experience this problem? (ALL, Firefox, Opera, etc)

ALL

In what version of SVG-edit does the problem occur? (Latest trunk, 2.5.1,
etc)

2.5.1

Please provide any additional information below.

The real problem is how svg-edit handles transforms.
In particular, in svgcanvas.js:recalculateDimensions(elem) is trying to 
simplify all entities.
It tries to remove empty transforms and remove a translate transform and 
instead moves the individual points on the rectangle.
I've written many graphics engines over the years and this logic is just wrong.
Each svg entity supports a transform attribute and this should be a simple 
matrix.
When you apply a transform to an svg entity, it should simply perform a matrix 
multiply of the transform, whether it is a rotate, scale or translate.
There should be no code trying to decompose and simplify a matrix - this is the 
beauty of a matrix - it requires the same amount of computation to render, no 
matter how many transforms have been applied. Decomposing a matrix is very 
difficult, error prone and will not work in all cases.

Another simple example would be drawing a line. When you move that line, the 
points on the line shouldn't change - instead a translate transform should be 
applied. This is what happens when you rotate a line. However, when you move a 
line, the individual points of the line are manipulated.

Original issue reported on code.google.com by d...@stardraw.com on 12 Jan 2011 at 6:54

[GoogleCodeExporter]

I'm not seeing the behavior you describe, the rectangles remain rotated when I 
try it. Could you tell me what browser exactly this occurs in?

I understand how the recalculateDimensions logic can seem wrong, however in 
order to preserve things like the rotation angle as well as the "rotate()" 
transform value in the source, this method is necessary. It's been our goal to 
keep our markup as lean and human-readable whenever possible, so we try to 
avoid matrix transforms (but accept that they are necessary after certain 
transforms).

So we feel that when rotating a line, the user is likely to want to keep the 
angle value intact, whereas for a move it makes more sense to just relocate the 
points than adding extraneous transform values. 

Original comment by adeve...@gmail.com on 12 Jan 2011 at 8:28

[GoogleCodeExporter]

I've just tested it again in FireFox 3.6.13 and Google Chrome 8.0.552.224 and 
IE8 with Chrome Frame and Safari 5.03

When I group 2 rectangles together, I get:
<svg width="640" height="480" xmlns="http://www.w3.org/2000/svg">
 <!-- Created with SVG-edit - http://svg-edit.googlecode.com/ -->
 <g>
  <title>Layer 1</title>
  <g transform="rotate(58.24860382080078 386.0000000000001,190.49999999999991) " id="svg_9">
   <rect id="svg_7" height="102" width="158" y="144" x="173" stroke-linecap="null" stroke-linejoin="null" stroke-dasharray="null" stroke-width="5" stroke="#000000" fill="#FF0000"/>
   <rect id="svg_8" height="106" width="197" y="135" x="402" stroke-linecap="null" stroke-linejoin="null" stroke-dasharray="null" stroke-width="5" stroke="#000000" fill="#FF0000"/>
  </g>
 </g>
</svg>

If you then ungroup and move a single rectangle, you will see it will lose the 
rotation.

In all my 30 years of graphics experience, I've never seen a graphics engine 
try to unpick arbitrary transforms on arbitrary entities - you will always find 
a case where it just won't work. You won't have any extraneous transform values 
as it will always be a single matrix. You should always have the coordinates of 
the entities in model space and the transform transforms those coordinates to 
world space and the view matrix transforms world space to device space. This is 
graphics 101.

I love the program and we are very serious about customizing it for our 
clients, but this fundamental issue of how it deals with transforms means I'm 
either going to have to abandon the project or make some serious modifications 
- or hopefully, convince you, or someone else to make the necessary changes.

Original comment by d...@stardraw.com on 12 Jan 2011 at 10:27

[GoogleCodeExporter]

Hi Dave,

Are you using the SVG-edit being served from GoogleCode or your own (perhaps 
modified) version?

I have just tried this myself from the version at 
http://svg-edit.googlecode.com/svn/trunk/editor/svg-editor.html on Chrome in 
both OSX and Windows.  As with Alexis, I do not see the behavior you're 
describing.  I draw two rectangles, group them, rotate the group and get the 
SVG that you show above.

Then I ungroup and get:

<svg width="640" height="480" xmlns="http://www.w3.org/2000/svg">
 <!-- Created with SVG-edit - http://svg-edit.googlecode.com/ -->
 <g>
  <title>Layer 1</title>
  <rect transform="rotate(44.79165267944336 320.0675964355469,121.3882064819336) " id="svg_1" height="43" width="105" y="99.88821" x="267.56759" stroke-width="5" stroke="#000000" fill="#FF0000"/>
  <rect transform="rotate(44.79165267944336 409.2241821289063,332.4903259277344) " id="svg_2" height="55" width="133" y="304.99032" x="342.72417" stroke-width="5" stroke="#000000" fill="#FF0000"/>
 </g>
</svg>

Moving either rectangle keeps the transform="rotate()" on the rectangle and 
updates its x/y.

Can you look at the JS console of the browser and see if there are any JS 
errors perhaps?  I wonder if some extension is causing a problem maybe... 
(please don't install the Firefox SVG-edit extension, it hasn't been updated in 
a long while).

As for your suggestion to change how recalculateDimensions() works - I'm 
actually open to the idea.  I spent way too long trying to get it to work so 
that we wouldn't have arbitrary matrix transforms on every single element that 
is manipulated - basically because I found it distasteful in the markup.  But 
in retrospect I would avoid doing it as I spent way too long on it and there 
are probably cases where it doesn't work right like you mention.  I also assume 
it would be a performance improvement since we wouldn't have to call 
recalculateDimensions() on every single mouseup.

Original comment by codedr...@gmail.com on 12 Jan 2011 at 11:02

[GoogleCodeExporter]

I was using:
http://svg-edit.googlecode.com/svn/branches/2.5.1/editor/svg-editor.html
(I did say I was using 2.5.1).
I just tried it in
http://svg-edit.googlecode.com/svn/trunk/editor/svg-editor.html
and it seems to work correctly.

If you like, I can resolve this as fixed in the alpha.
However, it doesn't solve the fundamental issue of how svg-edit deals with 
transforms.
Matrix Multiplication is not commutative.
For example, A translate followed by a rotate is NOT the same as a rotate 
followed by a translate.
All the code I have seen in the 2.5.1 version of recalculateDimensions seems to 
think it is the same as it tries to unpick parts of a sequence of transforms.
It would also be a performance improvement not only in terms of not having to 
call recalculateDimensions on each mouseUp, but the SVG renderer would only 
have one transform matrix to deal with rather than a translate and a rotate.

I have modified my code to not call recalculateDimensions and to instead call 
getTransformList(...).consolidate which reduces the matrix down to something 
like:

transform="matrix(-0.43458, -1.48753, 2.69948, -0.239472, -167.478, 750.936)"

Some might call this ugly - I call it elegant and efficient.
If you really wanted to make this pretty, then you could do some post 
processing and if the matrix began with 1,0,0,1 then you could take the last 2 
values and convert it to a simple translate transform, but IMO this is just 
being vain as I don't really care how the XML looks - I care about it being 
small, fast and efficient.

Original comment by d...@stardraw.com on 12 Jan 2011 at 11:21

[GoogleCodeExporter]

Yeah that's really disconcerting, since I tried it both on the trunk and 2.5.1 
and never saw an issue with ungrouping a rotated group then moving its pieces 
around.  I cannot fathom why you would see different behavior between 2.5.1 and 
2.6.

Yes, I'm aware that matrix multiplication is not commutative :)

Original comment by codedr...@gmail.com on 12 Jan 2011 at 11:50

[GoogleCodeExporter]

As for XML being small, fast, and efficient:

<rect x="105" y="5" width="100" height="200" />
<rect x="5" y="5" width="100" height="200" transform="translate(100,0")/>
<rect x="5" y="5" width="100" height="200" transform="matrix(1,0,0,1,100,0)"/>

I know which one I prefer :)

Original comment by codedr...@gmail.com on 12 Jan 2011 at 11:56

[GoogleCodeExporter]

I've recorded a youtube video that shows the problem:
http://www.youtube.com/watch?v=wapgvVBSI3c

The svg that I pasted in was:

<svg width="640" height="480" xmlns="http://www.w3.org/2000/svg">
 <!-- Created with SVG-edit - http://svg-edit.googlecode.com/ -->
 <g>
  <title>Layer 1</title>
  <g transform="rotate(58.24860382080078 386.0000000000001,190.49999999999991) " id="svg_9">
   <rect id="svg_7" height="102" width="158" y="144" x="173" stroke-linecap="null" stroke-linejoin="null" stroke-dasharray="null" stroke-width="5" stroke="#000000" fill="#FF0000"/>
   <rect id="svg_8" height="106" width="197" y="135" x="402" stroke-linecap="null" stroke-linejoin="null" stroke-dasharray="null" stroke-width="5" stroke="#000000" fill="#FF0000"/>
  </g>
 </g>
</svg>

You will notice that at the end of the video, the transform rotate has been 
removed from the rects.

As for being pretty, you can still do some safe massaging of the transforms but 
I still claim it is unsafe to unpick arbitrary transforms.

Here's the real problem I am trying to solve...

I have a database of images - plain old svg files that are in model space.
I have a "drawing" table in that database that specifies which images are in 
which location represented by a transform.
When I load the drawing, I enumerate that table and pull down the images and 
apply a transform and add my own id to the <g>
When the <g> is moved or scaled or rotated or whatever, I update my drawing 
table with the new transform for that image.
However, with the way that you have implemented transforms, a plain translate 
is not represented by a transform as you instead update the model space 
coordinates.
It just feels that you are violating fundamental graphic principles by trying 
to generate "nicer" human readable xml which can be achieved in a much easier, 
cleaner and safer way than trying to modify model space coordinates.

Original comment by d...@stardraw.com on 13 Jan 2011 at 12:54

[GoogleCodeExporter]

Okay, it would appear the bug is actually unrelated to the transform system, 
instead the problem lies with the "null" value attributes. Were these generated 
by SVG-edit? If so, would you be able to reproduce how? Quick attempts by me 
were unable to do.

Dealing with invalid values has been improved in 2.6, so that's why the bug 
doesn't appear there.

Original comment by adeve...@gmail.com on 13 Jan 2011 at 2:05

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Ah - thanks for clearing that mystery up, Alexis!

"It just feels that you are violating fundamental graphic principles by trying 
to generate "nicer" human readable xml which can be achieved in a much easier, 
cleaner and safer way than trying to modify model space coordinates."

I get where you're coming from.  I can't speak for Alexis, but I'm not totally 
adverse to changing the way transforms work these days if they came in the form 
of patches, especially if it was configurable behavior.  
recalculateDimensions() is something like 800 lines of JavaScript and is 
probably the single biggest function in the codebase and certainly would be my 
pick for least favorite piece to work on given its complexity and age :)

Original comment by jeffschi...@google.com on 13 Jan 2011 at 4:02

[GoogleCodeExporter]

Wouldn't mind an overhaul of recalculateDimensions either, but I do think 
there's quite some value in having things work the way they do now. 
Specifically the rotation angle is useful to keep around and using a matrix 
transform for this means that the actual angle that was specified is lost. This 
may not be important in many cases, but it seems to me it could often be of 
value to the user.

I'm also definitely against adding transforms that could be combined with the 
element's data, and should note that both Illustrator as well as Inkscape do 
this whenever possible. So if that's what counts as "violating fundamental 
graphic principles", then two of the most popular (if not THE most popular) 
vector graphics editors must be violators of this as well.

It should also be noted that we have been using our methods based on user 
feedback appreciating the clean markup, while I believe you are the first to 
express a problem with the methods used. But if you think similar clean output 
can be achieved more efficiently, I too am open to receiving patches to do so.

Original comment by adeve...@gmail.com on 13 Jan 2011 at 6:03

[GoogleCodeExporter]

You can calculate the rotation angle by transforming a (1,0) point through the 
objects transform matrix and calculating the resulting angle created by the 
difference with the third point being (0,0)

I'm not sure what you mean by "against adding transforms that could be combined 
with the element's data". All transforms are combined with their elements data 
- that's the whole point.

Matrix Transformations are used everywhere in graphics from Model to World to 
View to Device Space. Graphic Processors are designed to process transforms in 
their silicon and so are blindingly fast. Because you can combine matrices, you 
can go from Model Space to Device space in one transform.

As for Inkscape, if you draw 2 rectangles, group them together and rotate them 
and then ungroup them, it creates the following svg:

    <g
       id="g3001"
       transform="matrix(0.68492576,0.72861287,-0.72861287,0.68492576,433.62772,-69.473371)">
      <rect
         y="383.79074"
         x="61.42857"
         height="168.57143"
         width="184.28572"
         id="rect2997"
         style="fill:#0000ff;fill-rule:evenodd;stroke:#000000;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1" />
      <rect
         y="379.50504"
         x="304.28571"
         height="174.28572"
         width="228.57143"
         id="rect2999"
         style="fill:#0000ff;fill-rule:evenodd;stroke:#000000;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1" />
    </g>

Note the transform="matrix(...)"

IMHO, you should NEVER modify the model space coordinates, unless, of course, 
you are actually modifying the model. However, because you ALWAYS modify the 
model space coordinates, it is now impossible to implement a hierarchical 
graphics model. One such example would be support <use> so that we can have 
re-usable svg elements which is critical in a large drawing such as doors, 
windows, walls, chairs, tables, etc. etc.

<sarcasm>
Why stop at world space? Why not do away with a viewbox and convert the model 
space coordinates into view space?
</sarcasm>

Perhaps the easiest solution to all of this is to not traverse below a <g> when 
modifying model coordinates. I think this would make everyone happy.

Original comment by d...@stardraw.com on 13 Jan 2011 at 7:37

[GoogleCodeExporter]

Sorry, that svg was before the ungroup.
After the ungroup, it is:

<g
     inkscape:label="Layer 1"
     inkscape:groupmode="layer"
     id="layer1">
    <rect
       style="fill:#0000ff;fill-rule:evenodd;stroke:#000000;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
       id="rect2985"
       width="274.28571"
       height="297.14285"
       x="-288.97754"
       y="199.10677"
       transform="matrix(0.8358914,-0.54889486,0.54889486,0.8358914,0,0)" />
    <rect
       style="fill:#0000ff;fill-rule:evenodd;stroke:#000000;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
       id="rect2987"
       width="322.85715"
       height="325.71429"
       x="-66.120377"
       y="570.53534"
       transform="matrix(0.8358914,-0.54889486,0.54889486,0.8358914,0,0)" />
  </g>

Again, note the transform="matrix(...)"

Original comment by d...@stardraw.com on 13 Jan 2011 at 7:58

[GoogleCodeExporter]

I think I agree with the "not traverse below a <g> when modifying model 
coordinates" though we definitely don't modify the referenced content of a 
<use> at the moment (which would be nonsensical).  We'd have to agree on what 
to do if ungrouped.

As previously stated, patches are certainly welcome in this area (especially if 
the behavior is configurable and even more so if unit-tested).  That would get 
rid of about 350 lines of JS...

Original comment by codedr...@gmail.com on 14 Jan 2011 at 3:15

[GoogleCodeExporter]

In my test I used <path>s rather than <rect>s, which don't leave any matrix() 
transforms in Inkscape/Illustrator. Indeed for <rect>s in SVG-edit, when a 
transformation results in a skewed transform, the element uses a single 
matrix() instead.

I believe the reason we decided to modify the model coordinates for children of 
a group rather than the group itself was so elements with strokes would look 
the same before and after ungrouping, and possibly to make ungrouping 
easier...don't recall the exact details, but I know we spent quite some time 
discussing it before deciding it made most sense (at the time anyway). 

Original comment by adeve...@gmail.com on 14 Jan 2011 at 4:18

[GoogleCodeExporter]

I think there is a bigger issue in general that should be re-dressed....

svg-edit should leave my **** svg markup alone!!

We create specific svg elements with specific ids, transforms, attributes, 
coordinate space, groupings etc. etc.
It's very important to keep the structure because this directly maps onto 
underlying business logic.
For example, when an element is added, moved and deleted, we catch these 
events, analyze the svg and update the back-end database accordingly.

However, svg-edit relies on a specific use of transforms and cannot seemingly 
work with any arbitrary svg and so must convert that svg to something that it 
does like.

For example, svgcanvas.js is littered with code that relies on the rotation of 
an entity to be defined by a rotation transform in the list of transforms for 
that entity - have a look at getRotationAngle and how many times it is called. 
If the transform attribute does not contain a rotate transform, then it thinks 
it has no rotation. Even worse, if it has multiple rotations, it only returns 
the first one it finds and if it contains a matrix transform that may have a 
rotation in it, it just returns 0 angle.

Is the philosophy of svg-edit to create clean markup from a blank canvas and to 
even cleanup imported svg files?
Or, is it to take any arbitrary svg file and allow modification of it with the 
least amount of alteration of that file?

I am willing to submit improvements/patches to svg-edit, but not if it has a 
different goal that is incompatible with our project so that we cannot use the 
end result.

Original comment by d...@stardraw.com on 14 Jan 2011 at 4:47

[GoogleCodeExporter]

As I've stated, I agree that our transformation handling and manipulation is 
brittle.  I think our existing code could be improved to be more robust in its 
handling of rotational transforms, and I've already stated that an overhaul of 
the existing behavior is not out of the question from my perspective.

SVG-edit started as a very tiny editor project and has grown iteratively over 
the years.  

In fact, thanks to online hosting, you can actually play with all versions 
using just your browser.

Version 1.0: http://svg-edit.googlecode.com/svn/branches/1.0/svg-editor.html 
- had no concept of even selecting elements, importing SVG, etc.  It was 
basically a toy for quick sketching.

Version 2.0: http://svg-edit.googlecode.com/svn/branches/2.0/svg-editor.html
- provided a nice refactoring of the code, but no new functionality
- still not at all usable as a true vector graphics editor (still no selection 
of elements drawn)

Version 2.1: http://svg-edit.googlecode.com/svn/branches/2.0/svg-editor.html
- I became involved soon after 2.0 was released, this is when I started 
contributing patches to the project and took ownership for a time
- added selection and dragging of elements.  As you can see, the shapes are 
simple (ellipse, rectangle, free-hand scribble) and were easy enough to change 
geometry once the drag operation completed.  This was where my desire for clean 
markup was born since I was frustrated with Inkscape and Adobe Illustrators 
bloated markup.  See http://codedread.com/scour for a related project.
- still no concept of importing any existing SVG here

Version 2.2: 
http://svg-edit.googlecode.com/svn/branches/2.2/editor/svg-editor.html
- major feature in this release was undo/redo functionality (on top of a large 
re-skinning of the app)
- also allowed resizing of selected elements, more code got added to 
recalculateDimensions()

Version 2.3: 
http://svg-edit.googlecode.com/svn/branches/2.3/editor/svg-editor.html
- now allowed rotation of elements, more code added to recalculateDimensions()
- also brought into the concept of a source editor so you could finally paste 
in SVG from the outside for the first time

Version 2.4: 
http://svg-edit.googlecode.com/svn/branches/2.4/editor/svg-editor.html
- arbitrary path drawing now
- grouping exists for the first time, more code added to 
recalculateDimensions() for handling of group transforms

Version 2.5: 
http://svg-edit.googlecode.com/svn/branches/2.3/editor/svg-editor.html
- extensions, radial gradients
- first version to support opening local SVG files (in some browsers) and 
importing arbitrary SVG (though by all means not perfect)
- my involvement with the project dropped off significantly at this point and 
Alexis now owns the project.

My point of this is not to give a boring history lesson but to illustrate that 
the codebase has been iteratively improved with each release as new features 
were added.  It's not necessarily the cleanest code, I agree.  We have not had 
the opportunity yet to give an overhaul to the transformation system, it was 
built over several releases with me just focusing on getting each feature 
implemented and working.  One thing that's deliberately missing is skew 
transforms.  If the transformation system is overhauled, these would be trivial 
to implement.

Like many open source projects, we've always lived by the philosophy that "code 
talks".  If you're really passionate about some feature, the best way to make 
it show up in the project is by hacking on it and delivering a patch.  You 
needn't worry about us not accepting your patches, if it makes the editor 
better, we almost always accept patches.  And even if we didn't, you are still 
free to fork the entire codebase (it's under Apache 2 License) and go your own 
way with it, open source or not.

You should also know that no one on the project gets paid to work on it, this 
is very much a labour of love.

Hopefully that gives you some perspective on the project, why it does the funky 
things it does and whether you want to contribute.

Original comment by codedr...@gmail.com on 14 Jan 2011 at 9:42

[GoogleCodeExporter]

Hi all,
I found SVG-edit, the best among all browser based SVG editors.
However,I too encountered similar problem with transformation. Can anyone 
please suggest me some way, to fix this issue?

Original comment by momin.ak...@gmail.com on 14 Dec 2012 at 6:45