blender
/
blender
137
426
Fork 612
Code Issues 6.2k Pull Requests 779 Projects 19 Wiki Activity

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
Conversation 3 Commits 33 Files Changed 7 +46 -26
1 changed files with 46 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3b940b1047 - Show all commits

72
source/blender/editors/sculpt_paint/grease_pencil_tint.cc
View File

@ -10,9 +10,11 @@
#include "BKE_grease_pencil.hh"
#include "BKE_material.h"
#include "BLI_lasso_2d.hh"
#include "BLI_length_parameterize.hh"
#include "BLI_math_color.h"
#include "BLI_math_geom.h"
#include "BLI_utildefines.h"
#include "DEG_depsgraph_query.hh"
@ -20,7 +22,7 @@
#include "ED_grease_pencil.hh"
#include "ED_view3d.hh"
#include "GEO_smooth_curves.hh"
#include "UI_view2d.hh"
#include "WM_api.hh"
#include "WM_types.hh"
@ -96,7 +98,7 @@ void TintOperation::execute_tint(const bContext &C, const InputSample &extension
}
/* Attenuate factor to get a smoother tinting. */
strength /= 5.0f;
float fill_strength = strength / 10.0f;
float fill_strength = strength / 1000.0f;
strength = math::clamp(strength, 0.0f, 1.0f);
fill_strength = math::clamp(fill_strength, 0.0f, 1.0f);
ChengduLittleA marked this conversation as resolved Outdated
Falk David commented 2024-03-12 14:51:28 +01:00
Outdated
Review
Copy Link

No need for ( ) around brush.

No need for `(` `)` around `brush`.
Sietse Brouwer commented 2024-03-21 13:52:16 +01:00
Outdated
Review
Copy Link

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) {`
@ -142,32 +144,50 @@ void TintOperation::execute_tint(const bContext &C, const InputSample &extension
for (const int curve : range) {
bool stroke_touched = false;
for (const int curve_point : points_by_curve[curve].index_range()) {
ChengduLittleA marked this conversation as resolved Outdated
Falk David commented 2024-03-12 16:26:41 +01:00
Outdated
Review
Copy Link

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

use `.x` for `[0]` and `.y` for `[1]`
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);
if (influence <= 0.0f) {
continue;
}
stroke_touched = true;
if (tint_strokes) {
vertex_colors[point].premultiply_alpha();
float4 rgba = float4(
math::interpolate(float3(vertex_colors[point]), float3(this->color_), influence),
vertex_colors[point][3]);
rgba[3] = rgba[3] * (1.0f - influence) + influence;
vertex_colors[point] = ColorGeometry4f(rgba);
vertex_colors[point].unpremultiply_alpha();
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);
if (influence > 0.0f) {
stroke_touched = true;
vertex_colors[point].premultiply_alpha();
float4 rgba = float4(
math::interpolate(float3(vertex_colors[point]), float3(this->color_), influence),
vertex_colors[point][3]);
rgba[3] = rgba[3] * (1.0f - influence) + influence;
vertex_colors[point] = ColorGeometry4f(rgba);
vertex_colors[point].unpremultiply_alpha();
}
}
if (!fill_colors.span.is_empty() && tint_fills) {
ChengduLittleA marked this conversation as resolved Outdated
Falk David commented 2024-03-12 14:52:09 +01:00
Outdated
Review
Copy Link

Why should this check for stroke_changed ?

Why should this check for `stroke_changed` ?
YimingWu commented 2024-03-12 15:11:09 +01:00
Outdated
Review
Copy Link

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.
Falk David commented 2024-03-12 15:14:36 +01:00
Outdated
Review
Copy Link

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
YimingWu commented 2024-03-12 15:32:56 +01:00
Outdated
Review
Copy Link

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?
Falk David commented 2024-03-12 15:39:47 +01:00
Outdated
Review
Copy Link

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
YimingWu commented 2024-03-12 15:46:18 +01:00
Outdated
Review
Copy Link

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.
Falk David commented 2024-03-12 15:51:18 +01:00
Outdated
Review
Copy Link

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).
Sietse Brouwer commented 2024-03-12 16:46:16 +01:00
Outdated
Review
Copy Link

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.
YimingWu commented 2024-03-13 02:23:55 +01:00
Outdated
Review
Copy Link

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?
Sietse Brouwer commented 2024-03-13 11:57:49 +01:00
Outdated
Review
Copy Link

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.
Falk David commented 2024-03-13 11:59:49 +01:00
Outdated
Review
Copy Link

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..
YimingWu commented 2024-03-13 12:10:28 +01:00
Outdated
Review
Copy Link

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.
YimingWu commented 2024-03-20 12:08:40 +01:00
Outdated
Review
Copy Link

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

UI is added, falloff/smoothing/jitter also working.
blender::Array<blender::int2> screen_space_positions_int(
points_by_curve[curve].size());
for (const int local_point : points_by_curve[curve].index_range()) {
ChengduLittleA marked this conversation as resolved Outdated
Sietse Brouwer commented 2024-03-12 16:41:46 +01:00
Outdated
Review
Copy Link

.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.
screen_space_positions_int[local_point] = int2(
screen_space_positions[local_point + points_by_curve[curve].first()]);
}
rcti rect;
BLI_lasso_boundbox(&rect, screen_space_positions_int);
ChengduLittleA marked this conversation as resolved Outdated
Falk David commented 2024-03-12 16:06:30 +01:00
Outdated
Review
Copy Link

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.
/* Will tint fill color when either the brush being inside the fill region or touching
* the stroke. */
const bool fill_effective =
stroke_touched ||
(!ELEM(V2D_IS_CLIPPED, mouse_position[0], mouse_position[1]) &&
BLI_rcti_isect_pt(&rect, mouse_position[0], mouse_position[1]) &&
ChengduLittleA marked this conversation as resolved Outdated
Sietse Brouwer commented 2024-03-11 23:54:25 +01:00
Outdated
Review
Copy Link

Should be outside the if block.

Should be outside the `if` block.
Sietse Brouwer commented 2024-03-20 21:53:25 +01:00
Outdated
Review
Copy Link

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...
BLI_lasso_is_point_inside(
screen_space_positions_int, mouse_position[0], mouse_position[1], INT_MAX));
if (fill_effective) {
fill_colors.span[curve].premultiply_alpha();
float4 rgba = float4(math::interpolate(float3(fill_colors.span[curve]),
float3(this->color_),
fill_strength),
fill_colors.span[curve][3]);
rgba[3] = rgba[3] * (1.0f - fill_strength) + fill_strength;
fill_colors.span[curve] = ColorGeometry4f(rgba);
fill_colors.span[curve].unpremultiply_alpha();
ChengduLittleA marked this conversation as resolved Outdated
Falk David commented 2024-03-11 14:31:37 +01:00
Outdated
Review
Copy Link

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()`
YimingWu commented 2024-03-11 14:34:49 +01:00
Outdated
Review
Copy Link

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?
Falk David commented 2024-03-11 14:40:30 +01:00
Outdated
Review
Copy Link

It is! Will push a fix.

It is! Will push a fix.
Falk David commented 2024-03-11 15:14:55 +01:00
Outdated
Review
Copy Link

Fixed for the eraser

Fixed for the eraser
stroke_touched = true;
Sietse Brouwer commented 2024-03-12 00:03:05 +01:00
Outdated
Review
Copy Link

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.
YimingWu commented 2024-03-12 14:25:44 +01:00
Outdated
Review
Copy Link

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

@filedescriptor @amelief the eraser tool could use the same change I guess?
Falk David commented 2024-03-12 14:56:19 +01:00
Outdated
Review
Copy Link

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.
}
}
}
if (!fill_colors.span.is_empty() && stroke_touched && tint_fills) {
fill_colors.span[curve].premultiply_alpha();
float4 rgba = float4(math::interpolate(float3(fill_colors.span[curve]),
float3(this->color_),
fill_strength),
fill_colors.span[curve][3]);
rgba[3] = rgba[3] * (1.0f - fill_strength) + fill_strength;
fill_colors.span[curve] = ColorGeometry4f(rgba);
fill_colors.span[curve].unpremultiply_alpha();
}
if (stroke_touched) {
changed.store(true, std::memory_order_relaxed);