Page MenuHome
Differential D10482

GPencil: New modules for Import and Export
ClosedPublic
Actions

Authored by Antonio Vazquez (antoniov) on Feb 20 2021, 5:20 PM.
Tags
Subscribers
Hans Goudey (HooglyBoogly)
Julian Eisel (Severin)
Sam Cogheil (SimplSam)
Tokens
"Love" token, awarded by shader."Love" token, awarded by calli."Love" token, awarded by PeterEfe."Love" token, awarded by charlie."Love" token, awarded by Andrea_Monzini.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Group Reviewers
Grease Pencil
Maniphest Tasks
T83192: GPencil: Export to PDF
T83191: GPencil: Export to SVG
T79875: GPencil: Import SVG files
T83190: GPencil: Import/Export main task
Commits
rBa8a92cd15a52: GPencil: New modules for Import and Export
Summary

This patch adds support to export and import grease pencil in several formats.

Inlude:

  • Export SVG
  • Export PDF (always from camera view)
  • Import SVG

Requires libharu and pugixml.

For importing SVG, the NanoSVG lib is used, but this does not require installation (just a .h file embedded in the project folder)

Example of PDF export: https://youtu.be/BMm0KeMJsI4

Diff Detail

Repository
rB Blender
Branch
temp-gpencil-io
Build Status
Buildable 13680
Build 13680: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Hans Goudey (HooglyBoogly) added inline comments.Mar 16 2021, 6:48 PM
source/blender/io/gpencil/intern/gpencil_io_base.h
95

This could take a float3 as an argument and return a float2 value

source/blender/io/gpencil/intern/gpencil_io_capi.cc
122–123

Seems strange to use |= and then assign a value on the next line.

174–175

You can just assign the value, i.e. const bool done = gpencil_io_import_frame(&importer, iparams);

201

I would just return in the switch case itself:

case GP_EXPORT_TO_SVG: {
  /* Prepare document. */
  GpencilExporterSVG exporter = GpencilExporterSVG(filename, iparams);

  return gpencil_io_export_frame_svg(&exporter, iparams, true, true, true);
source/blender/io/gpencil/intern/gpencil_io_export_svg.cc
226

Use const bool

source/blender/io/gpencil/intern/gpencil_io_import_svg.cc
88

This can use `const float3 scale = float3(params_.scale);

154–156
for (bGPDspoint &pt : Span(gps->points, gps->totpoints)) {
  sub_v3_v33(&pt->x, gp_center);
}
This revision now requires changes to proceed.Mar 16 2021, 6:48 PM
Julian Eisel (Severin) added a subscriber: Julian Eisel (Severin).Mar 16 2021, 7:07 PM

Just some more C++ code-style nitpicks ;)

source/blender/io/gpencil/intern/gpencil_io_base.cc
64

For C++ I'd suggest always using (const) references instead of pointers for by-reference input parameters that cannot be nullptr. That way you know you don't have to (and can't) do a null-check.
If this is done consistently, you know that when you see a Foo * function parameter, it has to be null-checked.

155

C++ doesn't require void when the function parameter list is empty.

Antonio Vazquez (antoniov) updated this revision to Diff 35336.Mar 18 2021, 12:57 PM
  • Limit Export/Import to Object mode only
  • Remove redundant void parameter
  • Remove comment about context region
  • Change Poll methods
  • Cleanup const variables
  • Remove duplicate Enum items definition
  • Use blender::Vector
  • Cleanup return values
  • Cleanup in Import and return values
  • Change offset to use float2
  • Remove unused function
  • Move shared function to geometry module
Harbormaster completed remote builds in B13586: Diff 35336.Mar 18 2021, 12:57 PM
Antonio Vazquez (antoniov) marked 22 inline comments as done.Mar 18 2021, 3:48 PM
Antonio Vazquez (antoniov) added inline comments.
source/blender/io/gpencil/intern/gpencil_io_base.cc
155

I have removed all case like this.

source/blender/io/gpencil/intern/gpencil_io_base.h
89–95

These functions are specific for export, we already have more general functions in other areas of gpencil. Also, pass all parameters is very heavy and only adds condfusion to code... I just pass relevant values, all othe rvalues are already in the class and member data.

source/blender/io/gpencil/intern/gpencil_io_export_pdf.cc
258–262

I have changed to use first point thickness. For long strokes do the calculation can be avoided for constant thickness.

Andrea Monzini (Andrea_Monzini) awarded a token.Mar 19 2021, 8:03 PM
Matias Mendiola (mendio) accepted this revision as: Grease Pencil.Mar 22 2021, 4:47 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35480.Mar 22 2021, 5:05 PM
Antonio Vazquez (antoniov) marked 2 inline comments as done.
  • Merge branch 'master' into temp-gpencil-io
  • GPencil: No export in SVG and PDF 1 point strokes
Harbormaster completed remote builds in B13660: Diff 35480.Mar 22 2021, 5:05 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35484.Mar 22 2021, 6:14 PM

Remove option to reuse import object

Harbormaster completed remote builds in B13662: Diff 35484.Mar 22 2021, 6:14 PM
Hans Goudey (HooglyBoogly) requested changes to this revision.EditedMar 22 2021, 7:21 PM

In general I'm still uncomfortable with the architecture of this new code. I think too many of the functions are class methods when they don't need to be, which I honestly think is a misuse of classes in general.
I understand some of this is subjective, and since the code works, etc, I don't want to hold this up unnecessarily, but I don't feel comfortable accepting this as is for the reasons I've stated so far in review.
I understand the argument that adding many function arguments as a replacement for something like gps_current_color_set is a hassle, but there are other ways to deal with the problem.
Storing temporary information used briefly during the export in the IO class is just not a good option in my opinion. (We can always get a second opinion from someone else if anyone is available!)

It gives you code like this:

gps_current_color_set(ob, gps_duplicate);
if ((material_is_fill()) && (params_.flag & GP_EXPORT_FILL)) {
...

Here the relationship between these two statements is extremely important, but it's hidden, and there is just no reason to hide it.

Also, I'm not saying don't store lots of variables in an IO struct. It makes total sense to store arguments in a common place-- a list input objects, general controls for the whole operator, various settings, etc, absolutely!
But I just don't think temporary data belongs in the same place.

Some of my inline comments are "cleanups" that I don't necessarily see as blocking if there is a rush to get this patch in master.

I get the sense there are many unecessary includes in files like io_gpencil_import.c. Since the patch doesn't compile for me I couldn't test myself though. In general you should only add the includes you need for every file, not any more.

Besides the missing libraries for some reason, I got a few more warnings when compiling.

/home/hans/Blender-Git/blender/source/blender/io/gpencil/intern/gpencil_io_capi.cc: In function ‘bool gpencil_io_export(const char*, GpencilIOParams*)’:
/home/hans/Blender-Git/blender/source/blender/io/gpencil/intern/gpencil_io_capi.cc:180:36: warning: unused parameter ‘filename’ [-Wunused-parameter]
  180 | bool gpencil_io_export(const char *filename, GpencilIOParams *iparams)
      |                        ~~~~~~~~~~~~^~~~~~~~
/home/hans/Blender-Git/blender/source/blender/io/gpencil/intern/gpencil_io_capi.cc: At global scope:
/home/hans/Blender-Git/blender/source/blender/io/gpencil/intern/gpencil_io_capi.cc:67:13: warning: ‘bool is_keyframe_included(bGPdata*, int32_t, bool)’ defined but not used [-Wunused-function]
   67 | static bool is_keyframe_included(bGPdata *gpd_, int32_t framenum, bool use_selected)
      |             ^~~~~~~~~~~~~~~~~~~~
/home/hans/Blender-Git/blender/source/blender/editors/gpencil/gpencil_utils.c: In function ‘ED_gpencil_stroke_material_visible’:
/home/hans/Blender-Git/blender/source/blender/editors/gpencil/gpencil_utils.c:600:70: warning: unused parameter ‘gpl’ [-Wunused-parameter]
  600 | bool ED_gpencil_stroke_material_visible(Object *ob, const bGPDlayer *gpl, const bGPDstroke *gps)
      |                                                     ~~~~~~~~~~~~~~~~~^~~
/home/hans/Blender-Git/blender/source/blender/editors/io/io_gpencil_export.c:97:13: warning: ‘ui_gpencil_export_common_settings’ defined but not used [-Wunused-function]
   97 | static void ui_gpencil_export_common_settings(uiLayout *layout, PointerRNA *imfptr)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/hans/Blender-Git/blender/source/blender/editors/io/io_gpencil_export.c:71:13: warning: ‘gpencil_export_common_props_definition’ defined but not used [-Wunused-function]
   71 | static void gpencil_export_common_props_definition(wmOperatorType *ot)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/hans/Blender-Git/blender/source/blender/editors/io/io_gpencil_import.c:66:6: warning: no previous prototype for ‘wm_gpencil_import_svg_common_check’ [-Wmissing-prototypes]
   66 | bool wm_gpencil_import_svg_common_check(bContext *UNUSED(C), wmOperator *op)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
source/blender/blenkernel/BKE_gpencil_geom.h
30–31

These look unecessary

176–189

Use const arguments where possible

source/blender/blenkernel/intern/gpencil_geom.c
3979

No need for struct in this file

3989
for (int i = 0; i < gps->totpoints; i++) {
  const GPDspoint *pt = gps->points[i];
  ...
3997

This function uses the word "thickness" for pressure, while the above function uses "pressure". They should be consistent with each other at least.

4006

Use const, and I'd call this "first", rather than "prv", since it's just the first pressure, right?

source/blender/editors/io/io_gpencil_export.c
129

There should be any need for this function, just call gpencil_export_common_props_definition from WM_OT_gpencil_export_svg and then also add use_clip_camera

141–158

All of this is exactly the same between svg and pdf, it should just be shared.

309

This is not addressed yet

source/blender/editors/io/io_gpencil_import.c
84

Add UNUSED( in the arguments instead.

115

This is repeated from CTX_data_active_object above in this function.

115–124

With all this logic for retrieving an object, it should be split into a separate function, which could make it much more readable structure.

source/blender/io/gpencil/intern/gpencil_io_base.cc
64

I don't think struct is necessary?

385

No need to abbreviate in comments

386

set implies that it takes an argument. This function is really calculating based on the parameters, so that should be reflected in the name.

417–419
for (int i = 0; i < gps->totpoints; i++) {
  const GPDspoint *pt = gps->points[i];
  ...
source/blender/io/gpencil/intern/gpencil_io_capi.cc
58

I general I'm not sure why you need a C API for this new code, when you could add all of the new files as C++ files.

source/blender/io/gpencil/intern/gpencil_io_export_pdf.cc
258–262

I think this misses the point. In the case of is_thickness_const, BKE_gpencil_stroke_is_thickness_constant has already looped through the entire stroke. In that case skipping the average calculation and using the pressure from the first point is exactly the same as simply using the average pressure in the first place.

This revision now requires changes to proceed.Mar 22 2021, 7:21 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35496.Mar 23 2021, 9:25 AM
  • First round of code cleanup
  • Fix compiler warnings
Harbormaster completed remote builds in B13671: Diff 35496.Mar 23 2021, 9:25 AM
Antonio Vazquez (antoniov) updated this revision to Diff 35507.Mar 23 2021, 1:05 PM
  • Remove class member variables
  • Remove unused variable
  • More cleanup and rename of functions
Harbormaster completed remote builds in B13677: Diff 35507.Mar 23 2021, 1:05 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35508.Mar 23 2021, 1:13 PM
  • Remove unused header files
Harbormaster completed remote builds in B13678: Diff 35508.Mar 23 2021, 1:13 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35509.Mar 23 2021, 1:41 PM
  • Deduplicate code for filepath
Harbormaster completed remote builds in B13679: Diff 35509.Mar 23 2021, 1:41 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35511.Mar 23 2021, 3:16 PM
  • Rename function to clarify use
  • Merge branch 'master' into temp-gpencil-io
Harbormaster completed remote builds in B13680: Diff 35511.Mar 23 2021, 3:17 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35512.Mar 23 2021, 4:02 PM
  • Fix compiler warning
Harbormaster completed remote builds in B13681: Diff 35512.Mar 23 2021, 4:02 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35519.Mar 23 2021, 5:32 PM
  • Remove Submenu for Import/Export
Harbormaster completed remote builds in B13687: Diff 35519.Mar 23 2021, 5:32 PM
Hans Goudey (HooglyBoogly) added a comment.Mar 23 2021, 6:20 PM

This is the file name I get by default when exporting SVG

source/blender/editors/io/io_gpencil_export.c
65

Maybe this would be more precise as "Export stoke fill"? Not sure.

427

As far as I know frames aren't really "selected", maybe this "the active frame range" is more correct?

source/blender/io/gpencil/intern/gpencil_io_base.cc
113

Since you call this in all of the exporters, it seems like you shouldn't need to call this here, right?

Antonio Vazquez (antoniov) updated this revision to Diff 35525.Mar 23 2021, 6:33 PM
  • Various small cleanups
  • Cleanup: Remove unused includes
  • Cleanup: Remove unecessary file
  • Merge branch 'temp-gpencil-io' of git.blender.org:blender into temp-gpencil-io
  • Remove ui_gpencil_export_common_settings
  • Change tooltip text
  • Remove call to create_object_list in class creation and do it in export
  • Merge branch 'master' into temp-gpencil-io
Harbormaster completed remote builds in B13688: Diff 35525.Mar 23 2021, 6:33 PM
Antonio Vazquez (antoniov) added inline comments.Mar 23 2021, 6:34 PM
source/blender/editors/io/io_gpencil_export.c
427

Yes, they are selected in the dopesheet... this is how works all in gpencil like multiframe, you select the keyframes in dopesheet.

Hans Goudey (HooglyBoogly) updated this revision to Diff 35528.Mar 23 2021, 7:52 PM
  • Remove more unused includes
  • Use "strokes" in tooltip
  • Remove more unused includes, use C++ style for loop
  • Cleanup: Whitespace
Harbormaster completed remote builds in B13689: Diff 35528.Mar 23 2021, 7:52 PM
Hans Goudey (HooglyBoogly) accepted this revision.EditedMar 23 2021, 8:03 PM

With recent cleanups this code seems ready enough to be committed. I still think I would have architected it differently, but let's not spend too much time chasing perfection here, the code works.

A few further improvements that could be done in the future:

  • Use references instead of pointers where possible
  • Use std::stringstream instead of char * buffers, particularly where they are allocated and freed, like in gpencil_io_import_svg.cc.
  • Use better data structures in gpencil_geom.c
  • Use more specific types like float4x4, float3, and float2
source/blender/io/gpencil/intern/gpencil_io_import_svg.cc
76

Isn't this always true now that the exporter doesn't edit the data of an existing object?

This revision is now accepted and ready to land.Mar 23 2021, 8:03 PM
Antonio Vazquez (antoniov) updated this revision to Diff 35548.Mar 24 2021, 2:56 PM
  • Cleanup: More unused includes
  • Cleanup: Use C++ vector and matrix types
  • Cleanup: Remove extra if
  • Revert partially commit rB6fc993ba44ea
  • Use nullptr instead of NULL
  • Merge branch 'master' into temp-gpencil-io
Harbormaster completed remote builds in B13702: Diff 35548.Mar 24 2021, 2:56 PM
Closed by commit rBa8a92cd15a52: GPencil: New modules for Import and Export (authored by Antonio Vazquez (antoniov)). · Explain WhyMar 24 2021, 3:29 PM
This revision was automatically updated to reflect the committed changes.
Antonio Vazquez (antoniov) added a commit: rBa8a92cd15a52: GPencil: New modules for Import and Export.
Charlie Jolly (charlie) awarded a token.Mar 24 2021, 3:33 PM
Antonio Vazquez (antoniov) added a comment.Mar 24 2021, 3:37 PM

I want to thanks to @Hans Goudey (HooglyBoogly) for all the code review and help!

Peter Efe (PeterEfe) awarded a token.Mar 25 2021, 2:06 PM
Carsten (calli) awarded a token.Mar 29 2021, 2:14 PM
Bataev Artem (shader) awarded a token.Apr 1 2021, 2:20 PM

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
29 lines
source/
blender/
blenkernel/
16 lines
intern/
551 lines
editors/
gpencil/
15 lines
include/
1 line
io/
15 lines
io_gpencil.h
io_ops.c
50 lines
438 lines
204 lines
64 lines
11 lines
space_file/
2 lines
io/
3 lines
gpencil/
100 lines
92 lines
intern/
118 lines
412 lines
208 lines
38 lines
65 lines
70 lines
323 lines
91 lines
475 lines
41 lines
105 lines
56 lines
262 lines
nanosvg/
3313 lines
python/
intern/
8 lines
14 lines

Diff 35511

View Options

release/scripts/startup/bl_ui/space_topbar.py

Loading...
View Options

source/blender/blenkernel/BKE_gpencil_geom.h

Loading...
View Options

source/blender/blenkernel/intern/gpencil_geom.c

Loading...
View Options

source/blender/editors/gpencil/gpencil_utils.c

Loading...
View Options

source/blender/editors/include/ED_gpencil.h

Loading...
View Options

source/blender/editors/io/CMakeLists.txt

Loading...
View Options

source/blender/editors/io/io_gpencil.h

Loading...
View Options

source/blender/editors/io/io_gpencil_export.c

Loading...
View Options

source/blender/editors/io/io_gpencil_import.c

Loading...
View Options

source/blender/editors/io/io_gpencil_utils.c

Loading...
View Options

source/blender/editors/io/io_ops.c

Loading...
View Options

source/blender/editors/space_file/filelist.c

Loading...
View Options

source/blender/io/CMakeLists.txt

Loading...
View Options

source/blender/io/gpencil/CMakeLists.txt

Loading...
View Options

source/blender/io/gpencil/gpencil_io.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_base.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_base.cc

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_capi.cc

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_export_base.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_export_base.cc

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_export_pdf.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_export_pdf.cc

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_export_svg.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_export_svg.cc

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_import_base.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_import_base.cc

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_import_svg.h

Loading...
View Options

source/blender/io/gpencil/intern/gpencil_io_import_svg.cc

Loading...
View Options

source/blender/io/gpencil/nanosvg/nanosvg.h

Loading...
View Options

source/blender/python/intern/CMakeLists.txt

Loading...
View Options

source/blender/python/intern/bpy_app_build_options.c

Loading...