GPv3: Tint tool #119323
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119323
Loading…
Reference in New Issue
No description provided.
Delete Branch "ChengduLittleA/blender:gp3-tint"
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?
WIP of GPv3 Tint tool.
Stroke/Fill tinting working as intended. UI is also working.
@ -0,0 +184,4 @@
return;
}
execute_tint_on_drawing(active_layer.drawing_index_at(scene->r.cfra), scene->r.cfra, *drawing);
I think this is not correct. You should pass the layer index, not the drawing index here. Use
grease_pencil.get_layer_index()
Interesting... In
grease_pencil_erase.cc
line 817 they wrote this way too, then it's probably a mistake as well?It is! Will push a fix.
Fixed for the eraser
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;
this->strength = BKE_brush_alpha_get(scene, brush);
@ -0,0 +94,4 @@
float radius = self.radius;
float strength = self.strength;
if (BKE_brush_use_size_pressure(brush)) {
radius *= BKE_curvemapping_evaluateF(
This should be
radius *= extension_sample.pressure
. The falloff curve is affecting the strength (brush influence), not the brush radius.@ -0,0 +120,4 @@
bke::crazyspace::get_evaluated_grease_pencil_drawing_deformation(
ob_eval, *obact, layer_index, frame_number);
/* Compute screen space positions. */
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.Oh actually since the
extend_stroke
could be called on different drawings at a time, I'll probably would keep it there.@ -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(
In curves code this is commonly used:
I would propose to stick to that, for uniformity.
I copied this from the erase tool. Will check.
@ -0,0 +173,4 @@
}
}
});
fill_colors.finish();
Should be outside the
if
block.@ -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);
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 applyexecute_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.@filedescriptor @amelief the eraser tool could use the same change I guess?
Right I think that was written at a time where muti-frame editing didn't exist. Will look at this separately.
You can use this file to test directly
Cleanup comments
@ -609,3 +603,4 @@
brush_basic_grease_pencil_paint_settings,
)
if grease_pencil_tool == 'DRAW':
Looks like this change is unrelated. I'll clean it up later.
@ -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;
These should be
private
and have a trailing underscore. See https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names.@ -0,0 +68,4 @@
this->color = ColorGeometry4f(color_linear);
}
/* From BKE_gpencil_legacy.h.*/
Comment can be removed with changes below.
@ -0,0 +70,4 @@
/* From BKE_gpencil_legacy.h.*/
#define TINT_VERTEX_COLOR_STROKE(brush) \
No need for
#define
. just put them in aconst bool
in the function at the top.@ -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,
execute_tint
would be better to be consistent with other operations@ -0,0 +92,4 @@
Brush *brush = BKE_paint_brush(paint);
/* Get the tool's data. */
float2 mouse_position = extension_sample.mouse_position;
const
@ -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. */
This comment is not right
@ -0,0 +119,4 @@
const Layer &layer = *grease_pencil.layers()[layer_index];
bke::CurvesGeometry &strokes = drawing.strokes_for_write();
/* Evaluated geometry. */
This comment is not right, no geometry is getting evaluated.
WIP: GPv3: Tint toolto GPv3: Tint toolMore cleanup comments.
@ -0,0 +39,4 @@
void on_stroke_done(const bContext &C) override;
private:
float _radius;
Trailing underscore means ->
radius_
. Same for the other ones.@ -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) ||
No need for
(
)
aroundbrush
.@ -0,0 +159,4 @@
stroke_changed = true;
}
}
if (!fill_colors.span.is_empty() && stroke_changed && tint_fills) {
Why should this check for
stroke_changed
?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.That doesn't seem right.. The fill should be changed when the brush intersects on of the fill triangles
Looks like indeed. I'll try make it to work. Does GPv3 has utility functions that checks on this like GPv2 with pbuffer stuff?
You can use
isect_point_poly_v2
and pass the 2d points of the stroke into itLooks like I could use
BLI_lasso_boundbox
andBLI_lasso_is_point_inside
like the old tool. Doing that now.Please don't use the
lasso
API 😅 just usebounds::min_max
andisect_point_poly_v2
directly (maybe wrapped in some static function).@ -0,0 +195,4 @@
execute_tint_on_drawing(info.layer_index, info.frame_number, info.drawing);
});
if (changed.load()) {
Don't think you need
.load()
GPv3: Tint toolto WIP: GPv3: Tint tool@ -0,0 +167,4 @@
screen_space_positions[local_point + points_by_curve[curve].first()]);
}
rcti rect;
BLI_lasso_boundbox(&rect, screen_space_positions_int);
This shouldn't use the
lasso
API. There is also no need to convert toint2
.You can use
bounds::min_max(screen_space_positions)
to get theBounds<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.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();
add a
break;
@ -0,0 +59,4 @@
BKE_curvemapping_init(brush->gpencil_settings->curve_strength);
this->radius_ = BKE_brush_size_get(scene, brush);
You don't need to use
this->
when the variable as a trailing_
:)@ -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) {
const Span<float2> points
andconst float2 mouse
@ -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] ||
use
.x
for[0]
and.y
for[1]
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(
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
.@ -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);
BKE_brush_curve_strength
is usingbrush->curve
for the falloff and that is not the curve you want. So here you have to use something likeBKE_curvemapping_evaluateF(brush->gpencil_settings->curve_strength, 0.0f, distance / radius)
. But avoid division by zero, of course.Eh apparently the old tint tool uses
BKE_brush_curve_strength
... This indeed appears to be the falloff of the brush tho, I guesscurve_strength
is a different thing that maps pressure with a curve?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 whenBKE_brush_use_size_pressure(brush)
is true.brush->gpencil_settings->curve_strength
, must be applied to the strength whenBKE_brush_use_alpha_pressure(brush)
is true (like you have it now).brush->curve
, the falloff curve for the influence, used byBKE_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.
That seems like a bug yea..
I see. Thanks for the hint.
Good point. Will fix this.
Right we probably need to get the UI working too.
UI is added, falloff/smoothing/jitter also working.
@ -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();
.premultiply_alpha()
returns the new color, it isn't changing the existing one. So this statement is doing nothing at the moment.@ -0,0 +202,4 @@
Vector<ed::greasepencil::MutableDrawingInfo> drawings;
if (this->active_layer_only_) {
/* Tint only on the drawing at of the active layer. */
Nitpick:
/* Tint only on the drawings of the active layer. */
WIP: GPv3: Tint toolto GPv3: Tint toolI 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 theon_stroke_extended
while be fired often. So: a drawing of 500 points, 20 timeson_stroke_extended
-> already 10.000 screen space calculations. It adds up pretty quickly.Easy to implement, e.g. with an
Array<MutableDrawingInfo> drawings_
and correspondingArray<Array<float2>> screen_positions_per_drawing_
. Inon_stroke_extended
loop over the drawing indices withthreading::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) {
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...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) {
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) {
@ -0,0 +240,4 @@
fill_colors.finish();
};
threading::parallel_for(drawings_.index_range(), 128, [&](const IndexRange range) {
Same as above.
I'm getting a crash in
BKE_curvemapping_evaluateF
. I think the problem is that there is a missingin the
on_stroke_begin
. SeePaintOperation::on_stroke_begin
.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.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")
All these popovers seem to not be needed. They aren't there in GPv2
Ah OK I'll remove the extra ones, but keep the "Mode" menu here since that one was indeed there.
The mode is needed yes :)
@blender-bot build