GPv3: Fill texture coordinates system #119303

Merged
Falk David merged 60 commits from casey-bianco-davis/blender:GPv3-fill-texture-coordinates into main 2024-03-21 16:07:28 +01:00

This is implements the system texture coordinates for GPv3.

This pull request adds:

  • System for storing and viewing texture coordinates.
  • Texture coordinates are convert when covering from legacy to GPv3, (Tested with object and layer transformation)
  • Textures are set to the drawing plane.
This is implements the system texture coordinates for GPv3. This pull request adds: - System for storing and viewing texture coordinates. - Texture coordinates are convert when covering from legacy to GPv3, (Tested with object and layer transformation) - Textures are set to the drawing plane.
casey-bianco-davis added the
Interest
Grease Pencil
Module
Grease Pencil
labels 2024-03-11 02:26:42 +01:00
casey-bianco-davis added 22 commits 2024-03-11 02:26:47 +01:00
casey-bianco-davis added this to the Grease Pencil project 2024-03-11 02:26:51 +01:00
casey-bianco-davis requested review from Falk David 2024-03-11 02:27:13 +01:00
casey-bianco-davis requested review from Lukas Tönne 2024-03-11 02:28:43 +01:00
Iliya Katushenock reviewed 2024-03-11 10:08:11 +01:00
Iliya Katushenock left a comment
Member

Cleanup comments\

Cleanup comments\
@ -868,0 +878,4 @@
*/
void set_texture_matrix(blender::bke::greasepencil::Drawing &drawing,
int curve_i,
const blender::float4x2);

const blender::float4x2 -> const blender::float4x2 & or blender::float4x2

`const blender::float4x2` -> `const blender::float4x2 &` or `blender::float4x2`
casey-bianco-davis marked this conversation as resolved
@ -868,0 +885,4 @@
*/
void transfer_texture_matrices(const blender::bke::greasepencil::Drawing &src,
blender::bke::greasepencil::Drawing &dst,
const blender::Span<int> dst_to_src_curve);

const blender::Span<int> dst_to_src_curve -> blender::Span<int> dst_to_src_curve

`const blender::Span<int> dst_to_src_curve` -> `blender::Span<int> dst_to_src_curve`
casey-bianco-davis marked this conversation as resolved
@ -868,0 +887,4 @@
blender::bke::greasepencil::Drawing &dst,
const blender::Span<int> dst_to_src_curve);
void transfer_texture_matrices(const blender::bke::greasepencil::Drawing &src,
blender::bke::greasepencil::Drawing &dst,

Order should be: src, dst_to_src, dst

Order should be: `src, dst_to_src, dst`
casey-bianco-davis marked this conversation as resolved
@ -1461,0 +1473,4 @@
using namespace blender::math;
const bke::CurvesGeometry &curves = drawing.strokes();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();

offset_indices::OffsetIndices<int> -> OffsetIndices<int>

`offset_indices::OffsetIndices<int>` -> `OffsetIndices<int>`
casey-bianco-davis marked this conversation as resolved
@ -1461,0 +1483,4 @@
return float4x2::identity();
}
const float3 pt0 = positions[points.first()];

pt0 -> point_0 or point_a

`pt0` -> `point_0` or `point_a`
casey-bianco-davis marked this conversation as resolved
@ -1461,0 +1489,4 @@
/* Local X axis (p0 -> p1) */
const float3 locx = normalize(pt1 - pt0);
/* Local Y axis (cross to normal/x axis). */
const float3 locy = normalize(cross(normal, locx));

This looks like better to call them as normal and tangent

This looks like better to call them as `normal` and `tangent`
Author
Member

I personally disagree, There already is a variable called normal and it for the direction outside of the stroke plane, where as both of these two vectors are in the stroke's plane.

Although, I was wondering about naming them locu and locv

I personally disagree, There already is a variable called `normal` and it for the direction outside of the stroke plane, where as both of these two vectors are in the stroke's plane. Although, I was wondering about naming them `locu` and `locv`

This values are not locations, so i don't like loc naming.

This values are not locations, so i don't like `loc` naming.
Author
Member

Good point, what about dir, direction or basic?

Good point, what about `dir`, `direction` or `basic`?

As i can see, there is 2 perpendicular vector, and this is the reason why i'd treat them as normal and tangent.
But this can be base_normal and base_tangent if you'd like that.

As i can see, there is 2 perpendicular vector, and this is the reason why i'd treat them as `normal` and `tangent`. But this can be `base_normal` and `base_tangent` if you'd like that.
Member

I think just making the names a bit longer makes this explicit: local_y and local_x. So there is no confusion with location.

I think just making the names a bit longer makes this explicit: `local_y` and `local_x`. So there is no confusion with `location`.
casey-bianco-davis marked this conversation as resolved
@ -1461,0 +1544,4 @@
*/
/* Apply scale. */
textmat = from_scale<float2x2>(uv_scale_inv) * textmat;

const float3x2 textmat = rotation * from_scale<float2x2>(uv_scale_inv) + from_transform();?

`const float3x2 textmat = rotation * from_scale<float2x2>(uv_scale_inv) + from_transform();`?
Author
Member

const float3x2 textmat = rotation * from_scale<float2x2>(uv_scale_inv) + from_transform();
Does not quite work because the rotation times scale part is 2x2 where the transform part is 3x2 (order would also be wrong) and the from_location also includes a identity which would have to be removed.

It could be written as a one liner as:

const float3x2 texmat = float3x2(from_scale<float2x2>(uv_scale_inv) * rot) +
                          from_location<float3x2>(uv_translation) - float3x2::identity();

But I don't think this is as clear of what it does. What does @filedescriptor think?

`const float3x2 textmat = rotation * from_scale<float2x2>(uv_scale_inv) + from_transform();` Does not quite work because the rotation times scale part is 2x2 where the transform part is 3x2 (order would also be wrong) and the `from_location` also includes a identity which would have to be removed. It could be written as a one liner as: ``` const float3x2 texmat = float3x2(from_scale<float2x2>(uv_scale_inv) * rot) + from_location<float3x2>(uv_translation) - float3x2::identity(); ``` But I don't think this is as clear of what it does. What does @filedescriptor think?
Member

I think the multi-line version is fine, no need to make this more compact.

That being said, I'm not really sure what happens when you multiple a 2x2 with a 3x2 matrix. The first matrix would need to have another column to make this work naively. Is the 2x2 scale matrix implicitly converted into a MatView?

I think the multi-line version is fine, no need to make this more compact. That being said, I'm not really sure what happens when you multiple a 2x2 with a 3x2 matrix. The first matrix would need to have another column to make this work naively. Is the 2x2 scale matrix implicitly converted into a [MatView](https://projects.blender.org/blender/blender/src/commit/692de6d380c7860601b28ee0873f33452dde86f8/source/blender/blenlib/BLI_math_matrix_types.hh#L515)?
Author
Member

Multiplying a 2x2 with a 3x2 is just fine, no conversions happen, blender uses the reverse row column convention so this is 2x2*3x2 = 2x3, normal matrix multiplication is (AxB)*(BxC) = (AxC) but bender uses (BxA)*(CxB) = (AxC)

Multiplying a 2x2 with a 3x2 is just fine, no conversions happen, blender uses the reverse row column convention so this is `2x2*3x2 = 2x3`, normal matrix multiplication is `(AxB)*(BxC) = (AxC)` but bender uses `(BxA)*(CxB) = (AxC)`
casey-bianco-davis marked this conversation as resolved
@ -1461,0 +1693,4 @@
blender::bke::greasepencil::Drawing &dst,
const blender::IndexMask &dst_to_src_curve)
{
dst_to_src_curve.foreach_index([&](const int64_t index, const int64_t pos) {

Can not be parallel (GrainSize(4096))?

Can not be parallel (`GrainSize(4096)`)?
casey-bianco-davis marked this conversation as resolved
Falk David requested changes 2024-03-11 11:49:52 +01:00
Dismissed
Falk David left a comment
Member

Added some comments on the set_texture_matrix and get_texture_matrix functions.

Added some comments on the `set_texture_matrix` and `get_texture_matrix` functions.
@ -868,0 +869,4 @@
* Returns the matrix that transforms from a 3D point in layer-space to a 2D point in
* texture-space for the stroke `curve_i`
*/
blender::float4x2 get_texture_matrix(const blender::bke::greasepencil::Drawing &drawing,
Member

I think this function should not be a free function, but a member function on the Drawing class.

Here is what I would propose:

  • Add a function Span<float4x2> texture_matrices() const;
  • Similar to curve_plane_normals(), add a mutable SharedCache<Vector<float4x2>> curve_texture_matrices; that is lazily computed and returned (as a Span).
  • Add a tag_texture_matrices_changed() function that is called in tag_positions_changed() that calls this->runtime->curve_texture_matrices.tag_dirty();
I think this function should not be a free function, but a member function on the `Drawing` class. Here is what I would propose: - [x] Add a function `Span<float4x2> texture_matrices() const;` - [x] Similar to `curve_plane_normals()`, add a `mutable SharedCache<Vector<float4x2>> curve_texture_matrices;` that is lazily computed and returned (as a `Span`). - [x] Add a `tag_texture_matrices_changed()` function that is called in `tag_positions_changed()` that calls `this->runtime->curve_texture_matrices.tag_dirty();`
casey-bianco-davis marked this conversation as resolved
@ -868,0 +876,4 @@
* Sets the matrix the that transforms from a 3D point in layer-space to a 2D point in
* texture-space for the stroke `curve_i`
*/
void set_texture_matrix(blender::bke::greasepencil::Drawing &drawing,
Member

Similar to aboce, I think we should put this function in the Drawing.

  • Add a function void set_texture_matrices(const VArray<float4x2> &matrices, const IndexMask &selection); that computes and sets the matrices.
  • Make sure to call tag_texture_matrices_changed() at the end.
Similar to aboce, I think we should put this function in the `Drawing`. - [x] Add a function `void set_texture_matrices(const VArray<float4x2> &matrices, const IndexMask &selection);` that computes and sets the matrices. - [x] Make sure to call `tag_texture_matrices_changed()` at the end.
casey-bianco-davis marked this conversation as resolved
@ -868,0 +883,4 @@
/*
* Copy the texture matrices from `src` to `dst` for the strokes in `dst_to_src_curve`
*/
void transfer_texture_matrices(const blender::bke::greasepencil::Drawing &src,
Member

These seem to be unused. Any reason to have them in this patch?

These seem to be unused. Any reason to have them in this patch?
casey-bianco-davis marked this conversation as resolved
Lukas Tönne reviewed 2024-03-11 12:09:15 +01:00
@ -1461,0 +1605,4 @@
using namespace blender;
const float4x2 strokemat = get_local_to_stroke_matrix(drawing, curve_i);
const float3x2 textmat = get_stroke_to_texture_matrix(drawing.strokes(), curve_i);
Member

I think the convention is to use tex for textures and text for, well, text. texmat etc. is more consistent with other parts of Blender.

I think the convention is to use `tex` for textures and `text` for, well, text. `texmat` etc. is more consistent with other parts of Blender.
casey-bianco-davis marked this conversation as resolved
@ -1461,0 +1662,4 @@
* Our problem has the form of: `X = A * Y`
* We can solve for `A` using: `A = X * B`
*
* Where `B` is the Right-sided inverse, calculated as:
Member

If i'm not mistaken this is the Moore-Penrose pseudo inverse? Would be good to mention this name as a search term for others.

If i'm not mistaken this is the Moore-Penrose pseudo inverse? Would be good to mention this name as a search term for others.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis requested review from Falk David 2024-03-12 07:24:00 +01:00
casey-bianco-davis added 12 commits 2024-03-12 07:24:32 +01:00
Falk David requested changes 2024-03-12 15:17:36 +01:00
Dismissed
Falk David left a comment
Member

Some cleanup comments.

Some cleanup comments.
@ -415,0 +536,4 @@
return this->runtime->curve_texture_matrices.data().as_span();
}
static void set_stroke_to_texture_matrix(CurvesGeometry &curves, int curve_i, float3x2 texmat)
Member

You can make this a lambda in Drawing::set_texture_matrices. This way, the SpanAttributeWriters can be retrieved outside the selection.foreach_index loop (and call .finish() after the loop).

You can make this a `lambda` in `Drawing::set_texture_matrices`. This way, the `SpanAttributeWriter`s can be retrieved outside the `selection.foreach_index` loop (and call `.finish()` after the loop).
casey-bianco-davis marked this conversation as resolved
@ -415,0 +585,4 @@
const float4x2 texspace = matrices[pos];
/*
* WORKAROUND: This algorithm could work with floats but is prone to numerical error.
Member

Not sure this is really a workaround. Saying We do the computation using doubles to avoid numerical precision errors is fine imo.

Not sure this is really a workaround. Saying `We do the computation using doubles to avoid numerical precision errors` is fine imo.
casey-bianco-davis marked this conversation as resolved
@ -159,0 +172,4 @@
const float2 center = float2(0.5f, 0.5f);
const float2 uv_scale_inv = math::safe_rcp(uv_scale);
const float2 d = maxv - minv;
Member

d -> diagonal ?

`d` -> `diagonal` ?
casey-bianco-davis marked this conversation as resolved
@ -159,0 +173,4 @@
const float2 uv_scale_inv = math::safe_rcp(uv_scale);
const float2 d = maxv - minv;
const float s = sin(uv_rotation);
Member

s -> sin_rotation
c -> cos_rotation

`s` -> `sin_rotation` `c` -> `cos_rotation`
casey-bianco-davis marked this conversation as resolved
@ -159,0 +175,4 @@
const float2 d = maxv - minv;
const float s = sin(uv_rotation);
const float c = cos(uv_rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
Member

rot -> rotation

`rot` -> `rotation`
casey-bianco-davis marked this conversation as resolved
@ -159,0 +177,4 @@
const float c = cos(uv_rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
float3x2 texmat = float3x2::identity();
Member

texmat -> texture_matrix

`texmat` -> `texture_matrix`
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 5 commits 2024-03-13 01:53:03 +01:00
casey-bianco-davis requested review from Falk David 2024-03-13 01:54:13 +01:00
Falk David requested changes 2024-03-14 08:59:47 +01:00
Dismissed
Falk David left a comment
Member

Some more cleanup comments. It's getting close imo. @HooglyBoogly Could you maybe take a look at this? Since you were also in the meeting about this.

Some more cleanup comments. It's getting close imo. @HooglyBoogly Could you maybe take a look at this? Since you were also in the meeting about this.
@ -418,0 +479,4 @@
const float c = cos(uv_rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
float3x2 texmat = float3x2::identity();
Member

This should also be texture_matrix like in get_legacy_stroke_to_texture_matrix. I see other uses of texmat as well, please change them too.

This should also be `texture_matrix` like in `get_legacy_stroke_to_texture_matrix`. I see other uses of `texmat` as well, please change them too.
casey-bianco-davis marked this conversation as resolved
@ -159,0 +222,4 @@
const float3 pt3 = float3(point3->x, point3->y, point3->z);
/* Local X axis (p0 -> p1) */
const float3 locx = normalize(pt1 - pt0);
Member

Like in get_legacy_stroke_to_texture_matrix better to name this local_x to be a bit more clear.

Like in `get_legacy_stroke_to_texture_matrix` better to name this `local_x` to be a bit more clear.
casey-bianco-davis marked this conversation as resolved
@ -159,0 +226,4 @@
/* Point vector at 3/4 */
const float3 v3 = totpoints == 2 ? pt3 * 0.001f : pt3;
const float3 loc3 = v3 - pt0;
Member

I think the v3 variable is not necessary. const float3 local_3 = (totpoints == 2) ? (pt3 * 0.001f) - pt0 : pt3 - pt0;
And maybe add a comment why we multiply by 0.001f here.

I think the `v3` variable is not necessary. `const float3 local_3 = (totpoints == 2) ? (pt3 * 0.001f) - pt0 : pt3 - pt0;` And maybe add a comment why we multiply by `0.001f` here.
casey-bianco-davis marked this conversation as resolved
Hans Goudey requested changes 2024-03-14 15:40:50 +01:00
@ -418,0 +463,4 @@
using namespace blender::math;
const AttributeAccessor attributes = curves.attributes();
const VArray<float> uv_rotations = *attributes.lookup_or_default<float>(
Member

It's very important not to look up attributes for every curve index. That should be done once before the parallel loop.

Same with get_local_to_stroke_matrix. Better to add the data the function needs as arguments. If it has lots of arguments, so be it-- it's using a lot of data, it's okay for the signature to reflect that.

It's very important not to look up attributes for every curve index. That should be done once before the parallel loop. Same with `get_local_to_stroke_matrix`. Better to add the data the function needs as arguments. If it has lots of arguments, so be it-- it's using a lot of data, it's okay for the signature to reflect that.
casey-bianco-davis marked this conversation as resolved
@ -418,0 +481,4 @@
float3x2 texmat = float3x2::identity();
/*
* The order in which the three transforms are applied, has been carefully chosen to be easy to
Member

Comma unnecessary here applied, has -> applied has

Comma unnecessary here `applied, has` -> `applied has`
casey-bianco-davis marked this conversation as resolved
@ -418,0 +547,4 @@
SpanAttributeWriter<float> uv_rotations = attributes.lookup_or_add_for_write_span<float>(
"uv_rotation",
AttrDomain::Curve,
AttributeInitVArray(VArray<float>::ForSingle(0.0f, curves.curves_num())));
Member

0 is already the default for new attributes, you can just leave this part out. Same with uv_translations

0 is already the default for new attributes, you can just leave this part out. Same with `uv_translations`
casey-bianco-davis marked this conversation as resolved
@ -418,0 +557,4 @@
AttrDomain::Curve,
AttributeInitVArray(VArray<float2>::ForSingle(float2(1.0f, 1.0f), curves.curves_num())));
auto set_stroke_to_texture_matrix = [&](int curve_i, float3x2 texmat) {
Member

If this lambda is only used once, I'm not sure it really helps. I'd just write the code directly where it's used. That or exact it to a separate function (rather than a lambda)

If this lambda is only used once, I'm not sure it really helps. I'd just write the code directly where it's used. That or exact it to a separate function (rather than a lambda)
casey-bianco-davis marked this conversation as resolved
@ -418,0 +589,4 @@
*
* i.e.
* # # # # # # # #
* We need # # # # Instead of # # # #
Member

Could you extract this to a function so the logic and comment doesn't need to be duplicated?

Could you extract this to a function so the logic and comment doesn't need to be duplicated?
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis requested review from Falk David 2024-03-15 03:37:20 +01:00
casey-bianco-davis requested review from Hans Goudey 2024-03-15 03:37:21 +01:00
casey-bianco-davis added 8 commits 2024-03-15 03:37:31 +01:00
Falk David requested changes 2024-03-15 12:00:24 +01:00
Dismissed
Falk David left a comment
Member

Some more cleanup comments

Some more cleanup comments
@ -370,0 +468,4 @@
const float4x2 legacy_texture_matrix = get_legacy_texture_matrix(gps);
/* Ensure that everything is up to date. */
drawing.tag_positions_changed();
Member

I don't think this tag will do anything. The positions should already be tagged.

I don't think this tag will do anything. The positions should already be tagged.
Author
Member

The positions where up to date, but the normals where not.

The positions where up to date, but the normals where not.
casey-bianco-davis marked this conversation as resolved
@ -553,10 +555,11 @@ static void grease_pencil_geom_batch_ensure(Object &object,
int point_i,
int idx,
float length,
float4x2 texture_matrix,
Member

const float4x2 &

`const float4x2 &`
casey-bianco-davis marked this conversation as resolved
Falk David requested changes 2024-03-15 15:31:11 +01:00
Dismissed
Falk David left a comment
Member

Tested this locally (with !118664) and the conversion works great. I got only one concern for how it behaves while drawing.

Tested this locally (with !118664) and the conversion works great. I got only one concern for how it behaves while drawing.
@ -486,1 +491,4 @@
float3 u_dir;
float3 v_dir;
float3 origin = float3(0.0f);
Member

I think the origin needs to be based on the start sample (first point). Otherwise you might draw a fill where the texture is way out of place. This would also be consistent with GPv2. I do like how the texture is not rotated based on the first segment but rather sticks to the drawing plane. This (minus the origin) seems like a good improvement to keep.

I think the origin needs to be based on the start sample (first point). Otherwise you might draw a fill where the texture is way out of place. This would also be consistent with GPv2. I do like how the texture is not rotated based on the first segment but rather sticks to the drawing plane. This (minus the origin) seems like a good improvement to keep.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 3 commits 2024-03-16 00:45:28 +01:00
casey-bianco-davis requested review from Falk David 2024-03-16 00:57:24 +01:00
Falk David approved these changes 2024-03-16 10:48:19 +01:00
Falk David left a comment
Member

Thanks, this looks correct to me now. Will accept, but note that I'd like to land this after !118664 so that the rendering works as well.

Thanks, this looks correct to me now. Will accept, but note that I'd like to land this after !118664 so that the rendering works as well.
Hans Goudey changed title from GPv3: Fill texture coordinates system. to GPv3: Fill texture coordinates system 2024-03-18 19:37:56 +01:00
Falk David added 1 commit 2024-03-19 10:00:48 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 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
6272ff2bfa
Merge branch 'main' into GPv3-fill-texture-coordinates
Member

@blender-bot build

@blender-bot build
Falk David added 1 commit 2024-03-19 10:13:26 +01:00
buildbot/vexp-code-patch-windows-amd64 Build done. Details
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-coordinator Build done. Details
41e84a4c99
Merge branch 'main' into GPv3-fill-texture-coordinates
Member

@blender-bot build

@blender-bot build
Falk David added 1 commit 2024-03-19 11:11:59 +01:00
Lukas Tönne approved these changes 2024-03-19 11:12:30 +01:00
Falk David added 1 commit 2024-03-19 11:22:53 +01:00
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
1362105e15
Merge branch 'main' into GPv3-fill-texture-coordinates
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2024-03-19 13:10:31 +01:00
@ -74,2 +77,4 @@
void tag_topology_changed();
/*
* Returns the matrices that transforms from a 3D point in layer-space to a 2D point in
Member

matrices that transforms -> matrices that transform

`matrices that transforms` -> `matrices that transform`
casey-bianco-davis marked this conversation as resolved
@ -76,0 +82,4 @@
*/
Span<float4x2> texture_matrices() const;
/*
* Sets the matrices the that transforms from a 3D point in layer-space to a 2D point in
Member

Same

Same
casey-bianco-davis marked this conversation as resolved
@ -429,0 +436,4 @@
using namespace blender::math;
const CurvesGeometry &curves = drawing.strokes();
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
Member

These should all be retrieved just once rather than per curve as well. We just really don't want searching for attributes to show up in profiles. And for any hot loop like this, we should do the best we can to give the compiler all the tools to optimize it. Retrieving the input data at every iteration makes that much harder.

These should all be retrieved just once rather than per curve as well. We just really don't want searching for attributes to show up in profiles. And for any hot loop like this, we should do the best we can to give the compiler all the tools to optimize it. Retrieving the input data at every iteration makes that much harder.
Member

Looks like the function signature should be:

static float4x2 get_local_to_stroke_matrix(const IndexRange points, const Span<float3> positions, const float3 normal)
Looks like the function signature should be: ```cpp static float4x2 get_local_to_stroke_matrix(const IndexRange points, const Span<float3> positions, const float3 normal) ```
Member

I would just pass the sliced positions then, to avoid also passing the index range.

I would just pass the sliced positions then, to avoid also passing the index range.
Member

The code returns float4x2::identity() in case points.size() <= 2. It also uses points.first(). Doesn't feel like we should pull all that logic out of the function.

The code returns `float4x2::identity()` in case `points.size() <= 2`. It also uses `points.first()`. Doesn't feel like we should pull all that logic out of the function.
Member

points.size() <= 2 becomes curve_positions.size() <= 2 and points.first() becomes curve_positions.first(). That seems like an improvement to me TBH

`points.size() <= 2` becomes `curve_positions.size() <= 2` and `points.first()` becomes `curve_positions.first()`. That seems like an improvement to me TBH
Member

Sure that seems a bit nicer indeed

Sure that seems a bit nicer indeed
casey-bianco-davis marked this conversation as resolved
@ -429,0 +568,4 @@
AttrDomain::Curve,
AttributeInitVArray(VArray<float2>::ForSingle(float2(1.0f, 1.0f), curves.curves_num())));
selection.foreach_index([&](const int64_t curve_i, const int64_t pos) {
Member

Could this be parallel? (GrainSize(256) or so?)

Could this be parallel? (`GrainSize(256)` or so?)
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 6 commits 2024-03-19 22:04:12 +01:00
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
da3b7ea9dc
Cleanup: Use convert texture in batch.
casey-bianco-davis requested review from Hans Goudey 2024-03-19 22:05:45 +01:00
Lukas Tönne added a new dependency 2024-03-20 07:53:22 +01:00
Hans Goudey approved these changes 2024-03-21 15:35:03 +01:00
Member

@blender-bot build

@blender-bot build
Falk David merged commit 20b614ab8e into main 2024-03-21 16:07:28 +01:00
Sign in to join this conversation.
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
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.

Reference: blender/blender#119303
No description provided.