GPv3: Noise modifier #117057
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
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117057
Loading…
Reference in New Issue
No description provided.
Delete Branch "ChengduLittleA/blender:gp3-noise-modifier"
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?
Ported stroke random effect modifier to GPv3.
Internally it uses new c++ math and curves API so it's cleaner.
gp3-noise-modifierto WIP: GPv3: Noise modifierWIP: GPv3: Noise modifierto GPv3: Noise modifier@blender-bot build
@ChengduLittleA Please run
make format
. The linter is currently failing.Overall work fine, just notice you put Randomize panel inside Influence panel. It should go above in their own panel.
Ah looks like it's because of this sub-panel thing being kind of different than the in-place "Influence" panel.
@LukasTonne What do you suggest we do here? A lot of modifiers will have this kind of sub panels, and they will all go to the bottom of the modifier. Having a flag property dedicated to a lot of UI panels doesn't sound that brilliant...
You should just use the
uiLayoutPanel
function to create the Randomize subpanel, instead of the oldmodifier_subpanel_register
. Only limitation atm is that you can't draw a header with buttons, so for the time being i think the Randomize checkbox will have to be drawn in a separate row above.Separate the checkbox from the header seems an UI regression for me. Is this something that is going to be fixed soon?
@deadpin is looking into panel header layouts
I now made it like this:
I added some cleanup comments in the code, but I also have some higher-level feedback:
Calling e.g.
drawing->radii_for_write();
will create a copy if the array is implicitly shared. This should be avoided.I suggest to restructure the code like so:
mmd->factor_*
filtered_strokes.foreach_index
@ -2486,6 +2487,7 @@ typedef enum VolumeToMeshFlag {
VOLUME_TO_MESH_USE_SMOOTH_SHADE = 1 << 0,
} VolumeToMeshFlag;
/* Enum in old dna file. */
What is this comment for?
Seems to be a merge duplicate of Opacity modifier, will remove
@ -7615,3 +7621,3 @@
PropertyRNA *prop;
static const EnumPropertyItem color_mode_items[] = {
static const EnumPropertyItem color_mode_items[] = {
Formatting issue
@ -0,0 +58,4 @@
#include "DEG_depsgraph.hh"
#include "DEG_depsgraph_query.hh"
using namespace blender;
All these functions should be in the namesspace
blender
@ -0,0 +106,4 @@
return (mmd->flag & GP_NOISE_USE_RANDOM) != 0;
}
static float *noise_table(int len, int offset, int seed)
Instead of using
MEM_callocN
and returning a pointer, this should build anArray<float>
.@ -0,0 +126,4 @@
static void deform_stroke(ModifierData *md,
Depsgraph *depsgraph,
Object *ob,
bke::greasepencil::Drawing *drawing)
Pass these parameters by reference e.g.
bke::greasepencil::Drawing &drawing
. When the function expects these to be notnullptr
, it's better to use references to ensure they exist.@ -0,0 +136,4 @@
GreasePencilNoiseModifierData *mmd = (GreasePencilNoiseModifierData *)md;
/* Noise value in range [-1..1] */
float3 vec1, vec2;
const bool use_curve = (mmd->influence.flag & GREASE_PENCIL_INFLUENCE_USE_CUSTOM_CURVE);
(mmd->influence.flag & GREASE_PENCIL_INFLUENCE_USE_CUSTOM_CURVE) != 0
@ -0,0 +141,4 @@
const bool is_keyframe = (mmd->noise_mode == GP_NOISE_RANDOM_KEYFRAME);
/* Sanitize as it can create out of bound reads. */
float noise_scale = clamp_f(mmd->noise_scale, 0.0f, 1.0f);
use
math::clamp
@ -0,0 +148,4 @@
MutableSpan<float3> positions = strokes.positions_for_write();
MutableSpan<float> radiis = drawing->radii_for_write();
MutableSpan<float> opacities = drawing->opacities_for_write();
OffsetIndices<int> curves = strokes.points_by_curve();
curves
->points_by_curve
@ -0,0 +221,4 @@
}
/* Vector orthogonal to normal. */
vec2 = math::cross(vec1, normals[point]);
This whole calculation of
vec2
should be replaced withwhere
tangents
andnormals
are retrieved like so:and then
up_vector
is used when writing topositions
.@ -0,0 +232,4 @@
if (mmd->factor_thickness > 0.0f) {
float noise = table_sample(noise_table_thickness,
local_point * noise_scale + fractf(mmd->noise_offset));
radiis[point] *= max_ff(1.0f + (noise * 2.0f - 1.0f) * weight * mmd->factor_thickness, 0.0f);
Use
math::max
. Same below.@ -0,0 +233,4 @@
float noise = table_sample(noise_table_thickness,
local_point * noise_scale + fractf(mmd->noise_offset));
radiis[point] *= max_ff(1.0f + (noise * 2.0f - 1.0f) * weight * mmd->factor_thickness, 0.0f);
CLAMP_MIN(radiis[point], GPENCIL_STRENGTH_MIN);
This clamp is no longer needed
@ -0,0 +240,4 @@
float noise = table_sample(noise_table_strength,
local_point * noise_scale + fractf(mmd->noise_offset));
opacities[point] *= max_ff(1.0f - noise * weight * mmd->factor_strength, 0.0f);
CLAMP(radiis[point], GPENCIL_STRENGTH_MIN, 1.0f);
This clamp should not be needed (and it's clamping the radii atm.)
@ -0,0 +147,4 @@
/* Calculate or get stroke normals. */
Span<float3> normals = strokes.evaluated_normals();
MutableSpan<float3> positions = strokes.positions_for_write();
MutableSpan<float> radiis = drawing->radii_for_write();
radii
(it's already the plural ofradius
)Now i generate a noise table for all strokes. The side effect of this is that the noise table is continuous along separated strokes, and depending on how you are animating, the scrolling effect may or may not be visible.
Please only mark things as resolved when they are actually resolved 😅 Otherwise I'll have no idea what was changed.
@ -0,0 +243,4 @@
blender::bke::GeometrySet *geometry_set)
{
GreasePencilNoiseModifierData *mmd = (GreasePencilNoiseModifierData *)md;
GreasePencil *gp = geometry_set->get_grease_pencil_for_write();
Looks like you didn't make the changes here, but marked my request as resolved.
Ah it seems like I got confused with the review of the other modifier. Same feedback applies here though.
Ah that was from the subdiv PR. I saw it later 😅
c0cc12d33c
to196f1f4524
196f1f4524
toe1332951a9
@ -1135,3 +1132,1 @@
const Span<int> offsets,
const Span<int> materials,
const float4x4 &matrix)
bke::CurvesGeometry create_drawing_data(const Span<float3> positions,
This still needs to be removed.
@ -200,6 +200,13 @@ IndexMask retrieve_editable_and_selected_elements(Object &object,
bke::AttrDomain selection_domain,
IndexMaskMemory &memory);
bke::CurvesGeometry create_drawing_data(const Span<float3> positions,
Same as above.
@ -17,6 +17,7 @@ set(INC
../render
../windowmanager
../../../intern/eigen
../gpencil_modifiers_legacy
Not sure why this is needed 🤔
@ -0,0 +63,4 @@
static void init_data(ModifierData *md)
{
GreasePencilNoiseModifierData *gpmd = (GreasePencilNoiseModifierData *)md;
C++ style casts, same for the rest of the file.
I see that you went a little into the direction that I was thinking, but let me just write it out o make it more clear.
This is what I had in mind:
And you would do position, opacity, etc. each in their own loop. This means that we only ever change the attributes that we need to.
Huh I think I get it now. That way you can reduce the number of branches so the performance can be a bit better I guess? :D
Right, and it makes it clear what branch is modifiying what attribute :)
Some more comments
@ -0,0 +136,4 @@
&ob, strokes, mmd.influence, memory);
/* Noise value in range [-1..1] */
float3 up_vector;
No need for this declaration.
@ -0,0 +148,4 @@
return;
}
OffsetIndices<int> points_by_curve = strokes.points_by_curve();
Use
const
@ -0,0 +166,4 @@
}
int noise_len = ceilf(strokes.points_num() * noise_scale) + 2;
if (mmd.factor) {
Since these are floats, better to write as
mmd.factor > 0.0f
(otherwise it looks like they should be booleans when reading the code). Same for the others.@ -0,0 +174,4 @@
noise_len, int(floor(mmd.noise_offset)), seed + 2);
filtered_strokes.foreach_index([&](const int stroke_i) {
int point_count = points_by_curve[stroke_i].size();
Usually we iterate over the actual point indices and calculate the local point as needed. This makes it a bit easier to read (and with my suggested change below,
local_point
is not needed).Same for the other loops below.
@ -0,0 +177,4 @@
int point_count = points_by_curve[stroke_i].size();
for (const int local_point : points_by_curve[stroke_i].index_range()) {
int point = local_point + points_by_curve[stroke_i].start();
float weight = 1.0f;
Seems like a good idea to put this weight calculation in a lambda (so it can be reused in the other loops as well):
You can put this above
if (mmd.factor > 0.0f) {
and then call it like so:Use it in the other loops below too
@ -0,0 +183,4 @@
weight *= BKE_curvemapping_evaluateF(mmd.influence.custom_curve, 0, value);
}
/* Vector orthogonal to normal. */
up_vector = math::normalize(math::cross(tangents[point], normals[point]));
This should be
const float3 bi_normal
@ -0,0 +184,4 @@
}
/* Vector orthogonal to normal. */
up_vector = math::normalize(math::cross(tangents[point], normals[point]));
float noise = table_sample(noise_table_position,
Use
const
(same for the other loops)@ -0,0 +188,4 @@
point * noise_scale + fractf(mmd.noise_offset));
positions[point] += up_vector * (noise * 2.0f - 1.0f) * weight * mmd.factor * 0.1f;
}
});
We should call
drawing.tag_positions_changed();
after this line.We're almost there with this one too! I'll test this locally as well. Some more cleanup comments.
@ -0,0 +181,4 @@
filtered_strokes.foreach_index([&](const int stroke_i) {
const IndexRange points = points_by_curve[stroke_i];
for (const int point : points) {
Since we're using
stroke_i
, let's usepoint_i
to be consistent. Same for the other loops.@ -0,0 +187,4 @@
const float3 bi_normal = math::normalize(math::cross(tangents[point], normals[point]));
const float noise = table_sample(noise_table_position,
point * noise_scale + fractf(mmd.noise_offset));
positions[point] += up_vector * (noise * 2.0f - 1.0f) * weight * mmd.factor * 0.1f;
up_vector
->bi_normal
@ -0,0 +248,4 @@
int current_frame = DEG_get_evaluated_scene(ctx->depsgraph)->r.cfra;
IndexMaskMemory mask_memory;
IndexMask layer_mask = modifier::greasepencil::get_filtered_layer_mask(
Can be
const
@ -0,0 +199,4 @@
noise_len, int(floor(mmd.noise_offset)), seed);
filtered_strokes.foreach_index([&](const int stroke_i) {
int point_count = points_by_curve[stroke_i].size();
Unused variable
@ -0,0 +111,4 @@
return table;
}
BLI_INLINE float table_sample(Array<float> &table, float x)
No need for the
BLI_INLINE
, the compiler should do it anyway.@ -0,0 +158,4 @@
}
else {
/* If change every keyframe, use the last keyframe. */
/* TODO: not correct. */
Looks like this is still to be done? I think we need to just pass the frame number into this function.
This used to be
seed += gpf->framenum;
, but now we are operating on adrawing
which doesn't have frame info... How should we approach this?Hm we can pass the frame number from the keyframe that the drawing came from. It would mean that we need another variant of
get_drawings_for_write
. I can take care of that.There is a function
get_drawing_infos_for_write
now so you can get the frame numbers.@ -0,0 +122,4 @@
static void deform_drawing(ModifierData &md,
Depsgraph *depsgraph,
Object &ob,
bke::greasepencil::Drawing &drawing)
Add
const int start_frame_number
@ -0,0 +250,4 @@
const IndexMask layer_mask = modifier::greasepencil::get_filtered_layer_mask(
grease_pencil, mmd->influence, mask_memory);
const Vector<bke::greasepencil::Drawing *> drawings =
modifier::greasepencil::get_drawings_for_write(grease_pencil, layer_mask, current_frame);
Use
get_drawing_infos_for_write
here.@ -0,0 +252,4 @@
const Vector<bke::greasepencil::Drawing *> drawings =
modifier::greasepencil::get_drawings_for_write(grease_pencil, layer_mask, current_frame);
threading::parallel_for_each(drawings, [&](bke::greasepencil::Drawing *drawing) {
This becomes
@blender-bot build
Hi, I also tested the PR and works great
@ -7789,6 +7890,7 @@ static void rna_def_modifier_grease_pencil_subdiv(BlenderRNA *brna)
RNA_def_property_range(prop, 0, 65536);
RNA_def_property_ui_range(prop, 0, 32, 1, 0);
RNA_def_property_ui_text(prop, "Level", "Number of subdivisions");
accidental change :)
@ -0,0 +215,4 @@
Array<float> noise_table_strength = noise_table(
noise_len, int(floor(mmd.noise_offset)), seed + 3);
filtered_strokes.foreach_index([&](const int stroke_i) {
Guess calling
foreach_index
separately for writing each attribute would affect performance for big curves?I'm not expert though
Current code looks clean BTW.
It's mostly so that we can call the accessor for e.g.
drawing.opacities_for_write();
seperatly and avoid copying the data (when it's shared). So this means that if you just use one of the factors, only that attribute would be touched.This looks good to me. I added just a few cleanup comments for the math functions.
@ -0,0 +111,4 @@
return table;
}
static float table_sample(Array<float> &table, float x)
const float
@ -0,0 +113,4 @@
static float table_sample(Array<float> &table, float x)
{
return math::interpolate(table[int(ceilf(x))], table[int(floor(x))], fractf(x));
should use
math::ceil
,math::floor
andmath::fract
@ -0,0 +155,4 @@
seed += BLI_hash_string(md.name);
if (mmd.flag & GP_NOISE_USE_RANDOM) {
if (!is_keyframe) {
seed += cfra / mmd.step;
math::floor(float(cfra) / float(mmd.step))
This needs to be a integer division
int(cfra) / mmd.step
otherwise the seed is not correctly.@ -0,0 +162,4 @@
seed += start_frame_number;
}
}
int noise_len = ceilf(strokes.points_num() * noise_scale) + 2;
math:ceil
@ -0,0 +186,4 @@
/* Vector orthogonal to normal. */
const float3 bi_normal = math::normalize(math::cross(tangents[point], normals[point]));
const float noise = table_sample(noise_table_position,
point * noise_scale + fractf(mmd.noise_offset));
math::fract
@ -0,0 +203,4 @@
for (const int point : points) {
const float weight = get_weight(points, point);
const float noise = table_sample(noise_table_thickness,
point * noise_scale + fractf(mmd.noise_offset));
math::fract
@ -0,0 +220,4 @@
for (const int point : points) {
const float weight = get_weight(points, point);
const float noise = table_sample(noise_table_strength,
point * noise_scale + fractf(mmd.noise_offset));
math::fract
@ -0,0 +6,4 @@
* \ingroup modifiers
*/
#include "BLI_hash.h"
Next time, please check for unused includes. I can comment out all of these and the file still compiles:
For some reason I forgot to do this on this modifier...
The
DEG_depsgraph_query.hh
is needed forDEG_get_ctime()
, which is needed beside drawing key frame for the input of randomization. I'll keep it there.Right, for that I thought it could be accessed with the
current_frame
variable instead.Right... I thought it was two different thing. After testing it seems to be matching the time from depsgraph.
@ -0,0 +120,4 @@
* Apply noise effect based on stroke direction.
*/
static void deform_drawing(ModifierData &md,
Depsgraph *depsgraph,
I think you can pass
current_frame
from the caller instead of the depsgraph here@ -0,0 +122,4 @@
static void deform_drawing(ModifierData &md,
Depsgraph *depsgraph,
const int start_frame_number,
Object &ob,
Use const and references where possible
@ -0,0 +164,4 @@
}
int noise_len = ceilf(strokes.points_num() * noise_scale) + 2;
auto get_weight = [&](const IndexRange points, const int point_i) {
int noise_len
->const int noise_len
@ -0,0 +173,4 @@
};
if (mmd.factor > 0.0f) {
Span<float3> normals = strokes.evaluated_normals();
Span
->const Span
@ -0,0 +179,4 @@
Array<float> noise_table_position = noise_table(
noise_len, int(floor(mmd.noise_offset)), seed + 2);
filtered_strokes.foreach_index([&](const int stroke_i) {
Might as well parallelize these loops with
GrainSize(512)
or so@ -0,0 +182,4 @@
filtered_strokes.foreach_index([&](const int stroke_i) {
const IndexRange points = points_by_curve[stroke_i];
for (const int point : points) {
float weight = get_weight(points, point);
Rather than iterating over
points
directly and subtractingpoint - points.start()
to get the index in the curve, iterate overconst int i : points.index_range()
and thenconst int point = points[i]
That avoids the indirection and unnecessary step of subtraction, and it generally done elsewhere too
Heh, that's what @ChengduLittleA had there before, my bad..
Haha, oops. Good that we're on the same page now anyway I guess!
Wait I'm not sure I understand... Here in the loop we don't seem to need that
point - points.start()
value at all, we seem to just needpoint
in the global index...?If I'm understanding this correctly then we should do:
get_weight
needsi
in this case, though currently it's hidden inside the lambda.Ah I see. Nice catch
@ -0,0 +231,4 @@
static void modify_geometry_set(ModifierData *md,
const ModifierEvalContext *ctx,
blender::bke::GeometrySet *geometry_set)
blender::
is unnecessary, we're already in the blender namespace7988805e15
tof08750e917
Maybe I'm doing it wrong? The modifier isn't doing anything here.
@ -298,1 +298,4 @@
"Deform volume based on noise or other vector fields"}, /* TODO: Use correct icon. */
{eModifierType_GreasePencilNoise,
"GREASEPENCIL_NOISE",
ICON_GREASEPENCIL,
This isn't the right icon
@HooglyBoogly Looks like the "Noise Scale" is zero.
Hmm, right. Should that be the default value?
Anyway, the position noise seems not to work either way?
@HooglyBoogly Working for me. Maybe you forgot the experimental option?
I noticed that it doesn't use the right normals
@filedescriptor Thanks! Note that the plane normal implementation also isn't the same as the old one. The old one only uses 3 points, the two end points and a point at a index of
0.75*totpoints
(BKE_gpencil_stroke_normal
)... So the behavior is gonna be different anyway, and it looks to me that using individual normals feels better, and this way you can have natural direction changes over 3d. Any thoughts?Of course we'll need to document the difference in here too.
@ChengduLittleA Right the old method was a crude way of getting a triangle with the plane normal. The new method is a bit more robust, and will yield the same result for flat strokes (which is the common case). The normal from the curves is very different from what we need. We need one normal per stroke, not per point.
The behavior in the video while drawing doesn't happen in the legacy version of the modifier.
@ -0,0 +128,4 @@
const OffsetIndices<int> points_by_curve = strokes.points_by_curve();
int seed = mmd.seed + strokes.points_num();
I noticed the
seed = mmd.seed + strokes.points_num()
; is different than the legacy version of the noise modifier which usesseed = mmd->seed + BLI_findindex(&gpf->strokes, gps);
On chat we talked about making sure the results are exactly the same, so probably better to stick with the old seed?
@ -0,0 +233,4 @@
const IndexMask layer_mask = modifier::greasepencil::get_filtered_layer_mask(
grease_pencil, mmd->influence, mask_memory);
const Vector<modifier::greasepencil::FrameDrawingInfo> drawing_infos =
modifier::greasepencil::get_drawing_infos_by_frame(
Clang format needed here
@blender-bot build
small questions, is it possible to have where the noise is cyclic (like we see in cartoon 3steps ...ect), if yes can we know a what is the range
or it 's a feature that i'm questioning about ? if so will it be a future target ?
@hamza-el-barmaki I'm not exactly sure what you're asking about, but it's not a question that belongs in this PR.
@ChengduLittleA Looks like there are some formatting issues still.
Update: After making sure that we use the same stroke order (by using
Convert To -> Grease Pencil v3
) we do get consistent result in both v2 and v3 with per-stroke table.Hummm the result is still not exactly the same. The stroke on top seems flipped in noise direction but other strokes suggests otherwise... Not sure what's wrong here, probably seed is hashed differently@blender-bot build
Obviously not ideal to allocate an array for every curve, but this is still probably way faster than the old modifier so it should be good enough. Just a few comments inline.
@ -125,6 +125,7 @@ typedef enum eNoiseGpencil_Flag {
GP_NOISE_MOD_UV = (1 << 9), /* Deprecated (only for versioning). */
GP_NOISE_INVERT_LAYERPASS = (1 << 10),
GP_NOISE_INVERT_MATERIAL = (1 << 11),
MOD_GREASE_PENCIL_NOISE_OPEN_INFLUENCE_PANEL = (1 << 12),
This looks unused and can be removed
@ -0,0 +159,4 @@
filtered_strokes.foreach_index(GrainSize(512), [&](const int stroke_i) {
const IndexRange points = points_by_curve[stroke_i];
const int noise_len = math::ceil(points.size() * noise_scale) + 2;
const Array<float> noise_table_positions = noise_table(
This whole loop is about the positions, so this could have a simpler name like
noise_table
here@ -0,0 +219,4 @@
const ModifierEvalContext *ctx,
bke::GeometrySet *geometry_set)
{
GreasePencilNoiseModifierData *mmd = reinterpret_cast<GreasePencilNoiseModifierData *>(md);
Add const (with
const auto &
) here. Then passmmd
rather thanmd
to thedeform_drawing
function@blender-bot build