Updated the example skeleton add-on accordingly.

@Tamito Kajiyama (kjym3)

thanks for making this patch. While implementing the SVG exporter addon I encountered a few problems

first of all, the exporter requires the SVGPathShader to be inserted at a specific spot. Ideally, a callback is executed right before the create() function is executed. After the execution of the create function, acces is needed to the SVGPathShader object, to execute its write() method.

some other ideas/comments:

• shaders_list needs to be an argument for the callbacks, this seems the easiest way to solve the problems above
• layer doesn't need to be an argument, since it can be found by using scene.render.layers.active
• the errors don't provide quite enough information. why not raise them properly? (failing silently is really weird, if there is a problem, just turn the callback off)
• callbacks_base_style_post only has limitations compared to 'callbacks_style_post', executed before the create() function

first of all, the exporter requires the SVGPathShader to be inserted at a specific spot. Ideally, a callback is executed right before the create() function is executed.

SVGPathShader uses the base line color, alpha and thickness specified by a line style and does not look at the values of these properties specified at individual stroke vertices, so I thought it should be okay to place SVGPathShader somewhere after geometry modifier shaders. Then I thought of a point in the list of shaders to which a reasonably logical name can be given. Candidates could be:

1. just after the geometry modifier shaders.
2. just after those shaders that set the base color, alpha and thickness (i.e., base_style_post)
3. just before the call of Operators.create().

The first two seem equally okay for the SVG exporter add-on. SVGPathShader needs to appear before cap shaders, so the 3rd one is not a choice.

After the execution of the create function, acces is needed to the SVGPathShader object, to execute its write() method.

Two options:

• use a global variable. In Python that means a module-global variable, nothing outside will be messed up.
• define a class object where callbacks are implemented as methods. Then the SVGPathShader instance can be stored as a member variable.

• shaders_list needs to be an argument for the callbacks, this seems the easiest way to solve the problems above

I also thought that could be a design option. I then thought that exposing a function-local variable (shaders_list) as part of API would appear a bit naive.

If necessary we can introduce more hooks for modifiying the list of shaders, such as those discussed above and another hook for inserting shaders at the beginning of the list.

• layer doesn't need to be an argument, since it can be found by using scene.render.layers.active

The process() function of parameter_editor.py takes a render layer name and a line set name as the arguments, and then identifies the current layer and line set by name. Since Freestyle need to go over all render layers and line sets no matter which one is active, the layer is not necessarily the active one.

• the errors don't provide quite enough information. why not raise them properly? (failing silently is really weird, if there is a problem, just turn the callback off)

I agree that error handling could be more explicit and verbose. I will revise that part soon.

• callbacks_base_style_post only has limitations compared to 'callbacks_style_post', executed before the create() function

Not sure what you mean. Could you please elaborate the point?

In D839#13049, @Tamito Kajiyama (kjym3) wrote:

first of all, the exporter requires the SVGPathShader to be inserted at a specific spot. Ideally, a callback is executed right before the create() function is executed.

SVGPathShader uses the base line color, alpha and thickness specified by a line style and does not look at the values of these properties specified at individual stroke vertices, so I thought it should be okay to place SVGPathShader somewhere after geometry modifier shaders. Then I thought of a point in the list of shaders to which a reasonably logical name can be given. Candidates could be:

1. just after the geometry modifier shaders.
2. just after those shaders that set the base color, alpha and thickness (i.e., base_style_post)
3. just before the call of Operators.create().

The first two seem equally okay for the SVG exporter add-on. SVGPathShader needs to appear before cap shaders, so the 3rd one is not a choice.

it is if shaders_list is exposed (and mutable). any other insertion point is arbitrary, and may serve for this addon but not for the next project. this is probably bad

After the execution of the create function, acces is needed to the SVGPathShader object, to execute its write() method.

Two options:

• use a global variable. In Python that means a module-global variable, nothing outside will be messed up.

I'd rather not use global variables, I think this would be a very hacky solution and therefore undesirable

• define a class object where callbacks are implemented as methods. Then the SVGPathShader instance can be stored as a member variable.

I don't see how this would work

• shaders_list needs to be an argument for the callbacks, this seems the easiest way to solve the problems above

I also thought that could be a design option. I then thought that exposing a function-local variable (shaders_list) as part of API would appear a bit naive.

If necessary we can introduce more hooks for modifiying the list of shaders, such as those discussed above and another hook for inserting shaders at the beginning of the list.

I think exposing shaders_list is the most straigtforward solution. Creating more hooks that manipulate shaders_list is essentially the same thing as exposing it.

• layer doesn't need to be an argument, since it can be found by using scene.render.layers.active

The process() function of parameter_editor.py takes a render layer name and a line set name as the arguments, and then identifies the current layer and line set by name. Since Freestyle need to go over all render layers and line sets no matter which one is active, the layer is not necessarily the active one.

my mistake

• callbacks_base_style_post only has limitations compared to 'callbacks_style_post', executed before the create() function

Not sure what you mean. Could you please elaborate the point?

This assumed the exposure of shaders_list. a callback executed just before create() can modify the order of all added shaders, whilst a shader inserted after the basic style can't be put in a specific position

Exposing shaders_list is possible, but given that list how do you determine the position to insert SVGPathShader among the shaders in the list? For instance, a cap shader may or may not appear in the list. You might end up having implementation-specific "if" conditions that are dependent of a priori knowledge about the shaders used in the Parameter Editor. Add-on scripts may find unknown shaders inserted to the list by other add-ons.

In contrast, named locations such as base_style_post and a few other suggested insertion points provide add-on scripts with a reliable way to identify where to insert shaders without being worried about other shaders in the list.

As to keeping a reference to the SVGPathShader instance using a class object, I was thinking of something like below:

import parameter_editor


class Callbacks:
    def base_style_post(self, scene, layer, lineset):
        self.path_shader = SVGPathShader(...)
        return [self.path_shader]

    def lineset_post(self, scene, layer, lineset):
        self.path_shader.write()


callbacks = Callbacks()
parameter_editor.base_style_post.append(callbacks.base_style_post)
parameter_editor.lineset_post.append(callbacks.lineset_post)

re: last two messages.
Both points raised are valid,

• directly manipulating shader list is most flexible, (but more error prone)
• named hooks are more clearly defined but means you may end up having to use class methods to easily keep references to other data as Tamito suggests. (though there are other less Pythonic ways which work just as well)

Its hard to say if this is opening a can of worms, or just exposing functionality for a special use case. I expect its the latter, and this can be kept a limited API and only extend it if there are other good use cases that arise... which may not happen.

I don't have a strong opinion on this since IMHO this will remain a special-case, but Tamito's proposal seems reasonable and https://github.com/folkertdev/freestyle-svg-exporter/blob/master/parameter_editor.py#L1210 does seem a bit haphazard.

@Campbell Barton (campbellbarton)

My proposed method is admittedly a little hacky. It was made to work with the current addon in mind, to explore what could work. My problem with picking fixed and arbitrary locations to execute callbacks is that you never know what the future may hold. exposing a copy of shaders_list makes the callback system more future-proof. As I explored in the github project you could expose shaders_list as immutable, but that doesn't lead to very clear code.

I also have serious doubts about whether error-proneness is really a problem. As far as I can see, the worst that could happen is that a shader is inserted that has some kind of error in it. The addon causing the error can easily be disabled, and all should be fine again.

Storing the callbacks in a class object does seem a good idea to me. The current method for calling the write() method is prone to errors.

In principle, callback functions for the base_style_post hook should work on Stroke objects and not StrokeShader objects in the shaders_list.

The base_style_post hook is intended to allow callback functions to visit Stroke objects whose geometry and base style (color, alpha and thickness) have been already defined.

If the shaders_list is exposed to base_style_post callbacks, then it is likely that the callback functions get dependent on known StrokeShader classes that are part of the Parameter Editor internals. For instance the class names RoundCapShader and SquareCapShader are arbitrary and implementation-specific details. These names may change in the future, which breaks the callbacks having references to them. By the same token if the Parameter Editor adds new stroke shaders that interfere add-ons, then the add-ons need to be updated. From an API design perspective, I would prevent application programs from being suffered from internal changes in the Parameter Editor.

From a practical view point, the present implementation of the SVG export add-on needs to access stroke geometry and does not refer to stroke attributes except for vertex visibility. Hence the base_style_post hook without exposing the shaders_list should suffice. If in the future the SVG export add-on will have to inspect other stroke attributes including per-vertex color, alpha and thickness properties, then the lineset_post hook serves as a way to get access to them.

Hi TK,

Although I still have some doubts here, the arguments you raise are very valid and there really seems not to be an ideal solution. I would propose to just make the exporter work using your current patch (possibly with minor adjustments) and to document the design well. If things are changed in the future, the proces of updating everything should then be quite straightforward.

Hopefuly I'll be able to revise my current version this afternoon. It'll also include the custom objects for storing the callbacks.

A final issue, which I would like a bit of feedback on, is the procedure of saving the file. specifically

• should the user give a folder or filename
• should the frame number be pre- or appended
• what folder is the default (if any)

what is the most clear/logical way to do this?

(repost, accidentally send to a noreply-mailadress)

actually, I just had a better idea. to represent the render result as accurate as possible, the SVGPathshader has to be inserted before the capshaders. This is actually a well-defined spot, after the modifiers. We could introduce a callback there (modifiers_post for instance) and be done with all these problems.

Moving the base_style_post hook to a more appropriate spot (i.e., modifiers_post as you suggested) would be fine to me.

As to file naming, I would follow the Blender's way, that is, if only a directory name is given (/tmp/ for instance) then image files will be /tmp/0001.svg, /tmp/0002.svg and so on. Instead, if a base name is given (/tmp/frame for instance) then the output file names will be /tmp/frame0001.svg, /tmp/frame0002.svg and so on.

Just for your information, my SVGWriter module was using the existing render.filepath property for file I/O of still images, and then`svgwriter_concat.py` was accepting an output file name for concatenating still frames into an SVG animation file.

@kjym, agree. having a separate filepath for SVG is confusing and unnecessary.

following Blender's convention's shouldnt be so hard
(note, foo_###.svg -> foo_%0*d % (number_of_hashes, frame_number)`)

Probably easier to use scene.frame_path() and add the SVG extension.