Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(67)

Issue 6762043: GreasePencil enhancement (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 11 months ago by b.mont29
Modified:
11 years, 7 months ago
Reviewers:
aligorith
Base URL:
https://svn.blender.org/svnroot/bf-blender/trunk/blender/
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 36

Patch Set 2 : Updated patch… #

Total comments: 2

Patch Set 3 : Now handle nicely "old" timeless strokes, and UI polishing... #

Patch Set 4 : Minor optimizations #

Total comments: 21
Unified diffs Side-by-side diffs Delta from patch set Stats (+1051 lines, -78 lines) Patch
source/blender/editors/gpencil/gpencil_edit.c View 1 2 3 12 chunks +943 lines, -59 lines 21 comments Download
source/blender/editors/gpencil/gpencil_paint.c View 1 2 27 chunks +105 lines, -19 lines 0 comments Download
source/blender/editors/include/ED_gpencil.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
source/blender/makesdna/DNA_gpencil_types.h View 1 2 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9
mont29
11 years, 11 months ago (2012-10-24 09:04:33 UTC) #1
aligorith
Ok, I've had a look through most of the patch. Haven't carefully checked through some ...
11 years, 11 months ago (2012-10-24 11:12:03 UTC) #2
aligorith
http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/gpencil_edit.c File source/blender/editors/gpencil/gpencil_edit.c (right): http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/gpencil_edit.c#newcode558 source/blender/editors/gpencil/gpencil_edit.c:558: RNA_path_resolve(&id_ptr, "eval_time", &ptr, &prop); It's probably overkill to use ...
11 years, 11 months ago (2012-10-24 11:12:18 UTC) #3
mont29
updated patch is following… :) http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/gpencil_edit.c File source/blender/editors/gpencil/gpencil_edit.c (right): http://codereview.appspot.com/6762043/diff/1/source/blender/editors/gpencil/gpencil_edit.c#newcode558 source/blender/editors/gpencil/gpencil_edit.c:558: RNA_path_resolve(&id_ptr, "eval_time", &ptr, &prop); ...
11 years, 11 months ago (2012-10-25 06:50:04 UTC) #4
mont29
Updated patch…
11 years, 11 months ago (2012-10-25 06:53:13 UTC) #5
mont29
Now handle nicely "old" timeless strokes, and UI polishing...
11 years, 11 months ago (2012-11-04 18:13:15 UTC) #6
mont29
Minor optimizations
11 years, 11 months ago (2012-11-04 19:01:53 UTC) #7
aligorith
Hi, Just went through checking things more carefully. Found a few more things that could ...
11 years, 11 months ago (2012-11-11 10:28:03 UTC) #8
mont29
11 years, 11 months ago (2012-11-11 14:36:37 UTC) #9
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b