With such a big patch there is a lot to cover

From a quick look over the patch, my main concern is the kinds of operations modifiers are performing - manipulating G.main, adding objects to the scene and calling scene frame updates from within applyModifier operation.

It looks like grease pencil isn't integrated into the depsgraph so it's running its own update functions from within the modifiers.

Ignoring the details of Blender's API's and conventions, a modifier stack or node system's individual operations should execute a pipeline (also known as "pipes and filters pattern")

Without this it can be hard to get predictable output, since each modifier adjusts the global state.

Indeed, I also found a bit of strange stuff going on with the modifiers.

• I'm under the impression that a lot of the object/context usage is really for the "apply modifiers" case (i.e. make the modifier-generated geometry real geometry). If so, it's odd that it would seemingly run all the time, with no checks to only do this if actually baking the modifier results.
• The other strange thing isthat for many of the modifiers, everytime applyModifier is called, it goes over all GP frames, performing the modifier's operations on it (which is why it ends up doing all its own updates). I suspect this probably has to do something with the need to load all the geometry into the draw manager caches at once.

Perhaps @Antonio Vazquez (antoniov) can shed a bit more light on these things.

The changes for modifiers seem like they will need some iterating over.

Another issue is the modifiers are modifying data in-place (@Joshua Leung (aligorith) - is this what you mean by baking?).
The return value is ignored, making this seem more like some mis-use of the modifier stack.

Wonder if it would be better to review separately, since I have the impression other parts of this patch can probably be committed with only minor adjustments.

It may be better to remove derived-mesh from 2.8, so grease pencil can take advantage of copy-on-write.

In D2889#67923, @Campbell Barton (campbellbarton) wrote:

The changes for modifiers seem like they will need some iterating over.

Another issue is the modifiers are modifying data in-place (@Joshua Leung (aligorith) - is this what you mean by baking?).

No. By baking I was referring to the "Apply" button on each modifier.

But I do agree that it is a bit vague right now whether the modifiers are modifying stuff in place. For example, I was getting the disturbing feeling that this was happening with the palette colors. However, I wasn't sure whether there was already some COW stuff going on that would have mitigated that.

The return value is ignored, making this seem more like some mis-use of the modifier stack.

Wonder if it would be better to review separately, since I have the impression other parts of this patch can probably be committed with only minor adjustments.

It may be better to remove derived-mesh from 2.8, so grease pencil can take advantage of copy-on-write.

It depends whether modifiers are much of a priority for the Hero project I guess. I suspect though that at this stage, we may be able to get away with not having it in 2.8 for a few weeks until we figure it the design issues.

About "'I'm under the impression that a lot of the object/context usage is really for the "apply modifiers". Yes, the context is only used in apply. I pass the context for some operations I need to apply.

The modifiers are designed to modify the geometry of the stroke when the stroke is drawn. I considered to make a full copy like derived mesh, but GP has layers / frames / strokes / points / weights, etc... and the copy would take a lot of time.

About if this is necessary for Hero, I think is critical. They a main part of the open movie.

The arry modifier has sense in GP. There are two modifiers: Array and Duplication.

• Array create copies of the object and when apply needs to add new GP objects to the scene to hold the strokes.
• Duplication works very similar to array with meshes. Create a duplication of the strokes and when apply, all the strokes are linked to the same object.

Array is used in some situations to get fresh copies of the object. If you uses array modifier, the datablock is shared between all objects and the geometry cache too.

I forget something about modifiers. There are VFX modifiers that don't change the geometry but only the display of the geometry (vfx passes). I put them in the modifier stack to keep consistency.

After spending a lot of time digging through both the standard modifier system, and the GP branch modifier system as it stands now, I think I *finally* understand what's going on here. Basically, the GP branch just seems to be using various callbacks for the wrong purposes, leading to all the confusion we've been having:

• The actual modifier logic (i.e. the part that does all the magic modifying strokes) lives in gpencil_modifiers.c (e,g, functions like BKE_gpencil_noise_modifier). These calls only operate on the given (passed-in) GP frame/stroke data.

• For evaluating the modifiers, the GP draw manager/engine implements its own crude Copy-On-Write-like system.
  • AFAIK, it seems to only operate on the active GP frame (not on all frames at once). This is as expected, and is just like every other part of Blender.
  • What happens is that the GP draw engine (see DRW_gpencil_populate_datablock() in gpencil_draw_cache_impl.c) first makes a copy of this frame and all its stroke data, storing this *derived* frame (created using gpencil_frame_color_duplicate()) into the derived_gpf GHash on the GP datablock. That GHash acts as a cache/storage for all these modifier-affected copies of the frame data that ultimately gets drawn
  • In gpencil_draw_strokes(), the GP modifiers are then applied to this *derived frame* (not the original source data). This is done using functions BKE_gpencil_stroke_modifiers() and BKE_gpencil_geometry_modifiers() at the bottom of gpencil_modifier.c. This pair of functions loops over the modifers, and applies them one by one by calling the BKE_gpencil_*_modifier() functions one by one to mutate the derived frame data.

• The applyModifier() calls in the MOD_gpencil*.c files are implementing the behaviour for the "Apply Modifier" button (i.e. the one that appears on each Modifier in the UI). They are *not* used when evaluating the modifiers everytime we need to update the draw caches (i.e. in response to changes/ BKE_gpencil_batch_cache_dirty() tagging).
  • Thus, all the frame iteration, object/palette creation (in the main database), and other destructive whole-database operations in the MOD_gpencil*.c files aren't as harmful as they might look - it's just that they're all in the wrong places! (I propose instead that we move all this to a special file in editors/gpencil and reimplement special "Apply Modifier" operators for GP modifiers only, while keeping this stuff out of those files)

With those points in mind, here's a proposed path forward for resolving the current issues:

1. Move all the existing applyModifier() code out of those callbacks, and create a new "Apply Modifier" operator for GP modifiers. The code currently in the callbacks would instead go to one of the following places (either/or, not both):
   1. Move them into a new editors/gpencil/gpencil_modifier_ops.c file, and define these as different cases for the "Apply GP Modifier" operator, OR
   2. We keep all of them in the MOD_gpencil*.c files, but we add a new applyAndRemoveModifier() (or equivalent) callback to the ModifierTypeInfo structs

2. Potentially move the actual "business logic" for each modifier into the relevant MOD_gpencil*.c files, by adding two callbacks to ModifierTypeInfo:
   • deformStrokes (i.e. for modifiers handled via BKE_gpencil_stroke_modifiers()) and/or
   • generateStrokes (i.e. for modifiers handled via BKE_gpencil_geometry_modifiers()

3. updateDepsgraph() callbacks and other ID-link functions should be defined for each GP modifier that needs them

4. Possible depsgraph opnodes that call BKE_gpencil_batch_cache_dirty() to trigger a refresh of the stroke data caches (and modifiers to get reevaluated)?

5. Eliminate the BKE_gpencil_batch_cache_alldirty() calls. We probably shouldn't have any further needs for it, but we'll revisit this when we get there.

6. Move GP-object data from ob->gpd to ob->data. (Less important than all the other issues, nice to do from consistency standpoint, and relatively straightforward to sort out. That said, it does introduce one more place where we break more backwards compatability for files and also addons, though 2.8 breaks a lot of stuff anyway)

Thoughts?

@Joshua Leung (aligorith) You have explained perfect how works the modifiers. What I did was use the actual blender modifiers as a "placeholder" to gp modifiers. Basically, it reuses the add, remove, link, up, down, etc of the modifiers structure, but never uses the derived mesh or any other "update" data function. All "magic" is done in draw manager loop.

For the Apply, the callback was used only to handle the apply button, but never this function is evaluated in every draw loop (this is why I didn't understand the worries about this routine in every loop).

My design was based on my limited knowledge of Blender as a whole, so I totally agree in the proposed changed that will make the code cleaner and most aligned with other areas of Blender.

The last diff file includes a big change in the drawing brushes. Now the drawing brushes are using Blender Brushes and any reference to bGPDbrush was removed. It can be linked and append to new files.

Now the brushes can use a palette color associated to get some type of "premade" custom brush with material.

Matias Mendiola created a full set of new icons too.

There aren't conversion code from old brushes because the old parameters were not compatible with current brushes. Any conversion code of old versions was removed.

The drawing brushes has been improved a lot, adding code to capture very fast mouse movements and including textures in strokes and fills. The old Dots mode has been improved a lot too.

It has been added support for render with F12 with Eevee and Cycles and the grease pencil layers can be filtered by view layer.

About the pointer to palette and palettecolor, I think we have only problem with the palette.

The palettecolor pointer is only used in runtime but is not saved. The reference to the color is done using the colorname and the first time the color is used, a function get the pointer of this color using its color name. Reading your comments, I think we will have to do the same with palettes.