2009/8/6 Krzysztof Kosiński <tweenk.pl@...400...>:
Why LPE on a group is a hack? It is simple and valid SVG, and it works.
Again: the path-to-path limitation is indeed a limitation, but that's by design. These are PATH effects, and the entire API is oriented this way. For different things, invent different concepts.
You just answered yourself: the current repr would be satisfactory if LPEs were a strict path-to-path mapping, but they are not. For example, Bend is not a path-to-path mapping, it is two-paths-to-path mapping. A group is not a path, so LPE on a group doesn't fit the path-to-path paradigm either. I would have no problems with the current representation if it was really intended to be used for pure path effects, but it is not.
OK, I stand corrected: our LPEs are not path-to-path. They are more like anything-to-path. That anything can be a single path, multiple paths with different roles, links to paths, or any other data. For example, it is easy to imagine a LPE for variable-width strokes where the widths are recorded as an array, stored in the path effect in an attribute. The output path will then depend on this data. Yes, we will need a new parameter type and a new attribute for each new kind of such data, but adding new parameter types is relatively straightforward thanks to Johan's code. And you would need exactly the same - a new class and a new attribute for a new kind of source data - with your representation. So I don't see any gain here.
If all you do with your proposal is extend source objects from paths to paths plus shapes, then you don't need to bother - as I said, we already support LPE on shapes (those that use svg:path, at least, which means all except rect for now).
The old repr requires that we turn all input shapes into paths and preserve editing capability using extra attributes,
You mean, requires that we turn the only remaining one, rect, to using paths _when necessary_. This is not such a bad thing at all, because it gives us other advantages beyong LPEs, see (*) below.
so the manipulators need to know how to edit both those representations; in the new one they only need to know how to edit the correct semantic representation (svg:rect or svg:ellipse).
Yes, and once we have implemented and tested code for different-XML-depending-on-features on rect, we can reuse the same approach for ellipses too, and make them use svg:ellipse when this is possible and svg:path otherwise.
Yes, it can be MUCH simpler, because the manipulators can be created for the internal XML elements.
Just like the current nodepath can also be fed a path in defs, and edit it just fine :)
That's right - it's proven that editing invisible objects that have normal XML elements is simple, so why not take advantage of this for path effects as well, rather than inventing clever ways to preserve editability?
There's nothing especially clever about that - we just tell it to read and write to a different attribute (possibly of a different element) instead of d on svg:path. It's no big deal really. I don't remember the details but I think most of deciding what to use is done by the LPEItem itself, not by node tool - the node tool just asks the LPEItem for its parameter paths if any and uses what it gets in return.
svg:switch inkscape:envelope-deformation <svg:g inkscape:param-name="object"> svg:switch inkscape:pattern-along-path <svg:path inkscape:param-name="path" id="pathA" ... /> <svg:use inkscape:param-name="pattern" href="url(pathD)" ... /> </inkscape:pattern-along-path> <svg:path ...(result of pattern along path)... /> </svg:switch> svg:switch inkscape:bend <svg:path inkscape:param-name="path" id="pathB" ... /> <svg:use inkscape:param-name="bend-path" href="url(#pathC)" ... /> </inkscape:bend> <svg:path ...(result of bend)... /> </svg:switch> </svg:g> <svg:use inkscape:param-name="envelope-bottom" href="url(#pathC)" ... /> </inkscape:envelope-deformation> <svg:path ...(result of envelope)... /> </svg:switch>
1. The single result path does not allow different styles on the two paths, as in my original description. You will have a single-colored path while I have one red and one green, both under the same envelope. If you just add a second result path to the top-level switch, it will be extremely inelegant because it will not show the correspondence between the source paths in one branch and result paths in the other. That red transforms to red and green to green will then be an "implied knowledge" - something we should avoid. The flow of information in the document should be obvious from its structure and nothing else.
2. svg:use has its cost. Inkscape clones have a lot of code for behavior when the original is transformed or deleted. I guess most of this will not apply here where the use is only for linking to a path, so it will have to be special-cased in many places. Not nice.
3. Using inkscape-specific container elements like inkscape:envelope-deformation which contain normal SVG elements is at least risky. So far we only had extension stuff in attributes or in terminal elements. The only exception is flowtext (again, because we considered it standard SVG when it was implemented) which can contain shapes for the text flow as descendants - and it's not pretty. There's a lot of special-casing for this in hundreds of tree traversals we have all around the code, and some problems with transforms are still not resolved. What you propose amounts to adding more such stuff, one new Inkscape element type per LPE, which will be hardly maintainable. Also this violates the XML philosophy which says, "ignore anything you don't understand." If you have container elements in Inkscape namespace, other renderers will have to _descend_ into these elements instead of ignoring them completely. Yes, they are only present in the Inkscape branch of a switch, but it's a violation nevertheless.
4. Nested switches may not be wrong but they are at least weird, similar to:
if (x) { if (x) { ... } else ... } else ...
They are also inefficient, not because the same condition is evaluated multiple times (that is OK), but because the internal else is never reached no matter what. Neither Inkscape nor other renders will ever see the result paths in your nested switches. Yet, you will spend resources of generating and updating those elements, and I can tell you that XML tree updates and especially path attribute generation and parsing are very slow operations. The current implementation avoids this - path data is passed inside a stack of LPEs without serialization.
In the new repr every parameter can be given either 'by value' (by using it directly) or 'by reference' (by using its clone). By the way, if some other version of Inkscape does not understand e.g. inkscape:bend, then it could use its result path and only the bend part would not be editable, instead of the entire effect. Right now it seems that even if only one effect in the stack is not recognized, the entire stack is not editable.
Yes, this is indeed an advantage of your approach - due to the inefficiency of serializing the intermediate stages that I just described, you can recover partial stacks where some effects are broken or unknown. However I'm not sure it's such a big advantage: I don't think people would want to edit the remaining LPEs in such a broken stack. They would either upgrade Inkscape if they want to edit the entire stack properly (btw we can record the version-minimum attribute for each LPE and give a friendly suggestion), or they would just deal with the result path as a regular path without any LPEs, as the current Inkscape treats it.
Having multiple knotholders isn't a big problem, but to select only some of them we need a different concept of selection, because some of the shapes to edit share an XML node (for example, if I stack two effects on a path, the a single XML element represents 3 editable objects: two path effects and the original path.
Just as we do when a path has a clippath and a mask. If we can do this for clips/masks, why not do the same for LPE parameters?
I don't understand your point about selecting knotholders - they are not selectable and don't need to be selected. Just display all relevant knotholders for selected object and let the user deal with them using tooltips, knot colors, etc.
In the new repr we just put the items to edit in Inkscape::Selection.
You can't use desktop's selection for that - only user can change it. If I selected an object (which in your repr is a top-level switch), it must stay selected. Selection must not sink into its depths "on its own" to select some stuff inside. We have so much stuff hung upon selection-changed callbacks that it will be a messup of gigantic proportions :)
And if you just create your own Inkscape::Selection for the purpose, this is fine, but I just don't see how this is much simpler than the current approach where node tool just asks the LPE system what to edit and gets a complete list of paths that we soon (thanks to you) will be able to edit all simultaneously.
OK, maybe that was an overstatement. But if we have to edit two different representations of a rectangle, then something is wrong, especially since the second one (path + attributes) is mostly useless outside of LPEs.
No it's not useless at all. You forget about two more things: markers and text on path. With current SVG standard, you cannot have those on a rect element, but users want to use them with rects all the time. So, even without any LPEs, we have more than enough reasons to switch rects to svg:path. We would be quite justified to do this even unconditionally, without any svg:rect fallback; if we implement the conditional rect/path SVG backend it's only because we try to play nice with fans of semantic SVG :)
PS: Another possible change that gives similar benefits and probably needs less changes is if we replace inkscape:original-d with a hyperreference to the original shape, and just move that shape to defs instead of moving its data to a special attribute. That's how SVG 1.2 vector effects work - all shape parameters are given using a href.
Yes, this is possible too, but I don't see why duplicate the style, id, and the rest of it when I only need one attribute?