GPv3: Texture offset modifier #119050

Lukas Tönne · 2024-03-04T12:43:08+01:00

Lukas Tönne commented

2024-03-04 12:43:08 +01:00

Port of the GPv2 texture modifier for transforming UVs of stroke points.

Lukas Tönne added 8 commits 2024-03-04 12:43:18 +01:00

3c56e92a86 Boilerplate for the texture modifier.

1afb994384 Fix incorrect panel type.

7a4f39ca5a Optional "stroke_uv" point attribute to customize stroke UV coordinates.

ec1a43d12f Support both `u_stroke` (float) and `uv_fill` (float2) attributes.

98e3c852df Implemented 2D fill UVs generated by the texture modifier.

This is not currently rendered by GPv3, but geometry nodes can be used
to make a copy of the curves and use a simple material to test the UV
coordinates.

2d2dd365b0 Merge branch 'main' into gp3-texture-modifier

953f02b8f1 Stroke U coordinate generation.

2c9e34e168 Implement the fit method scaling.

Lukas Tönne added this to the Grease Pencil project 2024-03-04 12:43:24 +01:00

Lukas Tönne added 1 commit 2024-03-04 17:01:05 +01:00

766cc069ee Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 1 commit 2024-03-05 12:02:47 +01:00

8a495efaf5 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 1 commit 2024-03-06 15:33:27 +01:00

c42caf0f25 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 1 commit 2024-03-20 07:31:53 +01:00

56ca9c6889 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added a new dependency 2024-03-20 07:53:22 +01:00

#119303 GPv3: Fill texture coordinates system

Lukas Tönne added 1 commit 2024-03-20 08:31:29 +01:00

e930fc5911 Removed `uv_fill` vertex attribute filling.

This is being replaced with a per-stroke texture matrix in #119303.

Lukas Tönne added 1 commit 2024-03-20 09:36:00 +01:00

f923306f80 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 2 commits 2024-03-20 14:20:02 +01:00

c54d3ade65 Removed duplicate modifier in conversion switch (merge error).

f9f25deb31 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 1 commit 2024-03-22 13:48:04 +01:00

4e9f1d13b7 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 1 commit 2024-03-22 15:20:07 +01:00

74ee707869 Use the texture matrix attributes to define UVs.

Lukas Tönne requested review from Falk David 2024-03-22 15:20:56 +01:00

Lukas Tönne requested review from casey-bianco-davis 2024-03-22 15:22:13 +01:00

Lukas Tönne added 1 commit 2024-03-22 15:24:38 +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

d17a1044d6 Cleanup: removed unused headers.

Lukas Tönne commented

2024-03-22 15:25:01 +01:00

@blender-bot build

Falk David approved these changes 2024-03-22 15:45:58 +01:00

Lukas Tönne commented

2024-03-22 15:57:30 +01:00

A note on converting legacy uv_fac and other point attributes:

It looks at first like these attributes can be totally arbitrary and user-defined, and replacing them with a simple linear transform might lose data. However, these attributes are consistently overwritten by the BKE_gpencil_stroke_uv_update function in GPv2 whenever the geometry is modified and/or the batch cache is rebuilt. Even though these attribute can be modified through the RNA (they are mutable properties!), any such changes are volatile and will never be visible in renders. So ignoring the legacy UV attributes and not converting them is fine.

A note on converting legacy `uv_fac` and other point attributes: It looks at first like these attributes can be totally arbitrary and user-defined, and replacing them with a simple linear transform might lose data. However, these attributes are consistently overwritten by the `BKE_gpencil_stroke_uv_update` function in GPv2 whenever the geometry is modified and/or the batch cache is rebuilt. Even though these attribute _can_ be modified through the RNA (they are mutable properties!), any such changes are volatile and will never be visible in renders. So ignoring the legacy UV attributes and not converting them is fine.

👍 1

casey-bianco-davis requested changes 2024-03-22 19:43:57 +01:00

Dismissed

casey-bianco-davis left a comment

I don't think this currently properly converts texture from the legacy system.

source/blender/makesdna/DNA_modifier_types.h Outdated

						
				@ -3407,0 +3417,4 @@

				  float fill_scale;

				  /** Custom index for passes. */

				  int layer_pass;

				  /** Texture fit options. */

casey-bianco-davis commented

2024-03-22 19:15:27 +01:00

Do these comments need double asterisks? (i.e /* Texture fit options. */)

Do these comments need double asterisks? (i.e `/* Texture fit options. */`)

Lukas Tönne commented

2024-03-26 11:34:26 +01:00

I cleaned these up, but note that there are other GP2 modifiers that use this format. The style guide isn't explicit about this, i think both variants are fine. It's mostly copy/pasted code here anyway. We may want to do a cleanup pass once the last modifiers are finished.

LukasTonne marked this conversation as resolved

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

						
				@ -0,0 +135,4 @@

				          VArray<float2>::ForSingle(float2(1.0f, 1.0f), curves.curves_num())));

				  curves_mask.foreach_index(GrainSize(512), [&](int64_t curve_i) {

				    uv_translations.span[curve_i] += offset;

casey-bianco-davis commented

2024-03-22 19:26:54 +01:00

Have to be very careful here, the uv_translation, uv_rotation and uv_scale attributes are not the same as the legacy system. The legacy system were applied in the order: translation, rotation then scale. Where as the new system's order is: scale, rotation then translation. Other differences the legacy system had include a bounding box rescaling and the rotation was centered around the point (0.5, 0.5).

Because of all of the changes I think it would be better to get the texture matrices transform them, then set the texture matrices, rather than directly working with the attributes.

Have to be very careful here, the `uv_translation`, `uv_rotation` and `uv_scale` attributes are not the same as the legacy system. The legacy system were applied in the order: translation, rotation then scale. Where as the new system's order is: scale, rotation then translation. Other differences the legacy system had include a bounding box rescaling and the rotation was centered around the point (0.5, 0.5). Because of all of the changes I think it would be better to get the texture matrices transform them, then set the texture matrices, rather than directly working with the attributes.

Lukas Tönne commented

2024-03-26 11:39:50 +01:00

I've implemented it now so that the effect of the modifier is compatible with the GPv2 way of constructing the transform.

The texture_matrices contain the unknown 3D transform, using them directly isn't possible without constructing the 2D UV transform from the attributes. So might as well use the attributes directly.

The code converts the new uv_translation/uv_rotation/uv_scale attributes into the legacy values, then applies the modifier offset, then converts them back. I've considered calculating equivalent offsets for the new attributes directly, which would be slightly more efficient, but converting to the legacy format is clearer IMO and explains why we're doing this weird transformation. I hope the code comment is enough to make this understandable.

I've implemented it now so that the effect of the modifier is compatible with the GPv2 way of constructing the transform. The `texture_matrices` contain the unknown 3D transform, using them directly isn't possible without constructing the 2D UV transform from the attributes. So might as well use the attributes directly. The code converts the new `uv_translation`/`uv_rotation`/`uv_scale` attributes into the legacy values, then applies the modifier offset, then converts them back. I've considered calculating equivalent offsets for the new attributes directly, which would be slightly more efficient, but converting to the legacy format is clearer IMO and explains why we're doing this weird transformation. I hope the code comment is enough to make this understandable.

casey-bianco-davis marked this conversation as resolved

Lukas Tönne added 1 commit 2024-03-25 09:51:59 +01:00

f6a6fe68c4 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne added 1 commit 2024-03-26 11:29:41 +01:00

c8fec25056 Conversion to legacy transform attributes.

This ensures the result of the modifier is the same as in the legacy
GPv2 implementation.

Lukas Tönne added 2 commits 2024-03-26 11:31:38 +01:00

443188e7e7 Merge branch 'main' into gp3-texture-modifier

9b74003c5c Comment cleanup.

Lukas Tönne requested review from casey-bianco-davis 2024-03-26 11:40:32 +01:00

casey-bianco-davis requested changes 2024-03-26 23:23:24 +01:00

Dismissed

casey-bianco-davis left a comment

It looks like the modifier only works properly on counter-clockwise strokes (see image)

GPv3 legacy modififer convert.png

441 KiB

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

						
				@ -0,0 +128,4 @@

				                                  const float rotation,

				                                  const float scale)

				{

				  /* Texture matrices are a combination of an unknown 3D transform into UV space, with a known 2D

casey-bianco-davis commented

2024-03-26 23:15:49 +01:00

My formater does not like this multi line comment, it should start with /** and the comment should be on the second line.

My formater does not like this multi line comment, it should start with `/**` and the comment should be on the second line.

Lukas Tönne commented

2024-03-27 11:01:19 +01:00

This should be fine, the double asterisk is only needed for doxygen comments.
Counterexample: 5ae5614d1e/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc (L257-L266)

This should be fine, the double asterisk is only needed for doxygen comments. Counterexample: https://projects.blender.org/blender/blender/src/commit/5ae5614d1e0608858f9c7a2a509119aa193c80e9/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc#L257-L266

casey-bianco-davis marked this conversation as resolved

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

						
				@ -0,0 +130,4 @@

				{

				  /* Texture matrices are a combination of an unknown 3D transform into UV space, with a known 2D

				   * transform on top.

casey-bianco-davis commented

2024-03-26 22:27:03 +01:00

All lines in the comment block should have a *

All lines in the comment block should have a `*`

LukasTonne marked this conversation as resolved

Lukas Tönne commented

2024-03-27 10:39:14 +01:00

It looks like the modifier only works properly on counter-clockwise strokes (see image)

The GPv2 modifier actually does exactly the same! The reason is that the normal calculated by the 3-point transform is dependent on the winding order of points 0, 1, and totpoints*0.75 (BKE_gpencil_stroke_2d_flat and get_legacy_layer_to_stroke_matrix respectively).

If we want to "fix" this we might flip the normal to make it somewhat consistent across strokes, but that assumes we can determine a "forward" direction to compare against. In any case, i don't think this is something the texture offset modifier should be doing.

EDIT: Correction, i thought you were talking about the fact that the UVs are mirrored for clockwise strokes, but i guess you are referring to the difference in offset? I can't seem to reproduce this though, GPv2 strokes with the modifier look the same after converting. Do you have a test file?

> It looks like the modifier only works properly on counter-clockwise strokes (see image) The GPv2 modifier actually does exactly the same! The reason is that the normal calculated by the 3-point transform is dependent on the winding order of points 0, 1, and `totpoints*0.75` (`BKE_gpencil_stroke_2d_flat` and `get_legacy_layer_to_stroke_matrix` respectively). If we want to "fix" this we might flip the normal to make it somewhat consistent across strokes, but that assumes we can determine a "forward" direction to compare against. In any case, i don't think this is something the texture offset modifier should be doing. EDIT: Correction, i thought you were talking about the fact that the UVs are mirrored for clockwise strokes, but i guess you are referring to the difference in offset? I can't seem to reproduce this though, GPv2 strokes with the modifier look the same after converting. Do you have a test file?

Lukas Tönne added 2 commits 2024-03-27 11:01:10 +01:00

d10c734e1a Merge branch 'main' into gp3-texture-modifier

2f8c1eb3aa Fix comment.

casey-bianco-davis commented

2024-03-27 18:48:34 +01:00

Here's the file I used, the top stroke changes when converted. It appears that only the translation is incorrect, the rotation and scale works fine.

GPv3 modifier texture convert from legacy.blend

1.2 MiB

Lukas Tönne commented

2024-03-28 07:37:41 +01:00

@casey-bianco-davis Object type for both the "Legacy" and "Convert" objects is GPENCIL, it looks like the "Convert" object isn't actually converted? It should be GREASEPENCIL for GPv3.

There is something else going on with the GPv2 strokes: When there are multiple GPv2 objects, as in the test file, moving an object (enforing UV updates) creates a completely different pattern. I have a suspicion that this is a GPv2 bug of some kind.

@casey-bianco-davis Object type for both the "Legacy" and "Convert" objects is `GPENCIL`, it looks like the "Convert" object isn't actually converted? It should be `GREASEPENCIL` for GPv3. There is something else going on with the GPv2 strokes: When there are multiple GPv2 objects, as in the test file, moving an object (enforing UV updates) creates a completely different pattern. I have a suspicion that this is a GPv2 bug of some kind.

Lukas Tönne commented

2024-03-28 10:26:41 +01:00

Something funky with the get_legacy_layer_to_stroke_matrix i noticed: the normals it computes are pointing in the same direction (0,-1,0) for both cw and ccw strokes, while later get_local_to_stroke_matrix updates use a (0,1,0) normal for the cw stroke. Picking arbitrary points on a non-convex shape such as the Suzanne example can lead to an "inverted" normal and flip the Y component of the local-to-stroke matrix translation, unlike the more stable Newell cross product. I don't know if this is at all relevant here.

EDIT: FWIW i haven't been able to repro the issue when i make the GPv2 strokes convex (using the "to sphere" operator).

Something funky with the `get_legacy_layer_to_stroke_matrix` i noticed: the normals it computes are pointing in the same direction `(0,-1,0)` for both cw and ccw strokes, while later `get_local_to_stroke_matrix` updates use a `(0,1,0)` normal for the cw stroke. Picking arbitrary points on a non-convex shape such as the Suzanne example can lead to an "inverted" normal and flip the Y component of the local-to-stroke matrix translation, unlike the more stable Newell cross product. I don't know if this is at all relevant here. EDIT: FWIW i haven't been able to repro the issue when i make the GPv2 strokes convex (using the "to sphere" operator).

image.png

675 KiB

Falk David commented

2024-03-28 12:50:56 +01:00

I would not spend too much time on this if the GPv2 UVs are buggy and we can't match the old behavior exactly.

Lukas Tönne commented

2024-03-28 13:39:17 +01:00

What i think is happening:

GPv2 constructs UVs using the 3-point method. This isn't great, but at least consistent.
Converting to GPv3 uses the same 3-point method to construct the initial texture matrix, with uv loc/rot/scale calculated to match the result of GPv2.
Subsequent updates in GPv3 (e.g. from edit mode) recalculate texture matrices, but using a different stroke-to-layer transform based on Newell normals (is mostly the same for convex shapes). The uv loc/rot/scale attributes change to produce the same overall texture matrix.
Texture modifier assumes a particular relationship between the GPv2 and GPv3 uv loc/rot/scale attributes, as described in my code comment. But because the stroke-to-layer base transform changes after the initial conversion the assumptions of the modifier are broken and the UV offset is wrong.

To make the modifier behave the same way as in GPv2 it will have to compute the same 3-point stroke-to-layer transform, since it cannot rely on uv loc/rot/scale attributes to be interpreted the same way. Once the stroke-to-layer transform is factored out of the input texture matrix, the remaining uv transform should behave as expected, regardless of whether 3-point or Newell's normals are used. It's ugly, but necessary for consistent results if we can't make assumptions about the texture space basis.

What i think is happening: 1. GPv2 constructs UVs using the 3-point method. This isn't great, but at least consistent. 2. Converting to GPv3 uses the same 3-point method to construct the initial texture matrix, with uv loc/rot/scale calculated to match the result of GPv2. 3. Subsequent updates in GPv3 (e.g. from edit mode) recalculate texture matrices, but using a _different_ stroke-to-layer transform based on Newell normals (is mostly the same for convex shapes). The uv loc/rot/scale attributes change to produce the same overall texture matrix. 3. Texture modifier assumes a particular relationship between the GPv2 and GPv3 uv loc/rot/scale attributes, as described in my code comment. But because the stroke-to-layer base transform changes after the initial conversion the assumptions of the modifier are broken and the UV offset is wrong. To make the modifier behave the same way as in GPv2 it will have to compute the same 3-point stroke-to-layer transform, since it cannot rely on uv loc/rot/scale attributes to be interpreted the same way. Once the stroke-to-layer transform is factored out of the input texture matrix, the remaining uv transform should behave as expected, regardless of whether 3-point or Newell's normals are used. It's ugly, but necessary for consistent results if we can't make assumptions about the texture space basis.

casey-bianco-davis commented

2024-03-28 18:48:52 +01:00

Object type for both the "Legacy" and "Convert" objects is GPENCIL, it looks like the "Convert" object isn't actually converted? It should be GREASEPENCIL for GPv3.

Sorry about that, I should have made it clear in my comment, I left the both objects unconverted so that you could convert it.

The function get_legacy_stroke_to_texture_matrix at blender/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc:169 should be used to factor out the legacy texture matrix.

> Object type for both the "Legacy" and "Convert" objects is `GPENCIL`, it looks like the "Convert" object isn't actually converted? It should be `GREASEPENCIL` for GPv3. Sorry about that, I should have made it clear in my comment, I left the both objects unconverted so that you could convert it. The function `get_legacy_stroke_to_texture_matrix` at `blender/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc:169` should be used to factor out the legacy texture matrix.

Lukas Tönne added 2 commits 2024-03-29 08:20:21 +01:00

a663ca1715 Fix dependence on the old 3-point stroke transform method.

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

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

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

66604ec5f3 Merge branch 'main' into gp3-texture-modifier

Lukas Tönne commented

2024-03-29 08:22:46 +01:00

I've changed it now to compute the legacy layer-to-stroke transform locally in the modifier, so as to not make assumptions about how the uv attributes are applied. This fixes the difference in transforms for concave strokes.

👍 2

Lukas Tönne commented

2024-03-29 08:23:04 +01:00

@blender-bot build

Falk David added 2 commits 2024-03-29 11:22:22 +01:00

9c0ba44d55 Merge branch 'main' into gp3-texture-modifier

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

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