GPencil: Refactor grease pencil modifier to object modifiers (ob->greasepencil_modifiers -> ob->modifiers ) #101104

New Issue

Antonio Vazquez · 2022-09-15T23:04:12+02:00

Antonio Vazquez commented

2022-09-15 23:04:12 +02:00

This task is the main entry point for this refactor.

During the creation of Grease Pencil modifiers, the initial implementation put the modifiers in ob->modifiers but during the review process it was requested to move to ob->greasepencil_modifiers but now this makes impossible to use Geometry Nodes modifier without duplicating all.

The goal of this task is move back the GPencil modifiers, adding the corresponding Callbacks and tags to be handled in the ob->modifiers

New Callbacks

In order to manage grease pencil requiremenst we need add the following callbacks.

deformStroke
generateStrokes
bakeModifier
remapTime

Also, we need to review the read and write callbacks to check if we need to move some code to use them. I have noticed that some mesh modifiers don't use it and it's not clear when use or not.

New flag

We need to add a new flag to be able to filter the modifiers only for grease pencil. This is the same already done for modifiers not valid for curves.

Despgrah evaluation

Adapt current depsgraph code to evaluate grease pencil modifiers in actual modifiers loop.

Conversion of old files

TBD

Cleanup code

All code to handle grease pencil modifiers UI can be removed.
Fix modifiers panel and remove grease pencil case.

Python API

There will be an API change because grease_pencil_modifiers will not longer exist. Could we create an "alias" to keep running old scripts and remove the alias in 4.0?

This task is the main entry point for this refactor. During the creation of Grease Pencil modifiers, the initial implementation put the modifiers in `ob->modifiers` but during the review process it was requested to move to `ob->greasepencil_modifiers` but now this makes impossible to use `Geometry Nodes` modifier without duplicating all. The goal of this task is move back the GPencil modifiers, adding the corresponding Callbacks and tags to be handled in the `ob->modifiers` ### New Callbacks In order to manage grease pencil requiremenst we need add the following callbacks. * deformStroke * generateStrokes * bakeModifier * remapTime Also, we need to review the read and write callbacks to check if we need to move some code to use them. I have noticed that some mesh modifiers don't use it and it's not clear when use or not. ### New flag We need to add a new flag to be able to filter the modifiers only for grease pencil. This is the same already done for modifiers not valid for curves. ### Despgrah evaluation * Adapt current depsgraph code to evaluate grease pencil modifiers in actual modifiers loop. ### Conversion of old files TBD ### Cleanup code * All code to handle grease pencil modifiers UI can be removed. * Fix modifiers panel and remove grease pencil case. ### Python API * There will be an API change because `grease_pencil_modifiers` will not longer exist. Could we create an "alias" to keep running old scripts and remove the alias in 4.0?

Antonio Vazquez commented

2022-09-15 23:04:12 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Antonio Vazquez commented

2022-09-15 23:04:12 +02:00

Added subscribers: @antoniov, @pepe-school-land, @mendio, @HooglyBoogly, @filedescriptor, @amelief

Antonio Vazquez changed title from ~~GPencil: Backport grease pencil modifier to object modifiers (`ob->greasepencil_modifiers` -> `ob->modifiers` )~~ to GPencil: Refactor grease pencil modifier to object modifiers (`ob->greasepencil_modifiers` -> `ob->modifiers` )

2022-09-15 23:05:49 +02:00

hamza.SMA commented

2022-09-15 23:39:30 +02:00

Added subscriber: @hamza-el-barmaki

Antonio Vazquez commented

2022-09-16 15:45:16 +02:00

Added subscriber: @frogstomp-4

Antonio Vazquez commented

2022-09-22 12:06:20 +02:00

Added subscribers: @Sergey, @brecht

Antonio Vazquez commented

2022-09-22 12:06:20 +02:00

@brecht @Sergey What do you think about this proposal? Is it feasible or is it not worth spending time because it is something you don't want to do for design reasons?

If we go for that, I need to go over the details and maybe create a group of volunteers who can help with the task (I already have some people ready to help)

@brecht @Sergey What do you think about this proposal? Is it feasible or is it not worth spending time because it is something you don't want to do for design reasons? If we go for that, I need to go over the details and maybe create a group of volunteers who can help with the task (I already have some people ready to help)

Sergey Sharybin commented

2022-09-22 12:45:54 +02:00

but now this makes impossible to use Geometry Nodes modifier without duplicating all.

What "all" refers to in this sentence?

To me the proposal is lacking a lot of crucial details. For example:

Are there plans to unify the intermediate data structures and the result type somehow?
Are there plans to make geometry modifiers (like the deform ones) supported for grease pencil?

If we can somehow unify the data structures and implementation then it indeed could be beneficial to handle the stack from a single place.
If there is no overlap in the modifiers implementation and stack loop evaluation, then I do not see benefits of trying to re-use the same concepts for otherwise non-interchangeable/incompatible things.

> but now this makes impossible to use Geometry Nodes modifier without duplicating all. What "all" refers to in this sentence? To me the proposal is lacking a lot of crucial details. For example: - Are there plans to unify the intermediate data structures and the result type somehow? - Are there plans to make geometry modifiers (like the deform ones) supported for grease pencil? If we can somehow unify the data structures and implementation then it indeed could be beneficial to handle the stack from a single place. If there is no overlap in the modifiers implementation and stack loop evaluation, then I do not see benefits of trying to re-use the same concepts for otherwise non-interchangeable/incompatible things.

Brecht Van Lommel commented

2022-09-22 15:01:06 +02:00

The idea with geometry nodes in the stack is that the modifier input and output is a geometry set, which may contain any type of geometry. So mesh, curves, points, volumes, and now also grease pencil strokes?

Does that mean grease pencil objects would be able to output meshes, and perhaps even use mesh modifiers in the stack? And the same for mesh objects outputting grease pencil strokes? If the evaluation loop is unified as in the description then this seems possible, but the UI for that would need work (like the add modifier menu).

Adapt current depsgraph code to evaluate grease pencil modifiers in actual modifiers loop.

This is possible but the code is already very complicated and this would make it even more so. Some significant refactoring may be needed to get a decent implementation.

The idea with geometry nodes in the stack is that the modifier input and output is a geometry set, which may contain any type of geometry. So mesh, curves, points, volumes, and now also grease pencil strokes? Does that mean grease pencil objects would be able to output meshes, and perhaps even use mesh modifiers in the stack? And the same for mesh objects outputting grease pencil strokes? If the evaluation loop is unified as in the description then this seems possible, but the UI for that would need work (like the add modifier menu). > Adapt current depsgraph code to evaluate grease pencil modifiers in actual modifiers loop. This is possible but the code is already very complicated and this would make it even more so. Some significant refactoring may be needed to get a decent implementation.

Antonio Vazquez commented

2022-09-22 16:14:45 +02:00

This refactor of modifiers is part of bigger project. (https://hackmd.io/zxHLBMA9RcGnhfb0ZGFbHg)

Actually, we have grease pencil modifiers separated, so to use geometry modifier we would need to duplicate it to be stored in greasepencil_modifiers list. IIRC @HooglyBoogly explained to me that GN can handle different type of information, so we could create GP nodes, but the final goal would be to store the GP data in arrays similar to the actual curves. This task is NOT about adding anything for GPencil Geometry Nodes, just prepare the code for the future.

The point of this task is start this refactor and prepare the modifiers for the future data implementation, but it's not part of this task to refactor the internal structure of modifiers, just make them work in the main modifiers loop adding the corresponding calls to the added callbacks.

Sure @filedescriptor and @HooglyBoogly can explain better the project for the data conversion.

This refactor of modifiers is part of bigger project. (https://hackmd.io/zxHLBMA9RcGnhfb0ZGFbHg) Actually, we have grease pencil modifiers separated, so to use geometry modifier we would need to duplicate it to be stored in `greasepencil_modifiers` list. IIRC @HooglyBoogly explained to me that GN can handle different type of information, so we could create GP nodes, but the final goal would be to store the GP data in arrays similar to the actual curves. This task is NOT about adding anything for GPencil Geometry Nodes, just prepare the code for the future. The point of this task is start this refactor and prepare the modifiers for the future data implementation, but it's not part of this task to refactor the internal structure of modifiers, just make them work in the main modifiers loop adding the corresponding calls to the added callbacks. Sure @filedescriptor and @HooglyBoogly can explain better the project for the data conversion.

Brecht Van Lommel commented

2022-09-22 17:49:20 +02:00

For geometry nodes code reuse it will be good to share CurvesGeometry with curves. However the design of data structures to represent grease pencil geometry seems independent of the design questions raised by this task.

The goal is to refactor grease pencil modifier for geometry nodes support, but how to do the refactor depends on how this is going to work for end users, which is unclear to me still.

If the end goal is to be able to mix mesh and grease pencil modifiers in a single stack, then you will need a single modifier stack evaluation loop. But if you look at the implementation of mesh_calc_modifiers and editbmesh_calc_modifiers, you'll see it's already complicated and fitting in grease pencil there without any refactoring is going to be very messy. I think it can be refactored to make things fit well, but it would be a lot of work.
If the plan is just to add geometry nodes to grease pencil objects without any other modifiers, it seems best to keep the evaluation loop separate even if you share the ob->modifiers data structures. Point cloud and volume objects for example also have their own modifier evaluation loop while supporting geometry nodes, and doing the same for grease pencil seems reasonable.

Regarding the callbacks, as an intermediate step it may be ok to keep them as is, mesh modifiers also have specific callbacks not used by other types. But some refactoring to reduce them and/or make them more consistent with other types may simplify the implementation.

For geometry nodes code reuse it will be good to share `CurvesGeometry` with curves. However the design of data structures to represent grease pencil geometry seems independent of the design questions raised by this task. The goal is to refactor grease pencil modifier for geometry nodes support, but how to do the refactor depends on how this is going to work for end users, which is unclear to me still. * If the end goal is to be able to mix mesh and grease pencil modifiers in a single stack, then you will need a single modifier stack evaluation loop. But if you look at the implementation of `mesh_calc_modifiers` and `editbmesh_calc_modifiers`, you'll see it's already complicated and fitting in grease pencil there without any refactoring is going to be very messy. I think it can be refactored to make things fit well, but it would be a lot of work. * If the plan is just to add geometry nodes to grease pencil objects without any other modifiers, it seems best to keep the evaluation loop separate even if you share the `ob->modifiers` data structures. Point cloud and volume objects for example also have their own modifier evaluation loop while supporting geometry nodes, and doing the same for grease pencil seems reasonable. Regarding the callbacks, as an intermediate step it may be ok to keep them as is, mesh modifiers also have specific callbacks not used by other types. But some refactoring to reduce them and/or make them more consistent with other types may simplify the implementation.

Hans Goudey commented

2022-09-22 18:17:11 +02:00

I agree that the design of grease pencil data structures is independent from this task. Though I will say that significant changes will be necessary for proper geometry nodes support.

I think the task is generally a good direction to go in. Architecturally, a duplicate grease pencil modifier stack code doesn't hold up-- object_gpencil_modifier.c vs. object_modifier.cc, etc.
Adding "gpencil" to a concept and duplicating it should be avoided. Because this was done in the past with modifiers like "Array", reusing existing mesh modifiers doesn't seem like a useful goal.

Instead, I think the only new modifier we should aim to support is geometry nodes, just using the modifyGeometrySet callback. The evaluation look should stay separate.
This makes this task much simpler. Because modifier evaluation loops shouldn't need to change much at all.

Backwards compatibility is necessary, and relatively easy to handle with versioning.
However, forward compatibility and keeping the Python API would be much more difficult. If we could avoid those two things, that would be preferable IMO.

I agree that the design of grease pencil data structures is independent from this task. Though I will say that *significant* changes will be necessary for proper geometry nodes support. I think the task is generally a good direction to go in. Architecturally, a duplicate grease pencil modifier stack code doesn't hold up-- `object_gpencil_modifier.c` vs. `object_modifier.cc`, etc. Adding "gpencil" to a concept and duplicating it should be avoided. Because this was done in the past with modifiers like "Array", reusing existing mesh modifiers doesn't seem like a useful goal. Instead, I think the only new modifier we should aim to support is geometry nodes, just using the `modifyGeometrySet` callback. The evaluation look should stay separate. This makes this task much simpler. Because modifier evaluation loops shouldn't need to change much at all. Backwards compatibility is necessary, and relatively easy to handle with versioning. However, forward compatibility and keeping the Python API would be much more difficult. If we could avoid those two things, that would be preferable IMO.

Sergey Sharybin commented

2022-09-23 10:16:31 +02:00

I am still unsure what is the problem this task is addressing. The evaluation loops are proposed to be separate, no modifier implementation entry points are shared between mesh/curve and grease pencil. What are we gaining from re-using the same list for modifiers then?

Aiming to move from modifiers stack to geometry nodes sounds like a good plan. But what is the stopper for adding geometry modifier to the grease pencil modifiers? Is it the amount of code in the MOD_nodes.cc? Can this code be moved out of the file and made more re-usable?

A good design consists of:

Statement of a problem it is aiming to solve, or goal which is being achieved
Top-level overview of the proposed design change
Its cons and pros
Technical description of the design (in the terms of code changes, data structures, algorithms)
Other considered solutions to the problem, and why they were discarded

While one might say that it is extra work to write design in this way, in reality it actually does save time and makes design reviews more inviting. The same or even greater amount of time would otherwise need to be spent by a reviwer to ask those questions, or to do investigation on their own.

I am still unsure what is the problem this task is addressing. The evaluation loops are proposed to be separate, no modifier implementation entry points are shared between mesh/curve and grease pencil. What are we gaining from re-using the same list for modifiers then? Aiming to move from modifiers stack to geometry nodes sounds like a good plan. But what is the stopper for adding geometry modifier to the grease pencil modifiers? Is it the amount of code in the `MOD_nodes.cc`? Can this code be moved out of the file and made more re-usable? --- A good design consists of: * Statement of a problem it is aiming to solve, or goal which is being achieved * Top-level overview of the proposed design change * Its cons and pros * Technical description of the design (in the terms of code changes, data structures, algorithms) * Other considered solutions to the problem, and why they were discarded While one might say that it is extra work to write design in this way, in reality it actually does save time and makes design reviews more inviting. The same or even greater amount of time would otherwise need to be spent by a reviwer to ask those questions, or to do investigation on their own.

Antonio Vazquez commented

2022-09-24 11:05:09 +02:00

As this task depends on whether you decide to make geometry nodes for grease pencil and on the refactor of the internal data structure, we put it on hold and we will come back to it in the future when these tasks are more advanced.

It doesn't make much sense to spend time designing new grease pencil modifiers structure when we don't yet know what the rest of the modules it depends on will look like.

As this task depends on whether you decide to make geometry nodes for grease pencil and on the refactor of the internal data structure, we put it on hold and we will come back to it in the future when these tasks are more advanced. It doesn't make much sense to spend time designing new grease pencil modifiers structure when we don't yet know what the rest of the modules it depends on will look like.

Antonio Vazquez commented

2022-09-26 10:47:06 +02:00

Changed status from 'Confirmed' to: 'Archived'

Antonio Vazquez closed this issue

2022-09-26 10:47:06 +02:00

Sergey Sharybin commented

2022-09-26 19:21:10 +02:00

As I've mentioned before, consolidating modifiers and making more things go via the geometry nodes is a very nice goal. Avoiding code duplication is another good goal.
There are many ways to achieve the both, with the different strong and weak points. And this is part of a design task to make it clear why the achieved goal is outweights the downsides of the change.

I've talked to Brecht and he showed practical benefits to merging the grease pencil modifiers with mesh modifiers.

@HooglyBoogly Am I understanding you correctly that in the long term we want all modifiers to implement a single modifyGeometrySet?

As I've mentioned before, consolidating modifiers and making more things go via the geometry nodes is a very nice goal. Avoiding code duplication is another good goal. There are many ways to achieve the both, with the different strong and weak points. And this is part of a design task to make it clear why the achieved goal is outweights the downsides of the change. I've talked to Brecht and he showed practical benefits to merging the grease pencil modifiers with mesh modifiers. @HooglyBoogly Am I understanding you correctly that in the long term we want all modifiers to implement a single `modifyGeometrySet`?

Hans Goudey commented

2022-09-26 19:33:30 +02:00

In #101104#1422976, @Sergey wrote:
@HooglyBoogly Am I understanding you correctly that in the long term we want all modifiers to implement a single modifyGeometrySet?

Yes, that would be great. But ideally (and maybe a bit more radically), there would only be a single modifier type: geometry nodes. All other modifiers would be implemented as nodes.
In that case, the whole idea of modifier callbacks and implementations would become much less important. This has other benefits too-- implementing a node is easier than implementing a modifier since they don't have as much context and work more generically.
The old modifiers could either be implemented as node groups shipped with Blender (so they appear in search and elsewhere) or they could be implemented somewhere just for the sake of backwards compatibility.

We wrote more about this in #86839 a while ago.

> In #101104#1422976, @Sergey wrote: > @HooglyBoogly Am I understanding you correctly that in the long term we want all modifiers to implement a single `modifyGeometrySet`? Yes, that would be great. But *ideally* (and maybe a bit more radically), there would only be a single modifier type: geometry nodes. All other modifiers would be implemented as nodes. In that case, the whole idea of modifier callbacks and implementations would become much less important. This has other benefits too-- implementing a node is easier than implementing a modifier since they don't have as much context and work more generically. The old modifiers could either be implemented as node groups shipped with Blender (so they appear in search and elsewhere) or they could be implemented somewhere just for the sake of backwards compatibility. We wrote more about this in #86839 a while ago.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPencil: Refactor grease pencil modifier to object modifiers (ob->greasepencil_modifiers -> ob->modifiers ) #101104

New Callbacks

New flag

Despgrah evaluation

Conversion of old files

Cleanup code

Python API

GPencil: Refactor grease pencil modifier to object modifiers (`ob->greasepencil_modifiers` -> `ob->modifiers` ) #101104