Modify Path -> Add nodes: what's wrong?

On 2015-09-11 21:52 (+0200), Hachmann wrote:
> but not with:
> hand drawn Béziers / pecil drawn shapes / letters converted to path (no nodes inserted).

Works for me (0.91 r13725 on OS X 10.7.5, default new prefs, default options for the extension).

Could you upload (and share the link to) a file with examples of use cases listed above where no nodes are added, and also with a pasted screenshot of the options of the extension?

Things to look out for:

- hand drawn Béziers / pecil drawn shapes
any path effects active (e.g. because drawn with 'Shape' other than 'None')?

- letters converted to path
Did you ungroup after converting text to path? The extension doesn't work on groups, it expects that the selection contains paths.

Yeah. The 'copy from clipboard' effect was still active - with empty clipboard one can't see... meh... sorry! (found out about the group of one letter myself already after having selected it with the node tool...).

So: reason for feeling silly found :) Thanks suv, and sorry for wasting your time.

np - glad you figured it out!

(maybe an error message could help to make this more obvious?... just an idea to support silly people when using Inkscape...)

TBH I'm not sure about adding an error message - currently, the extension intentionally"skips" objects in the selection which are not paths (i.e. it does not fail, it simply does not process them, e.g. groups):
https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/share/extensions/addnodes.py#L84
i.e. you can run it on a selection without tediously making sure beforehand that only paths are among the selected objects, and don't have to click away error messages about objects not modified by the extension (because not being paths).

Path effects are a special case, because the path-modifying extensions like 'Add nodes' modify the 'd' attribute of <path> elements - however, if the path has a path effect applied, 'd' is a generated output by the path effect (the original path is stored in a custom attribute). The 'd' attribute modified by the extension is probably "destroyed" (overwritten) when inkscape regenerates the path effect's output 'd' when loading the SVG content (document) returned by the extension script (this needs checking in detail what happens - the modified 'd' attribute is not "valid" or not "effective" in any case though if the path has a path effect applied).
How should the extension script know whether one or more path(s) in the selection have an intentional path effect applied (and thus are not really modifiable by the extension), or do have an unintentional effect applied (and the user should be notified every time about this)?

In a similar case I'm currently holding back a patch for the 'Convert to Dashes' extension (bug #1183473) to transparently handle paths within groups in the selection - the latest version of the patch always prompts the user with the number of 'omitted' objects, but strictly speaking that's mostly an annoyance without helpful information (likely it's e.g. ok to silently skip a bitmap image which might be within a selected group along with stroked paths). I'm considering for that extension whether it would be better to only prompt the user with an error message if the selection is a single object (and not a group), and silently skip objects which don't have a modifiable style attribute in other cases; or even to return no error messages at all (once groups are handled transparently by default).
Either way - for me it is always difficult to decide on when error messages are helpful, or when they are mostly annoying. More important to me in general is to correctly handle all possible objects of a selection without triggering a failure of the script itself (e.g. by making assumptions about the selection instead of testing types or presence of certain attributes).

> you can run it on a selection without tediously making sure beforehand that only paths are among the selected objects

- Maybe an option would be to only show an error if really *nothing* would happen? If some effect occurs, the user will notice that something is amiss with the selection of objects, if the output is different than expected, but if nothing happens, it's a guessing game.

> How should the extension script know whether one or more path(s) in the selection have an intentional path effect applied (and thus are not really modifiable by the extension), or do have an unintentional effect applied (and the user should be notified every time about this)?

- That's a tough question... maybe same solution as above could be possible (don't know about all possible cases)?

> 'Convert to Dashes' extension

- Your solution (with the single object) sounds similar to the above ;)

In most cases I find the error messages helpful (mostly those happen for me because I forgot to select an object...) - I'm usually annoyed by my error, not by the message... unless it's about Uniconvertor ;) - I guess currently extensions don't have access to the status line?

On 2015-09-12 24:08 (+0200), Hachmann wrote:
> I guess currently extensions don't have access to the status line?

They don't - the only way to provide feedback to the user is via stderr output of the extension script, which is then shown in the default 'Inkscape has received additional data from the script executed. ...' dialog.

One issue you encountered at least won't occur with 0.92 anymore: if the clipboard is empty, using 'From clipboard' as shape with the pen/pencil tools no longer adds the pattern-along-path effect (can be tested with current trunk).

Two possible scenarios:
a) Implement helper functions for selection handling, checking and error/feedback messages in a shared python module, and unify usage (error messages) for all extensions bundled with Inkscape. This might also include unifying selection order vs stack order (e.g. Perspective/Envelope vs Pattern/Scatter along path) as far as possible / feasible, and possibly some general rules whether to treat groups as objects, or to transparently act on the contained elements only.
b) Implement (as before) solutions for individual extensions, as needed or asked for.

I might take a look at 'Add nodes' during the next days, based on the original question (personally, I'd favour a more unified solution - similar behavior and feedback for all extensions which operate on selections - but that's much less likely to happen I guess).

> One issue you encountered at least won't occur with 0.92 anymore

- Good to know :) - and good idea, too.

Ah - yes, today I wondered about envelope turning things around by 90° in contrast to perspective... (Still preparing for that workshop - I'm trying to anticipate what people might stumble upon when trying to follow, and trying out all kinds of silly things - this answers section has prepared me quite well, though :)).

It would be great to know what to expect when working with extensions - but that re-work looks like it's still far in the future, according to the Roadmap - and making changes now would probably make it harder for people who are already used to all quirks...

Would you mind testing a modified version of the extension?
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/files/head:/addnodes-ui/

You can download the two files:
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/download/suv-sf%40users.sourceforge.net-20150913124458-lcln9ti6kqxef0yp/addnodes.inx-20150913120214-wrzads25n5a8vg2p-2/addnodes-debug.inx
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/download/suv-sf%40users.sourceforge.net-20150913124458-lcln9ti6kqxef0yp/addnodes.py-20150913120214-wrzads25n5a8vg2p-3/addnodes-debug.py
and install them in "${HOME}/.config/inkscape/extensions" (on linux, you usually have to make the python script executable first). The modified version of the extension lists separately as 'Extensions > Modify Path > Add nodes (Debug)' ( the original one bundled with Inkscape is not affected).

Changes:
- new feature to recursively process groups in the selection (e.g. for text converted to path)
- prompt the user to select path(s) if not used in recursive mode (text of the message needs improving)
- warns about skipping paths which have a path effect applied (in either mode)

 :D

Of course I don't mind!

It works as you described :)

Some ideas:

Is it possible to filter out duplicate error messages? If I've got two independent objects that can't be used by the extension, I get the message twice (in one single popup, fortunately ;) http://staging.inkscape.org/en/gallery/item/29/).

The message "No paths where found. Please convert all objects you want to add nodes to into paths." doesn't make sense for groups. Maybe just leave out the 'Please...'? I think most extensions have some kind of standard message like 'This extension needs at least x paths to work', or something along those lines... Could work, too.

There are no error messages on spirals / stars / clones / (linked) offsets, but for consistency, there probably should be (don't know if there are other kinds of objects I may have missed).

There's also no error message if you check 'recurse into groups' but only have a (for example) rectangle, circle or box (and the others from above) selected. I understand this is intentional, as you can probably expect a user to not check that option if there are no groups. I like error messages (kind of... I mean I like a program to be verbose), so I wouldn't mind if it checks if it makes a change, and then tells you why it didn't. But that's a design decision where I'm too inexperienced to give useful input, I guess.

(just a theoretical question: Is it possible for the extension to fetch selected segments / adjacent nodes instead of full paths, and insert nodes just into/in between those? - This is not a feature request to you, I'd just like to know if it's possible at all. The extension seems to be the only thing in Inkscape that can kind of measure the length of segments in a curved path.)

Thank you :D

On 2015-09-13 17:52 (+0200), Hachmann wrote:
> just a theoretical question: Is it possible for the extension to
> fetch selected segments / adjacent nodes instead of full paths, and
> insert nodes just into/in between those?

Not possible - extensions are only passed a list of selected objects, there is no feature to pass them a list of selected nodes of a selected path (bug #171640).

Okay, thanks!

(Guess you found the typo in 'No paths where found' by now ;) )

I have reverted the first attempt, and pushed a second, simpler one:
- internally counts number of paths actually modified
- internally counts number of skipped objects
- reports back if the count of not modified objects > 0

http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/files/head:/addnodes-ui/
(if you prefer the direct download links, let me know).

Details I need to think about more:
- report back in detail which objects failed? if so - with object type, count per object type, and/or the list of ids?
- implement the changes for type checking and reporting with an external module in mind (so that the basic new functions could be shared / reused among other path-modifying extensions)
- keep this additional processing at a minimal level - it better not introduce more complexity than the core task of the extension.

For me, I think, it would be absolutely sufficient to say something like 'x of y objects in the selection have not been modified, because they are not paths or have a path effect applied'. (That would bring complexity down a lot ;) ) If I wanted, I could then check myself for objects that are not paths.

I find ids are not so very helpful, as they have low readability, and you'd need to copy them from the error message, paste them somewhere, and then somehow search for them each in turn. This is a task we can easily accomplish, but a less experienced user would perhaps just look at all those numbers and say 'Huh? How does this help?'... (of course I don't know - it's just a general feeling that many numbers in error messages tend to repell people).

Of course, if those details would be part of a reusable, standardized thing, it would make more sense to me to include them (one could even imagine - far, far in the future - of somehow highlighting objects that didn't work). Maybe even use a collapsible thing 'Show details'. Again, far, far in the future, and after consulting someone who has UI design skills, and after porting the extension system....

In 'Add nodes' alone, I think I wouldn't need so much detail. Do you use that on groups in your normal work? (I'd be interested to know what different people use it for - I was just playing around, and didn't have a real world use case...).

Works well, btw. :-) The only thing I'd change is the error message to count the other way around - tell what not worked, instead of what worked.

(ignore bad grammar, please)

Updated and applied to other path-modifying extensions as well:
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/files/head:/modify-path/

Zip file of 'modify-path/':
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/download/head:/modifypath.zip-20150914085057-ot710gd8rw1sppzq-1/modify-path.zip

You're great, suv - trying this out now :)

Thanks for making this available for all modify path extensions :) (and for zipping!)

I'm not sure about the 'in x groups' thing - the objects are not necessarily all *inside* the groups...
(it counts all groups, and it counts all objects, but the skipped objects are not all inside those groups)
It's more a grammar thing... Maybe there's a better way to phrase it? (nitpicking...)

If I haven't selected anything, there's no message - wasn't that different before? (I'd find it helpful, but maybe it's intentional?)

Flatten Beziers freezes Inkscape during live preview:
Draw a closed path, open dialog, set flatness to zero, then use mouse wheel to increase value again. Freeze, CPU spikes when I get to 0.3 or 0.4 - looping? Only force close seems to end it. Python progress keeps running in background... urghs... Would you like a bug report? (also happens in original extension)

This file has been modified to include your proposed changes (warn if extension is run without a selection, change how group count is reported):
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/download/suv-sf%40users.sourceforge.net-20150914230118-6oqjb2wskhxm1hzr/modify_path_helper.p-20150914070431-uwh1dm6soru8zozi-1/modify_path_helper.py

TBH though I'm getting increasingly uneasy with the changes, and if it is common agreement that inkscape needs to report back always (with a disruptive dialog for extensions), and never ever may silently skip objects which are not the target of the chosen command/extension, then the feature to recurse into groups needs to go - it does not make sense otherwise (just try to adjust options with 'Live preview' toggled on in a mildly complex document with various object types in nested groups -> that's even for new users very annoying, and beyond helpful). For a brief moment I was thinking about adding a [x] verbose checkbox for extensions which include the 'Live Preview' feature, so that advanced users could turn the messages off, but at this point it is getting absurd (IMvHO). And no - extension scripts have no way to know whether they are run for a 'Live Preview' or for 'Apply' (from their perspective, it's the same - they cannot know whether Inkscape will later undo the changes in case of 'Live preview' toggled off again or not).

The idea to transparently handle paths inside (nested) groups originally came up in response to new/one-time users who are not aware of what groups are in Inkscape (and file bug reports because of this), and are unaware of circumstances they might encounter them (import of foreign vector formats, Inkscape SVG files saved as optimized or plain SVG, etc.). Your initial query why text converted to path was not edited by 'Add nodes' supported the idea that transparent processing of objects inside groups could be a welcome feature. However - not being able to use that feature in arbitrary documents without breaking workflows unconditionally by having to click away a message dialog (unless the user spends time either to get rid of the groups anyway and directly selects paths only, or - more time-consuming and complex - to convert any non-path elements inside (possibly deeply nested) groups to paths first (and delete objects not convertible or move them outside the group structure), or - equally complex for new users - to create a selection set of modifiable paths only, found within the nested group structure) indicates to me that this was a wrong assumption on my side - at least it is not compatible with the requested 'report always' feature.

"if it is common agreement that inkscape needs to report back always"

How would we find that out? Did someone else comment on this already? (I'm quite sure I'm not the 'average user', if that exists at all... and probably not very representative)

And we are back to the use cases I asked about in #17, too. I would only ever intend to use those extensions on a single path, or maybe on a few ungrouped paths (or things of which I think that they are paths...), but I wouldn't go and use them on a whole, possibly nested group (intentionally...). But I have no clue how others want to use them - how can we find out?

On 2015-09-14 17:46 (+0200), Hachmann wrote:
> Flatten Beziers freezes Inkscape during live preview: (...)

1) I would not really recommend using the scroll wheel to adjust parameters while in 'Live Preview' - this could easily trigger lots of reruns of the extension not really intended (if parameter was modified, inkscape undoes earlier preview, sends SVG to extension script, extension script processes SVG content based on new parameters, sends SVG content back to inkscape, inkscape replaces content of current document with content received from script, etc. - for each state of the spinbox which is caught by inkscape). I imagine that there could be easily some kind of kind of a race condition if too many scroll events occur rather quickly.

2) As far as I understand the smaller the flatness factor, the shorter the generated straight line segments (and the more precise but also much larger the generated SVG output). It seems that the user can "hang" the 'Flatten Beziers' extension in Python with any path (closed or open) if '0.0' was entered for the flatness factor (no scrolling involved). Possibly that's what happened in your case too (scroll wheel adjusting the spinbox to its minium value).

A somewhat related report had been fixed by converting the recursion used earlier to a loop:
https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/10076
(bug #340123)

Possibly the extension needs more checking when to exit the loop than currently done:
http://bazaar.launchpad.net/~inkscape.dev/inkscape/RELEASE_0_91_BRANCH/view/head:/share/extensions/cspsubdiv.py#L28

Also to be considered:
Should the lower limit of the spinbox be adjusted to > 0 (e.g. 0.1)? Are there any legit use cases for flatness = 0.0?

Unless there's already a related report I failed to find last night when searching, a new one should be filed, requesting to investigate any necessary (but missing or dangerous) limits as well as checks for the flatness factor in 'Flatten Bezier'.

Additional context:
flatness parameter in current flatten.inx:
<param name="flatness" type="float" min="0.0" max="1000.0" _gui-text="Flatness:">10.0</param>

flatness parameter in current HPGL output:
<param name="flat" type="float" min="0.1" max="10.0" precision="1" _gui-text="Curve flatness:">1.2</param>

On 2015-09-15 01:32 (+0200), Hachmann wrote:
> but I wouldn't go and use them on a whole, possibly nested group
> (intentionally...)

I personally would use it intentionally if that feature existed (depending on use case and drawing structure of course) - but I likely would avoid it as much as possible if it forced me to click away repeatedly dialogs with messages about skipped objects.

===
Simple fictional use case:
Two groups in the current layer, each with a mix of curved paths drawn on-top of a bitmap image, and text (as text).

Goal:
'Fractalize' (or flatten, straighten segments, etc.) the paths inside each group, with different parameters per group, keeping text as text and bitmap as bitmap (intentionally).

The extension technically would handle this well - skipping silently any objects which are not editable paths. Any prompts telling the user that not all objects inside the groups have been modified is not helpful in this context (the user knows that, and relies on this being the case - the text needs to stay editable text, there is no need to trace the bitmap images to turn them into editable paths because they are not meant to be modified the same way as the overlayed paths).
===

Anyway - I'll probably remove the recursive feature for now, and instead will investigate whether the error messages of the path-modifying extensions could be further unified (e.g. for extensions which required exactly two selected paths). If not really feasible, I'll commit the changes for 'Add nodes' only (prompt if run without selection, prompt if the selection contained objects to which the core feature of the extension is not applicable).

Thank you, suv, for taking all this time to write those answers!

> I would not really recommend using the scroll wheel to adjust parameters while in 'Live Preview'

- If it's possible, then there will be a user who tries it ;) ... (it usually works okay, but all that recalculation/rerendering makes things slow, of course). (and no, I don't mean that Inkscape should prevent mouse wheel usage there - it's just something devs should probably anticipate to happen)

> Unless there's already a related report I failed to find last night when searching, a new one should be filed

- I haven't found anything, either. Would you like me to make one?

> I'll probably remove the recursive feature for now

- Just because the two of us couldn't find the best way to show error messages yet? Maybe add a help tab, issue less messages... There are a lot of options to be explored without throwing away functionality that you would use. If it is helpful for someone, why not keep it? The little checkbox doesn't overload the interface, I think.
But whatever you'll decide, I know you've put a lot of thought into it. Which is impressive :) Thank you for being so thorough!

On 2015-09-15 11:27 (+0200), ~suv wrote:
> Unless there's already a related report I failed to find last night when
> searching, a new one should be filed, requesting to investigate any
> necessary (but missing or dangerous) limits as well as checks for the
> flatness factor in 'Flatten Bezier'.

Bug report with proposed simple fix (raise lower limit to '0.1') filed:
https://bugs.launchpad.net/inkscape/+bug/1496804

On 2015-09-17 13:27 (+0200), ~suv wrote:
> On 2015-09-15 11:27 (+0200), ~suv wrote:
>> Unless there's already a related report I failed to find last night when
>> searching, a new one should be filed, requesting to investigate any
>> necessary (but missing or dangerous) limits as well as checks for the
>> flatness factor in 'Flatten Bezier'.
>
> Bug report with proposed simple fix (raise lower limit to '0.1') filed:
> https://bugs.launchpad.net/inkscape/+bug/1496804

JFYI - fix committed to trunk, and backported to the stable release branch (in case there's a 0.91.1 bug fix release before 0.92 comes out).

P.S. still working on refactoring path-modifying extensions (to unify behavior by reusing the same error messages and selection checking) - I'm somewhat indecisive at the moment how to proceed, but likely I'll create a trunk branch with a nicer commit history (separate PEP8 and formatting changes from the rest), and propose it for merging (to get comments wrt refactoring of python modules).

Latest sources (kind of WIP, no new ZIP archive atm):
http://bazaar.launchpad.net/~suv-lp/+junk/inkscape-extensions/files/head:/modify-path/

> fix committed to trunk

Good news, then :) - (I forgot to subscribe to the bug (unfortunate default setting on lp...), wouldn't have noticed if you hadn't told me.)

> I'm somewhat indecisive at the moment how to proceed

I guess I can't really help with that ;) - but I'm sure you'll find a good way. Tell me when you'd like me to try something out (but don't ask me about standard python formatting... I re-read that PEP8 today, though, can't hurt).

<off-topic>
On 2015-09-23 02:32 (+0200), Hachmann wrote:
> (but don't ask me about standard python formatting... I re-read that
> PEP8 today, though, can't hurt).

I'm lazy and rely on the output of the command line tool from
https://pypi.python.org/pypi/pep8/

Sometimes, when feeling extra motivated, I use pylint too (not lately though - it's often too verbose and nitpicky):
https://www.logilab.org/project/pylint
</off-topic>

Lazy? Efficient ;)