SVG: io_curve_svg - Add support to grease pencil strokes
AbandonedPublic
Actions

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

Details

Reviewers

Matias Mendiola (mendio)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T79875: GPencil: Import SVG files

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

Antonio Vazquez (antoniov) requested review of this revision.Sep 4 2020, 4:25 PM

Antonio Vazquez (antoniov) created this revision.

Antonio Vazquez (antoniov) added a task: T79875: GPencil: Import SVG files.

Antonio Vazquez (antoniov) edited the summary of this revision. (Show Details)

@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:

io_curve_svg.zip20 KBDownload

Sam Cogheil (SimplSam) added a subscriber: Sam Cogheil (SimplSam).Sep 8 2020, 12:36 PM

Dalai Felinto (dfelinto) added a subscriber: Dalai Felinto (dfelinto).Sep 22 2020, 5:20 PM

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.

Dalai Felinto (dfelinto) added a project: Add-ons (BF-Blender).Sep 22 2020, 5:26 PM

Dalai Felinto (dfelinto) removed a subscriber: Add-ons (BF-Blender).

Dalai Felinto (dfelinto) mentioned this in BF Blender (2.91).Sep 22 2020, 5:29 PM

Metin Seven (MetinSeven) awarded a token.Sep 22 2020, 6:30 PM

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

Harbormaster completed remote builds in B10360: Diff 29182.Sep 22 2020, 7:11 PM

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

Dalai Felinto (dfelinto) added inline comments.Sep 23 2020, 2:47 PM

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

Harbormaster completed remote builds in B10376: Diff 29216.Sep 23 2020, 7:11 PM

Raimund Klink (Raimund58) awarded a token.Sep 25 2020, 11:53 AM

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.