GPv3: Envelope modifier implementation #118272

Lukas Tönne · 2024-02-14T14:18:26+01:00

Lukas Tönne commented

2024-02-14 14:18:26 +01:00

Ports the Envelope modifier from GPv2.

TODO

Fix segments mode, the result does not look quite like GP2. Hard to tell what is going on in the code.
Support the Skip feature fully (only use every n-th point, might affect range of points in segments mode?)
Support Thickness and Strength factors.

Ports the _Envelope_ modifier from GPv2. ## TODO - [x] Fix segments mode, the result does not look quite like GP2. Hard to tell what is going on in the code. - [x] Support the _Skip_ feature fully (only use every n-th point, might affect range of points in segments mode?) - [x] Support _Thickness_ and _Strength_ factors.

Lukas Tönne added 21 commits 2024-02-14 14:18:41 +01:00

9460bfc7dd Skeleton code for envelope modifier.

5f37bb399e Merge branch 'main' into gp3-envelope-modifier

030521eef0 Merge branch 'main' into gp3-envelope-modifier

b7c6ee6070 Merge branch 'main' into gp3-envelope-modifier

84fc70de36 Function for deform mode.

9988829003 Implement main radius calculation for envelope modifier in deform mode.

6cf9a2b3f2 Merge branch 'main' into gp3-envelope-modifier

9b8d056f8f Merge branch 'main' into gp3-envelope-modifier

4f427a3570 Implemented radius limiting function for envelope modifier.

2ec50de3e8 Fix points range: Use slicing to make sure curve ranges are wrapped.

51a1b9ced4 Fix factor 2 on center point blending calculation.

452f23bacc Merge branch 'main' into gp3-envelope-modifier

f51372ef40 Temp: Adding segment/fill modes for envelope modifier.

410df5a948 Cleanup: Separate functions to reduce number of local variables.

0d2e898b23 Info struct for common constants passed during modifier eval.

21caf4ca08 Removed "keep_original" feature, none of the modes needs this in fact.

12e1aeb5e8 Fix generated curves, only one per point needed.

d52fca04c2 Fixed index range for envelope strokes.

85c8d71053 Added back "keep_original" option, this is used for both fill and segments modes.

This reverts commit 21caf4ca08.

0bcc5b5b87 Support material index setting of the envelope modifier.

bc7e627b90 Attempt at implementing segments mode like GP2.

Doesn't quite look the same, it's hard to tell what the GPv2 modifier
actually does (and what it's supposed to do which are probably not the
same thing).

Lukas Tönne added this to the Grease Pencil project 2024-02-14 14:21:56 +01:00

Lukas Tönne added 1 commit 2024-02-17 10:14:20 +01:00

cda9e456a2 Merge branch 'main' into gp3-envelope-modifier

Lukas Tönne commented

2024-02-17 13:55:38 +01:00

The GPv2 implementation of this modifier isn't well documented, so i took some time to try and understand what it does in detail, specifically for the generative Segments and Fills modes. The Deform mode is working as it is right now, even though the math is a little underexplained.

Expand wall of text

Segments mode

Lets start with a simple curve:

Segment mode creates simple 2-point lines between neighboring points. For a given spread S a line is created for every point pair (p-S+i, p+1+i). For example with a spread S=3 around point p the new lines would be [p-3, p+], [p-2, p+2], [p-1, p+3].

Most of these lines would be generated for neighboring points as well. In the example above, the line [p-2, p+2] would also be generated by the neighboring point p-1 as [(p-1)-1, (p-1)+3] and by point p+1 as [(p+1)-3, (p+1)+2]. Generally each point only needs to create one line, which drastically reduces the amount of redundant geometry.

One caveat (possibly a bug?) is that lines at the end points of a curve would stray into negative index territory, so the indices get clamped (for non-cyclic curves). The result is that multiple curves between the same points get generated. For example, around p=0 the generated lines
[-3, +1], [-2, +2], [-1, +3]
are clamped to
[0, 1], [0, 2], [0, 3]
which overlaps with lines by point p=1:
[0, 2], [0, 3], [0, 4].

These lines are not deduplicated, which is the reason the modifier generates "thicker" lines at the end points:

Fills mode

This mode is more mysterious/buggy. Instead of creating individual 2-point lines for every envelope pair, it creates a closed curve of surrounding points:

The way it uses the Spread setting is a bit weird: adding +1 to the spread alternatingly expands the neighborhood left or right, but not symmetrically:

The curves also have gaps at the end points, which are probably a bug too.

The segments mode may not end up working exactly the same in GPv3.

The GPv2 implementation of this modifier isn't well documented, so i took some time to try and understand what it does in detail, specifically for the generative _Segments_ and _Fills_ modes. The _Deform_ mode is working as it is right now, even though the math is a little underexplained. <details> <summary>Expand wall of text</summary> ## Segments mode Lets start with a simple curve: ![image](/attachments/df720eb9-c0da-48d5-9746-f5199454752f) Segment mode creates simple 2-point lines between neighboring points. For a given spread `S` a line is created for every point pair `(p-S+i, p+1+i)`. For example with a spread `S=3` around point `p` the new lines would be `[p-3, p+]`, `[p-2, p+2]`, `[p-1, p+3]`. ![image](/attachments/b7e8befe-0883-433a-976a-a8b7a4200314) Most of these lines would be generated for neighboring points as well. In the example above, the line `[p-2, p+2]` would also be generated by the neighboring point `p-1` as `[(p-1)-1, (p-1)+3]` and by point `p+1` as `[(p+1)-3, (p+1)+2]`. Generally each point only needs to create one line, which drastically reduces the amount of redundant geometry. One caveat (possibly a bug?) is that lines at the end points of a curve would stray into negative index territory, so the indices get clamped (for non-cyclic curves). The result is that multiple curves between the same points get generated. For example, around `p=0` the generated lines `[-3, +1]`, `[-2, +2]`, `[-1, +3]` are clamped to `[0, 1]`, `[0, 2]`, `[0, 3]` which overlaps with lines by point `p=1`: `[0, 2]`, `[0, 3]`, `[0, 4]`. These lines are not deduplicated, which is the reason the modifier generates "thicker" lines at the end points: ![image](/attachments/1d2b1192-bd2c-4c23-b41b-69f5a30c7856) ## Fills mode This mode is more mysterious/buggy. Instead of creating individual 2-point lines for every envelope pair, it creates a closed curve of surrounding points: ![image](/attachments/5b0f0ecd-30a7-4954-9bf9-66876cd13c82) The way it uses the _Spread_ setting is a bit weird: adding +1 to the spread alternatingly expands the neighborhood left or right, but not symmetrically: ![image](/attachments/cab4dddc-7b14-4645-adba-c4fd4195abff) ![image](/attachments/20337a91-69bc-416a-bce0-9a878857569a) ![image](/attachments/46ec06b4-30ab-4e28-9072-8575202fe391) ![image](/attachments/11247499-b8ea-446f-9819-8707da8784e4) The curves also have gaps at the end points, which are probably a bug too. ![image](/attachments/5972a646-5a4d-4e36-897a-bf40743cdedf) The segments mode may not end up working exactly the same in GPv3. </details>

image.png

60 KiB

image.png

46 KiB

image.png

49 KiB

image.png

788 KiB

image.png

48 KiB

image.png

47 KiB

image.png

41 KiB

image.png

43 KiB

image.png

64 KiB

image.png

46 KiB

👍 1

dupoxy commented

2024-02-17 14:35:24 +01:00

Maybe @weasel as the author of the current modifier can help (https://archive.blender.org/developer/D14341).

Lukas Tönne added 1 commit 2024-02-19 11:52:02 +01:00

f4802f43cc Merge branch 'main' into gp3-envelope-modifier

Lukas Tönne added 1 commit 2024-02-19 14:55:55 +01:00

2cd72a4c14 Implement segments mode "correctly" (whatever that means).

Lukas Tönne added 2 commits 2024-02-19 15:22:20 +01:00

f0a693f0a6 Use skip value to skip strokes.

e5616975f0 Cleanup: remove debug printing.

Lukas Tönne added 2 commits 2024-02-19 15:56:24 +01:00

5c2c989810 Modify radius and opacity.

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

f8da9073c8 CLeanup: remove debug prints.

Lukas Tönne changed title from ~~WIP: GPv3: Envelope modifier implementation~~ to GPv3: Envelope modifier implementation

2024-02-19 15:56:31 +01:00

Lukas Tönne commented

2024-02-19 15:57:39 +01:00

@blender-bot build

Lukas Tönne requested review from Falk David 2024-02-19 17:36:53 +01:00

Lukas Tönne requested review from Henrik D. 2024-02-19 17:37:03 +01:00

Falk David requested changes 2024-02-20 11:49:41 +01:00

Falk David left a comment

Overall this looks good. Just a have a small cleanup comment.

source/blender/modifiers/intern/MOD_grease_pencil_envelope.cc Outdated

						
				@ -0,0 +118,4 @@

				  const float diff_dir = p1_dir - p2_dir;

				  float u;

				  if (diff_dir != 0.0f) {

Falk David commented

2024-02-20 11:43:52 +01:00

Maybe this could be a function? So that we can do:

const float u = calculate_u(...);

or as a lambda

const float u = [&]() {
   ...
}();

Maybe this could be a function? So that we can do: ``` const float u = calculate_u(...); ``` or as a lambda ``` const float u = [&]() { ... }(); ```

LukasTonne marked this conversation as resolved

Lukas Tönne added 2 commits 2024-02-21 12:05:46 +01:00

1538a6acad Merge branch 'main' into gp3-envelope-modifier

cae4f272bc Wrap calculation of u in a local function to keep it const.

Falk David approved these changes 2024-02-21 12:17:07 +01:00

Falk David left a comment

Will let @weasel check the math, but overall this looks good to me.

Lukas Tönne added 1 commit 2024-02-26 12:49:11 +01:00

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

e73be9a3af Merge branch 'main' into gp3-envelope-modifier

Lukas Tönne commented

2024-02-26 12:49:41 +01:00

@blender-bot build

Iliya Katushenock reviewed 2024-02-26 12:51:47 +01:00

source/blender/modifiers/intern/MOD_grease_pencil_envelope.cc Outdated

						
				@ -0,0 +446,4 @@

				                                              const bool src_curve_cyclic,

				                                              const VArray<int> &src_material_indices,

				                                              const IndexRange dst_points,

				                                              const MutableSpan<int> curve_offsets,

Iliya Katushenock commented

2024-02-26 12:51:47 +01:00

const MutableSpan<int> curve_offsets -> MutableSpan<int> curve_offsets
There is no point in having a constant if the values themselves will change.

`const MutableSpan<int> curve_offsets` -> `MutableSpan<int> curve_offsets` There is no point in having a constant if the values themselves will change.

Lukas Tönne commented

2024-02-26 12:56:07 +01:00

Oh? I've used this a lot before, pretty sure Hans wanted it this way in other reviews. The values are still mutable when the span itself is const. It's unlikely that anyone would accidentally change the span instead of accessing elements of it, but making it const can't hurt.

Iliya Katushenock commented

2024-02-26 12:58:36 +01:00

It took me a lot of thinking just to realize that this was data to be changed. I see a bunch of constants in parameters, and the void result, and my first thought is: what does this function even do?

Iliya Katushenock commented

2024-02-26 13:00:07 +01:00

@HooglyBoogly /

Falk David commented

2024-02-26 13:40:57 +01:00

To me the more important is: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#return-arguments since this is basically "returning" the result in these mutable spans.
So these should be named r_curve_offsets, etc. const or no const. That will make it clear.

To me the more important is: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#return-arguments since this is basically "returning" the result in these mutable spans. So these should be named `r_curve_offsets`, etc. `const` or no `const`. That will make it clear.

Hans Goudey commented

2024-02-26 14:54:53 +01:00

Personally I don't use const in cases like this since visually it looks more separate. So I don't think I would have asked for it. But I think it's a very reasonable choice, and being more "strict" about const makes sense too! I don't really think a mutable span argument always needs a r_ prefix, it's often not helpful.

Personally I don't use `const` in cases like this since visually it looks more separate. So I don't think I would have asked for it. But I think it's a very reasonable choice, and being more "strict" about const makes sense too! I don't really think a mutable span argument always needs a `r_` prefix, it's often not helpful.

Lukas Tönne added 1 commit 2024-02-26 16:58:59 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-arm64 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-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

27047acedb Merge branch 'main' into gp3-envelope-modifier

Lukas Tönne commented

2024-02-26 16:59:18 +01:00

@blender-bot build

Henrik D. commented

2024-02-26 23:53:49 +01:00

Testing this, it seems to work exactly like the old code, except for the discussed bugfix on the end of fills. However I found a bug in the old version which is still there. Imo it is a bug, that in case the spread is higher than the length of the stroke, it keeps duplicating the end to end stroke. For non cyclic strokes, the spread should be clamped to point_count-2 imo, such that the end to end stroke is never duplicated.

Looking at the code I found no math errors. Just a minor performance regression in a weird special case.

Testing this, it seems to work exactly like the old code, except for the discussed bugfix on the end of fills. However I found a bug in the old version which is still there. Imo it is a bug, that in case the spread is higher than the length of the stroke, it keeps duplicating the end to end stroke. For non cyclic strokes, the spread should be clamped to `point_count-2` imo, such that the end to end stroke is never duplicated. Looking at the code I found no math errors. Just a minor performance regression in a weird special case.

Henrik D. requested changes 2024-02-26 23:56:41 +01:00

source/blender/modifiers/intern/MOD_grease_pencil_envelope.cc

						
				@ -0,0 +180,4 @@

				    if (point == 0 || point >= point_num - 1) {

				      return unlimited_radius;

				    }

				    for (const int line_i : IndexRange(spread)) {