GPv3: Hard Eraser tool #110063
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110063
Loading…
Reference in New Issue
No description provided.
Delete Branch "amelief/blender:gpv3-erase-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?
Implementation of the hard eraser for grease pencil.
The tool "cuts" the strokes, meaning it removes points that are inside the eraser's radius, while adding points at intersections between the eraser and the strokes.
This is WIP, because the code still needs to be optimized.
Very nice! I really like the way you've implemented this, and found the code pretty readable. A few high level suggestions (some with corresponding more detailed comments inline):
array_utils
orattribute_math
that do what this is doing. It's nice to use those where possible, to make common patterns easier to recognize, make code easier to optimize, and even decrease binary size. So whenever you feel like "this shouldn't be unique to this operator", it's worth splitting code out, at least as a separate function, or maybe even to some common header file@ -0,0 +77,4 @@
ob_eval, *obact, drawing_index);
/* Compute some useful curves geometry data. */
CurvesGeometry &src = drawing.strokes_for_write();
Looks like
src
can be declared const@ -0,0 +81,4 @@
const VArray<bool> src_cyclic = src.cyclic();
const int src_points_num = src.points_num();
const int src_curves_num = src.curves_num();
offset_indices::OffsetIndices<int> src_points_by_curves = src.points_by_curve();
@ -0,0 +106,4 @@
Array<int> nb_intersections(src_points_num, 0);
threading::parallel_for(src.curves_range(), 256, [&](const IndexRange src_curves_range) {
for (const int src_curve_index : src_curves_range) {
IndexRange src_curve_point_range = src_points_by_curves[src_curve_index];
It's standard not to include "range" in the variable name, since that info is in the type name anyway
@ -0,0 +110,4 @@
threading::parallel_for(
src_curve_point_range.drop_back(1), 256, [&](const IndexRange src_points_range) {
for (int src_point_index : src_points_range) {
We found that using
src_point
instead ofsrc_point_index
simplifies reading this sort of code, since things get shorter-- conceptually then, the "point" is just the index anyway. Same for curves.@ -0,0 +135,4 @@
}
});
if (src_cyclic[src_curve_index]) {
+1 for dealing with the last cyclic segment outside of the first loop.
I think the code could be shared though, if you split the logic to a separate function that had the two point indices as arguments
We can even create a function
isect_segment_sphere_v2
that's analog toisect_line_sphere_v2
but deals with finite segments instead of infinite lines, and returns float parameters instead of float2 points. I feel like the code would be more readable + the computation would be more direct this way.The only difference with your suggestion would be that the function does not take the indices as arguments, but the coordinates. I feel like it's okay, though.
I'll try, please let me know what you think.
I finally decided to make it a class function member, with mouse position and radius as class attributes.
@ -0,0 +173,4 @@
threading::parallel_for(src.points_range(), 256, [&](const IndexRange src_points_range) {
for (const int src_point_index : src_points_range) {
const float2 pos_view = screen_space_positions[src_point_index];
is_point_inside[src_point_index] = (len_squared_v2v2(pos_view, mouse_position) <=
Try to use the C++ math functions where possible. For example:
dot_v2v2
->math::dot
len_squared_v2v2
->math::distance_squared
len_v2
->math::length
Some like
isect_line_sphere_v2
don't have a replacement yet though, that's fine@ -0,0 +217,4 @@
/* Compute intersections between the current segment and the eraser's area. */
float2 inter0{};
float2 inter1{};
const int nb_inter = isect_line_sphere_v2(
Possibly as a performance improvement, it does look like more of this data could be stored in the first parallel pass and retrieved later.
Reason I mention it is because this branch will take a lot more time than just adding up the indices to calculate
dst_point_index
(which is the only truly serial part AFAICT).Reusing some data would probably help reduce some code duplication too.
You'll definitely know better how possible that is though!
I agree. I feel that's the tricky part, because it is easier to store the intersections parameters in the destination domain than in the source domain.
But I think we can make it work, we just have to be careful with the indices that we are manipulating here.
Actually, it was way easier than expected !
@ -0,0 +325,4 @@
/* Create the new curves geometry. */
CurvesGeometry dst(dst_points_num, dst_curves_num);
MutableSpan<int> dst_offsets_for_write = dst.offsets_for_write();
threading::parallel_for(dst.curves_range(), 256, [&](const IndexRange dst_curves_range) {
I guess this is just a parallel copy, in that case,
array_utils::copy()
should do this already :)@ -0,0 +340,4 @@
for (bke::AttributeTransferData &attribute : bke::retrieve_attributes_for_transfer(
src_attributes, dst_attributes, ATTR_DOMAIN_MASK_CURVE, propagation_info, {"cyclic"}))
{
bke::attribute_math::convert_to_static_type(attribute.dst.span.type(), [&](auto dummy) {
Great job keeping the attribute interpolation simple and reusable. Because of that, this can be even simpler and use some existing utilities:
@ -0,0 +365,4 @@
/* Display intersections with flat caps. */
if (!self.keep_caps) {
SpanAttributeWriter<int8_t> dst_start_caps =
dst.attributes_for_write().lookup_or_add_for_write_span<int8_t>("start_cap",
dst.attributes_for_write()
->dst_attributes
@ -0,0 +375,4 @@
threading::parallel_for(dst.curves_range(), 256, [&](const IndexRange dst_curves_range) {
for (const int dst_curve_index : dst_curves_range) {
IndexRange dst_curve_range = dst_points_by_curve[dst_curve_index];
IndexRange dst_curve_range
->const IndexRange dst_points
More canonical variable naming
@ -0,0 +390,4 @@
}
/* Copy/Interpolate point attributes. */
const Array<int> src_points_to_curve = src.point_to_curve_map();
Usually to avoid the need for this point to curve map, we'd just structure the loop as a nested loop over curves, then points. Then the branch for the last point can be skipped like above too, and the already likely-memory bound attribute transfer doesn't have to read as much data
@ -0,0 +426,4 @@
}
/* Set the new geometry. */
drawing.geometry.wrap() = dst;
Otherwise this is probably doing a copy
@ -0,0 +427,4 @@
/* Set the new geometry. */
drawing.geometry.wrap() = dst;
drawing.tag_positions_changed();
Even if it does the same thing internally for now, it would be good to have a function like
drawing.tag_topology_changed()
in case there's are more caches on the drawing that don't just depend on the positions at some pointafa1596bb1
tobcbb9e2fb9
WIP: GPv3 : Hard Eraser toolto WIP: GPv3: Hard Eraser tool3faf281b51
to2206b69828
WIP: GPv3: Hard Eraser toolto GPv3: Hard Eraser toolHi @HooglyBoogly :)
Thanks for your instructive review !
I think I addressed all your comments and suggestions. I tried refactoring some parts of the main function so that it's more readable. I am not sure my refactoring is great though, seems a bit random right now. The thing is, we might need to re-use some parts of this code for the soft eraser, but it's difficult to know before actually implementing it, which would be done in another PR.
So maybe we can improve the refactoring after implementing the two other modes of eraser (strokes, and soft), which I am planning to do this week.
@ -0,0 +315,4 @@
int length_of_current = 0;
for (int dst_point : dst_points) {
const int src_point = std::floor(dst_points_parameters[dst_point]);
Instead of storing the
index
andmu
value inside of one float, I think it makes sense to usestd::pair
instead. That way we know we won't run into rounding issues resulting in wrong indices.I was wondering: the eraser tool now works on the active layer only, if I understand the code correctly. Shouldn't it work on all editable layers? That's the behaviour of the erase tool in legacy GP.
@ -0,0 +61,4 @@
*/
int intersections_with_segment(const float2 &point,
const float2 &point_after,
float &mu0,
Use
r_
prefix for output parameters.@SietseB Yes, that should be the default option. I think it makes sense to be able to change that though. From what I understood with the artists I've worked with, the eraser should work like the "inverse" of the drawing tool. E.g. you want to be able to switch between drawing and erasing, without having to worry about locking all the other layers.
b5341db958
to2ac9ac6477
@SietseB Erasing on active layer only is now an option.
Default is false (=erase on all editable drawings).
Very nice!
Some small comments + one issue.
@ -0,0 +174,4 @@
/**
* Checks if a point is inside the eraser or not.
*/
bool contains_point(const float2 &point) const
This should probably an
inline
function.@ -0,0 +457,4 @@
/* Get the grease pencil drawing. */
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(obact->data);
const auto execute_in_drawing = [&](int drawing_index, bke::greasepencil::Drawing &drawing) {
Maybe
execute_eraser_on_drawing
@ -0,0 +497,4 @@
if (self.active_layer_only) {
/* Erase only on the drawing at the current frame of the active layer. */
const int drawing_index = grease_pencil.get_active_layer()->drawing_index_at(scene->r.cfra);
This will crash if the
drawing_index
is -1 (e.g. the active layer doesn't have a drawing at the current frame).d95cd725a5
tof42f33ffe0
I think this is ready to be merged now. Further refactors of the code can be done in
main
.@amelief while you're working on eraser, i saw the video above #110063 (comment) and came to my mind how eraser can interact with the new cap
, if the cap 'Flat' has in an angle property instead of depending on positions of last two point to draw the flat cap, like this mock up eg
and this idea will open the possibility to align the flat cap with path of the eraser when intersect with the stroke .. hope my idea is clear . this needs an angle property in end point stroke.
i don't know if it's good to mention it here.
@hamza-el-barmaki Yes we talked about this idea. But we didn't want to go this far. This can be done after the GPv3 refactor.
ooooh nice to heard it, good lucks