@Joshua Leung (aligorith) As I have done a lot of changes requested, could you update the patch and ask reviewers to mark as done the tasks completed?

@Sergey Sharybin (sergey) Agree with some points, but strongly disagree with many of the rest (particularly some of the larger high-level concerns raised).

Regarding the whole issue of support for non-3d editor views:
These are explicitly non-supported right now, with the understanding that they will be restored post-merge (but before the first 2.8 releases). If we adequately communicate this limitation in the PR/marketing, it should not be an issue (regarding bugreports, etc.); Also, it helps to note that we're not actually making things too much worse than the current 2.8 state here, as actually, Grease Pencil/Annotations *do not* work in 2.8 now.

With the broader interests of the Blender project in mind, it was decided in multiple meetings the past few months that support for "annotations" (i.e. the new name for non-3d/art Grease Pencil functionality - or in other words, basic freehand annotation functionality, not the 2d/3d art tool) would be worked on following the merge of the branch into 2.8. Specifically, the branch already has enough non-trivial changes introduced in it as-is now, with the merge + review process dragging on for far too long already, that requiring such support to be in place before a merge can take place is IMO draconian and needlessly nitpicky (especially when WIP toolbar/topbar/etc. breaking changes are constantly happening in 2.8 daily anyway). The changes to support "annotations" however will also be non-trivial in some cases, and would only serve to further complicate and muddy the review in many cases, further delaying the whole process.

So no, it is *not* a blocking issue that this support is not in place now.

BTW, there is a Phabricator task for this already - T55455 - so it is a known issue already.

Regarding modifiers:
Hmm... just to be clear, when referring to "geometry modifiers", are we talking about the ones that operate on Mesh/Curve/etc.?

Regarding the point about GP modifiers not using the existing infrastructure and basically doing their own loops - Yes, this was an issue that I did attempt to rectify during code quest, but concluded would require too much work at the present time. Ideally though, it could still be done - though there are tricky issues here presented by the Onion Skinning functionality (for which no equivalents exist in 2.8 current), the draw buffers used when artists are drawing new strokes, and the modifiers that create new geometry (e.g. "instancing" modifier, which needs the draw-engine hack to produce correct behaviour, due to technical limitations with z-ordering or similar - check with Clement if you have any queries about this).

Regarding the notion of just putting this in a separate list, with separate functions to manage all of it, etc. - IMO that is a nonsense idea:

1. Think about the implications for the user experience: Conceptually, from a user-model POV, we're still dealing with "modifier-like" things here. Currently, users can reuse their existing mental models to know that they can just do "object.modifiers["GP Modifier"]" to access the relevant modifier settings from the API. However, where would they go if we went with this separate list? What would it be called? What would we call these things?
2. IMO, if the branch had done it this way in the first place, I'm sure that there would be complaints flying the opposite way about "code duplication" and not reusing existing infrastructure to handle what is essentially similar functionality to existing Blender features.
3. What exactly is wrong with just having separate callbacks for handling GP data in the modifiers? We have separate callbacks for different types of mesh, and control points vs mesh verts, why not other types of data?

Regarding modifier baking:
Yes, you make a good point regarding the baking not really being possible for just isolated modifiers. That said, we'd also have similar problems with normal modifiers too in many ways, yet those go fine.

I don't however really get what you meant by your comment that baking shouldn't be part of the modifier, but then, you go on to say that it should be done in a way that the modifier handles that itself. That seems a bit contradictory when these are callbacks that the modifier provides to plug in to various parts of Blender, and not necessarily a part of evaluating the modifier itself? Are you referring to the cross-time baking stuff that these are doing (i.e. so perhaps, the time stepping should be abstracted out of the individual modifier bake callbacks, and done in the operator instead), or something else?

Regarding deprecation of scene->gpd:
An issue that did arise at one point was that some of the uses of Grease Pencil in the 3D viewport in 2.7 were for "annotation" and not "3d art". The problem was that for many of those, we don't necessarily want to automatically convert annotations to GP objects that then go on to show up in renders. (Plus, there's the added problem of how to add objects to collections/view layers/etc., when no context is available - i.e. from versioning code - something that isn't supported AFAIK right now). The current solution was to require artists to manually run an operator to convert 3D art to GP objects where needed, as a way around these issues.

I propose just un-marking scene->gpd as DNA_DEPRECATED for now (solving the warnings/compile errors people are seeing), and addressing this as part of the post-merge annotations work once we have a design for it.

Regarding inline comments:
From the looks of things, most are minor style/formatting tweaks. Some are leftovers from previous changes that have now been reverted/changed (i.e. the struct Palette ones), and should be able to be resolved quickly. So, overall, relatively minor things, and again - small enough to do either in the branch pre-merge, or post-merge without too much trouble. So, again, I lean towards saying that these are "non-blocking" things that, yes, we can/should try to clean up as much as we can when we have time, but otherwise, we should just accept that some things may not be perfect, but with the explicit understanding that there needs to be a focus on cleaning these things up while stabilising 2.8 (as opposed to only working on new features). Again, this was one of the conclusions of prior meetings we've had on this whole merge process.

Replies to @Joshua Leung (aligorith)

Regarding the whole issue of support for non-3d editor views

These are explicitly non-supported right now

There is a difference between "non-supported" and "crash every single time you try to use it".

If we adequately communicate this limitation in the PR/marketing, it should not be an issue (regarding bugreports, etc.);

This is not a limitation, this is removing whole feature for no obvious reason. I do not see how you can justify removal of a feature which allowed us to track lots of footage.

What is even worse here, is that you don't display any strokes here, but they are still used by the tracker algorithm itself.

Also, it helps to note that we're not actually making things too much worse than the current 2.8 state here, as actually, Grease Pencil/Annotations *do not* work in 2.8 now.

Not sure how that is true. Open Motion Tracking workspace, open some clip and do some strokes. Even strokes assigned to the Track are visible (granted, they are not displayed with a proper thickness, but that's sholuldn't be hard to address).

Same applies to image editor. And nodes editor. And even sequencer.

The changes to support "annotations" however will also be non-trivial in some cases, and would only serve to further complicate and muddy the review in many cases, further delaying the whole process.

I don't see why it is so hard to keep system which is working untouched with the grease pencil object.

So no, it is *not* a blocking issue that this support is not in place now.

Did you become the one to decide which issues are blocking and which are not for motion tracking?

Regarding modifiers:

Hmm... just to be clear, when referring to "geometry modifiers", are we talking about the ones that operate on Mesh/Curve/etc.?

Yes.

Regarding the point about GP modifiers not using the existing infrastructure and basically doing their own loops

This is not an issue of using own loop. The only acceptable solution to share same loop would be if callbacks signature is the same for all object types. If the signature is different, then it's indeed easier to use own loop for grease pencil. But then grease pencil should stay away from existing modifier type info. There is no reason for grease pencil to use it if it does all own things.

Bottom line: non-related things shouldn't be living together. If you don't take the word here, look at hair/particle code, which became unmaintainable just because all the exceptions on what is valid and when.

Think about the implications for the user experience: Conceptually, from a user-model POV, we're still dealing with "modifier-like" things here.

Use aspect can stay exactly unchanged. There are three different things:

1. User interface
2. Python API
3. Underlying code

First two can be kept as-is, without much worry about how exactly modifier stack is implemented. The latter one is important to have clearly separated.

IMO, if the branch had done it this way in the first place, I'm sure that there would be complaints flying the opposite way about "code duplication" and not reusing existing infrastructure to handle what is essentially similar functionality to existing Blender features.

There is code duplication already. Lattice modifier is re-implemented specifically for grease pencil, for example. If same Lattice modifier was used for both mesh and grease pencil, it wasn't be a problem. But here you're trying to justify this decision by "avoid code duplication", while in practice you do duplicate the code. With the exception that you duplicate it within a single system.

What exactly is wrong with just having separate callbacks for handling GP data in the modifiers?

Nothing. They just shouldn't be living with same system.

We have separate callbacks for different types of mesh, and control points vs mesh verts, why not other types of data?

We do not separate mesh verticies from control points. Both pf them are handled via float3 array passed to deform-verticies. Also, all those callbacks are used from within single modifier stack loop.

But in case of grease pencil, you introduce flags and callbacks which are only used by grease pencil's modifier stack. And greasepencil modifier stack doesn't use geoemtry's modifier stack callbacks. So why to have them defined in one place? Why would one be worrying about grease pencil when porting geoemtry modifier stack t oa node system?

Regarding modifier baking:

Yes, you make a good point regarding the baking not really being possible for just isolated modifiers. That said, we'd also have similar problems with normal modifiers too in many ways, yet those go fine.

":Normal" modifiers do not perform full scene update. They do have "binding" process. But that binding is more like "save some extra data while calculating this modifier".

Things don't go fine here, btw. There are big issues with this.

I don't however really get what you meant by your comment that baking shouldn't be part of the modifier, but then, you go on to say that it should be done in a way that the modifier handles that itself.

"Modifier handles itself". No "that". This means that modifier is a black box: [input] => [MODIFIER] => [output]. Modifier does what it is asked to do, it doesn't tell scene what to do.

Regarding deprecation of scene->gpd

I propose just un-marking scene->gpd as DNA_DEPRECATED for now (solving the warnings/compile errors people are seeing), and addressing this as part of the post-merge annotations work once we have a design for it.

You can't just mark on unmark something as DEPRECATED. It is not about warnings, it is about design.

Regarding inline comments:

Some are leftovers from previous changes that have now been reverted/changed (i.e. the struct Palette ones), and should be able to be resolved quickly. So, overall, relatively minor things, and again - small enough to do either in the branch pre-merge, or post-merge without too much trouble

Those things are weay easier to cleanup before the merge. Simply because it's way easier to see no-functional-changes/cleanup/residue-of-older-code as a diff between branches.

My 2 cents on all this:

The annotation system is currently working in 2.80 but with some bugs

• In image editor the drawing is not responding to the screen zoom.
• The Grease Pencil Colors panel is invisible everywhere (poll is deliberately False for this and other GP panels).
• In 3D viewport you can draw but the drawing itself is not visible.

Although it is tempting to ditch the annotation system until after the merge it is really to do it given that there is no plans to how to integrate them back.
Also, it seems it wouldn't take a lot of work to bring them back to the 2.7x state in blender2.8 so it seems like a big regression to remove them, even if temporarily.

Either way the current state of UI with FIXME: Placeholder for deprecated functionality as well as the crashes is unacceptable. If we are to remove it do it properly, hiding it entirely from the user.
But before doing that we need a plan to how to bring them back, otherwise is a showstopper.

The modifier stack has either to be separated or properly merged

We have the following options moving forward:

• Merge the modifier evaluation stack so it takes the same data struct/callbacks for all data types (Mesh, Curve, GP).
• Separate the grease pencil from the regular modifier stack.

We didn't expect the former to be doable in a short time frame, that's why we settled to whatever the person willing to implement it decided to do after the merge.
But since Joshua expressed unavailability to commit to that, we need to re-evaluate the plan.

@Joshua Leung (aligorith) can you confirm your intentions regarding your involvement with the grease pencil project? Maybe I got it wrong and you are still interested to tackle it?

In other words, to separate the grease pencil from the modifier stack seems to be the best short term solution as well as a showstopper.
For the users nothing changes, UI, RNA (e.g., ob.modifiers) should all work (i.e., rna refine).

Plan of action

My suggestion for annotation system:

1. Decide/agree on final design for annotation system - simplified functionality, no editing, ...
2. Get it decoupled from the GP object in the GP branch, so we fix it in 2.8 fully, before the merge.
3. Profit - With this separation, grease-pencil object branch won't be breaking anything, it will only be adding more things to Blender.

Optionally the annotation can be implemented directly in the grease-pencil object branch. In fact the details of why they are missing there escapes me.
That said, that will make the code to more complex for the merge.

My suggestion for 2d animation system:

1. Decide to which extent we separate the modifier stacks (use the same DNA storage for both? BKE_object_support_modifier_type_check()? Add/Copy/Apply).
2. Separate it.
3. Merge.

Imho, The annotation system must be fixed in greasepencil-branch because now there are areas of the code with hacks and unnedeed complexity to support both systems. I f we separate, we can create a "clean" and simple annotation operator and remove complexity of the current gp object paint.

Besides, the current 2.8 system is using Palettes and this is something we want remove totally of annotations. Also we have the current shared uI panels full of "if annotation then"...we need separate panel for annotations and this implify a lot both systems.

Here's a proposal doc for how the annotations could work:
https://docs.google.com/document/d/1HvogywJQPMHQPKp1oUMGotdIGY-7gM2TBhfcFZPkOZ8/edit?usp=sharing

@Antonio Vazquez (antoniov) I looked at the splitting task (even have a wip branch) and I think you are overrating its complexity. I can lend you a hand if you want, do parts you may find hard, you name it. But really, it is mostly monkey work for 90% of it.
It is a mix of copying files and stripping the unneeded parts, reverting the changes in files that are modifier specific, and duplicating code for utils (move up, down, copy, new, del).

With the help of @Dalai Felinto (dfelinto), I have splitted the grease pencil modifiers in a separated stack. Now all is totally separated of mesh modifiers.

Still some menor cleanup pending. As soon I have the code ready, I will prepare a patch for review before merge it in grease pencil branch.

Patch for splitting modifiers submited for review: D3488

@Dalai Felinto (dfelinto) I think we need add the evaluation of grease pencil modifiers in void DepsgraphRelationBuilder::build_object_data_geometry(Object *object) function

What do you think?

Patch D3493 created to include grease pencil modifiers in depsgraph