GPv3: Cutter tool #113953
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#113953
Loading…
Reference in New Issue
No description provided.
Delete Branch "SietseB/blender:gpv3-cutter-tool"
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 the Cutter Tool for GPv3. The Cutter tool deletes
points in between intersecting strokes. New points are created at the
exact intersection points, so as a result the cutted strokes will fit
perfectly.
For feature parity, the tool follows the GPv2 behavior:
so intersection of curves on seperate layers are not handled.
Technical notes
The implementation uses the
compute_topology_change
functioncreated for the Hard Eraser. So at intersection points, point
attributes will be interpolated.
To do:
grease_pencil_stroke_cutter_exec
isn't called when you selectthe Cutter tool in the toolbar.
WIP: GPv3: Cutter toolto GPv3: Cutter tool@blender-bot build
First pass, this should fix the toolbar issue.
Some higher-level remarks:
EraseOperationExecutor
. It's not a painting operator, so I'd like to avoid mixing it in. I would suggest to put it ineditors/grease_pencil/
into it's own file (maybe justgrease_pencil_cutter.cc
).compute_topology_change
(correct me if I'm wrong). So I would refactor that out and also put it in theed::greasepencil
namespace.grease_pencil_utils.cc
seems like a good candidate.@ -1793,0 +1804,4 @@
label="Cutter",
icon="ops.gpencil.stroke_cutter",
cursor='KNIFE',
widget=None,
widget=None,
can be removed@ -254,0 +262,4 @@
Vector<MutableDrawingInfo> editable_drawings;
if (!grease_pencil.has_active_layer()) {
return editable_drawings.as_span();
return {};
@ -254,0 +267,4 @@
const Layer &layer = *grease_pencil.get_active_layer();
const std::optional<int> layer_index = grease_pencil.get_layer_index(layer);
if (!layer.is_editable() || !layer_index.has_value()) {
return editable_drawings.as_span();
return {};
@ -305,0 +313,4 @@
ot->invoke = WM_gesture_lasso_invoke;
ot->modal = WM_gesture_lasso_modal;
ot->exec = greasepencil::grease_pencil_stroke_cutter_exec;
ot->poll = ed::greasepencil::editable_grease_pencil_poll;
This needs to be
grease_pencil_painting_poll
.@HooglyBoogly I suspect that
compute_topology_change
could be refactored. It sort of does agather_attributes
, but some values in the destination are interpolated between two sources. Also handles changes to thecyclic
attribute and thecaps
.@filedescriptor Thanks a lot for fixing that tool-system mystery. It's working now.
I moved the operator into its own file in
editors/grease_pencil/
.Another pass
@ -0,0 +549,4 @@
/* Lambda function for executing the cutter on each drawing. */
const auto execute_cutter_on_drawing =
[&](const int layer_index, const int frame_number, Drawing &drawing) {
This lambda is getting long! I would consider putting the code into a static function.
@ -0,0 +570,4 @@
/* Compute bounding boxes of curves in screen space. The bounding boxes are used to speed
* up the search for intersecting curves. */
Array<rcti> screen_space_bbox(src.curves_num());
This should be using
Bounds<float2>
orBounds<int2>
.I surely want to, but it's a bit difficult at the moment. There are no alternatives for
BLI_rcti_isect
andBLI_rcti_isect_pt_v
yet in theBounds
type, as I understood it.Ah dang it, I thought that was merged! Ok no problem then.
@ -0,0 +589,4 @@
/* Apply cutter. */
bke::CurvesGeometry dst;
const bool cutted = stroke_cutter_find_and_remove_segments(
Return an
std::optional<bke::CurvesGeometry> cut_strokes
.@ -0,0 +592,4 @@
const bool cutted = stroke_cutter_find_and_remove_segments(
src, dst, mcoords, mcoords_len, screen_space_positions, screen_space_bbox, keep_caps);
if (cutted) {
if (cut_strokes.has_value())
@ -0,0 +596,4 @@
/* Set the new geometry. */
drawing.geometry.wrap() = std::move(dst);
drawing.tag_topology_changed();
changed = true;
Since we're running this in parallel, this should be using
std::atomic<bool> changed
@ -327,6 +352,7 @@ void ED_operatortypes_grease_pencil_draw()
using namespace blender::ed::sculpt_paint;
WM_operatortype_append(GREASE_PENCIL_OT_brush_stroke);
WM_operatortype_append(GREASE_PENCIL_OT_draw_mode_toggle);
WM_operatortype_append(GREASE_PENCIL_OT_stroke_cutter);
We don't need to append the operator here anymore. I suggest to put
GREASE_PENCIL_OT_stroke_cutter
in the same file (grease_pencil_cutter.cc
) and declare it inED_grease_pencil.hh
underneath the otherED_operatortypes_*
functions. Then you can put this lineWM_operatortype_append(GREASE_PENCIL_OT_stroke_cutter);
inED_operatortypes_grease_pencil()
.@ -34,6 +36,8 @@
namespace blender::ed::sculpt_paint::greasepencil {
using namespace blender::ed::greasepencil;
I think it's better to just write
ed::greasepencil::compute_topology_change(...);
. We're already doing this for e.g.ed::greasepencil::retrieve_editable_drawings
in this file.Tested it and it feels quite nice.
I noticed that GPv2 cutter only deletes one segment per stroke, while this implementation removes anything within the lasso (generally, some corner cases below). It seems more logical this way, so no need to change that IMO.
The segments it actually deletes seem to depend a lot on the actual shape of of the lasso. I think this is because it requires the lasso to contain actual points. If the lasso threads the needle between two points the segment does not count as covered by the lasso (see video). This seems to be more common with the more aggressive simplification in GPv3. I'm not sure how hard it would be to detect lasso intersections with line segments, maybe that can be a TODO.
Looks like there is an API for it. Instead of
BLI_lasso_is_point_inside
, it would beBLI_lasso_is_edge_inside
.But I agree that this is not crictical and could also be done later.
Quite a complex feature, and it works better than the GPv2 version! Just some minor nitpicks and some possible performance improvements. Not critical, but I'd like to confirm with someone who understands thread behavior better than me.
@ -0,0 +34,4 @@
*/
enum class SegmentRangeIndex : uint8_t { Start, End };
template<typename T> struct SegmentRange {
IndexRange
can be used instead of a custom range class. That also comes with a bunch of useful functions, e.g.contains
to check the range inpoint_is_in_segment
.Using
SegmentRange
for theintersection_distance
andis_intersected
flag doesn't add much value, these are not really ranges. Just start/end properties seems more appropriate.@ -0,0 +230,4 @@
BLI_rcti_pad(&bbox_ab, BBOX_PADDING, BBOX_PADDING);
/* Loop all curves, looking for intersecting segments. */
threading::parallel_for(src.curves_range(), 512, [&](const IndexRange curves) {
Second opinion would be welcome, but i think calling
parallel_for
here could have significant overhead. This function is called for every point, and then starts a threaded loop over all the points, and waits before moving to the next point. If my intuition is correct it would be faster to do the threading in the outer loop (stroke_cutter_find_and_remove_segments
) and use a simplefor
loop here.Profiler indicates a lot of time spent in
receive_or_steal_task
(65%). I guess doing threading in the outer loop is a more difficult because of howCutterSegments
gets extended and needs to be immutable while extending from a given point. Finding a way to parallelize this could be challenging. I won't mind leaving it as it is now and make a ToDo task, if this turns out to be too complicated.I think this can be made more parallelizable by doing the intersection tests up-front: There is essentially one intersection test per point. The
expand_cutter_segment
loop may get started multiple times for each point, but only do the intersection test the first time, then insert the point into a segment and skip subsequent checks. So the intersections can be detected for all the lasso'd strokes in advance, and this can be neatly parallelized.After that the building of segments is just linear, and can be parallelized in the stroke domain as well. No complicated data structure required.
@ -0,0 +245,4 @@
/* Find intersecting curve segments. */
for (const int point_c : points) {
if (!is_cyclic[curve] && point_c == points.last()) {
Can avoid this check inside the loop by modifying the range
@ -0,0 +352,4 @@
&distance_min,
&distance_max);
/* Avoid orphant points at the end of a curve. */
orphaned
@ -0,0 +389,4 @@
/* When a curve end is reached and the curve is cyclic, we add an extra cutter segment for the
* cyclic second part. */
if (check_cyclic && is_cyclic[curve] &&
It would be easier to understand if this last part for cyclic curves is moved out of the
expand_cutter_segment
function. Currently it looks at first glance like the function is recursive. Doing both calls for the non-cyclic and the cyclic part outside of the function is easier to understand i think.@ -0,0 +442,4 @@
const IndexRange src_points = src_points_by_curve[src_curve];
for (const int src_point : src_points) {
/* Skip point when it is already part of a cutter segment. */
if (cutter_segments.point_is_in_segment(src_curve, src_point)) {
I think it would be easier and more efficient to just have a per-point bool array to mark points in segments. We're not interested in which segment a point is part of, so just setting the flag when a point gets added to any segment should be sufficient.
The
point_is_in_segment
linear search is reasonably fast when there are few segments, but with dense drawings might degrade into a N^2 loop when each segment contains just a few points. I did a simple test with ~13k strokes and 100k points, and it spends a significant amount of time in this function.@ -0,0 +595,4 @@
});
/* Apply cutter. */
std::optional<bke::CurvesGeometry> cutted_strokes = stroke_cutter_find_and_remove_segments(
nit picking: past tense of "cut" is "cut", so
cut_strokes
I skipped the use of
BLI_lasso_is_edge_inside
for now, because it adds some complexity. Line segments touched by the lasso can be intersected by other curves.Handling cases like:
is doable, but not entirely trivial.
Something to save for a later date, I agree with that.
@ -0,0 +444,4 @@
}
/* Create new cutter segment. */
mutex.lock();
This is probably fine, but if you want to optimize it a little bit more you can use the
EnumerableThreadSpecific
data to construct segments without locking a mutex, then combine them after the threaded construction. (example).@blender-bot build
Have a few cleanup comments, but in general it looks very close!
@ -0,0 +156,4 @@
const float c2 = a2 * co_c[0] + b2 * co_c[1];
const float det = float(a1 * b2 - a2 * b1);
BLI_assert(det != 0.0f);
Are we sure that this can never happen? Would be better if this could return something if
det
is0.0f
, otherwise we just crash.We are sure (because the segments intersect), but I changed it anyway. You never know, in a leap year with full moon... 😉
@ -0,0 +158,4 @@
const float det = float(a1 * b2 - a2 * b1);
BLI_assert(det != 0.0f);
float2 isect;
Would prefer
const float2 isect((b2 * c1 - b1 * c2) / det, (a1 * c2 - a2 * c1) / det);
@ -0,0 +291,4 @@
const int8_t directions[2] = {-1, 1};
/* Walk along the curve in both directions. */
for (const int8_t direction : directions) {
Would be a bit nicer imo if the body of this loop was a function
expand_cutter_segment_direction
and you would just call it two times with-1
and1
.@ -0,0 +555,4 @@
}
/* Create the new curves geometry. */
bke::CurvesGeometry dst;
Hm this makes me think that
compute_topology_change
should probably return a newCurvesGeometry
. Could be done in a separate cleanup later.@ -0,0 +693,4 @@
WM_event_add_notifier(C, NC_GEOM | ND_DATA, &grease_pencil);
}
return (changed ? OPERATOR_FINISHED : OPERATOR_CANCELLED);
This should just return
OPERATOR_FINISHED
(even if no drawing was changed).@ -0,0 +704,4 @@
return OPERATOR_PASS_THROUGH;
}
const int result = stroke_cutter_execute(op, C, mcoords);
Just
return stroke_cutter_execute(op, C, mcoords);
is fine.@blender-bot build