Page MenuHome

SVG: io_curve_svg - Add support to grease pencil strokes
Needs ReviewPublic

Authored by Antonio Vazquez (antoniov) on Sep 4 2020, 4:25 PM.

Details

Summary

This patch contains the modification of the add-on io_curve_svg to convert to grease pencil. Importing to curves or grease pencil strokes is now supported.

The modification only adds the necessary information for grease pencil materials, since all the conversion is done by the curves to grease pencil conversion routines already existing in Blender.

The add-on just executes that conversion and removes the curves to leave the grease pencil object only.

Diff Detail

Event Timeline

@Matias Mendiola (mendio) We need create the list of not supported features.

For example, it's not supported because cannot be converted:

  • Textures.
  • Gradients.

For testing, you can replace the default io_curve_svg folder in ..\2.91\scripts\addons with the version included in this file:

Dalai Felinto (dfelinto) added inline comments.
io_curve_svg/import_svg.py
2208

You can use the context from the function, no need to use bpy.context

Adding Sergey as reviewer since he is one of the owners of the add-on.

Antonio Vazquez (antoniov) retitled this revision from SVG: io_curve_svg - Add support to grease pencil strokes to SVG: io_curve_svg - Add support to grease pencil strokes.

Remove bpy. from context. This is available as parameter.

Sergey Sharybin (sergey) requested changes to this revision.Sep 23 2020, 10:05 AM

General notes:

  • Reduce vertical space of functions, splitting them into individual independent parse state is the most straightforward way of doing this.
  • Avoid ambiguous and unnecessary shorting of variables: c, cla, matname. All those (and others) can become way more explicit
  • Denote "units" better. For example, is it a layer or a `layer_name?

From quick read it seems you first create curves and then convert them to grease pencil and remove the curves. This is not nice.
You can easily abstract the Blender-side geometry creation using an interface of some sort (interface as in programming terms, not in user interface terms).

This revision now requires changes to proceed.Sep 23 2020, 10:05 AM
io_curve_svg/import_svg.py
419

use elif instead of if for every case after the first one

2002

use elif for these cases

2132

Try running pep-8, I think it will suggest adding an extra line in between different functions. (if the old code doesn't comply to pep-8 then I leave at your discretion to decide whether to ignore this or not).

First round of Cleanup

Comments by Sergey:
For a more robust solution recommend looking at https://github.com/RazrFalcon/resvg

I would like to know what is the longer term plan is. If using more complete SVG importer using exiting library is a priority, probably better to focus on that. If that is more of a distant future plan, then the code in the importer is to be more aggressively refactored (aggressively != spend weeks on the refactor; aggressively == get rid of legacy and entanglement, which could happen relatively easily).

As it is now, this is quite an entangled code, with issues on the "engineering" level and on the "dont-touch-what-dont-belong-to-you" levels.

io_curve_svg/import_svg.py
37

Is clskey a term used by SVN specification?

37

Make it a class, not a dictionary.

Using dictionaries was a wrong decision from many ears ago. Using class will also make you to not need to worry about doing .copy().

210

matname -> material_name.

342

Seems like c, cls, cla are used interchangeably. Please stick to a single notation, preferably more clear from local code point of view.

Here cla can be class_key.
c can be cls (since the class is a reserved keyword).
cls can stay as it is, if we all agree that class is something what comes form the class attribute.

367

cla -> class_name. To make it clear whether cls is an object with fields or just a string name of the class.

369

I would really like to see this long if-else split into a helper functions.

370

c -> style.
Keep meaning of variable clear from its name.

395–396

You can not expect that string starting with url is a valid url. For example, you can not expect that url will be followed up with ://.

Also, the URL can have credentials and other parts. To access individual parts you don't need to implement own parser. Use urllib which is a standard library of Python.

1373–1374

Repetition of node.getAttribute('inkscape:label')

1382

What is this prev_layer_name about?
Intuitively, sounds more like you need a hasmap rather than comparison with previous layer, so that you don't create new layers when layer names are interleaved (aka, Layer1 Layer2 Layer1 Layer2).

1689

Move is a wrong technical term here.

1851–1852

This seems quite repetitive, should be easy and more clear to move to an utility function.

Also, unclear what is supposed to happen when the curve is closed and style is using fill (cu.materials.append from few lines above).

2170

That will remove all currently orphaned data which might be there before the import is run. The operator should not touch anything it is not operating on.