GPv3: Build Modifer migration #118739
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#118739
Loading…
Reference in New Issue
No description provided.
Delete Branch "ChengduLittleA/blender:gp3-mod-build"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Reimplemented build modifier using the new
CurvesGeometry
logic.0c6e744332
toa09aab2179
@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
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?
@ChengduLittleA Yes, we shouldn't change the behavior during migration as much as possible.
Nice! Did a first pass on this.
@ -2019,2 +2046,4 @@
}
const EnumPropertyItem *grease_pencil_build_time_mode_filter(bContext * /*C*/,
PointerRNA *ptr,
Seems like this didn't get formatted (maybe forgot to save?).
@ -0,0 +17,4 @@
#include "DNA_defaults.h"
#include "DNA_gpencil_modifier_types.h"
#include "DNA_node_types.h" /* For `GeometryNodeCurveSampleMode` */
I'm not seeing any uses of this enum.
@ -0,0 +94,4 @@
const int time_alignment,
const int transition,
const float factor,
int &out_curves_num,
Naming convention for return parameters is
r_curves_num
@ -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) {
get_stroke_factor
?@ -0,0 +185,4 @@
const IndexMask &selection,
const int transition,
const float factor,
int &out_curves_num,
Same as in the other funtion. Use
r_
@ -0,0 +278,4 @@
return dst_curves;
}
static float get_factor(const int time_mode,
Pass the mode as
const GreasePencilBuildTimeMode time_mode
.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 useint
here.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.
@ -0,0 +284,4 @@
const int length,
const float percentage)
{
if (time_mode == MOD_GREASE_PENCIL_BUILD_TIMEMODE_FRAMES) {
Probably makes more sense to use a
switch
on the mode here.@ -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) {
Same here, would make sense to use a switch.
@ -0,0 +418,4 @@
// TODO: Time offset modifier not merged yet:
// if (BKE_gpencil_modifiers_findby_type(ob, eGpencilModifierType_Time) != nullptr) {
This can just be removed. The modifiers will work together, but might give nonsensical results.
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
?)Yes, have them run even if it might not make sense
WIP: GPv3: Build Modifer migrationto GPv3: Build Modifer migrationThere 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.
@ -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(,)
Here @filedescriptor if you are interested this is where I'm trying to get the previous "Drawing".
This is the file that can be used to test hand drawn strokes
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");
GreasePencilBuildTimeMode time_mode = GreasePencilBuildTimeMode(RNA_enum_get(ptr, "time_mode"));
@ -0,0 +473,4 @@
}
}
static void deform_drawing(const ModifierData &md,
maybe
build_drawing
?@ -983,12 +983,6 @@
.shadow_camera_size = 200.0f, \
}
#define _DNA_DEFAULT_GreasePencilArmatureModifierData \
Looks like these changes are unrelated
@ -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.
Is this still a
TODO
? From what I understood users were saying is that this is used in combination with theAdditive
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.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 😅
This
grease_pencil.get_drawing_at(layer, current_frame - 1);
should give you the drawing that's visible one frame before the current one.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.
@ -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 =
Use
get_drawing_infos_by_layer
here to get the drawing and the layer it is on.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.
10cf24cec8
toa0203a2af4
@ -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);
Outer parentheses are unnecessary here
@ -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);
Use the offsets array from the new curves directly instead of a separate array
@ -0,0 +210,4 @@
Array<int> dst_to_src_point(dst_points_num);
dst_offsets[0] = 0;
int next_curve = 0, next_point = 0;
Declare each variable on a separate line
@ -0,0 +212,4 @@
int next_curve = 0, next_point = 0;
for (const int i : IndexRange(curves.curves_num())) {
if (points_per_curve[i]) {
Flip the check and use
continue
@ -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])),
abs
->std::abs
Also there's so much happening in this one statement. Could it be split to multiple lines?
@ -0,0 +259,4 @@
const bke::AttributeAccessor src_attributes = curves.attributes();
bke::MutableAttributeAccessor dst_attributes = dst_curves.attributes_for_write();
gather_attributes(
Looks like curve domain attributes aren't copied?
@ -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();
This should not be a reference.
points_by_curve()
returns by value.@ -0,0 +304,4 @@
if (select[stroke] && counted_points_num >= effective_points_num) {
continue;
}
else {
else after continue is unnecessary
@ -0,0 +427,4 @@
const Span<float3> positions = curves.positions();
const float3 center = object.object_to_world().location();
struct _Pair {
There should be no need for the underscore
@ -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) {
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
I tested this locally and ran into a few issues:
BLI_assert failed: source/blender/blenkernel/BKE_curves.hh:895, curve_type_counts(), at 'this->runtime->type_counts == calculate_type_counts(this->curve_types())'
Turns out it will only not crash if I copied the original
CurvesGeometry
instead of creating a new one.If you move the reference object, it will start building from the closest stroke to that object.
@blender-bot build
@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.Ah so that's the thing. Will update.
@blender-bot build
Materials are not preserved, the output strokes all have the same default material.
@ -0,0 +1,812 @@
/* SPDX-FileCopyrightText: 2005 Blender Authors
Copyright year is wrong
@ -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();
evaluated_length_total_for_curve
@ -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);
Use the
dst_curves
offsets array directly instead of a separate array@ -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.
Looks like this should be resolved before this is committed
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:
Then find the drawing of the current frame, as with the evaluated data, and subtract the curve amount.
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.@ -0,0 +634,4 @@
const ModifierEvalContext *ctx,
blender::bke::GeometrySet *geometry_set)
{
auto *mmd = reinterpret_cast<GreasePencilBuildModifierData *>(md);
const
@ -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();
Should this be
previous_drawing.strokes()
? Otherwise looks likeprevious_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.
Indeed. Looks like I missed this one
@blender-bot build
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). */
Putting this in an
if (time_alignment == MOD_GREASE_PENCIL_BUILD_TIMEALIGN_END) {
would be a good safe-guard.@ -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;
points
->num_points
@ -0,0 +152,4 @@
result[i] = points;
if (clamp_points) {
r_points_num += points;
if (points) {
if (num_points > 0)
Tested this locally and seem to be working well.
@ -2089,6 +2116,35 @@ static void rna_GreasePencilDashModifier_segments_begin(CollectionPropertyIterat
nullptr);
}
const EnumPropertyItem *grease_pencil_build_time_mode_filter(bContext * /*C*/,
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)
Oh interesting... I'll try change it to this
the
enum_items_filter
is in theblender::node
namespace. I guess we could actually move this to a common RNA utility place. But it can happen later.Ah sorry, i didn't realize. In that case maybe just leave it as is to avoid mixing the namespaces.
@ -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());
Here you're copying the array to itself... Same in
build_sequential
@ -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);
Looks like this should skip "radius" and "opacity", and
target_vgname
no? Otherwise the data above is just getting overwritten. Same forpoints_info_sequential
radius
,opacity
and weights are written to the original drawing/curves, and thengather_attributes
would copy them to the new one, looks correct to me?Ah, interesting, I didn't see that structure. I guess that's fine
@ -0,0 +305,4 @@
selection.to_bools(select.as_mutable_span());
int counted_points_num = 0;
for (const int i : IndexRange(stroke_count)) {
IndexRange(stroke_count)
->curves.curves_range()
@ -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>(
Use the
*
operator on the lookup and just store the VArray in the variable here.Also declare
time_attributes
,init_times
, anddelta_times
const@ -0,0 +479,4 @@
float current_time = 0;
float previous_init_time = init_times.varray[0];
for (const int i : IndexRange(curves.curves_num())) {
IndexRange(curves.curves_num())
->curves.curves_range()
Same thing in one other place
@ -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) {
i
->curve
p
->point
@ -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()) {
Extract this to a local
const IndexRange points = points_by_curve[i];
variable@ -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)) {
work_on_select.take_front(prev_strokes).fill(false);
@ -0,0 +554,4 @@
for (const int i : IndexRange(prev_strokes)) {
work_on_select[i] = false;
}
selection.from_bools(work_on_select, memory);
from_bools
is a static constructor and shouldn't be called on an instance of the class. It's meant to be used likeIndexMask::from_bools
. I would be surprised if this worked as you expected. And it looks like you use it correctly below 😅@ -0,0 +499,4 @@
return 1.0f;
}
static float get_build_factor(const int time_mode,
int
->GreasePencilBuildTimeMode
@ -0,0 +510,4 @@
const float max_gap,
const float fade)
{
Unnecessary whitespace
@ -0,0 +520,4 @@
return get_factor_from_draw_speed(
curves, float(current_frame) / scene_fps, speed_fac, max_gap) *
(1.0f + fade);
default:
Remove
default
so there is a compiler warning when adding a new enum value.Add
BLI_assert_unreachable()
after the switch and return0.0f
there instead@ -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)) {
IndexRange(stroke_count)
->curves.curves_range()
@ -0,0 +216,4 @@
int next_curve = 0;
int next_point = 0;
for (const int curve : curves.curves_range()) {
if (!points_per_curve[curve]) {
CurvesGeometry
won't contain empty curves, this check should be removed. Then I thinknext_curve
is the same ascurve
?points_per_curve
is the calculated "per curve" count that should be preserved and a lot of times will contain 0s (calculated bypoints_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".I left a comment about this. The naming here is very confusing.
@ -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]) {
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 sizeSeems like it.
@ -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();
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.
@ -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();
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) {
@ -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(
Naming this
points_per_curve
is not ideal, because that name is already used by thecurves.points_per_curve()
offset indices. That makes the rest of the code very confusing.Oh I didn't know that we also have that as an API function... How about
keep_per_curve
? Or justpoint_counts_to_keep
?point_counts_to_keep
sounds betterAdded 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();
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
.It's also wrong to do here because
curves.points_by_curve()
returns by value.@ -0,0 +220,4 @@
continue;
}
dst_offsets[next_curve] = points_per_curve[curve];
const int curve_size = points_by_curve[curve].size();
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
for this.
@ -0,0 +380,4 @@
1.0f);
};
for (const int point : points_by_curve[stroke]) {
Same as above.
@ -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()];
Same as above.
@blender-bot build
@ -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();
evaluated_length_total_for_curve
@ -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);
Pass
mmd
here instead ofmd