Page MenuHome

GPencil: Support for Sequence Images in Trace Tool
ClosedPublic

Authored by Antonio Vazquez (antoniov) on Oct 22 2020, 7:33 PM.

Details

Summary

Now it's possible trace image sequences or videos.

In order to support this, now a job is created when trace more than one image, and a new option allows to select if trace one image or the full sequence.

Diff Detail

Repository
rB Blender

Event Timeline

Antonio Vazquez (antoniov) requested review of this revision.Oct 22 2020, 7:33 PM
Antonio Vazquez (antoniov) created this revision.
Antonio Vazquez (antoniov) retitled this revision from GPencil: Basic trace sequence images to GPencil: Support for Sequence Images in Trace Tool.Oct 22 2020, 7:35 PM
Antonio Vazquez (antoniov) edited the summary of this revision. (Show Details)

@Hans Goudey (HooglyBoogly) Could you take a look at the job section? I never used before the batch job and would like to get a second pair of eyes on it.

This revision is now accepted and ready to land.Oct 22 2020, 7:52 PM
Hans Goudey (HooglyBoogly) requested changes to this revision.Oct 22 2020, 10:06 PM

Overall it seems quite odd to have the duplication between the new operator implemented in python and the current GPENCIL_OT_trace_image operator. All of these RNA properties shouldn't really be defined twice.
I'm not sure I see the reason to also define an operator in Python?

Aside from that, the job code seems to make sense. I did run into an issue testing the operator with this (admittedly very bad) example:

It only gpencil strokes for the first couple seconds of the video, but it goes on much longer than that.

release/scripts/startup/bl_operators/gpencil_bake_trace_image.py
46 ↗(On Diff #30276)

Typo

103 ↗(On Diff #30276)

Copy and paste error here

source/blender/editors/gpencil/gpencil_trace_ops.c
234

I would just declare the variable where its value is assigned. The style guide mentions this: "Keep variable scope as small as possible."

249–250

How about else if (trace_job->image->type == IMA_TYPE_IMAGE) {?
Would save one level of indentation.

380

I would give this function a more specfic name, since the entire function up to this point has been initializing the job struct.

393

I wonder if using RNA_property_is_set would be better than setting the string to a specific value and using a string comparison.

393

Extra whitespace is unecessary

394–396

It would be nice to avoid calling the CTX_* functions twice.

398

There should probably be a check for whether ob_gpencil is linked too, right?

434

This should use RNA_def_property_flag(prop, PROP_SKIP_SAVE);, otherwise the name will be empty when you delete the object.
I would try to keep the pointer property type working, the interface for this string property isn't as good. Or define a custom ui callback for the operator so there can be a proper search menu.

This revision now requires changes to proceed.Oct 22 2020, 10:06 PM
  • Changes after first review.
  • Changes to remove python and use default Invoke panel.
  • Remove object parameter as string.
  • Cleanup: Remove double call to CTX functions
Harbormaster completed remote builds in B10890: Diff 30306.

Works well for me! The issue I was having before was that the frame range of image empties isn't properly set when dragging them in.

Just two suggestions for wording inline. Also unrelated to this patch, but it might be nice to add some explanation to "Determine what is considered white and what black" for whether higher values mean that lighter areas are filled more or less.

source/blender/editors/gpencil/gpencil_trace_ops.c
410

Suggest: "Trace the current frame of the image"

433

Suggest: "Leave empty to create a new object"

This revision is now accepted and ready to land.Wed, Oct 28, 3:28 PM