GPv3: Build Modifer migration #118739

Merged
YimingWu merged 55 commits from ChengduLittleA/blender:gp3-mod-build into main 2024-03-20 13:28:39 +01:00
Member

Reimplemented build modifier using the new CurvesGeometry logic.

Reimplemented build modifier using the new `CurvesGeometry` logic.
YimingWu added the
Module
Grease Pencil
label 2024-02-26 11:15:51 +01:00
YimingWu added 3 commits 2024-02-26 11:16:03 +01:00
YimingWu force-pushed gp3-mod-build from 0c6e744332 to a09aab2179 2024-02-26 13:26:45 +01:00 Compare
First-time contributor

@ChengduLittleA hi, i was waiting this to start , thank you for working on this, if that possible can you add into account to make the cyclic stroke uncycled while building until the stroke get builded 100% then make it cyclic stroke

@ChengduLittleA hi, i was waiting this to start , thank you for working on this, if that possible can you add into account to make the cyclic stroke uncycled while building until the stroke get builded 100% then make it cyclic stroke
Author
Member

I think this is a good suggestion. However the current goal is migration and keep everything the same behaviour as much as possible, maybe later this can be an option? (since there must be old files relying on such effect to work)

@filedescriptor any opinions?

I think this is a good suggestion. However the current goal is migration and keep everything the same behaviour as much as possible, maybe later this can be an option? (since there must be old files relying on such effect to work) @filedescriptor any opinions?
Member

@ChengduLittleA Yes, we shouldn't change the behavior during migration as much as possible.

@ChengduLittleA Yes, we shouldn't change the behavior during migration as much as possible.
Falk David requested changes 2024-02-26 14:12:24 +01:00
Dismissed
Falk David left a comment
Member

Nice! Did a first pass on this.

Nice! Did a first pass on this.
@ -2019,2 +2046,4 @@
}
const EnumPropertyItem *grease_pencil_build_time_mode_filter(bContext * /*C*/,
PointerRNA *ptr,
Member

Seems like this didn't get formatted (maybe forgot to save?).

Seems like this didn't get formatted (maybe forgot to save?).
ChengduLittleA marked this conversation as resolved
@ -0,0 +17,4 @@
#include "DNA_defaults.h"
#include "DNA_gpencil_modifier_types.h"
#include "DNA_node_types.h" /* For `GeometryNodeCurveSampleMode` */
Member

I'm not seeing any uses of this enum.

I'm not seeing any uses of this enum.
ChengduLittleA marked this conversation as resolved
@ -0,0 +94,4 @@
const int time_alignment,
const int transition,
const float factor,
int &out_curves_num,
Member

Naming convention for return parameters is r_curves_num

Naming convention for return parameters is `r_curves_num`
ChengduLittleA marked this conversation as resolved
@ -0,0 +112,4 @@
math::clamp(factor, 0.0f, 1.0f) :
math::clamp(1.0f - factor, 0.0f, 1.0f);
auto get_factor = [&](const float factor_to_keep, const int index) {
Member

get_stroke_factor ?

`get_stroke_factor` ?
ChengduLittleA marked this conversation as resolved
@ -0,0 +185,4 @@
const IndexMask &selection,
const int transition,
const float factor,
int &out_curves_num,
Member

Same as in the other funtion. Use r_

Same as in the other funtion. Use `r_`
ChengduLittleA marked this conversation as resolved
@ -0,0 +278,4 @@
return dst_curves;
}
static float get_factor(const int time_mode,
Member

Pass the mode as const GreasePencilBuildTimeMode time_mode.

Pass the mode as `const GreasePencilBuildTimeMode time_mode`.
Author
Member

If I use enum in here the compiler will give me error if I don't cast when pass the modifier value into it (which is int), since the enum name is pretty long, I guess it would be fine to just use int here.

If I use enum in here the compiler will give me error if I don't cast when pass the modifier value into it (which is `int`), since the enum name is pretty long, I guess it would be fine to just use `int` here.
Member

I think it's better to just add the cast and use the proper type. The enum name being long isn't really a reason not to do that.

I think it's better to just add the cast and use the proper type. The enum name being long isn't really a reason not to do that.
ChengduLittleA marked this conversation as resolved
@ -0,0 +284,4 @@
const int length,
const float percentage)
{
if (time_mode == MOD_GREASE_PENCIL_BUILD_TIMEMODE_FRAMES) {
Member

Probably makes more sense to use a switch on the mode here.

Probably makes more sense to use a `switch` on the mode here.
ChengduLittleA marked this conversation as resolved
@ -0,0 +315,4 @@
const float factor = get_factor(
mmd.time_mode, current_time, mmd.start_delay, mmd.length, mmd.percentage_fac);
if (mmd.mode == MOD_GREASE_PENCIL_BUILD_MODE_CONCURRENT) {
Member

Same here, would make sense to use a switch.

Same here, would make sense to use a switch.
ChengduLittleA marked this conversation as resolved
@ -0,0 +418,4 @@
// TODO: Time offset modifier not merged yet:
// if (BKE_gpencil_modifiers_findby_type(ob, eGpencilModifierType_Time) != nullptr) {
Member

This can just be removed. The modifiers will work together, but might give nonsensical results.

This can just be removed. The modifiers will work together, but might give nonsensical results.
Author
Member

The original modifier will inhibit itself if there's a time modifier, is it our intention to have it run even if it doesn't make sense?

(Or it can be put into is_disabled?)

The original modifier will inhibit itself if there's a time modifier, is it our intention to have it run even if it doesn't make sense? (Or it can be put into `is_disabled`?)
Member

Yes, have them run even if it might not make sense

Yes, have them run even if it might not make sense
ChengduLittleA marked this conversation as resolved
YimingWu added 3 commits 2024-02-27 08:36:43 +01:00
YimingWu added 1 commit 2024-02-27 09:07:01 +01:00
YimingWu added 1 commit 2024-02-27 09:13:26 +01:00
YimingWu added 2 commits 2024-02-27 10:04:44 +01:00
YimingWu added 1 commit 2024-02-27 10:14:02 +01:00
YimingWu added 1 commit 2024-02-28 04:41:03 +01:00
YimingWu added 1 commit 2024-02-28 10:33:52 +01:00
YimingWu added 2 commits 2024-02-29 06:52:08 +01:00
YimingWu added 1 commit 2024-02-29 07:11:46 +01:00
YimingWu changed title from WIP: GPv3: Build Modifer migration to GPv3: Build Modifer migration 2024-02-29 07:12:11 +01:00
YimingWu added this to the Grease Pencil project 2024-02-29 07:12:20 +01:00
Author
Member

There are some problem with the usage of the weight accessor... It will need a "ensure vertex group available" call just like vertex weight angle/proximity modifier, otherwise it will not write to a completely empty group.

There are some problem with the usage of the weight accessor... It will need a "ensure vertex group available" call just like vertex weight angle/proximity modifier, otherwise it will not write to a completely empty group.
YimingWu added 1 commit 2024-02-29 13:27:55 +01:00
YimingWu reviewed 2024-02-29 13:29:11 +01:00
@ -0,0 +601,4 @@
threading::parallel_for_each(drawings, [&](bke::greasepencil::Drawing *drawing) {
const bke::greasepencil::Drawing *prev_drawing=nullptr;
//modifier::greasepencil::get_drawing_infos_by_layer(,)
Author
Member

Here @filedescriptor if you are interested this is where I'm trying to get the previous "Drawing".

Here @filedescriptor if you are interested this is where I'm trying to get the previous "Drawing".
filedescriptor marked this conversation as resolved
YimingWu added 1 commit 2024-03-01 08:07:38 +01:00
YimingWu added 1 commit 2024-03-01 10:31:11 +01:00
YimingWu added 1 commit 2024-03-01 10:35:41 +01:00
Author
Member

This is the file that can be used to test hand drawn strokes

This is the file that can be used to test hand drawn strokes
YimingWu added 1 commit 2024-03-01 10:50:13 +01:00
Falk David requested changes 2024-03-01 10:51:25 +01:00
Dismissed
Falk David left a comment
Member

This PR is missing conversion code still.

This PR is missing conversion code still.
@ -0,0 +451,4 @@
PointerRNA *ptr = modifier_panel_get_property_pointers(panel, &ob_ptr);
const int mode = RNA_enum_get(ptr, "mode");
int time_mode = RNA_enum_get(ptr, "time_mode");
Member

GreasePencilBuildTimeMode time_mode = GreasePencilBuildTimeMode(RNA_enum_get(ptr, "time_mode"));

`GreasePencilBuildTimeMode time_mode = GreasePencilBuildTimeMode(RNA_enum_get(ptr, "time_mode"));`
ChengduLittleA marked this conversation as resolved
@ -0,0 +473,4 @@
}
}
static void deform_drawing(const ModifierData &md,
Member

maybe build_drawing ?

maybe `build_drawing` ?
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-01 10:53:55 +01:00
YimingWu added 1 commit 2024-03-01 12:24:10 +01:00
YimingWu added 1 commit 2024-03-01 12:24:31 +01:00
Falk David requested changes 2024-03-01 12:55:00 +01:00
Dismissed
@ -983,12 +983,6 @@
.shadow_camera_size = 200.0f, \
}
#define _DNA_DEFAULT_GreasePencilArmatureModifierData \
Member

Looks like these changes are unrelated

Looks like these changes are unrelated
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-01 13:03:29 +01:00
Falk David reviewed 2024-03-04 11:14:11 +01:00
@ -0,0 +607,4 @@
mmd.target_vgname);
break;
case MOD_GREASE_PENCIL_BUILD_MODE_ADDITIVE:
// Todo: I'm not sure what this mode means, looks like the same to me.
Member

Is this still a TODO ? From what I understood users were saying is that this is used in combination with the Additive draw mode. With that mode (and auto-key), when a new keyframe is created, the content of the previous frame is copied into the new keyframe and the user is drawing "ontop". So I guess in this mode in the modifier, the curves from the previous drawing have to be "skipped" when building the strokes. Essentially, we just need to know the number of curves in the previous drawing and then start from that index within the current drawing.

Is this still a `TODO` ? From what I understood users were saying is that this is used in combination with the `Additive` draw mode. With that mode (and auto-key), when a new keyframe is created, the content of the previous frame is copied into the new keyframe and the user is drawing "ontop". So I guess in this mode in the modifier, the curves from the previous drawing have to be "skipped" when building the strokes. Essentially, we just need to know the number of curves in the previous drawing and then start from that index within the current drawing.
Author
Member

It is gonna work if we do have the "get previous drawing" part sorted out, but under current iteration method we can't get that information 😅

It is gonna work if we do have the "get previous drawing" part sorted out, but under current iteration method we can't get that information 😅
Member

This grease_pencil.get_drawing_at(layer, current_frame - 1); should give you the drawing that's visible one frame before the current one.

This `grease_pencil.get_drawing_at(layer, current_frame - 1);` should give you the drawing that's visible one frame before the current one.
Author
Member

Then another thing we need to know is how to get the layer from a drawing. It doesn't seem to have a reference in it, or we need to look up.

Then another thing we need to know is how to get the layer from a drawing. It doesn't seem to have a reference in it, or we need to look up.
ChengduLittleA marked this conversation as resolved
Falk David reviewed 2024-03-04 13:37:27 +01:00
@ -0,0 +641,4 @@
IndexMaskMemory mask_memory;
const IndexMask layer_mask = modifier::greasepencil::get_filtered_layer_mask(
grease_pencil, mmd->influence, mask_memory);
const Vector<bke::greasepencil::Drawing *> drawings =
Member

Use get_drawing_infos_by_layer here to get the drawing and the layer it is on.

Use `get_drawing_infos_by_layer` here to get the drawing and the layer it is on.
ChengduLittleA marked this conversation as resolved
Author
Member

Pushed a solution that does work with additive mode by manually getting previous frame... It's kinda unnecessarily complex. Not sure if we could have a simpler API for situation like this.

Pushed a solution that does work with additive mode by manually getting previous frame... It's kinda unnecessarily complex. Not sure if we could have a simpler API for situation like this.
YimingWu force-pushed gp3-mod-build from 10cf24cec8 to a0203a2af4 2024-03-04 14:49:46 +01:00 Compare
Hans Goudey requested changes 2024-03-04 16:31:52 +01:00
@ -0,0 +147,4 @@
selection.to_bools(select.as_mutable_span());
Array<int> result(stroke_count);
for (const int i : IndexRange(stroke_count)) {
const float local_factor = (select[i] ? get_stroke_factor(factor_to_keep, i) : 1.0f);
Member

Outer parentheses are unnecessary here

Outer parentheses are unnecessary here
ChengduLittleA marked this conversation as resolved
@ -0,0 +206,4 @@
const bool is_vanishing = transition == MOD_GREASE_PENCIL_BUILD_TRANSITION_VANISH;
bke::CurvesGeometry dst_curves(dst_points_num, dst_curves_num);
Array<int> dst_offsets(dst_curves_num + 1);
Member

Use the offsets array from the new curves directly instead of a separate array

Use the offsets array from the new curves directly instead of a separate array
ChengduLittleA marked this conversation as resolved
@ -0,0 +210,4 @@
Array<int> dst_to_src_point(dst_points_num);
dst_offsets[0] = 0;
int next_curve = 0, next_point = 0;
Member

Declare each variable on a separate line

Declare each variable on a separate line
ChengduLittleA marked this conversation as resolved
@ -0,0 +212,4 @@
int next_curve = 0, next_point = 0;
for (const int i : IndexRange(curves.curves_num())) {
if (points_per_curve[i]) {
Member

Flip the check and use continue

Flip the check and use `continue`
ChengduLittleA marked this conversation as resolved
@ -0,0 +219,4 @@
auto get_fade_weight = [&](const int local_index) {
if (is_vanishing) {
return 1.0f - std::clamp(float(local_index - curve_size + ends_per_curve[i]) /
float(abs(ends_per_curve[i] - starts_per_curve[i])),
Member

abs -> std::abs

Also there's so much happening in this one statement. Could it be split to multiple lines?

`abs` -> `std::abs` Also there's so much happening in this one statement. Could it be split to multiple lines?
ChengduLittleA marked this conversation as resolved
@ -0,0 +259,4 @@
const bke::AttributeAccessor src_attributes = curves.attributes();
bke::MutableAttributeAccessor dst_attributes = dst_curves.attributes_for_write();
gather_attributes(
Member

Looks like curve domain attributes aren't copied?

Looks like curve domain attributes aren't copied?
ChengduLittleA marked this conversation as resolved
@ -0,0 +274,4 @@
int &r_points_num)
{
const int stroke_count = curves.curves_num();
const OffsetIndices<int> &points_by_curve = curves.points_by_curve();
Member

This should not be a reference. points_by_curve() returns by value.

This should not be a reference. `points_by_curve()` returns by value.
ChengduLittleA marked this conversation as resolved
@ -0,0 +304,4 @@
if (select[stroke] && counted_points_num >= effective_points_num) {
continue;
}
else {
Member

else after continue is unnecessary

else after continue is unnecessary
ChengduLittleA marked this conversation as resolved
@ -0,0 +427,4 @@
const Span<float3> positions = curves.positions();
const float3 center = object.object_to_world().location();
struct _Pair {
Member

There should be no need for the underscore

There should be no need for the underscore
ChengduLittleA marked this conversation as resolved
@ -0,0 +448,4 @@
distances.begin(), distances.end(), [](_Pair &a, _Pair &b) { return a.value < b.value; });
Array<int> new_order(curves.curves_num());
threading::parallel_for(curves.curves_range(), 65536, [&](const IndexRange range) {
Member

Never seen a grain size that large TBH. Maybe just don't multi-thread this loop. It's going to be memory-bound anyway, so it won't necessarily be faster anyway

Never seen a grain size that large TBH. Maybe just don't multi-thread this loop. It's going to be memory-bound anyway, so it won't necessarily be faster anyway
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-05 06:17:20 +01:00
YimingWu added 1 commit 2024-03-05 06:23:27 +01:00
Falk David requested changes 2024-03-06 11:32:27 +01:00
Dismissed
Falk David left a comment
Member

I tested this locally and ran into a few issues:

  • Crash when switching to "Concurrent": BLI_assert failed: source/blender/blenkernel/BKE_curves.hh:895, curve_type_counts(), at 'this->runtime->type_counts == calculate_type_counts(this->curve_types())'
  • Using "Sequential" and "Natural Drawing Speed" doesn't seem to be working. I assume this is because the draw tool doesn't write the "initial_time" and "delta_time" attributes. I think it would be fine if this PR adds this.
  • The "Object" property doesn't seem to do anything
I tested this locally and ran into a few issues: * Crash when switching to "Concurrent": `BLI_assert failed: source/blender/blenkernel/BKE_curves.hh:895, curve_type_counts(), at 'this->runtime->type_counts == calculate_type_counts(this->curve_types())'` * Using "Sequential" and "Natural Drawing Speed" doesn't seem to be working. I assume this is because the draw tool doesn't write the "initial_time" and "delta_time" attributes. I think it would be fine if this PR adds this. * The "Object" property doesn't seem to do anything
YimingWu added 2 commits 2024-03-06 12:44:23 +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
38faec3723
Fix crash by duplicating original curve
Author
Member

Turns out it will only not crash if I copied the original CurvesGeometry instead of creating a new one.

The "Object" property doesn't seem to do anything

If you move the reference object, it will start building from the closest stroke to that object.

Turns out it will only not crash if I copied the original `CurvesGeometry` instead of creating a new one. > The "Object" property doesn't seem to do anything If you move the reference object, it will start building from the closest stroke to that object.
Author
Member

@blender-bot build

@blender-bot build
Member

@ChengduLittleA Looks like the issue is that when creating the curves from scratch, the types are catmul rom by default. I think when you copy over the attributes that will be correct, but it was missing a update_curve_types() call.

@ChengduLittleA Looks like the issue is that when creating the curves from scratch, the types are catmul rom by default. I think when you copy over the attributes that will be correct, but it was missing a `update_curve_types()` call.
Author
Member

missing a update_curve_types() call.

Ah so that's the thing. Will update.

> missing a `update_curve_types()` call. Ah so that's the thing. Will update.
YimingWu added 1 commit 2024-03-06 14:32:44 +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
c180a5037d
Use `update_curve_types`
Author
Member

@blender-bot build

@blender-bot build
Member

Materials are not preserved, the output strokes all have the same default material.

Materials are not preserved, the output strokes all have the same default material.
YimingWu added 2 commits 2024-03-07 12:38:53 +01:00
YimingWu added 1 commit 2024-03-07 12:39:31 +01:00
YimingWu added 1 commit 2024-03-08 13:46:05 +01:00
YimingWu added 1 commit 2024-03-11 01:12:59 +01:00
Hans Goudey requested changes 2024-03-11 15:32:39 +01:00
@ -0,0 +1,812 @@
/* SPDX-FileCopyrightText: 2005 Blender Authors
Member

Copyright year is wrong

Copyright year is wrong
ChengduLittleA marked this conversation as resolved
@ -0,0 +115,4 @@
curves.ensure_evaluated_lengths();
float max_length = 0;
for (const int stroke : curves.curves_range()) {
const float len = curves.evaluated_lengths_for_curve(stroke, false).last();
Member

evaluated_length_total_for_curve

`evaluated_length_total_for_curve`
ChengduLittleA marked this conversation as resolved
@ -0,0 +347,4 @@
const bool is_vanishing = transition == MOD_GREASE_PENCIL_BUILD_TRANSITION_VANISH;
bke::CurvesGeometry dst_curves(dst_points_num, dst_curves_num);
Array<int> dst_offsets(dst_curves_num + 1);
Member

Use the dst_curves offsets array directly instead of a separate array

Use the `dst_curves` offsets array directly instead of a separate array
ChengduLittleA marked this conversation as resolved
@ -0,0 +611,4 @@
mmd.target_vgname);
break;
case MOD_GREASE_PENCIL_BUILD_MODE_ADDITIVE:
// Todo: I'm not sure what this mode means, looks like the same to me.
Member

Looks like this should be resolved before this is committed

Looks like this should be resolved before this is committed
Member

It uses gpf_orig to get the number of original strokes to cut from the beginning. This is set to the unmodified data before modifier eval (gpencil_copy_structure_for_eval).

To get the original data in GPv3 you can use the depsgraph:

  const Object *orig_ob = DEG_get_original_object(ctx.object);
  BLI_assert(orig_ob->type == OB_GREASE_PENCIL);
  const GreasePencil &orig_grease_pencil = *static_cast<const GreasePencil *>(orig_ob->data);

Then find the drawing of the current frame, as with the evaluated data, and subtract the curve amount.

It uses `gpf_orig` to get the number of original strokes to cut from the beginning. This is set to the unmodified data before modifier eval (`gpencil_copy_structure_for_eval`). To get the original data in GPv3 you can use the depsgraph: ```cpp const Object *orig_ob = DEG_get_original_object(ctx.object); BLI_assert(orig_ob->type == OB_GREASE_PENCIL); const GreasePencil &orig_grease_pencil = *static_cast<const GreasePencil *>(orig_ob->data); ``` Then find the drawing of the current frame, as with the evaluated data, and subtract the curve amount.
Author
Member

Oh actually this is resolved... I should just remove the comment. The mask is calculated in the Remove a count of #prev_strokes part above.

Oh actually this is resolved... I should just remove the comment. The mask is calculated in the `Remove a count of #prev_strokes` part above.
ChengduLittleA marked this conversation as resolved
@ -0,0 +634,4 @@
const ModifierEvalContext *ctx,
blender::bke::GeometrySet *geometry_set)
{
auto *mmd = reinterpret_cast<GreasePencilBuildModifierData *>(md);
Member

const

`const`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-12 04:13:07 +01:00
Lukas Tönne requested changes 2024-03-12 08:23:20 +01:00
Dismissed
@ -0,0 +543,4 @@
/* Remove a count of #prev_strokes. */
if (mmd.mode == MOD_GREASE_PENCIL_BUILD_MODE_ADDITIVE && previous_drawing != nullptr) {
const bke::CurvesGeometry &prev_curves = drawing.strokes_for_write();
Member

Should this be previous_drawing.strokes()? Otherwise looks like previous_drawing is unused. But that's also the previous frame, rather than the "original" (unmodified) drawing, which is what i think the GPv2 build modifier did?

In the current form this would just always be zero.

Should this be `previous_drawing.strokes()`? Otherwise looks like `previous_drawing` is unused. But that's also the previous frame, rather than the "original" (unmodified) drawing, which is what i think the GPv2 build modifier did? In the current form this would just always be zero.
Author
Member

Indeed. Looks like I missed this one

Indeed. Looks like I missed this one
ChengduLittleA marked this conversation as resolved
YimingWu added 2 commits 2024-03-13 12:18:07 +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
80304152b0
Clean up typo "previous_drawing"
Author
Member

@blender-bot build

@blender-bot build
Falk David requested changes 2024-03-14 09:06:38 +01:00
Dismissed
Falk David left a comment
Member

Looks pretty good to me. Some smaller cleanup comments. Will make sure to test this today for approval.

Looks pretty good to me. Some smaller cleanup comments. Will make sure to test this today for approval.
@ -0,0 +134,4 @@
}
return factor * max_factor;
}
/* Else: (#MOD_GREASE_PENCIL_BUILD_TIMEALIGN_END). */
Member

Putting this in an if (time_alignment == MOD_GREASE_PENCIL_BUILD_TIMEALIGN_END) { would be a good safe-guard.

Putting this in an `if (time_alignment == MOD_GREASE_PENCIL_BUILD_TIMEALIGN_END) {` would be a good safe-guard.
ChengduLittleA marked this conversation as resolved
@ -0,0 +148,4 @@
Array<int> result(stroke_count);
for (const int i : IndexRange(stroke_count)) {
const float local_factor = select[i] ? get_stroke_factor(factor_to_keep, i) : 1.0f;
const int points = points_by_curve[i].size() * local_factor;
Member

points -> num_points

`points` -> `num_points`
ChengduLittleA marked this conversation as resolved
@ -0,0 +152,4 @@
result[i] = points;
if (clamp_points) {
r_points_num += points;
if (points) {
Member

if (num_points > 0)

`if (num_points > 0)`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-14 10:11:20 +01:00
YimingWu added 1 commit 2024-03-14 10:16:24 +01:00
Falk David approved these changes 2024-03-14 10:37:50 +01:00
Dismissed
Falk David left a comment
Member

Tested this locally and seem to be working well.

Tested this locally and seem to be working well.
Falk David requested review from Lukas Tönne 2024-03-14 10:38:18 +01:00
Falk David requested review from Hans Goudey 2024-03-14 10:38:18 +01:00
Lukas Tönne reviewed 2024-03-14 11:13:40 +01:00
@ -2089,6 +2116,35 @@ static void rna_GreasePencilDashModifier_segments_begin(CollectionPropertyIterat
nullptr);
}
const EnumPropertyItem *grease_pencil_build_time_mode_filter(bContext * /*C*/,
Member

There is a enum_items_filter utility function to make filtering existing enum items lists more compact. It's used in geometry node runtime RNA (the outer lambda won't work for static RNA, but the function body can be a two-liner):
a3587ee078/source/blender/nodes/geometry/nodes/node_geo_menu_switch.cc (L389-L393)

There is a `enum_items_filter` utility function to make filtering existing enum items lists more compact. It's used in geometry node runtime RNA (the outer lambda won't work for static RNA, but the function body can be a two-liner): https://projects.blender.org/blender/blender/src/commit/a3587ee078d3101d4569328cb4b183ce8c984c44/source/blender/nodes/geometry/nodes/node_geo_menu_switch.cc#L389-L393
Author
Member

Oh interesting... I'll try change it to this

Oh interesting... I'll try change it to this
Author
Member

the enum_items_filter is in the blender::node namespace. I guess we could actually move this to a common RNA utility place. But it can happen later.

the `enum_items_filter` is in the `blender::node` namespace. I guess we could actually move this to a common RNA utility place. But it can happen later.
Member

Ah sorry, i didn't realize. In that case maybe just leave it as is to avoid mixing the namespaces.

Ah sorry, i didn't realize. In that case maybe just leave it as is to avoid mixing the namespaces.
ChengduLittleA marked this conversation as resolved
Lukas Tönne approved these changes 2024-03-14 11:17:56 +01:00
YimingWu added 1 commit 2024-03-14 12:19:55 +01:00
YimingWu added 2 commits 2024-03-15 14:44:33 +01:00
Hans Goudey requested changes 2024-03-15 15:02:35 +01:00
@ -0,0 +256,4 @@
weights.finish();
offset_indices::accumulate_counts_to_offsets(dst_offsets);
array_utils::copy(dst_offsets.as_span(), dst_curves.offsets_for_write());
Member

Here you're copying the array to itself... Same in build_sequential

Here you're copying the array to itself... Same in `build_sequential`
ChengduLittleA marked this conversation as resolved
@ -0,0 +262,4 @@
bke::MutableAttributeAccessor dst_attributes = dst_curves.attributes_for_write();
gather_attributes(
src_attributes, bke::AttrDomain::Point, {}, {}, dst_to_src_point, dst_attributes);
Member

Looks like this should skip "radius" and "opacity", and target_vgname no? Otherwise the data above is just getting overwritten. Same for points_info_sequential

Looks like this should skip "radius" and "opacity", and `target_vgname` no? Otherwise the data above is just getting overwritten. Same for `points_info_sequential`
Author
Member

radius, opacity and weights are written to the original drawing/curves, and then gather_attributes would copy them to the new one, looks correct to me?

`radius`, `opacity` and weights are written to the original drawing/curves, and then `gather_attributes` would copy them to the new one, looks correct to me?
Member

Ah, interesting, I didn't see that structure. I guess that's fine

Ah, interesting, I didn't see that structure. I guess that's fine
filedescriptor marked this conversation as resolved
@ -0,0 +305,4 @@
selection.to_bools(select.as_mutable_span());
int counted_points_num = 0;
for (const int i : IndexRange(stroke_count)) {
Member

IndexRange(stroke_count) -> curves.curves_range()

`IndexRange(stroke_count)` -> `curves.curves_range()`
ChengduLittleA marked this conversation as resolved
@ -0,0 +472,4 @@
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
Array<float> times(curves.points_num());
bke::AttributeAccessor time_attributes = curves.attributes();
bke::AttributeReader<float> init_times = time_attributes.lookup_or_default<float>(
Member

Use the * operator on the lookup and just store the VArray in the variable here.

Also declare time_attributes, init_times, and delta_times const

Use the `*` operator on the lookup and just store the VArray in the variable here. Also declare `time_attributes`, `init_times`, and `delta_times` const
ChengduLittleA marked this conversation as resolved
@ -0,0 +479,4 @@
float current_time = 0;
float previous_init_time = init_times.varray[0];
for (const int i : IndexRange(curves.curves_num())) {
Member

IndexRange(curves.curves_num()) -> curves.curves_range()

Same thing in one other place

`IndexRange(curves.curves_num())` -> `curves.curves_range()` Same thing in one other place
ChengduLittleA marked this conversation as resolved
@ -0,0 +480,4 @@
float current_time = 0;
float previous_init_time = init_times.varray[0];
for (const int i : IndexRange(curves.curves_num())) {
if (i > 0) {
Member
  • i -> curve
  • p -> point
- `i` -> `curve` - `p` -> `point`
ChengduLittleA marked this conversation as resolved
@ -0,0 +484,4 @@
current_time += math::max(init_times.varray[i] - previous_init_time, max_gap);
previous_init_time = init_times.varray[i];
}
for (const int p : points_by_curve[i].index_range()) {
Member

Extract this to a local const IndexRange points = points_by_curve[i]; variable

Extract this to a local `const IndexRange points = points_by_curve[i];` variable
ChengduLittleA marked this conversation as resolved
@ -0,0 +551,4 @@
if (added_strokes > 0) {
Array<bool> work_on_select(curves.curves_num());
selection.to_bools(work_on_select.as_mutable_span());
for (const int i : IndexRange(prev_strokes)) {
Member

work_on_select.take_front(prev_strokes).fill(false);

`work_on_select.take_front(prev_strokes).fill(false);`
ChengduLittleA marked this conversation as resolved
@ -0,0 +554,4 @@
for (const int i : IndexRange(prev_strokes)) {
work_on_select[i] = false;
}
selection.from_bools(work_on_select, memory);
Member

from_bools is a static constructor and shouldn't be called on an instance of the class. It's meant to be used like IndexMask::from_bools. I would be surprised if this worked as you expected. And it looks like you use it correctly below 😅

`from_bools` is a static constructor and shouldn't be called on an instance of the class. It's meant to be used like `IndexMask::from_bools`. I would be surprised if this worked as you expected. And it looks like you use it correctly below 😅
ChengduLittleA marked this conversation as resolved
Hans Goudey requested changes 2024-03-15 15:05:42 +01:00
@ -0,0 +499,4 @@
return 1.0f;
}
static float get_build_factor(const int time_mode,
Member

int -> GreasePencilBuildTimeMode

`int` -> `GreasePencilBuildTimeMode`
ChengduLittleA marked this conversation as resolved
@ -0,0 +510,4 @@
const float max_gap,
const float fade)
{
Member

Unnecessary whitespace

Unnecessary whitespace
ChengduLittleA marked this conversation as resolved
@ -0,0 +520,4 @@
return get_factor_from_draw_speed(
curves, float(current_frame) / scene_fps, speed_fac, max_gap) *
(1.0f + fade);
default:
Member

Remove default so there is a compiler warning when adding a new enum value.

Add BLI_assert_unreachable() after the switch and return 0.0f there instead

Remove `default` so there is a compiler warning when adding a new enum value. Add `BLI_assert_unreachable()` after the switch and return `0.0f` there instead
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-15 15:17:37 +01:00
YimingWu added 1 commit 2024-03-15 15:20:52 +01:00
Hans Goudey requested changes 2024-03-15 16:17:26 +01:00
@ -0,0 +148,4 @@
Array<bool> select(stroke_count);
selection.to_bools(select.as_mutable_span());
Array<int> result(stroke_count);
for (const int i : IndexRange(stroke_count)) {
Member

IndexRange(stroke_count) -> curves.curves_range()

`IndexRange(stroke_count)` -> `curves.curves_range()`
ChengduLittleA marked this conversation as resolved
@ -0,0 +216,4 @@
int next_curve = 0;
int next_point = 0;
for (const int curve : curves.curves_range()) {
if (!points_per_curve[curve]) {
Member

CurvesGeometry won't contain empty curves, this check should be removed. Then I think next_curve is the same as curve?

`CurvesGeometry` won't contain empty curves, this check should be removed. Then I think `next_curve` is the same as `curve`?
Author
Member

points_per_curve is the calculated "per curve" count that should be preserved and a lot of times will contain 0s (calculated by points_per_curve_concurrent). next_curve is the curve count that's actually not zero, this loop essentially will skip "curves that haven't appeared yet".

`points_per_curve` is the calculated "per curve" count that should be preserved and a lot of times will contain 0s (calculated by `points_per_curve_concurrent`). `next_curve` is the curve count that's actually not zero, this loop essentially will skip "curves that haven't appeared yet".
Member

I left a comment about this. The naming here is very confusing.

I left a comment about this. The naming here is very confusing.
ChengduLittleA marked this conversation as resolved
@ -0,0 +236,4 @@
points_by_curve[curve].size() - points_per_curve[curve] :
0;
for (const int stroke_point : IndexRange(points_per_curve[curve])) {
if (stroke_point >= points_per_curve[curve]) {
Member

Looks like this check can be moved out of the loop or just removed? Since you're iterating through the size of points_per_curve[curve], the iteration index will never be greater than or equal to the size

Looks like this check can be moved out of the loop or just removed? Since you're iterating through the size of `points_per_curve[curve]`, the iteration index will never be greater than or equal to the size
Author
Member

Seems like it.

Seems like it.
ChengduLittleA marked this conversation as resolved
Hans Goudey requested changes 2024-03-15 16:22:37 +01:00
@ -0,0 +469,4 @@
const float max_gap)
{
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
const bke::AttributeAccessor time_attributes = curves.attributes();
Member

time_attributes -> attributes

This is just a regular attribute accessor, there is nothing specific to time about it, you're just using it to retrieve attributes that happen to be related to the time data later.

`time_attributes` -> `attributes` This is just a regular attribute accessor, there is nothing specific to time about it, you're just using it to retrieve attributes that happen to be related to the time data later.
ChengduLittleA marked this conversation as resolved
@ -0,0 +483,4 @@
current_time += math::max(init_times[curve] - previous_init_time, max_gap);
previous_init_time = init_times[curve];
}
const IndexRange points = points_by_curve[curve].index_range();
Member

It would be worth reading through other uses of IndexRange, this sort of misses the point. You're meant to just iterate over the indices directly: for (const int point : points) {

It would be worth reading through other uses of `IndexRange`, this sort of misses the point. You're meant to just iterate over the indices directly: `for (const int point : points) {`
ChengduLittleA marked this conversation as resolved
Falk David reviewed 2024-03-16 10:54:27 +01:00
@ -0,0 +175,4 @@
{
int dst_curves_num, dst_points_num;
const bool has_fade = factor_start != factor;
const Array<int> points_per_curve = points_per_curve_concurrent(
Member

Naming this points_per_curve is not ideal, because that name is already used by the curves.points_per_curve() offset indices. That makes the rest of the code very confusing.

Naming this `points_per_curve` is not ideal, because that name is already used by the `curves.points_per_curve()` offset indices. That makes the rest of the code very confusing.
Author
Member

Oh I didn't know that we also have that as an API function... How about keep_per_curve? Or just point_counts_to_keep?

Oh I didn't know that we also have that as an API function... How about `keep_per_curve`? Or just `point_counts_to_keep`?
Member

point_counts_to_keep sounds better

`point_counts_to_keep` sounds better
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-16 11:38:34 +01:00
Falk David requested review from Hans Goudey 2024-03-18 10:44:12 +01:00
Falk David requested changes 2024-03-18 10:52:09 +01:00
Dismissed
Falk David left a comment
Member

Added some more cleanup comments.

Added some more cleanup comments.
@ -0,0 +110,4 @@
int &r_points_num)
{
const int stroke_count = curves.curves_num();
const OffsetIndices<int> &points_by_curve = curves.points_by_curve();
Member

The offset indices is a class that is a thin wrapper around a span. So there isn't a good reason to get this by reference. Use const OffsetIndices<int> points_by_curve.

The offset indices is a class that is a thin wrapper around a span. So there isn't a good reason to get this by reference. Use `const OffsetIndices<int> points_by_curve`.
Member

It's also wrong to do here because curves.points_by_curve() returns by value.

It's also wrong to do here because `curves.points_by_curve()` returns by value.
ChengduLittleA marked this conversation as resolved
@ -0,0 +220,4 @@
continue;
}
dst_offsets[next_curve] = points_per_curve[curve];
const int curve_size = points_by_curve[curve].size();
Member

Since you're using points_by_curve multiple times in the loop, would be good to create a variable for the index range.
Usually we use

const IndexRange points = points_by_curve[curve];

for this.

Since you're using `points_by_curve` multiple times in the loop, would be good to create a variable for the index range. Usually we use ```cpp const IndexRange points = points_by_curve[curve]; ``` for this.
ChengduLittleA marked this conversation as resolved
@ -0,0 +380,4 @@
1.0f);
};
for (const int point : points_by_curve[stroke]) {
Member

Same as above.

Same as above.
ChengduLittleA marked this conversation as resolved
@ -0,0 +441,4 @@
Array<Pair> distances(curves.curves_num());
for (const int stroke : curves.curves_range()) {
const float3 p1 = positions[points_by_curve[stroke].first()];
Member

Same as above.

Same as above.
ChengduLittleA marked this conversation as resolved
YimingWu added 2 commits 2024-03-20 08:10: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-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
a6c82761c7
Merge branch 'main' into gp3-mod-build
Author
Member

@blender-bot build

@blender-bot build
Falk David approved these changes 2024-03-20 10:53:20 +01:00
Hans Goudey approved these changes 2024-03-20 13:20:51 +01:00
@ -0,0 +127,4 @@
}
auto get_stroke_factor = [&](const float factor, const int index) {
const float max_factor = max_length / curves.evaluated_lengths_for_curve(index, false).last();
Member

evaluated_length_total_for_curve

`evaluated_length_total_for_curve`
ChengduLittleA marked this conversation as resolved
@ -0,0 +654,4 @@
const bke::greasepencil::Drawing *prev_drawing = grease_pencil.get_drawing_at(
*layers[drawing_info.layer_index], eval_frame - 1);
build_drawing(
*md, *ctx->object, *drawing_info.drawing, prev_drawing, eval_frame, scene_fps);
Member

Pass mmd here instead of md

Pass `mmd` here instead of `md`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-20 13:25:43 +01:00
YimingWu added 1 commit 2024-03-20 13:27:56 +01:00
YimingWu merged commit 3689dfca4f into main 2024-03-20 13:28:39 +01:00
YimingWu deleted branch gp3-mod-build 2024-03-20 13:28:42 +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.

Dependencies

No dependencies set.

Reference: blender/blender#118739
No description provided.