Grease Pencil Branch (greasepencil-object)
Needs RevisionPublic

Authored by Joshua Leung (aligorith) on Oct 19 2017, 3:57 PM.

Details

Summary

Here's the GP branch for 2.8

Docs:

Changes since last review (in ~October 2017):

  • GP Object's data is stored in ob.data not ob.grease_pencil (ob->gpd) now
  • Modifier stack handling/usage has been refactored
  • Lots of code cleanup & review
  • New features

WIP stuff:

  • Restoration of simple GP "Annotation" functionality for 2D editors

Diff Detail

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

release/scripts/startup/bl_ui/properties_grease_pencil_common.py
519

Should use set syntax, also below. {...}

557

this is in fact checking if the string is inside the other. If mode were P this would succeed. Just check equality.

source/blender/draw/engines/gpencil/gpencil_draw_cache_impl.c
80–99

Too verbose, best add braces and assign a variable.

Could also memset to zero to avoid all explicit NULL assignments.

source/blender/makesdna/DNA_modifier_types.h
2173

This isn't really good to use the context in a modifier.

Modifiers should be able to operate with their input arguments only.

source/blender/makesrna/intern/rna_modifier.c
4920

should use separator pass_index

source/blender/makesrna/intern/rna_palette.c
383

Think this should be texture_offset - rna_smoke.c:1031 names it this way.

source/blender/modifiers/intern/MOD_gpencilarray.c
86–105

Why is this needed? adding new objects within applyModifier is quite strange.

source/blender/modifiers/intern/MOD_gpencillattice.c
78

This should use md->scene

95–106

Doing update for new frame within a modifier is highly likely to cause problems.

source/blender/modifiers/intern/MOD_gpenciltint.c
56

This had bad code smell, doing global call (which accesses G.main) within local modifier initialization seems like something that could easily backfire later on.

Isn't this something the depsgraph should handle?

82–86

Low priority: for slight performance improvement BLI_ghash_ensure_p can be used here (does the lookup and insertion in a single step).

89–108

Modifiers manipulating G.main means these modifiers can't be threaded (or will need to lock).

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Sync with 2.8, minor cleanup

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.

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.

source/blender/modifiers/intern/MOD_gpenciltint.c
89–108

Really we need to thread when apply a modifier? are there any alternatives to use G.main?

source/blender/modifiers/intern/MOD_gpenciltint.c
89–108

The problem is that with the depsgraph evaluation, multiple datablock get evaluated in parallel. Now, if GP modifiers get run during the standard depsgraph driven evaluation process, you're going to get crashes and other threading bugs.

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.

source/blender/modifiers/intern/MOD_gpenciltint.c
89–108

But this is only evaluated when apply, no? so how the user can apply several at the same time?

source/blender/modifiers/intern/MOD_gpencillattice.c
95–106

This is only done when apply, not in every drawing cycle.

source/blender/modifiers/intern/MOD_gpenciltint.c
89–108

Apply is not just when the user presses apply. - It is to calculate the result which may be animated. So its not unusual for apply to run on every frame-step, for every modifier that has animation.

Sergey Sharybin (sergey) requested changes to this revision.Oct 20 2017, 3:04 PM

First round on review.

Data

If GreasePencil becomes an object type, the grease pencil data is to be stored in Object->data, same as for any other object type. I do not see why GreasePencil type of object should be an exception here.

Modifiers

I do not think this is helpful trying to re-use existing modifier system. You're ignoring lots of stuff from it (for example, you do not need any of DerivedMesh stuff) and implement stack evaluation next to existing one.

Smaller thing here is that having all modifiers implemented in a single file will turn out being more problematic in a longer term.

Depsgraph

It seems that you're avoiding depsgraph as much as possible, which only causes problems (the batch cache dirty all will likely cause lots of race conditions).

Use an existing examples of light probes or meshes to see how it is all integrated together.

release/scripts/startup/bl_ui/properties_grease_pencil_common.py
81

That is somewhat weird to use space as a label.

105

Only use XXX for dirty hacks which needs to be addressed ASAP. Here it sounds more a TODO.

430

Again, what is the use of empty panel labels?

443

While we are on this, do we really need to use type-cast to bool?

520

Why not to simply return context.active_object and is_gpmode?

1075

Remove dead code.

1395

Dead code.

1497

If that's an XXX, solve it before merge. Otherwise is a TODO.

release/scripts/startup/bl_ui/properties_material_gpencil.py
27

Dead code.

102

Dead code.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
110

return isgpmode

166

Same as above.

Also, does it worth making an utility function of some sort of base class, this lines of code are duplicated in quite few places.

Check other cases in this file as well.

source/blender/blenkernel/intern/gpencil.c
90

Replace this with explicit bMain being passed here. No G.main in the new code please!

510–511

float curcolor[3] = {1.0f, 1.0f, 1.0f};

905

Why do you fix NULL pointers here instead of doing this in do_versiosn?

999–1002

Bet this is to be handled differently now. @Bastien Montagne (mont29)?

1587

Don't use XXX for not-a-hacks.

This applies everywhere. Reduce amount of XXX as much as possible, ideally to 0 ;)

1923

No G.main please.

1954

Tempting to think it's better to pass bmain explicitly here rather than via bContext. Will make the function easier for re-use.

2024

Nitpicking: no camel case for variables.

source/blender/blenkernel/intern/gpencil_modifier.c
168

This isn't any less arbitrary or more better than using first 3 points. So what is the reasoning here and why can't we consider all points?

208

Why would scene be NULL?

249

Should avoid such a runtime data in DNA.

702

Dead code.

source/blender/blenkernel/intern/lattice.c
1240

This does not belong here. This will make it so ANY lattice's update will invalidate ALL GP.

You should be introducing dependency between lattice and object if GP uses lattice, and invalidate GP corresponding to only that object.

source/blender/blenkernel/intern/object_update.c
331

Why do you need special case here and can not do same what mesh objects are doing?

This is also wrong because is only invalidating case when transform changes, but not modifiers or anything.

source/blender/blenloader/intern/versioning_270.c
1621

Next versionn bump code should be at the bottom.

Also not sure why such a versioning is in 2.70 series?

source/blender/makesdna/DNA_brush_types.h
194

Palette is like a tool setting. I don't see why you want to those. It is like animating sculpt brush size -- not that useful at all, only gives issues.

source/blender/makesdna/DNA_modifier_types.h
90

GP modifiers seems to have completely own stack implementation, so i don't think it's beneficial trying to re-use existing modifier flags / structures.

source/blender/makesrna/intern/rna_palette.c
416

Dead code.

source/blender/modifiers/intern/MOD_gpencilarray.c
86–105

There must be NO dependency graph changes from within modifier stack. Period.

114

You must not use bContext from inside modifier. It is undefined.

115

No bmain access either, only access data which is used by modifier itself.

source/blender/modifiers/intern/MOD_gpencilblur.c
49

This does not belong to modifier stack.

source/blender/modifiers/intern/MOD_gpencillattice.c
99

You can not do this. Period.

You must not modify global state from inside modifier. And should not re-calculate the whole scene for a single object. This will get unusably slow when you try to apply lattice modifier in a scene with 10s of characters.

Not only you're modifying global state, you're also leaving it in a broken state (scene evaluated for a wrong frame).

This revision now requires changes to proceed.Oct 20 2017, 3:04 PM
source/blender/makesdna/DNA_brush_types.h
194

Not really. Palettes group colors and color properties can be animated. It's not a tool is a property of the strokes.

source/blender/makesdna/DNA_brush_types.h
194

It is used for strokes in GP, but it was not before (was bGPPalette or so before). And as far as i remember it was only used by tools.

source/blender/makesdna/DNA_brush_types.h
194

Yes, before the Blender palettes were used only as tools, but after change GP to use Blender palettes instead of bGPDpalette, it was necessary to add animation options for colors and setttings.

Joshua Leung (aligorith) marked 7 inline comments as done.Oct 24 2017, 1:54 PM

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
  1. 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()
  1. updateDepsgraph() callbacks and other ID-link functions should be defined for each GP modifier that needs them
  1. Possible depsgraph opnodes that call BKE_gpencil_batch_cache_dirty() to trigger a refresh of the stroke data caches (and modifiers to get reevaluated)?
  1. 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.
  1. 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.

Joshua Leung (aligorith) updated this revision to Diff 9581.EditedNov 21 2017, 2:44 PM
Joshua Leung (aligorith) edited the summary of this revision. (Show Details)

Changes since last patch:

  • Refactored the code as per https://developer.blender.org/D2889#68153
    1. GP Objects store their data in ob->data instead of ob->gpd
    2. Refactored modifier code for GP modifiers by adding 3 new callbacks to ModifierTypeInfo (i.e. deformStrokes, generateStrokes, and bakeModifierGP).
    3. Use depsgraph-based relations/tagging in places of the bad BKE_gpencil_batch_cache_alldirty() calls. Most remaining calls to this don't do anything by default (they only run when debug_value = 66), and can eventually be removed.
    4. General code cleanup/refactoring
  • Removed "Duplication" modifier. Its functionality effectively overlapped with the "Array" modifier. I've ended up merging the two together, as there are benefits of both approaches (e.g. the Array modifier generates separate objects in the draw cache so that we don't get z-ordering bugs; these temporary objects however only exist temporarily when drawing, and are not added to the Main db).
  • Added (temporary) new settings to the GP brushes to facilitate easier experimentation with new settings to improve the drawing experience (i.e. smoother strokes, with better spacing between points).
  • A whole bunch of UI tweaks, bugfixes, small features, etc.

(Hopefully the patch isn't too messed up. Arc kept hanging/timing out, so I had to manually update by uploading a diff)

WIP update of the changes from the branch (as of ~5 minutes ago).

Some fixes are still needed to various things - so, no rush to review this update yet. I'm mainly just uploading this as I needed to make a diff to review some recent changes in a few areas anyway :)

Joshua Leung (aligorith) edited the summary of this revision. (Show Details)Feb 8 2018, 4:34 AM
Joshua Leung (aligorith) updated this revision to Diff 10204.EditedMar 19 2018, 10:37 AM

Update mega-patch to reflect latest GP Branch code. There are too many changes to list...

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.

Bastien Montagne (mont29) requested changes to this revision.Wed, Apr 18, 3:33 PM

Just did quick check on the library management/DNA side of things.

About pointers to sub-data of *other* datablocks: that kind of pointer is strictly forbidden in new code (we already have a few ones in existing one and they are a tremendous pain to maintain and work with). Whole library generic handling code assumes every datablock handles its own data as it likes, and there is no way to update other ID's pointers when they become invalid for some reason or another (like lib reloading, remapping, etc.).

source/blender/blenkernel/intern/library_query.c
469–1113

Whole handling of new ID pointers is missing here (palette pointer in GD strokes, etc.). Fixing this is mandatory, it's the core of the whole ID management system...

source/blender/makesdna/DNA_gpencil_types.h
195

Pointer to data belonging to another data-block, strictly forbidden and show-stopper, see main comment for details.

source/blender/makesdna/DNA_scene_types.h
987–995

Why the reordering here?

997

This is very weak? Means every type you add a new sculpt brush you shift all you weight ones?

Also, why put them in same enum, since they are used in two different modes... and even assigned to two different variables!

This revision now requires changes to proceed.Wed, Apr 18, 3:33 PM
source/blender/makesdna/DNA_scene_types.h
997

I think there are some misunderstanding here:

  1. The user cannot add new brushes or delete existing ones. These are predefined tools, not like paint or sculpt brushes. This is the original sculpt tools implementation since version 2.7x. The brushes are predefined in an array and depending of the index a different tools is called.
  1. The reason to reuse is because the weight paint is only a sculpt brush that reuse the same tool management and code loops. The reason to put at the end is only to manage the UI and keymaps.

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.