GPv3: Apply Modifier #124543

Merged
Falk David merged 28 commits from filedescriptor/blender:gpv3-apply-modifier into main 2024-08-13 15:02:40 +02:00
Member

This implements the Apply Modifier operator for GPv3.

Applies the modifier to the current frame. Drawings at the current frame will be updated.
If the result of the modifier creates new layers, the layers will be added and a drawing
at the current frame will be inserted.

Applying a modifier will run the following steps:

  • Create a localized copy of the original Grease Pencil data and insert it into a geometry set that owns it.
  • Execute the modifiy_geometry_set function of the modifier with this geometry set.
  • In the resulting Grease Pencil data loop over all layers:
    • If the original data has a layer with the same name, overwrite its drawing data in the current frame.
    • Otherwise create a new layer in the original data and insert a resulting drawing at the current frame.
  • Remove all original layers that are not mapped to from a result layer.
  • Remap the material indices for all drawings that have not been updated (e.g. the ones that are not on the current frame).
  • Copy all the layer attributes to the original Grease Pencil.
This implements the `Apply Modifier` operator for GPv3. Applies the modifier to the current frame. Drawings at the current frame will be updated. If the result of the modifier creates new layers, the layers will be added and a drawing at the current frame will be inserted. Applying a modifier will run the following steps: * Create a localized copy of the original Grease Pencil data and insert it into a geometry set that owns it. * Execute the `modifiy_geometry_set` function of the modifier with this geometry set. * In the resulting Grease Pencil data loop over all layers: * If the original data has a layer with the same name, overwrite its drawing data in the current frame. * Otherwise create a new layer in the original data and insert a resulting drawing at the current frame. * Remove all original layers that are not mapped to from a result layer. * Remap the material indices for all drawings that have not been updated (e.g. the ones that are not on the current frame). * Copy all the layer attributes to the original Grease Pencil.
Falk David added 1 commit 2024-07-11 18:11:38 +02:00
Falk David added this to the Grease Pencil project 2024-07-11 18:11:44 +02:00
Falk David added 3 commits 2024-07-11 19:06:36 +02:00
Falk David added 1 commit 2024-07-12 14:54:50 +02:00
Falk David force-pushed gpv3-apply-modifier from 74bcc48345 to d17c36c547 2024-07-22 17:00:08 +02:00 Compare
Falk David added 4 commits 2024-08-09 13:19:12 +02:00
Falk David added 2 commits 2024-08-09 15:40:39 +02:00
Falk David added 1 commit 2024-08-09 15:41:15 +02:00
Merge branch 'main' into gpv3-apply-modifier
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
dd88e78af0
Falk David changed title from WIP: GPv3: Apply Modifier to GPv3: Apply Modifier 2024-08-09 15:50:04 +02:00
Falk David requested review from Jacques Lucke 2024-08-09 15:50:33 +02:00
Falk David requested review from Hans Goudey 2024-08-09 15:50:37 +02:00
Author
Member

@blender-bot build

@blender-bot build
Iliya Katushenock reviewed 2024-08-09 16:03:33 +02:00
@ -98,1 +98,4 @@
template<typename T, typename IndexT>
inline void scatter(const VArray<T> &src,
const Span<IndexT> indices,

I do not think indices should be templated.

I do not think indices should be templated.
Author
Member

Maybe @HooglyBoogly can comment on why he added this in 6935cd0798.
I can remove the template but maybe there is a reason I don't know about.

Maybe @HooglyBoogly can comment on why he added this in 6935cd0798cb545c2bb973f325550f09cfb3885f. I can remove the template but maybe there is a reason I don't know about.
filedescriptor marked this conversation as resolved
@ -1148,0 +1180,4 @@
grease_pencil_result.attributes_for_write().remove_anonymous();
/* Get the original material pointers from the result geometry. */
VectorSet<Material *> all_result_materials;

original_materials

`original_materials`
filedescriptor marked this conversation as resolved
@ -1148,0 +1184,4 @@
for (Material *eval_material :
Span{grease_pencil_result.material_array, grease_pencil_result.material_array_num})
{
BLI_assert(eval_material->id.orig_id != nullptr);

Do we still able to have nullptr materials in empty slots?

Do we still able to have nullptr materials in empty slots?
filedescriptor marked this conversation as resolved
@ -1148,0 +1192,4 @@
for (const int mat_i : IndexRange(grease_pencil_orig.material_array_num)) {
Material *material = grease_pencil_orig.material_array[mat_i];
const int map_index = all_result_materials.index_of_try(material);
BLI_assert(map_index != -1);

Does this imply that you can not add new materials to the GP in geometry nodes?

Does this imply that you can not add new materials to the GP in geometry nodes?
Author
Member

It's the opposite I think: We expect the evaluated geometry to have the materials of the original geometry (maybe more).

It's the opposite I think: We expect the evaluated geometry to have the materials of the original geometry (maybe more).

I mean, materials sets of the original object and of geometry which is result of the geometry nodes can completely do not intersect at all\

I mean, materials sets of the original object and of geometry which is result of the geometry nodes can completely do not intersect at all\
Author
Member

I tested this and it seems to work without hitting the assert.

I tested this and it seems to work without hitting the assert.
filedescriptor marked this conversation as resolved
@ -1148,0 +1205,4 @@
Drawing *drawing_eval = grease_pencil_result.get_drawing_at(*layer_eval, eval_frame);
if (drawing_eval) {
/* Anonymous attributes shouldn't be available on original geometry. */
drawing_eval->strokes_for_write().attributes_for_write().remove_anonymous();

Think you should split this loop body in separate steps\

Think you should split this loop body in separate steps\
filedescriptor marked this conversation as resolved
Falk David added 1 commit 2024-08-09 16:29:20 +02:00
Hans Goudey requested changes 2024-08-09 16:57:03 +02:00
Dismissed
Hans Goudey left a comment
Member

Great to have applying modifiers working! I don't really have an opinion on the design-level, anything that generally works is a great start.

Great to have applying modifiers working! I don't really have an opinion on the design-level, anything that generally works is a great start.
@ -1148,0 +1152,4 @@
if (mti->modify_geometry_set == nullptr) {
return false;
}
GreasePencil &grease_pencil_orig = *static_cast<GreasePencil *>(ob->data);
Member

This should happen for all these if statement cases later, but I think this would be easier to read if it was split to a separate function

This should happen for all these if statement cases later, but I think this would be easier to read if it was split to a separate function
filedescriptor marked this conversation as resolved
@ -1148,0 +1263,4 @@
bke::MutableAttributeAccessor attributes =
drawing.strokes_for_write().attributes_for_write();
bke::SpanAttributeWriter<int> material_indices =
attributes.lookup_or_add_for_write_span<int>("material_index", bke::AttrDomain::Curve);
Member

There should be some case where we don't have to add the material index attribute, probably worth implementing that?

There should be some case where we don't have to add the material index attribute, probably worth implementing that?
Author
Member

I'm not sure what that case could be

I'm not sure what that case could be
Member

I'm thinking of when there are no materials, or when the attribute didn't exist on the original or evaluated geometry

I'm thinking of when there are no materials, or when the attribute didn't exist on the original or evaluated geometry
filedescriptor marked this conversation as resolved
@ -1148,0 +1273,4 @@
}
/* Copy layer attributes from the result to the original. */
bke::scatter_attributes(grease_pencil_result.attributes(),
Member

We discussed just using a attributes.for_all loop rather than adding a new API function in chat. Just writing it here for history's sake.

We discussed just using a `attributes.for_all` loop rather than adding a new API function in chat. Just writing it here for history's sake.
filedescriptor marked this conversation as resolved
Falk David added 2 commits 2024-08-09 17:43:59 +02:00
Hans Goudey requested changes 2024-08-09 17:57:02 +02:00
Dismissed
@ -988,0 +1129,4 @@
if (meta_data.data_type == CD_PROP_STRING) {
return true;
}
const GAttributeReader src = src_attributes.lookup(id, AttrDomain::Layer);
Member

const GVArraySpan src = *src_attributes.lookup(id, AttrDomain::Layer);

Then you can remove the new scatter array utils function

`const GVArraySpan src = *src_attributes.lookup(id, AttrDomain::Layer);` Then you can remove the new scatter array utils function
filedescriptor marked this conversation as resolved
@ -988,0 +1139,4 @@
using T = decltype(dummy);
array_utils::scatter(
src.varray.typed<T>(), eval_to_orig_layer_indices_map.as_span(), dst.span.typed<T>());
src.varray.type().default_construct_indices(dst.span.data(), unmapped_original_layers);
Member
   * Call the default constructor at the given memory location.
   * The memory should be uninitialized before this method is called.
   * For some trivial types (like int), this method does nothing.

This should use type.fill_construct_indices(type.default_value(), dst.span.data(), mask);

``` * Call the default constructor at the given memory location. * The memory should be uninitialized before this method is called. * For some trivial types (like int), this method does nothing. ``` This should use `type.fill_construct_indices(type.default_value(), dst.span.data(), mask);`
filedescriptor marked this conversation as resolved
Falk David requested review from Hans Goudey 2024-08-09 18:09:37 +02:00
Falk David added 3 commits 2024-08-09 18:09:43 +02:00
Avoid creating material index attribute
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
c2076c2071
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2024-08-09 18:15:32 +02:00
Falk David added 1 commit 2024-08-09 18:54:49 +02:00
Jacques Lucke requested changes 2024-08-12 13:30:58 +02:00
Dismissed
@ -988,0 +1089,4 @@
for (Material *eval_material :
Span{grease_pencil_result.material_array, grease_pencil_result.material_array_num})
{
BLI_assert(eval_material->id.orig_id != nullptr);
Member

Material slots can be null.

Material slots can be null.
Author
Member

Ok then I'll just turn this into an if.

Ok then I'll just turn this into an `if`.

I think we mean eval_material can be null

I think we mean `eval_material` can be null
filedescriptor marked this conversation as resolved
@ -988,0 +1098,4 @@
for (const int mat_i : IndexRange(grease_pencil_orig.material_array_num)) {
Material *material = grease_pencil_orig.material_array[mat_i];
const int map_index = original_materials.index_of_try(material);
BLI_assert(map_index != -1);
Member

It's possible that the generated grease pencil contains other materials.

It's possible that the generated grease pencil contains other materials.
Author
Member

Ok then I'll do the same as above: turn it into an if.

Ok then I'll do the same as above: turn it into an `if`.

If you have multiple sets of mateirals you should merge them\

If you have multiple sets of mateirals you should merge them\
filedescriptor marked this conversation as resolved
Falk David added 2 commits 2024-08-12 14:15:16 +02:00
Falk David requested review from Jacques Lucke 2024-08-12 14:15:37 +02:00
Jacques Lucke requested changes 2024-08-12 16:26:21 +02:00
Dismissed
Jacques Lucke left a comment
Member

When I apply the modifier in the attached file, I somehow still see Suzanne in the viewport.

When I apply the modifier in the attached file, I somehow still see Suzanne in the viewport.
@ -988,0 +1027,4 @@
for (const int layer_eval_i : grease_pencil_result.layers().index_range()) {
const Layer *layer_eval = grease_pencil_result.layer(layer_eval_i);
/* Check if the original geometry has a layer with the same name. */
TreeNode *node_orig = grease_pencil_orig.find_node_by_name(layer_eval->name());
Member

Looks like this may be incorrect if there is a group with the same name already.

Looks like this may be incorrect if there is a group with the same name already.
filedescriptor marked this conversation as resolved
@ -988,0 +1060,4 @@
drawing_eval->strokes_for_write().attributes_for_write().remove_anonymous();
}
Drawing *drawing_orig = grease_pencil_orig.get_drawing_at(*layer_orig, eval_frame);
if (drawing_orig && drawing_eval) {
Member

What happens when drawing_orig is null? Is the drawing just ignored?

What happens when `drawing_orig` is null? Is the drawing just ignored?
Author
Member

Yes. In that case the layer doesn't have a visible drawing at the current frame, so there's nothing to do.

Yes. In that case the layer doesn't have a visible drawing at the current frame, so there's nothing to do.
@ -988,0 +1089,4 @@
for (Material *eval_material :
Span{grease_pencil_result.material_array, grease_pencil_result.material_array_num})
{
if (eval_material->id.orig_id != nullptr) {
Member

eval_material may be null.

`eval_material` may be null.
filedescriptor marked this conversation as resolved
@ -988,0 +1095,4 @@
}
/* Build material indices mapping. */
Array<int> material_indices_map(grease_pencil_orig.material_array_num);
Member

Would be good to mention what is mapped to what. I originally assumed that you map indices used on the result data to indices on the original data, but it seems to be the other way around?

Would be good to mention what is mapped to what. I originally assumed that you map indices used on the result data to indices on the original data, but it seems to be the other way around?
filedescriptor marked this conversation as resolved
Falk David added 4 commits 2024-08-12 17:19:40 +02:00
Falk David added 2 commits 2024-08-13 12:44:45 +02:00
Merge branch 'main' into gpv3-apply-modifier
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
6815f6fa3a
Falk David requested review from Jacques Lucke 2024-08-13 12:45:48 +02:00
Jacques Lucke approved these changes 2024-08-13 13:55:15 +02:00
Author
Member

@blender-bot build

@blender-bot build
Falk David merged commit 81d78e3416 into main 2024-08-13 15:02:40 +02:00
Falk David referenced this issue from a commit 2024-08-13 15:02:42 +02:00
Falk David deleted branch gpv3-apply-modifier 2024-08-13 15:02:44 +02:00

I don't think this PR is working as expected. When you apply a modifier, you must apply to all frames.

For example, if you have an animation and you want increase the thickness, you want increase in all frames, not only current. I have attached a videos with GPv2 and GPv3 apply.

I don't think this PR is working as expected. When you apply a modifier, you must apply to all frames. For example, if you have an animation and you want increase the thickness, you want increase in all frames, not only current. I have attached a videos with GPv2 and GPv3 apply.
Jeroen Bakker referenced this issue from a commit 2024-08-15 14:44:50 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#124543
No description provided.