GreasePencil enhancement

This patch adds timing data to GP strokes. These are used, when converting them to curves, to create an EvaluationTime f-curve reproducing how the stroke was drawn.

This is useful to easily reproduce e.g. natural hand writing, or drawing… Small demo: http://youtu.be/VwWEXrnQAFI .

Wiki doc: http://wiki.blender.org/index.php/User:Mont29/Dev/Dynamic_Sketch

committed as r52096.

[aligorith, 2012-10-24 11:12:03]

Ok, I've had a look through most of the patch. Haven't carefully checked through
some of the more complicated parts yet.

Overall, functionality wise, +1. This looks like useful functionality, and is in
fact something I'd considered having way back when adding this, though back then
I thought that perhaps just animating the resultant strokes at a uniform pace
would have been enough.

I've indicated quite a few places where some tweaks are needed. As it stands
now, some parts need rework to be less confusing.

[aligorith, 2012-10-24 11:12:18]

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
File source/blender/editors/gpencil/gpencil_edit.c (right):

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:558: RNA_path_resolve(&id_ptr,
"eval_time", &ptr, &prop);
It's probably overkill to use path resolve here.
1) We can safely assume that ptr = id_ptr
2) Use something like prop = RNA_float_get(...);

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:574:
insert_keyframe_direct(reports, ptr, prop, fcu, cfra, 0);
Since you're adding quite a few keyframes like this directly, I'd recommend you
pass INSERTKEY_FAST for the "flag" arg when calling insert_keyframe_direct().
This makes things go a bit faster for creating lots of keyframes, as it avoids
the recalc step after adding each item.

Then, after creating all the keyframes (i.e. after these blocks of if's), call
calchandles_fcurve(fcu); on the fcurve once to get things sorted nicely.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:616: time_range = gtd->tot_time *
sce->r.frs_sec / sce->r.frs_sec_base; /* Seconds to frames. */
IIRC there should be a macro for this so you don't need to type it out

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:631: for (j = i + 1; j <
gtd->num_points; j++) {
Hmm... the indention is getting quite deep here. Consider refactoring out this
inner loop into a separate helper function.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:641: if (rem_gaps < 2)
if (...) {    <-- add brace
   ...
}             <-- add brace
else {
    ...
}

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:696: else
Better put braces around this.

My general rule-of-thumb here is: if the statement following if/else spans more
than 1 line (or is another if/for/while statement), use braces.

Then again, Campbell prefers the stricter version of always use braces

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:820: for (i = stitch ? 1 : 0, pt =
gps->points + (stitch ? 1 : 0), bp = nu->bp + old_nbp; i < gps->totpoints; i++,
pt++, bp++) {
Looks like it's time to split each part of this into a separate line:

for (i = stitch ... ;
     i < gps->totpoints;
     i++, pt++, bp++)
{
    ...

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:1095: while (nu) {
Use for loop here. Expanded iterator pattern is excuse for when language doesn't
have nice syntax for this stuff.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:1099: for (; i--; bezt++)
Argh! Side effects for what should be a predictable "condition test" only

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
File source/blender/editors/gpencil/gpencil_paint.c (right):

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:104: double inittime;    /* Used
when converting to path */
From what I can see, these don't need to be doubles, since practically
everywhere we're just clamping these down to floats when we save anyway. Unless
of course using floats actually creates nasty artifacts when doing really fast
scribbles, in which case, this might be ok...

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:465: /* XXX DO NOT smooth first
and last points! */
Ugh... the changes here look confusing. Unless it was broken before, would
prefer that this wasn't changed.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:501: /*	double inittime =
gpd->sbuffer_inittime;*/
Are any of these lines needed still?

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:529: time +=
old_points[offs].time * sfac;  /* XXX Check this works for time! */ \
Has this been resolved?

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:532: /* XXX Here too, do not lose
start and end points! */
Hmm... so why is this needed now (and not before)?

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:782: /* We must adjust timings!
*/
Eh? What's the point here? Comment could explain a bit better

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:786: bGPDspoint *pts =
gps->points + j;
Personal nitpick:
As a general rule, I put defines for structs/custom types before primitive
types. There are two benefits to that
1) Clarity - You notice more quickly if/when you've got mixed declarations/code.
Skimming past, I almost thought they were mixed here.
2) "Big bulky stuff" before "small, well defined, easy to pack" stuff. Probably
doesn't really make any difference in practice (for compiler output)...

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:788: for (; j--; pts--)
Eww... backwards iteration.

"j--" <-- too many side effects here...

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:810: /* We must adjust timings of
this new stroke! */
See comments above.

[mont29, 2012-10-25 06:50:04]

updated patch is following… :)

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
File source/blender/editors/gpencil/gpencil_edit.c (right):

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:558: RNA_path_resolve(&id_ptr,
"eval_time", &ptr, &prop);
On 2012/10/24 11:12:19, aligorith wrote:
> It's probably overkill to use path resolve here.
> 1) We can safely assume that ptr = id_ptr
> 2) Use something like prop = RNA_float_get(...);

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:574:
insert_keyframe_direct(reports, ptr, prop, fcu, cfra, 0);
On 2012/10/24 11:12:19, aligorith wrote:
> Since you're adding quite a few keyframes like this directly, I'd recommend
you
> pass INSERTKEY_FAST for the "flag" arg when calling insert_keyframe_direct().
> This makes things go a bit faster for creating lots of keyframes, as it avoids
> the recalc step after adding each item.
> 
> Then, after creating all the keyframes (i.e. after these blocks of if's), call
> calchandles_fcurve(fcu); on the fcurve once to get things sorted nicely.

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:616: time_range = gtd->tot_time *
sce->r.frs_sec / sce->r.frs_sec_base; /* Seconds to frames. */
On 2012/10/24 11:12:19, aligorith wrote:
> IIRC there should be a macro for this so you don't need to type it out

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:631: for (j = i + 1; j <
gtd->num_points; j++) {
On 2012/10/24 11:12:19, aligorith wrote:
> Hmm... the indention is getting quite deep here. Consider refactoring out this
> inner loop into a separate helper function.

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:641: if (rem_gaps < 2)
On 2012/10/24 11:12:19, aligorith wrote:
> if (...) {    <-- add brace
>    ...
> }             <-- add brace
> else {
>     ...
> }

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:696: else
On 2012/10/24 11:12:19, aligorith wrote:
> Better put braces around this.
> 
> My general rule-of-thumb here is: if the statement following if/else spans
more
> than 1 line (or is another if/for/while statement), use braces.
> 
> Then again, Campbell prefers the stricter version of always use braces

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:820: for (i = stitch ? 1 : 0, pt =
gps->points + (stitch ? 1 : 0), bp = nu->bp + old_nbp; i < gps->totpoints; i++,
pt++, bp++) {
On 2012/10/24 11:12:19, aligorith wrote:
> Looks like it's time to split each part of this into a separate line:
> 
> for (i = stitch ... ;
>      i < gps->totpoints;
>      i++, pt++, bp++)
> {
>     ...

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:1095: while (nu) {
On 2012/10/24 11:12:19, aligorith wrote:
> Use for loop here. Expanded iterator pattern is excuse for when language
doesn't
> have nice syntax for this stuff.

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_edit.c:1099: for (; i--; bezt++)
On 2012/10/24 11:12:19, aligorith wrote:
> Argh! Side effects for what should be a predictable "condition test" only

Well, it is predictable (as post-dec is always done after evaluation), and
optimized... But I agree it makes loops a bit hard to follow.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
File source/blender/editors/gpencil/gpencil_paint.c (right):

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:104: double inittime;    /* Used
when converting to path */
On 2012/10/24 11:12:19, aligorith wrote:
> From what I can see, these don't need to be doubles, since practically
> everywhere we're just clamping these down to floats when we save anyway.
Unless
> of course using floats actually creates nasty artifacts when doing really fast
> scribbles, in which case, this might be ok...

At least under Unix, these store values in "seconds since the epoch" (as
returned by PIL_check_seconds_timer()), which needs at least 12 digits to get a
1/100 of second precision timings... So we can't use floats.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:465: /* XXX DO NOT smooth first
and last points! */
On 2012/10/24 11:12:19, aligorith wrote:
> Ugh... the changes here look confusing. Unless it was broken before, would
> prefer that this wasn't changed.

It was broken before :)

This is more like a bugfix, in current code when you draw a stroke very quickly
(i.e. have quite some space between each stroke point), you get a noticeable gap
between the points where you started/ended the stroke, and the actual stroke's
start and end, which is not acceptable imho. And definitively wrong when we get
strokes created by overflow of points in previous one (i.e. where consecutive
strokes should match exactly, instead of having a gap between them).

This code simply avoid smoothing the first and last points.

And it fixes a code bug (i-1 > 0 is wrong, as 0 is a valid index, should be
either i-1 >= 0 or i > 0, the same goes for i - 2)...

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:501: /*	double inittime =
gpd->sbuffer_inittime;*/
On 2012/10/24 11:12:19, aligorith wrote:
> Are any of these lines needed still?

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:529: time +=
old_points[offs].time * sfac;  /* XXX Check this works for time! */ \
On 2012/10/24 11:12:19, aligorith wrote:
> Has this been resolved?

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:532: /* XXX Here too, do not lose
start and end points! */
On 2012/10/24 11:12:19, aligorith wrote:
> Hmm... so why is this needed now (and not before)?

It was needed before too! :)

Like above, this is more like a bugfix, in current code when you draw a stroke
very quickly (i.e. have quite some space between each stroke point), you get a
noticeable gap between the points where you started/ended the stroke, and the
actual stroke's start and end, which is not acceptable imho.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:782: /* We must adjust timings!
*/
On 2012/10/24 11:12:19, aligorith wrote:
> Eh? What's the point here? Comment could explain a bit better

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:786: bGPDspoint *pts =
gps->points + j;
On 2012/10/24 11:12:19, aligorith wrote:
> Personal nitpick:
> As a general rule, I put defines for structs/custom types before primitive
> types. There are two benefits to that
> 1) Clarity - You notice more quickly if/when you've got mixed
declarations/code.
> Skimming past, I almost thought they were mixed here.
> 2) "Big bulky stuff" before "small, well defined, easy to pack" stuff.
Probably
> doesn't really make any difference in practice (for compiler output)...

Done.

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:788: for (; j--; pts--)
On 2012/10/24 11:12:19, aligorith wrote:
> Eww... backwards iteration.
> 
> "j--" <-- too many side effects here...

Yeah, was kind of an "optimized" loop... But I agree it makes it more hard to
understand, and probably it's not worth it!

http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/g...
source/blender/editors/gpencil/gpencil_paint.c:810: /* We must adjust timings of
this new stroke! */
On 2012/10/24 11:12:19, aligorith wrote:
> See comments above.

Done.

[mont29, 2012-10-25 06:53:13]

Updated patch…

[mont29, 2012-11-04 18:13:15]

Now handle nicely "old" timeless strokes, and UI polishing...

[mont29, 2012-11-04 19:01:53]

Minor optimizations

[aligorith, 2012-11-11 10:28:03]

Hi,

Just went through checking things more carefully. Found a few more things that
could be tweaked as noted below.

Also, a few general comments:
* Remove period ('.') from end of single-line comments
* Put closing */ for multiline comments on new lines
* For property descriptions, capitalise first word after colon. In most cases,
these things are "Short Header/Type: Long form message explaining the why header
is so"

After these tweaks, I think it's probably safe to put into trunk :)

Regards,
Aligorith

http://codereview.appspot.com/6762043/diff/8001/source/blender/editors/gpenci...
File source/blender/editors/gpencil/gpencil_paint.c (right):

http://codereview.appspot.com/6762043/diff/8001/source/blender/editors/gpenci...
source/blender/editors/gpencil/gpencil_paint.c:470: copy_v2_v2_int(&spc->x,
&((tGPspoint *)gpd->sbuffer)->x);
((tGPspoint *)gpd->sbuffer)

It might be better if you defined a var up top, and just use that instead of
casting everytime you need to fetch stuff.

Do:
tGPspoint *sbuf = (tGPspoint *)gpd->sbuffer;
...
spc = smoothArray;
copy_v2_v2_int(&spc->x, &sbuf->x);
for (...) { 
    ...
}
copy_v2_v2_int(&spc->x, &(sbuf + i)->x);

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
File source/blender/editors/gpencil/gpencil_edit.c (right):

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:65: #include "BKE_nla.h"
Hmm... is this include actually needed?

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:430: static EnumPropertyItem
*rna_GPConvert_mode_items(bContext *UNUSED(C), PointerRNA *ptr, PropertyRNA
*UNUSED(prop),
On closer inspection, this function seems to be doing some slightly dubious
things. In particular, modifying the enum value. The enum modifying stuff should
probably happen in the invoke() method instead (exec() is probably fine too, if
you must ensure consistency).

Double check that the "operator cheat sheet"  tool will not crash as a result of
this.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:441: mode = RNA_boolean_get(ptr,
"use_link_strokes") ? RNA_enum_get(ptr, "timing_mode") :
This line looks long enough that you might as well just use an if statement for
it

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:554: /* In frames! Binary search
for FCurve keys have a threshold of 0.01, so we can’t set
FCurves?! :P

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:658: else {
This case looks quite long. Split the logic for these cases into separate helper
functions.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:779: DAG_ids_flush_update(bmain,
0);
DAG_id_tag_update(cu, 0); 

might be enough here actually. ids_flush_update() is a bit heavy handed

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:902: i++, pt++, bp++) {
{ on newline

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1355: } while((gps = gps->next));
space between while and opening paren

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1446: strcmp(prop_id,
"use_link_strokes") == 0)
if (...
    ...
    ...
    ...)
{            <-- add brace here
    ... stmt ...
}            <-- add brace here

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1461: strcmp(prop_id,
"start_frame") == 0)
if (... ||
    ...)
{     <--- add brace here
    ...; 
}     <--- add brace here

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1546: "Custom Gap mode: number of
frames the gap can randomly vary of", 0.0f, 1000.0f);
"can randomly vary of" sounds a bit weird. Maybe it would be better to use
something like:
"number of frames that gap lengths can vary"

[mont29, 2012-11-11 14:36:37]

Addressed all issues pointed out… will commit soon. Thanks again for the review!
:)

http://codereview.appspot.com/6762043/diff/8001/source/blender/editors/gpenci...
File source/blender/editors/gpencil/gpencil_paint.c (right):

http://codereview.appspot.com/6762043/diff/8001/source/blender/editors/gpenci...
source/blender/editors/gpencil/gpencil_paint.c:470: copy_v2_v2_int(&spc->x,
&((tGPspoint *)gpd->sbuffer)->x);
On 2012/11/11 10:28:03, aligorith wrote:
> ((tGPspoint *)gpd->sbuffer)
> 
> It might be better if you defined a var up top, and just use that instead of
> casting everytime you need to fetch stuff.
> 
> Do:
> tGPspoint *sbuf = (tGPspoint *)gpd->sbuffer;
> ...
> spc = smoothArray;
> copy_v2_v2_int(&spc->x, &sbuf->x);
> for (...) { 
>     ...
> }
> copy_v2_v2_int(&spc->x, &(sbuf + i)->x); 

Actually, found a much nicer optimization! We don't need the whole array of
tGpSmoothCo (nor even that struct at all), memory consumption and manipulation
is completely overkill here, as we only need the last two points org coords!

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
File source/blender/editors/gpencil/gpencil_edit.c (right):

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:65: #include "BKE_nla.h"
On 2012/11/11 10:28:03, aligorith wrote:
> Hmm... is this include actually needed?

Looks like it is not! :)

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:430: static EnumPropertyItem
*rna_GPConvert_mode_items(bContext *UNUSED(C), PointerRNA *ptr, PropertyRNA
*UNUSED(prop),
On 2012/11/11 10:28:03, aligorith wrote:
> On closer inspection, this function seems to be doing some slightly dubious
> things. In particular, modifying the enum value. The enum modifying stuff
should
> probably happen in the invoke() method instead (exec() is probably fine too,
if
> you must ensure consistency).
> 
> Double check that the "operator cheat sheet"  tool will not crash as a result
of
> this.

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:441: mode = RNA_boolean_get(ptr,
"use_link_strokes") ? RNA_enum_get(ptr, "timing_mode") :
On 2012/11/11 10:28:03, aligorith wrote:
> This line looks long enough that you might as well just use an if statement
for
> it

Done (actually, moved away into op exec func).

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:658: else {
On 2012/11/11 10:28:03, aligorith wrote:
> This case looks quite long. Split the logic for these cases into separate
helper
> functions.

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:779: DAG_ids_flush_update(bmain,
0);
On 2012/11/11 10:28:03, aligorith wrote:
> DAG_id_tag_update(cu, 0); 
> 
> might be enough here actually. ids_flush_update() is a bit heavy handed

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:902: i++, pt++, bp++) {
On 2012/11/11 10:28:03, aligorith wrote:
> { on newline

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1355: } while((gps = gps->next));
On 2012/11/11 10:28:03, aligorith wrote:
> space between while and opening paren

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1446: strcmp(prop_id,
"use_link_strokes") == 0)
On 2012/11/11 10:28:03, aligorith wrote:
> if (...
>     ...
>     ...
>     ...)
> {            <-- add brace here
>     ... stmt ...
> }            <-- add brace here

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1461: strcmp(prop_id,
"start_frame") == 0)
On 2012/11/11 10:28:03, aligorith wrote:
> if (... ||
>     ...)
> {     <--- add brace here
>     ...; 
> }     <--- add brace here

Done.

http://codereview.appspot.com/6762043/diff/14002/source/blender/editors/gpenc...
source/blender/editors/gpencil/gpencil_edit.c:1546: "Custom Gap mode: number of
frames the gap can randomly vary of", 0.0f, 1000.0f);
On 2012/11/11 10:28:03, aligorith wrote:
> "can randomly vary of" sounds a bit weird. Maybe it would be better to use
> something like:
> "number of frames that gap lengths can vary"

Done.