GPv3: Multiple Strokes Modifier #117796

Merged
Falk David merged 15 commits from ChengduLittleA/blender:gp3-mod-multiple-strokes into main 2024-02-15 14:19:59 +01:00
Member

Multiple Strokes modifier now migrated to Grease Pencil v3.

All features seems to be working as intended.

Note:

  • The calculation of the minter vector is not exactly the same as the old modifier, since the old one uses stroke plane normal and this one uses per-vertex normal, which is closer to the initial intention.
  • If we want to preserve the exact same behaviour to the old one, maybe an additional flag could be added.

图片

Multiple Strokes modifier now migrated to Grease Pencil v3. All features seems to be working as intended. Note: - The calculation of the minter vector is not exactly the same as the old modifier, since the old one uses stroke plane normal and this one uses per-vertex normal, which is closer to the initial intention. - If we want to preserve the exact same behaviour to the old one, maybe an additional flag could be added. ![图片](/attachments/05dcd5be-4bb8-4a4b-930d-2e56c0cb9909)
YimingWu added the
Interest
Grease Pencil
Module
Grease Pencil
labels 2024-02-04 07:06:10 +01:00
YimingWu added 4 commits 2024-02-04 07:06:21 +01:00
YimingWu added this to the Grease Pencil project 2024-02-04 07:06:33 +01:00
Iliya Katushenock reviewed 2024-02-04 12:27:53 +01:00
@ -0,0 +112,4 @@
const int masked_handle = instances->add_reference(bke::InstanceReference{masked_geo});
const int unselected_handle = instances->add_reference(bke::InstanceReference{unselected_geo});
for (int i = 0; i < count; i++) {

[[maybe_unused]] const int i : IndexRange(count)

`[[maybe_unused]] const int i : IndexRange(count)`
ChengduLittleA marked this conversation as resolved
@ -0,0 +171,4 @@
threading::parallel_for(duplicated_strokes.points_range().take_front(points_num_pending),
1024,
[&](const IndexRange parallel_range) {

parallel_range -> range (usually we use this name everywhere), same below

`parallel_range` -> `range` (usually we use this name everywhere), same below
ChengduLittleA marked this conversation as resolved
@ -0,0 +189,4 @@
Span<float3> use_pos_r = pos_r.as_span().slice(this_stroke);
const float offset_fac = (mmd.duplications == 1) ? 0.5f :
(float(i) / float(mmd.duplications - 1));
const float thickness_factor = use_fading ? interpf(1.0f - fading_thickness,

bke::attribute_math::mix2<float> (maybe with using bke::attribute_math::mix2; in top of loop to make it shorter)

`bke::attribute_math::mix2<float>` (maybe with `using bke::attribute_math::mix2;` in top of loop to make it shorter)
ChengduLittleA marked this conversation as resolved
@ -0,0 +227,4 @@
GreasePencil &grease_pencil = *geometry_set->get_grease_pencil_for_write();
const int frame = grease_pencil.runtime->eval_frame;
IndexMaskMemory mask_memory;

maske_memory -> memory

`maske_memory` -> `memory`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-05 03:16:47 +01:00
Falk David requested changes 2024-02-05 10:49:42 +01:00
Falk David left a comment
Member

Looks really good already! Need to test locally still. Only a few cleanup comments.

Looks really good already! Need to test locally still. Only a few cleanup comments.
@ -0,0 +94,4 @@
const IndexMask curves_mask,
const IndexMask unselected_mask,
const int count,
int &original_point_count)
Member

r_original_point_count to indicate this is a return value

`r_original_point_count` to indicate this is a return value
ChengduLittleA marked this conversation as resolved
@ -0,0 +149,4 @@
const float offset = math::length(math::to_scale(float4x4(ctx.object->object_to_world))) *
mmd.offset;
const float distance = mmd.distance;
bool use_fading = (mmd.flag & MOD_GREASE_PENCIL_MULTIPLY_ENABLE_FADING) != 0;
Member

const bool

`const bool`
ChengduLittleA marked this conversation as resolved
@ -0,0 +173,4 @@
1024,
[&](const IndexRange range) {
for (const int point : range) {
const float3 minter = math::cross(normals[point], tangents[point]) *
Member

I think this should be named miter ?

I think this should be named `miter` ?
ChengduLittleA marked this conversation as resolved
YimingWu added 2 commits 2024-02-05 10:59:15 +01:00
YimingWu added 1 commit 2024-02-05 12:02:55 +01:00
Falk David requested changes 2024-02-05 12:13:21 +01:00
Falk David left a comment
Member

More cleanup

More cleanup
@ -2767,0 +2774,4 @@
int flag;
/* ? */
int flags;
Member

Right, this can be removed.

Right, this can be removed.
ChengduLittleA marked this conversation as resolved
@ -8424,0 +8447,4 @@
RNA_define_lib_overridable(true);
prop = RNA_def_property(srna, "use_fade", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "flags", MOD_GREASE_PENCIL_MULTIPLY_ENABLE_FADING);
Member

Use "flag"

Use `"flag"`
ChengduLittleA marked this conversation as resolved
Falk David requested changes 2024-02-05 12:22:29 +01:00
Falk David left a comment
Member

Some more cleanup.

While testing it seemed like the "Fade" options don't do anything. (and the fade toggle is not in the header)

Some more cleanup. While testing it seemed like the "Fade" options don't do anything. (and the fade toggle is not in the header)
@ -0,0 +142,4 @@
const IndexMask unselected_mask = curves_mask.complement(curves.curves_range(), mask_memory);
int original_point_count;
Member

src_point_count

`src_point_count`
ChengduLittleA marked this conversation as resolved
@ -0,0 +164,4 @@
bke::SpanAttributeWriter<float> radii = attributes.lookup_or_add_for_write_span<float>(
"radii", bke::AttrDomain::Point);
const int points_num_pending = (mmd.duplications + 1) * original_point_count;
Member

dst_point_count

`dst_point_count`
ChengduLittleA marked this conversation as resolved
@ -0,0 +182,4 @@
for (const int i : IndexRange(mmd.duplications)) {
using bke::attribute_math::mix2;
const IndexRange this_stroke = IndexRange(original_point_count * i, original_point_count);
Member

this_stroke -> stroke

`this_stroke` -> `stroke`
ChengduLittleA marked this conversation as resolved
@ -0,0 +186,4 @@
MutableSpan<float3> instance_positions = positions.slice(this_stroke);
MutableSpan<float> instance_opacity = opacities.span.slice(this_stroke);
MutableSpan<float> instance_radii = radii.span.slice(this_stroke);
Span<float3> use_pos_l = pos_l.as_span().slice(this_stroke);
Member

Maybe stroke_pos_l (and stroke_pos_r below)

Maybe `stroke_pos_l` (and `stroke_pos_r` below)
ChengduLittleA marked this conversation as resolved
Falk David reviewed 2024-02-05 12:40:00 +01:00
@ -0,0 +194,4 @@
1.0f - fading_thickness,
1.0f) :
1.0f;
const float opacity_factor = use_fading ? interpf(1.0f - fading_opacity,
Member

Use mix2 here as well.

Use `mix2` here as well.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-05 14:25:29 +01:00
Author
Member

and the fade toggle is not in the header

The uiLayoutPanelProp doesn't support ui in header yet. Other modifiers have been like this too.

Cleaned up the rest. For some reason my instance_radii[point] is always 0, I added a printf there, not sure where exactly is wrong...

> and the fade toggle is not in the header The `uiLayoutPanelProp` doesn't support ui in header yet. Other modifiers have been like this too. Cleaned up the rest. For some reason my `instance_radii[point]` is always 0, I added a printf there, not sure where exactly is wrong...
Falk David reviewed 2024-02-05 14:36:53 +01:00
@ -0,0 +162,4 @@
bke::SpanAttributeWriter<float> opacities = attributes.lookup_or_add_for_write_span<float>(
"opacity", bke::AttrDomain::Point);
bke::SpanAttributeWriter<float> radii = attributes.lookup_or_add_for_write_span<float>(
"radii", bke::AttrDomain::Point);
Member

"radius"

`"radius"`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-05 14:51:22 +01:00
Author
Member

Also checked against legacy modifier to make sure the offsetting direction is the same for both.

Also checked against legacy modifier to make sure the offsetting direction is the same for both.
Member

While testing this again I noticed that some memory might be leaking:

Blender quit
Error: Not freed memory blocks: 6, total unfreed memory 0.000221 MB
Data from GP len: 64 0x60c0000f91f8
Data from GP len: 56 0x60c0000f9378
info_for_mem_free len: 32 0x60b00014f880
info_for_mem_free len: 32 0x60b00014e960
CurvesGeometry len: 16 0x607000274fb8
info_for_mem_free len: 32 0x60b0003550c0

Otherwise it seems to be working correctly.

While testing this again I noticed that some memory might be leaking: ``` Blender quit Error: Not freed memory blocks: 6, total unfreed memory 0.000221 MB Data from GP len: 64 0x60c0000f91f8 Data from GP len: 56 0x60c0000f9378 info_for_mem_free len: 32 0x60b00014f880 info_for_mem_free len: 32 0x60b00014e960 CurvesGeometry len: 16 0x607000274fb8 info_for_mem_free len: 32 0x60b0003550c0 ``` Otherwise it seems to be working correctly.
Falk David requested changes 2024-02-06 11:50:51 +01:00
@ -223,1 +223,4 @@
"Duplicate strokes like a mirror"},
{eModifierType_GreasePencilMultiply,
"GREASE_PENCIL_MULTIPLY",
ICON_MOD_CURVE,
Member

I think this should be GP_MULTIFRAME_EDITING (should have it's own icon, but that's what GPv2 used)

I think this should be `GP_MULTIFRAME_EDITING` (should have it's own icon, but that's what GPv2 used)
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-06 13:20:03 +01:00
Author
Member

Had to use curves = std::move(duplicated_strokes); to avoid the unfreed reference, still not sure exactly how this works 😅

Had to use `curves = std::move(duplicated_strokes);` to avoid the unfreed reference, still not sure exactly how this works 😅
YimingWu added 1 commit 2024-02-06 13:21:32 +01:00
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
89eaf9cc2b
Merge branch 'main' into gp3-mod-multiple-strokes
Member

Had to use curves = std::move(duplicated_strokes); to avoid the unfreed reference, still not sure exactly how this works 😅

Right that makes sense, you don't want to copy the curves at that point. The operator= when assigning one CurvesGeometry to another, creates a copy. Moving the curves does not make a copy.

> Had to use `curves = std::move(duplicated_strokes);` to avoid the unfreed reference, still not sure exactly how this works 😅 Right that makes sense, you don't want to copy the curves at that point. The `operator=` when assigning one `CurvesGeometry` to another, creates a copy. Moving the curves does not make a copy.
Member

@blender-bot build

@blender-bot build
Hans Goudey reviewed 2024-02-06 15:48:55 +01:00
@ -0,0 +155,4 @@
const float fading_center = mmd.fading_center;
MutableSpan<float3> positions = duplicated_strokes.positions_for_write();
const Span<float3> normals = duplicated_strokes.evaluated_normals();
Member

It may be much faster to get the normals from the original curves if possible, since there are many fewer points, and the normals might be cached already. I wouldn't spend time on that unless you have a good idea how to do it though.

Two potentially problematic things though:

  • Reading the normals and tangents like this only works for poly curves. Otherwise they are given per evaluated point rather than per control point.
  • Do these really give the same behavior as the old modifier? I would be sort of surprised
It may be much faster to get the normals from the original curves if possible, since there are many fewer points, and the normals might be cached already. I wouldn't spend time on that unless you have a good idea how to do it though. Two potentially problematic things though: - Reading the normals and tangents like this only works for poly curves. Otherwise they are given per _evaluated_ point rather than per control point. - Do these really give the same behavior as the old modifier? I would be sort of surprised
Author
Member

Reading the normals and tangents like this only works for poly curves.

I guess I'll add a check to ensure they only work on poly curves (maybe also change the name into extend_polycurves ?)

Do these really give the same behavior as the old modifier? I would be sort of surprised.

I'm thinking of adding an option for toggle between plane normal and per-vertex normal. The per-vertex was initially the intention but a bit more complicated when I was working on the old modifier. per-vertex would look nicer on 3d curves.

> Reading the normals and tangents like this only works for poly curves. I guess I'll add a check to ensure they only work on poly curves (maybe also change the name into `extend_polycurves` ?) > Do these really give the same behavior as the old modifier? I would be sort of surprised. I'm thinking of adding an option for toggle between plane normal and per-vertex normal. The per-vertex was initially the intention but a bit more complicated when I was working on the old modifier. per-vertex would look nicer on 3d curves.
Member

I'm thinking of adding an option for toggle between plane normal and per-vertex normal

Let's keep it simple and use the plance normal.

> I'm thinking of adding an option for toggle between plane normal and per-vertex normal Let's keep it simple and use the plance normal.
ChengduLittleA marked this conversation as resolved
Member

@blender-bot build

@blender-bot build
YimingWu force-pushed gp3-mod-multiple-strokes from d81f068672 to 944cf022ca 2024-02-15 12:50:40 +01:00 Compare
YimingWu added 1 commit 2024-02-15 13:17:52 +01:00
Falk David requested changes 2024-02-15 13:50:27 +01:00
@ -0,0 +154,4 @@
const float fading_center = mmd.fading_center;
MutableSpan<float3> positions = duplicated_strokes.positions_for_write();
const Span<float3> normals = duplicated_strokes.evaluated_normals();
Member

This should be const Span<float3> normals = drawing.curve_plane_normals();

This should be `const Span<float3> normals = drawing.curve_plane_normals();`
Author
Member

I might have somehow ctrl-z 'ed on that. It should have been changed to that already 😅 Sorry about the confusion.

I might have somehow `ctrl-z` 'ed on that. It should have been changed to that already 😅 Sorry about the confusion.
filedescriptor marked this conversation as resolved
YimingWu added 1 commit 2024-02-15 13:56:39 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
497097d8f6
Use plan normal (missing change).
Member

@blender-bot build

@blender-bot build
Falk David approved these changes 2024-02-15 14:18:23 +01:00
Falk David merged commit 76b0976500 into main 2024-02-15 14:19:59 +01:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
4 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#117796
No description provided.