GPv3: Tint tool #119323

Merged
Falk David merged 33 commits from ChengduLittleA/blender:gp3-tint into main 2024-03-29 12:43:24 +01:00
Member

WIP of GPv3 Tint tool.

Stroke/Fill tinting working as intended. UI is also working.

WIP of GPv3 Tint tool. Stroke/Fill tinting working as intended. UI is also working.
YimingWu added the
Module
Grease Pencil
label 2024-03-11 14:23:04 +01:00
YimingWu added 3 commits 2024-03-11 14:23:16 +01:00
YimingWu added this to the Grease Pencil project 2024-03-11 14:23:21 +01:00
Falk David reviewed 2024-03-11 14:31:37 +01:00
@ -0,0 +184,4 @@
return;
}
execute_tint_on_drawing(active_layer.drawing_index_at(scene->r.cfra), scene->r.cfra, *drawing);
Member

I think this is not correct. You should pass the layer index, not the drawing index here. Use grease_pencil.get_layer_index()

I think this is not correct. You should pass the layer index, not the drawing index here. Use `grease_pencil.get_layer_index()`
Author
Member

Interesting... In grease_pencil_erase.cc line 817 they wrote this way too, then it's probably a mistake as well?

Interesting... In `grease_pencil_erase.cc` line 817 they wrote this way too, then it's probably a mistake as well?
Member

It is! Will push a fix.

It is! Will push a fix.
Member

Fixed for the eraser

Fixed for the eraser
ChengduLittleA marked this conversation as resolved
Falk David requested review from Falk David 2024-03-11 14:32:11 +01:00
YimingWu added 1 commit 2024-03-11 14:44:52 +01:00
YimingWu added 1 commit 2024-03-11 14:55:45 +01:00
Sietse Brouwer reviewed 2024-03-12 00:05:32 +01:00
Sietse Brouwer left a comment
Member

I know it's only a WIP, but I couldn't help noticing a few things ;-)

I know it's only a WIP, but I couldn't help noticing a few things ;-)
@ -0,0 +58,4 @@
BKE_curvemapping_init(brush->gpencil_settings->curve_strength);
this->radius = BKE_brush_size_get(scene, brush);
this->strength = brush->alpha;
Member

this->strength = BKE_brush_alpha_get(scene, brush);

`this->strength = BKE_brush_alpha_get(scene, brush);`
ChengduLittleA marked this conversation as resolved
@ -0,0 +94,4 @@
float radius = self.radius;
float strength = self.strength;
if (BKE_brush_use_size_pressure(brush)) {
radius *= BKE_curvemapping_evaluateF(
Member

This should be radius *= extension_sample.pressure. The falloff curve is affecting the strength (brush influence), not the brush radius.

This should be `radius *= extension_sample.pressure`. The falloff curve is affecting the _strength_ (brush influence), not the brush radius.
ChengduLittleA marked this conversation as resolved
@ -0,0 +120,4 @@
bke::crazyspace::get_evaluated_grease_pencil_drawing_deformation(
ob_eval, *obact, layer_index, frame_number);
/* Compute screen space positions. */
Member

For performance/efficiency, perhaps an idea to move this to TintOperation::on_stroke_begin? That's possible, because the curve positions stay the same during the tint operation.

For performance/efficiency, perhaps an idea to move this to `TintOperation::on_stroke_begin`? That's possible, because the curve positions stay the same during the tint operation.
Author
Member

Oh actually since the extend_stroke could be called on different drawings at a time, I'll probably would keep it there.

Oh actually since the `extend_stroke` could be called on different drawings at a time, I'll probably would keep it there.
ChengduLittleA marked this conversation as resolved
@ -0,0 +124,4 @@
Array<float2> screen_space_positions(strokes.points_num());
threading::parallel_for(strokes.points_range(), 4096, [&](const IndexRange range) {
for (const int point : range) {
ED_view3d_project_float_global(
Member

In curves code this is commonly used:

const RegionView3D *rv3d = CTX_wm_region_view3d(&C);
const ARegion *region = CTX_wm_region(&C);
const float4x4 projection = ED_view3d_ob_project_mat_get(rv3d, obact);
...
  screen_space_positions[point] = 
    ED_view3d_project_float_v2_m4(region, deformation.positions[point], projection);

I would propose to stick to that, for uniformity.

In curves code this is commonly used: ``` const RegionView3D *rv3d = CTX_wm_region_view3d(&C); const ARegion *region = CTX_wm_region(&C); const float4x4 projection = ED_view3d_ob_project_mat_get(rv3d, obact); ... screen_space_positions[point] = ED_view3d_project_float_v2_m4(region, deformation.positions[point], projection); ``` I would propose to stick to that, for uniformity.
Author
Member

I copied this from the erase tool. Will check.

I copied this from the erase tool. Will check.
ChengduLittleA marked this conversation as resolved
@ -0,0 +173,4 @@
}
}
});
fill_colors.finish();
Member

Should be outside the if block.

Should be outside the `if` block.
ChengduLittleA marked this conversation as resolved
@ -0,0 +185,4 @@
if (self.active_layer_only) {
/* Tint only on the drawing at the current frame of the active layer. */
const Layer &active_layer = *grease_pencil.get_active_layer();
Drawing *drawing = grease_pencil.get_editable_drawing_at(active_layer, scene->r.cfra);
Member

Perhaps we should discuss this? I would say this has to take the multi frame editing settings into account. For that, you could use retrieve_editable_drawings_from_layer() and apply execute_tint_on_drawing to all drawings in the returned vector. Now only the current frame is affected.

In the UI the switch says Active Layer, no mention of current frame only. So I would argue it should honour the multi frame editing setting.

Perhaps we should discuss this? I would say this has to take the multi frame editing settings into account. For that, you could use `retrieve_editable_drawings_from_layer()` and apply `execute_tint_on_drawing` to all drawings in the returned vector. Now only the current frame is affected. In the UI the switch says `Active Layer`, no mention of current frame only. So I would argue it should honour the multi frame editing setting.
Author
Member

@filedescriptor @amelief the eraser tool could use the same change I guess?

@filedescriptor @amelief the eraser tool could use the same change I guess?
Member

Right I think that was written at a time where muti-frame editing didn't exist. Will look at this separately.

Right I think that was written at a time where muti-frame editing didn't exist. Will look at this separately.
YimingWu added 1 commit 2024-03-12 04:33:30 +01:00
YimingWu added 2 commits 2024-03-12 04:58:09 +01:00
YimingWu added 1 commit 2024-03-12 04:59:31 +01:00
YimingWu added 1 commit 2024-03-12 10:55:49 +01:00
Author
Member

You can use this file to test directly

You can use this file to test directly
YimingWu added 2 commits 2024-03-12 12:05:00 +01:00
YimingWu added 1 commit 2024-03-12 13:27:36 +01:00
Falk David requested changes 2024-03-12 14:22:18 +01:00
Dismissed
Falk David left a comment
Member

Cleanup comments

Cleanup comments
@ -609,3 +603,4 @@
brush_basic_grease_pencil_paint_settings,
)
if grease_pencil_tool == 'DRAW':
Member

Looks like this change is unrelated. I'll clean it up later.

Looks like this change is unrelated. I'll clean it up later.
ChengduLittleA marked this conversation as resolved
@ -0,0 +38,4 @@
void on_stroke_extended(const bContext &C, const InputSample &extension_sample) override;
void on_stroke_done(const bContext &C) override;
float radius;
Member

These should be private and have a trailing underscore. See https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names.

These should be `private` and have a trailing underscore. See https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names.
ChengduLittleA marked this conversation as resolved
@ -0,0 +68,4 @@
this->color = ColorGeometry4f(color_linear);
}
/* From BKE_gpencil_legacy.h.*/
Member

Comment can be removed with changes below.

Comment can be removed with changes below.
ChengduLittleA marked this conversation as resolved
@ -0,0 +70,4 @@
/* From BKE_gpencil_legacy.h.*/
#define TINT_VERTEX_COLOR_STROKE(brush) \
Member

No need for #define. just put them in a const bool in the function at the top.

No need for `#define`. just put them in a `const bool` in the function at the top.
ChengduLittleA marked this conversation as resolved
@ -0,0 +77,4 @@
(((brush)->gpencil_settings->vertex_mode == GPPAINT_MODE_FILL) || \
((brush)->gpencil_settings->vertex_mode == GPPAINT_MODE_BOTH))
static void tint_perform_dab(TintOperation &self,
Member

execute_tint would be better to be consistent with other operations

`execute_tint` would be better to be consistent with other operations
ChengduLittleA marked this conversation as resolved
@ -0,0 +92,4 @@
Brush *brush = BKE_paint_brush(paint);
/* Get the tool's data. */
float2 mouse_position = extension_sample.mouse_position;
Member

const

`const`
ChengduLittleA marked this conversation as resolved
@ -0,0 +109,4 @@
strength = math::clamp(strength, 0.0f, 1.0f);
fill_strength = math::clamp(fill_strength, 0.0f, 1.0f);
/* Get the grease pencil drawing. */
Member

This comment is not right

This comment is not right
ChengduLittleA marked this conversation as resolved
@ -0,0 +119,4 @@
const Layer &layer = *grease_pencil.layers()[layer_index];
bke::CurvesGeometry &strokes = drawing.strokes_for_write();
/* Evaluated geometry. */
Member

This comment is not right, no geometry is getting evaluated.

This comment is not right, no geometry is getting evaluated.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-12 14:45:43 +01:00
YimingWu changed title from WIP: GPv3: Tint tool to GPv3: Tint tool 2024-03-12 14:45:52 +01:00
Falk David requested changes 2024-03-12 14:53:40 +01:00
Dismissed
Falk David left a comment
Member

More cleanup comments.

More cleanup comments.
@ -0,0 +39,4 @@
void on_stroke_done(const bContext &C) override;
private:
float _radius;
Member

Trailing underscore means -> radius_. Same for the other ones.

Trailing underscore means -> `radius_`. Same for the other ones.
ChengduLittleA marked this conversation as resolved
@ -0,0 +101,4 @@
strength = math::clamp(strength, 0.0f, 1.0f);
fill_strength = math::clamp(fill_strength, 0.0f, 1.0f);
const bool tint_strokes = (((brush)->gpencil_settings->vertex_mode == GPPAINT_MODE_STROKE) ||
Member

No need for ( ) around brush.

No need for `(` `)` around `brush`.
ChengduLittleA marked this conversation as resolved
@ -0,0 +159,4 @@
stroke_changed = true;
}
}
if (!fill_colors.span.is_empty() && stroke_changed && tint_fills) {
Member

Why should this check for stroke_changed ?

Why should this check for `stroke_changed` ?
Author
Member

Actually that variable should be assigned outside the vertex tint branch. I'll change it to stroke_touched, since it will only tint the fill when stroke is touched.

Actually that variable should be assigned outside the vertex tint branch. I'll change it to `stroke_touched`, since it will only tint the fill when stroke is touched.
Member

That doesn't seem right.. The fill should be changed when the brush intersects on of the fill triangles

That doesn't seem right.. The fill should be changed when the brush intersects on of the fill triangles
Author
Member

Looks like indeed. I'll try make it to work. Does GPv3 has utility functions that checks on this like GPv2 with pbuffer stuff?

Looks like indeed. I'll try make it to work. Does GPv3 has utility functions that checks on this like GPv2 with pbuffer stuff?
Member

You can use isect_point_poly_v2 and pass the 2d points of the stroke into it

You can use `isect_point_poly_v2` and pass the 2d points of the stroke into it
Author
Member

Looks like I could use BLI_lasso_boundbox and BLI_lasso_is_point_inside like the old tool. Doing that now.

Looks like I could use `BLI_lasso_boundbox` and `BLI_lasso_is_point_inside` like the old tool. Doing that now.
Member

Please don't use the lasso API 😅 just use bounds::min_max and isect_point_poly_v2 directly (maybe wrapped in some static function).

Please don't use the `lasso` API 😅 just use `bounds::min_max` and `isect_point_poly_v2` directly (maybe wrapped in some static function).
ChengduLittleA marked this conversation as resolved
@ -0,0 +195,4 @@
execute_tint_on_drawing(info.layer_index, info.frame_number, info.drawing);
});
if (changed.load()) {
Member

Don't think you need .load()

Don't think you need `.load()`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-12 15:13:56 +01:00
YimingWu changed title from GPv3: Tint tool to WIP: GPv3: Tint tool 2024-03-12 15:33:08 +01:00
YimingWu added 1 commit 2024-03-12 16:00:07 +01:00
Falk David requested changes 2024-03-12 16:06:32 +01:00
Dismissed
@ -0,0 +167,4 @@
screen_space_positions[local_point + points_by_curve[curve].first()]);
}
rcti rect;
BLI_lasso_boundbox(&rect, screen_space_positions_int);
Member

This shouldn't use the lasso API. There is also no need to convert to int2.
You can use bounds::min_max(screen_space_positions) to get the Bounds<float2> of the stroke.
And you can write a local helper function to check if the mouse position is inside the stroke/bounding box:
Use isect_point_poly_v2(pt, reinterpret_cast<const float(*)[2]>(positions.data()), uint(positions.size())) to test if the point is inside the stroke.

This shouldn't use the `lasso` API. There is also no need to convert to `int2`. You can use `bounds::min_max(screen_space_positions)` to get the `Bounds<float2>` of the stroke. And you can write a local helper function to check if the mouse position is inside the stroke/bounding box: Use `isect_point_poly_v2(pt, reinterpret_cast<const float(*)[2]>(positions.data()), uint(positions.size()))` to test if the point is inside the stroke.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-12 16:23:01 +01:00
Falk David requested changes 2024-03-12 16:29:29 +01:00
Dismissed
Falk David left a comment
Member

Looks close now! Added some more comments.

Looks close now! Added some more comments.
@ -53,2 +53,4 @@
operation = greasepencil::new_erase_operation().release();
break;
case GPAINT_TOOL_TINT:
operation = greasepencil::new_tint_operation().release();
Member

add a break;

add a `break;`
ChengduLittleA marked this conversation as resolved
@ -0,0 +59,4 @@
BKE_curvemapping_init(brush->gpencil_settings->curve_strength);
this->radius_ = BKE_brush_size_get(scene, brush);
Member

You don't need to use this-> when the variable as a trailing _ :)

You don't need to use `this->` when the variable as a trailing `_` :)
ChengduLittleA marked this conversation as resolved
@ -0,0 +137,4 @@
bke::AttrDomain::Curve);
OffsetIndices<int> points_by_curve = strokes.points_by_curve();
auto point_inside_stroke = [&](Span<float2> points, float2 mouse) {
Member

const Span<float2> points and const float2 mouse

`const Span<float2> points` and `const float2 mouse`
ChengduLittleA marked this conversation as resolved
@ -0,0 +143,4 @@
return false;
}
Bounds<float2> &box = bbox.value();
if (mouse[0] < box.min[0] || mouse[0] > box.max[0] || mouse[1] < box.min[1] ||
Member

use .x for [0] and .y for [1]

use `.x` for `[0]` and `.y` for `[1]`
ChengduLittleA marked this conversation as resolved
Sietse Brouwer reviewed 2024-03-12 16:49:18 +01:00
Sietse Brouwer left a comment
Member

A few remarks on brush falloff, among others.

A few remarks on brush falloff, among others.
@ -0,0 +90,4 @@
radius *= extension_sample.pressure;
}
if (BKE_brush_use_alpha_pressure(brush)) {
strength *= BKE_curvemapping_evaluateF(
Member

The brush falloff should only be used once (see below), when you calculate the influence.
So this has to be a plain strength *= extension_sample.pressure.

The brush falloff should only be used once (see below), when you calculate the influence. So this has to be a plain `strength *= extension_sample.pressure`.
SietseB marked this conversation as resolved
@ -0,0 +159,4 @@
if (tint_strokes) {
const int point = curve_point + points_by_curve[curve].first();
const float distance = math::distance(screen_space_positions[point], mouse_position);
const float influence = strength * BKE_brush_curve_strength(brush, distance, radius);
Member

BKE_brush_curve_strength is using brush->curve for the falloff and that is not the curve you want. So here you have to use something like BKE_curvemapping_evaluateF(brush->gpencil_settings->curve_strength, 0.0f, distance / radius). But avoid division by zero, of course.

`BKE_brush_curve_strength` is using `brush->curve` for the falloff and that is not the curve you want. So here you have to use something like `BKE_curvemapping_evaluateF(brush->gpencil_settings->curve_strength, 0.0f, distance / radius)`. But avoid division by zero, of course.
Author
Member

Eh apparently the old tint tool uses BKE_brush_curve_strength... This indeed appears to be the falloff of the brush tho, I guess curve_strength is a different thing that maps pressure with a curve?

Eh apparently the old tint tool uses `BKE_brush_curve_strength`... This indeed appears to be the falloff of the brush tho, I guess `curve_strength` is a different thing that maps pressure with a curve?
Member

I'm sorry, you're right! I bothered you with wrong comments!
It's confusing, because there are 3 falloff curves at play here:
brush->gpencil_settings->curve_sensitivity, must be applied to the radius when BKE_brush_use_size_pressure(brush) is true.
brush->gpencil_settings->curve_strength, must be applied to the strength when BKE_brush_use_alpha_pressure(brush) is true (like you have it now).
brush->curve, the falloff curve for the influence, used by BKE_brush_curve_strength().

So the curve init in on_stroke_begin
BKE_curvemapping_init(brush->gpencil_settings->curve_strength);
should be
BKE_curvemapping_init(brush->curve);
The other curves are already set in BKE_brush_init_gpencil_settings().

And the weird thing is: the brush->curve falloff isn't shown in the UI. So the user can't know what the falloff is, nor adjust it.
I think it would be good to expose that in the toolbar.

I'm sorry, you're right! I bothered you with wrong comments! It's confusing, because there are 3 falloff curves at play here: `brush->gpencil_settings->curve_sensitivity`, must be applied to the radius when `BKE_brush_use_size_pressure(brush)` is true. `brush->gpencil_settings->curve_strength`, must be applied to the strength when `BKE_brush_use_alpha_pressure(brush)` is true (like you have it now). `brush->curve`, the falloff curve for the influence, used by `BKE_brush_curve_strength()`. So the curve init in `on_stroke_begin` `BKE_curvemapping_init(brush->gpencil_settings->curve_strength);` should be `BKE_curvemapping_init(brush->curve);` The other curves are already set in `BKE_brush_init_gpencil_settings()`. And the weird thing is: the `brush->curve` falloff isn't shown in the UI. So the user can't know what the falloff is, nor adjust it. I think it would be good to expose that in the toolbar.
Member

the brush->curve falloff isn't shown in the UI.

That seems like a bug yea..

> the `brush->curve` falloff isn't shown in the UI. That seems like a bug yea..
Author
Member

brush->gpencil_settings->curve_sensitivity, must be applied to the radius when BKE_brush_use_size_pressure(brush) is true.

I see. Thanks for the hint.

So the curve init in on_stroke_begin
BKE_curvemapping_init(brush->gpencil_settings->curve_strength);
should be
BKE_curvemapping_init(brush->curve);

Good point. Will fix this.

the brush->curve falloff isn't shown in the UI.

Right we probably need to get the UI working too.

> brush->gpencil_settings->curve_sensitivity, must be applied to the radius when BKE_brush_use_size_pressure(brush) is true. I see. Thanks for the hint. > So the curve init in on_stroke_begin > BKE_curvemapping_init(brush->gpencil_settings->curve_strength); > should be > BKE_curvemapping_init(brush->curve); Good point. Will fix this. > the brush->curve falloff isn't shown in the UI. Right we probably need to get the UI working too.
Author
Member

UI is added, falloff/smoothing/jitter also working.

UI is added, falloff/smoothing/jitter also working.
ChengduLittleA marked this conversation as resolved
@ -0,0 +162,4 @@
const float influence = strength * BKE_brush_curve_strength(brush, distance, radius);
if (influence > 0.0f) {
stroke_touched = true;
vertex_colors[point].premultiply_alpha();
Member

.premultiply_alpha() returns the new color, it isn't changing the existing one. So this statement is doing nothing at the moment.

`.premultiply_alpha()` returns the new color, it isn't changing the existing one. So this statement is doing nothing at the moment.
ChengduLittleA marked this conversation as resolved
@ -0,0 +202,4 @@
Vector<ed::greasepencil::MutableDrawingInfo> drawings;
if (this->active_layer_only_) {
/* Tint only on the drawing at of the active layer. */
Member

Nitpick: /* Tint only on the drawings of the active layer. */

Nitpick: `/* Tint only on the drawings of the active layer. */`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-13 02:27:47 +01:00
YimingWu added 1 commit 2024-03-13 02:28:17 +01:00
YimingWu added 2 commits 2024-03-13 12:12:07 +01:00
YimingWu added 2 commits 2024-03-14 10:13:31 +01:00
YimingWu added 1 commit 2024-03-20 08:26:16 +01:00
YimingWu added 1 commit 2024-03-20 12:06:05 +01:00
YimingWu added 1 commit 2024-03-20 13:20:54 +01:00
YimingWu changed title from WIP: GPv3: Tint tool to GPv3: Tint tool 2024-03-20 13:21:21 +01:00
Sietse Brouwer reviewed 2024-03-20 22:30:48 +01:00
Sietse Brouwer left a comment
Member

I would still say it's far more efficient to do the computation of screen space positions only once, in on_stroke_begin. The tint tool is deliberately slow (due to the 'attenuate factor') to give the user fine control over the tinting. It means that the on_stroke_extended while be fired often. So: a drawing of 500 points, 20 times on_stroke_extended -> already 10.000 screen space calculations. It adds up pretty quickly.

Easy to implement, e.g. with an Array<MutableDrawingInfo> drawings_ and corresponding Array<Array<float2>> screen_positions_per_drawing_. In on_stroke_extended loop over the drawing indices with threading::parallel_for_each(drawings_.index_range(), ...) and that's more or less it...

I would still say it's far more efficient to do the computation of screen space positions only once, in `on_stroke_begin`. The tint tool is deliberately slow (due to the 'attenuate factor') to give the user fine control over the tinting. It means that the `on_stroke_extended` while be fired often. So: a drawing of 500 points, 20 times `on_stroke_extended` -> already 10.000 screen space calculations. It adds up pretty quickly. Easy to implement, e.g. with an `Array<MutableDrawingInfo> drawings_` and corresponding `Array<Array<float2>> screen_positions_per_drawing_`. In `on_stroke_extended` loop over the drawing indices with `threading::parallel_for_each(drawings_.index_range(), ...)` and that's more or less it...
@ -0,0 +173,4 @@
premul_to_straight_v4_v4(vertex_colors[point], rgba);
}
}
if (!fill_colors.span.is_empty() && tint_fills) {
Member

This block should be outside the for (const int curve_point...) loop. Now it is doing the same inside-the-curve check for every point of that curve. That's a bit inefficient 😉 And it has an odd side effect: curves with many points would tint much faster than curves with only a few points...

This block should be outside the `for (const int curve_point...)` loop. Now it is doing the same inside-the-curve check for every point of that curve. That's a bit inefficient 😉 And it has an odd side effect: curves with many points would tint much faster than curves with only a few points...
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-21 09:03:28 +01:00
YimingWu added 1 commit 2024-03-21 09:37:59 +01:00
Sietse Brouwer reviewed 2024-03-21 13:56:04 +01:00
Sietse Brouwer left a comment
Member

Sorry I keep bothering you 😺 A small thing about the threaded looping over drawings_.
And now I will be silent 😉

Sorry I keep bothering you 😺 A small thing about the threaded looping over `drawings_`. And now I will be silent 😉
@ -0,0 +101,4 @@
screen_positions_per_drawing_.reinitialize(drawings_.size());
threading::parallel_for(drawings_.index_range(), 128, [&](const IndexRange range) {
Member

A grain size of 128 on drawings isn't very useful (who has more than 128 layers or selected keyframes?). I'd say it's better to have something like you had before:
threading::parallel_for_each(drawings_.index_range(), [&](const int drawing_index) {

A grain size of 128 on drawings isn't very useful (who has more than 128 layers or selected keyframes?). I'd say it's better to have something like you had before: `threading::parallel_for_each(drawings_.index_range(), [&](const int drawing_index) {`
ChengduLittleA marked this conversation as resolved
@ -0,0 +240,4 @@
fill_colors.finish();
};
threading::parallel_for(drawings_.index_range(), 128, [&](const IndexRange range) {
Member

Same as above.

Same as above.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-21 14:04:52 +01:00
Falk David requested changes 2024-03-28 13:50:59 +01:00
Dismissed
Falk David left a comment
Member

I'm getting a crash in BKE_curvemapping_evaluateF. I think the problem is that there is a missing

BKE_curvemapping_init(brush->gpencil_settings->curve_sensitivity);
BKE_curvemapping_init(brush->gpencil_settings->curve_strength);

in the on_stroke_begin. See PaintOperation::on_stroke_begin.

I'm getting a crash in `BKE_curvemapping_evaluateF`. I think the problem is that there is a missing ```cpp BKE_curvemapping_init(brush->gpencil_settings->curve_sensitivity); BKE_curvemapping_init(brush->gpencil_settings->curve_strength); ``` in the `on_stroke_begin`. See `PaintOperation::on_stroke_begin`.
YimingWu added 2 commits 2024-03-29 07:23:11 +01:00
Author
Member

Still looking at a problem where fill_colors.span[curve] will have initial value of (1,1,1,1), this is the reason that makes the fill tint jump to white initially and then shift to actual tint color.

Still looking at a problem where `fill_colors.span[curve]` will have initial value of `(1,1,1,1)`, this is the reason that makes the fill tint jump to white initially and then shift to actual tint color.
YimingWu added 1 commit 2024-03-29 12:04:49 +01:00
Falk David requested changes 2024-03-29 12:14:03 +01:00
Dismissed
Falk David left a comment
Member

Got one more comment. Tested the tool again and it seem to work well :)

Got one more comment. Tested the tool again and it seem to work well :)
@ -1443,1 +1443,4 @@
layout.prop(gp_settings, "use_active_layer_only")
elif grease_pencil_tool == 'TINT':
layout.prop(gp_settings, "vertex_mode", text="Mode")
layout.popover("VIEW3D_PT_tools_brush_settings_advanced", text="Brush")
Member

All these popovers seem to not be needed. They aren't there in GPv2

All these popovers seem to not be needed. They aren't there in GPv2
Author
Member

Ah OK I'll remove the extra ones, but keep the "Mode" menu here since that one was indeed there.

Ah OK I'll remove the extra ones, but keep the "Mode" menu here since that one was indeed there.
Member

The mode is needed yes :)

The mode is needed yes :)
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-03-29 12:19:01 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
268a73f5cb
Remove extra U popovers
Member

@blender-bot build

@blender-bot build
Falk David approved these changes 2024-03-29 12:42:46 +01:00
Falk David merged commit 127f879be9 into main 2024-03-29 12:43:24 +01:00
Falk David referenced this issue from a commit 2024-03-29 12:43:25 +01:00
Sign in to join this conversation.
No reviewers
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
3 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#119323
No description provided.