GPv3: Initial drawing tool #110093
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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#110093
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "filedescriptor/blender:gpv3-drawing-operator"
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?
This PR implements an initial drawing tool with the following implemented features:
It also changes some things in the UI to make the drawing process more usable.
How to use it:
You will need to download a build and the startup file. Make sure to enable Grease Pencil v3
Preferences
>Interface
> checkDeveloper Extras
, thenExperimental
> checkGrease Pencil 3.0
. Restart Blender.add144c6f0
to9dde8af4c7
bfbc6c47c5
tode42110dd4
fec6f4ee8f
to1064dd03bd
1064dd03bd
tobdc3a95341
@blender-bot package
Package build started. Download here when ready.
4100146739
tocc2175f63e
@blender-bot package
Package build started. Download here when ready.
Test file: startup_gpv3.blend
WIP: GPv3: Initial drawing toolto GPv3: Initial drawing tool@blender-bot package
Package build started. Download here when ready.
0ad79de634
tod21034c364
d21034c364
toff5cebbb56
ff5cebbb56
to73e2248c5d
cfa0ae0629
toc7eaddbf48
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
7471e25306
to3d79364b22
@blender-bot package
Package build started. Download here when ready.
5831301dda
tod9e759b26a
d9e759b26a
to27ea06c1a8
I wasn't so thorough this time, but here are some high level comments:
StrokeCache
seems redundant-- just storing this as aCurvesGeometry
would be simpler and would make code more generic and easier to extend in the futureblender::length_parameterize
that looks similar to some of the stuff here and is hopefully reusable.@ -476,0 +475,4 @@
[positions, radii](int64_t first_index, int64_t last_index, int64_t index) {
const float dist_position = dist_to_line_v3(
positions[index], positions[first_index], positions[last_index]);
/* We devide the distance by 2000.0f to convert from "pixels" to an actual distance.
devide
->divide
storkes
->strokes
?@ -0,0 +28,4 @@
if (align_flag & GP_PROJECT_CURSOR) {
return float3(scene->cursor.location);
}
else {
else after return
@ -0,0 +62,4 @@
/* Use an (arbitrary) screen space offset in the x direction to measure the size. */
const int x_offest = 64;
const float brush_size = static_cast<float>(BKE_brush_size_get(scene, brush));
static_cast<float>(...)
->float(...)
@ -128,0 +146,4 @@
BKE_report(op->reports, RPT_ERROR, "No Grease Pencil frame to draw on");
return OPERATOR_CANCELLED;
}
else {
else after return
@ -22,0 +30,4 @@
static constexpr int64_t STOKE_CACHE_ALLOCATION_CHUNK_SIZE = 1024;
/** Forward differencing of a bezier curve segment (q0,q1,q2,q3). Steps are written to \a r_p.
Rather than re-implementing this, it probably makes sense to make
bke::curves::evaluate_segment
into a template and add explicit instantiations of the float2 and float3 types in the cc file.@ -22,0 +76,4 @@
}
return points;
}
Didn't have a ton of time to dig into this function, but on the surface it looks like it has the same goal as the code in
blender::length_parameterize
. If so, I'd really hope that could be reused, since this stuff is pretty fiddly, especially with floating point error (though it looks like you're doing a good job dealing with that here :)@ -58,0 +255,4 @@
const float max_radius_px = 30.0f;
const int64_t max_samples = 64;
const float angle_threshold = 0.6f;
IndexMask corner_mask = blender::ed::greasepencil::polyline_detect_corners(
Unnecessary
blender::
?27ea06c1a8
to30dd25aefa
08caa165e8
to1508ad175b
@HooglyBoogly About switching to use
CurvesGeometry
. I somewhat agree, and I already started a refactor for this. But here are some pain-points:stroke_buffer
on every drawing update. I was planning on reducing the number of reallocations by usingblender::Vector::reserve
to reserve memory beforehand (say 1024 elements). Ideally, for smaller strokes, we don't need to reallocate ever. I'm not sure how this will work withcurves.resize
and calling that every update.VArrays
all the time makes the code much less readable IMHO.1508ad175b
to04826e6917
04826e6917
tob4891dba8a
I think I'd still push for using
CurvesGeometry
here, instead of another data structure. I think the future benefits of treating the currently drawn stroke as just another curves (drawing, geometry processing, etc) will make it worth it. To address the points you brought up:I haven't gotten back to refactoring this. Something else that I thought of yesterday is that it would be nice if we could do depsgraph updates while drawing. This would be slower and not really useful for normal drawing (just grease pencil), but if there are modifiers.
E.g. if there is some custom geo nodes curves->mesh "brush" then you wouldn't get live interaction at the moment, the result would pop up after the stroke is done.
I'd like to experiment with a "live" mode. But for that especially it would be nice to have some capacity on the
CurvesGeometry
. Since copying all of it everytime a new point is added would be very slow I presume.StrokeCache
3dfe779cd1Ok I refactored the code to completely remove
StrokeCache
and to draw into theCurvesGeometry
of the active drawing directly.This is probably less efficient (not that the previous code was really optimized) but I would prefer to do any optimizations in subsequent PRs.
There's definitely room to optimize this, but seems like the main logic is there. My one larger comment is about reusing the existing length parameterization code.
@ -22,0 +42,4 @@
const Span<float2> curve_segments = curve_points.drop_front(1).drop_back(1);
for (const int64_t segment_i : IndexRange(num_segments)) {
IndexRange segment_range(segment_i * resolution, resolution);
bke::curves::bezier::evaluate_segment(curve_segments[segment_i * 3 + 0],
Rather than evaluating a single segment at a time from here, how about giving the whole
curve_points
span tobke::curves::bezier
? That's generally aligned with the goal of that module, to handle more than one thing at a time wherever possible.If I am not mistaken that module currently only works with
float3
s.Pretty sure you're mistaken :P It's all templated. Even the tests use
float2
sometimes.I just added a
threading::parallel_for
for now.@ -22,0 +52,4 @@
}
/** Interpolate \a dst such that the points in \a dst lie evenly distributed on \a src. */
static void interp_polyline_to_polyline(Span<float2> src, MutableSpan<float2> dst)
This still looks like it has the same goal as the code in
BLI_length_parameterize.hh
. I'd really like that to be reused here, this is too much tricky logic to duplicate IMO.@ -58,0 +191,4 @@
return proj_point;
}
ScreenSpacePoint point_from_input_sample(const InputSample &sample)
Better not to mix this data together in a single struct IMO. It's clearer if the code is consistent about handling them separately. In practice, the way this function is used, the struct is just immediately unpacked anyway.
Since this code really does have to process just one point at a time, I'd suggest just separating radius, opacity, and colors into separate functions.
@ -65,0 +254,4 @@
const int64_t corner_max_samples = 64;
const float corner_angle_threshold = 0.6f;
IndexMaskMemory memory;
IndexMask corner_mask = ed::greasepencil::polyline_detect_corners(
IndexMask corner_mask
->const IndexMask corner_mask
@ -65,0 +353,4 @@
bke::CurvesGeometry &curves = drawing_->strokes_for_write();
float2 prev_co = self.screen_space_coordinates_.last();
Declare variables const
@ -65,0 +388,4 @@
new_coordinates[i] = bke::attribute_math::mix2<float2>(factor, prev_co, point.co);
new_radii[i] = bke::attribute_math::mix2<float>(factor, prev_radius, point.radius);
new_opacities[i] = bke::attribute_math::mix2<float>(factor, prev_opacity, point.opacity);
new_vertex_colors[i] = bke::attribute_math::mix2<ColorGeometry4f>(
Seems like it would be simpler to mix these directly into the new slice of the attribute.
@ -76,0 +491,4 @@
void PaintOperation::simplify_stroke(bke::greasepencil::Drawing &drawing, const float epsilon_px)
{
const int stroke_index = drawing.strokes().curves_range().last();
const IndexRange points_range = drawing.strokes().points_by_curve()[stroke_index];
points_range
->points
(the standard name for this sort of variable)@ -76,0 +498,4 @@
/* Distance function for `ramer_douglas_peucker_simplify`. */
const Span<float2> positions_2d = this->screen_space_smoothed_coordinates_.as_span();
const auto dist_function =
[points_range, positions_2d, radii](int64_t first_index, int64_t last_index, int64_t index) {
Use simple reference capture:
[&]
. It should amount to the same thing here.@ -76,0 +514,4 @@
return math::max(dist_position_px, dist_radii_px);
};
Array<bool> points_to_delete(curves.points_num(), false);
Feels important not to create arrays that are the size of the entire curves geometry, but it looks like that would require changes to
ramer_douglas_peucker_simplify
. Could be done later.@ -76,0 +532,4 @@
/* Remove points at the end that have a radius close to 0. */
int64_t points_to_remove = 0;
for (int64_t index = points_range.last(); index >= points_range.first(); index--) {
if (drawing.radii()[index] < 1e-5f) {
Put the
.radii()
in a span outside the loop.interp_polyline_to_polyline
6a57c99feeconst
4172c6ce00drawing->radii()
3b8b888017points_range
topoints
fc8765c08dScreenSpacePoint
struct 8bc9fc468cJust some cleanup stuff now.
@ -0,0 +82,4 @@
{
Array<int32_t> indices(corner_mask.size());
corner_mask.to_indices(indices.as_mutable_span());
uint *indicies_ptr = (corner_mask.size() > 0) ? reinterpret_cast<uint *>(indices.data()) :
uint *indicies_ptr = corner_mask.is_empty() ? nullptr : reinterpret_cast<uint *>(indices.data());
@ -0,0 +45,4 @@
float brush_radius_world_space(bContext &C, int x, int y)
{
using namespace blender;
This is unnecessary, the function is already in
blender::ed::greasepencil
@ -124,2 +126,4 @@
FunctionRef<float(int64_t, int64_t, int64_t)> dist_function,
MutableSpan<bool> dst);
Array<float2> fit_curve_polyline_2d(Span<float2> points,
Suggest renaming these two functions for consistency:
polyline_fit_curve
polyline_detect_corners
@ -126,0 +133,4 @@
IndexMask polyline_detect_corners(Span<float2> points,
float radius_min,
float radius_max,
int64_t samples_max,
int64_t
->int
?@ -22,0 +29,4 @@
static constexpr float POINT_OVERRIDE_THRESHOLD_PX = 3.0f;
static constexpr float POINT_RESAMPLE_MIN_DISTANCE_PX = 10.0f;
static constexpr int64_t STOKE_CACHE_ALLOCATION_CHUNK_SIZE = 1024;
STOKE_CACHE_ALLOCATION_CHUNK_SIZE
is unused@ -22,0 +32,4 @@
static constexpr int64_t STOKE_CACHE_ALLOCATION_CHUNK_SIZE = 1024;
/** Sample a bezier curve at a fixed resolution and return the sampled points in an array. */
static Array<float2> sample_curve_2d(Span<float2> curve_points, const int64_t resolution)
I'd suggest:
Span<float2> curve_points
->Span<float2> positions
@ -22,0 +41,4 @@
Array<float2> points(num_points);
const Span<float2> curve_segments = curve_points.drop_front(1).drop_back(1);
threading::parallel_for(IndexRange(num_segments), 512 * resolution, [&](const IndexRange range) {
512 * 32 is 16k, that's huge, large enough that multithreading will probably never be used. What about
32 * resolution
?@ -65,0 +258,4 @@
Array<float2> coords_smoothed(screen_space_coords_smooth_slice.size());
morph_points_to_curve(screen_space_coords_smooth_slice, sampled_curve_points, coords_smoothed);
MutableSpan<float2> smoothed_coordinates_slice =
I'd suggest consistently abbreviating "coordinates" to "coords"
@ -65,0 +334,4 @@
int new_points_num = 1;
const float distance_px = math::distance(position, prev_position);
if (distance_px > POINT_RESAMPLE_MIN_DISTANCE_PX) {
const int subdivisions = static_cast<int>(
static_cast<int>
->int(...)
Same for float
@ -65,0 +355,4 @@
const float step = 1.0f / static_cast<float>(new_points_num);
float factor = step;
for (const int64_t i : new_range.index_range()) {
new_screen_space_coordinates[i] = bke::attribute_math::mix2<float2>(
How about copying this from the subdivide curves node?
With the same name reused everywhere it's easy to extract to some common header later on.
@ -65,0 +383,4 @@
}
/* Active smoothing is done in a window at the end of the new stroke. */
const IndexRange smooth_window(self.active_smooth_index_,
points.index_range().drop_front(self.active_smooth_index_)
maybe?@ -125,2 +448,2 @@
SpanAttributeWriter<int> materials = attributes.lookup_or_add_for_write_span<int>(
"material_index", ATTR_DOMAIN_CURVE);
if (dist1 + dist2 > 1e-5f) {
const float interpolated_val = interpf(valB, valA, dist1 / (dist1 + dist2));
math::interpolate