I will upload a new diff with redesign of the close stroke operator using Severin idea of cyclic drawing. The new operator has 3 options: Close all, Open all, Toggle.

Aligorith, the alpha value for selection is not by sculpt brush. It is defined in GP_BrushEdit_Settings struct, just below GP_BRUSHEDIT_FLAG_SELECT_MASK flag field.

This settings are defined in the ToolSetting in gp_sculpt field.

Add comment

Add a comment

The reason to create the join y mainly due fill. For example, if you draw a comic human face and the strokes are not continuous (for artistic reasons), if you don't join with invisible lines you cannot fill. The join speed up a lot the filling process.

Join&Copy is used to create automatically a duplicate with the current active color.

This is the same technique used by ToonBoom Animate/Harmony.

About change color, this operator must change all layers, not only active. What you propose, change the name of the operator?

Comment Added

Add Comment

Add comment

Looking through a bit more of the code, I came across a few places where it's now iterating over the layers, then the strokes manually, so that we have access to the layer that a stroke belongs to, in order to apply parenting corrections. The problem is that this just makes maintenance worse, in that now you have to modify all the places where this happens (which happens to be most of the tools).

There are two options to solve this:

1. Using a pair of context-iterator macros - similar to the DRIVER_TARGETS_LOOPER + DRIVER_TARGETS_LOOPER_END pair in BKE_fcurve.h
2. Using the standard context collection stuff - but instead of having bGPDstroke* entries in there, we instead allocate special structs that contain a pair of points (i.e. bGPDlayer * + bGPDstroke *)
3. Something akin to the keyframe editing api's - Basically, you have an API call that allows looping over the editable strokes in the current context, and then you supply a callback function with context for doing stuff with those strokes and their points. You could have extra args to specify whether you want to auto-apply all the parent transforms to the points or not.

I lean towards the first one, since the context stuff ends up being used by the Python API too, making it messy to define adhoc pairs of items. Meanwhile, the third one borders on being overkill and a bit heavy handed (not to mention needing a lot more helper functions to make everything work); at this stage, it might be too much trouble to recode everything to work this way, so I'm not keen on doing that here for now.

So, I imagine the context iterator macros to look something like this:

/**
  * Iterate over all editable strokes in the current context, 
  * stopping on each usable layer + stroke pair (i.e. gpl and gps)
  * to perform some operations on the stroke.
  *
  * \param gpl  The identifier to use for the layer of the stroke being processed.
  *                    Choose a suitable value to avoid name clashes.
  * \param gps The identifier to use for current stroke being processed.
  *                    Choose a suitable value to avoid name clashes.
  */
#define GP_EDITABLE_STROKES_BEGIN(C, gpl, gps)                                                  \
   {                                                                                                                             \
       CTX_DATA_BEGIN(C, bGPDlayer*, gpl, editable_gpencil_layers)                            \
       {                                                                                                                         \
           /* calculate difference matrix if parent object */
           float diffmat[4][4];                                                                                           \
           ED_gpencil_parent_location(gpl, diff_mat);                                                        \
           /* loop over strokes */
           for (bGPDstroke *gps = gpl->actframe->strokes.first; gps; gps = gps->next) { \
                /* skip strokes that are invalid for current view */
                if (ED_gpencil_stroke_can_use(C, gps) == false)                                            \
                    continue;                                                                                                 \
                /* check if the color is editable */
                if (ED_gpencil_stroke_color_use(gpl, gps) == false)                                       \
                    continue;                                                                                                 \

/* ... Do Stuff With Strokes ...  */

#define GP_EDITABLE_STROKES_END  \
          }                                                \
    }                                                      \
} (void)0

In D2115#49268, @Julian Eisel (Severin) wrote:

Second round of (partially really rough) review. Some more notes:

• Join OP: I'm not really convinced about this operator. What is the benefit of having 2 strokes joined? If it's just for having them share some settings (thickness, color, etc), wouldn't a copy_settings operator make more sense? For duplicating there's still GPENCIL_OT_duplicate_move. Also note that this breaks the 1 stroke == 1 continuous line idea, making operations such as cyclic_toggle and select_linked behave glitchy (would be good to hear feedback from users and @Joshua Leung (aligorith)).

Let's address the two points here separately:

1. I'm all for the Join operator. In fact, I would've implemented it myself at some point anyway :) The main reason it's useful is when creating fills. Specifically, sometimes it's easier to draw some kinds of strokes one way, and other strokes another way; by allowing users to sketch-out the fill shape in segments makes it easier to create such shapes, as you don't have to start being really careful to not mess up a particularly hairy/complex area that comes at the end of a long stroke.

2. If the operator does not create links between the different segments that have been joined together, then we might have a bit of a problem. The way I've always thought of a Join operator working is that it would just tack the second stroke onto the end of the points buffer of the first stroke (keeping the start/end points relatively consistent), resulting in a single continuous stroke anyway.

• @Joshua Leung (aligorith), noticed some functions, esp. in BKE_gpencil.h don't use the conventional prefix. Is that intentional? (Would propose to cleanup after branch landed)

This is something I've never bothered to change (apart from a little "argh!" at some bits of the conventions too ;) So, was it intentional - 50/50 :) But yes, this is something to consider for later and not as part of this patch.

The join operator does exactly this: Join the end of one stroke with the next one.

I have implemented a flip routine to reverse stroke direction in order to join properly the strokes. The routine try to do the flip automatically if necessary, but the result is not perfect in all situations, so I implemented a visual hint to see start and end points and a flip operator. I think we could implement better joining/flipping algorithms in future versions.

Some more code review. Sorry for all the noise with separate comments, but this editor seems to get laggy if I do too many of these in one go :/

• I just noticed some more potential quirks with the parenting stuff:
  • Specifically, the point should only get transformed when dealing with a 3D stroke. 2D strokes (view or in 2d space) should never get transformed in response to a parent object.
  • Perhaps there might be a case for being able to parent screenspace strokes to empties to jiggle them around - @Daniel Martinez Lara (pepeland) Is this something that would even make any sense in practice? I mean, in the 3d-viewport, you'd just see the empty as being in 3d, but the effect would only be the xy components of that mapped to screenspace... Hmm, maybe it could be useful?
  • (In practice, most of the time this might not really be a problem anymore, since you can't easily set parents for layers from non-3d views)

• Just out of curiosity, why did you end up storing the names of palettes colors in each stroke instead of just storing the index?
  • With an index, searching for the colors becomes faster (no need to do string comparisons)
  • We also wouldn't need all the "strokename update" functions, and file sizes would be better for this (i.e. a saving of some 64-4 = 60 bytes per stroke! Considering that there are quite a lot of strokes per file... :)
  • Rearranging color order might get messier though, if you want to keep the strokes keeping the colors they currently have. (Otherwise, rearranging might be an interesting way to quickly cycle through different colors)
  • What is current behaviour if you name a color differently in one palette, create some strokes using that color, and then switch to another palette? Or for that matter, if you don't have the same number (or set of names) of colors in each palette (if that's even possible)?

• Personal preference: But, although we can now declare variables anywhere in the code, could we as much as possible try keeping them near the start of the inner-most block where they are used (Note: block not start of each function - so, where you have an opening curly brace { ). It just makes it easier to figure out what the main variables we're dealing with are (instead of having extra ones suddenly popping up out of nowhere). So for instance, it becomes easier to say - "we're doing a calculation involving 5 vectors - two temp ones (and related to each other), leading to a intermediate result, leading to the final result"
  • Initialising/declaring new vars in for-loops are fine, as long as you only need a single iterator variable. With two (for the "it = next) { next = it->next;" pattern, it's nicer to have them declared together outside the loop),
  • If you're allocating stuff that needs to have had some other calcs done beforehand (e.g. as is common when allocating an array where the number of items to store needs to be calculated) delaying when you declare that variable might also be fine.

My first idea about colors was to save the index of the color (faster and shorter) but after some test with artists, sometimes they have different number of color in each palette and the order of the colors could be different, so the index is not a solution.

If we lock the palettes to have the same colors in the same order, then we could use the index.

Add comment

Add comment

I still don't like it that the join OP messes up the 1 stroke == 1 continuous line concept, but I don't see a better way to do it offhand either.

stroke_lock_color and palette_lock_layer both apply to all colors and to all strokes if I understand correctly. However they are placed within options that only apply to the currently active color. I'd either move them on top of the panel (even before the palette selector), or into the toolbar. Can be done after branch is merged.

Another thing to think about before merging: What's up with documentation for release notes and manual?

Committed to master :) Thanks for all the hard work @Antonio Vazquez (antoniov), keep up the great work!

Alrighty, so here are some further steps:

• Documentation TODOs:
  • Release notes
  • Blender manual - This probably means quite some work so maybe some users could help here.
  • Would be really cool to have some of your code design documentation here (we try to get into the habbit of adding code documentation)
• Fix GPENCIL_OT_stroke_cyclical_set not working with redo
• Find a better place for stroke_lock_color and palette_lock_layer buttons

About the operator, I think they are in the correct location. Both of them lock/hide colors in the palette, If we move to top, then the artist will not know what colors are affected. These operators are not related to layers, but palette colors.

Maybe the problem is the name of the operators, but not the location in UI panel.