GPv3: Hard Eraser tool #110063

Merged
Falk David merged 67 commits from amelief/blender:gpv3-erase-operator into main 2023-07-20 13:56:29 +02:00

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.

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.
Amélie Fondevilla added this to the Grease Pencil project 2023-07-13 15:59:57 +02:00
Author
Member

This is WIP, because the code still needs to be optimized.

This is WIP, because the code still needs to be optimized.
Hans Goudey requested changes 2023-07-13 19:23:31 +02:00
Hans Goudey left a comment
Member

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):

  • The main function would probably be easier to read if some of the larger loops were split to separate functions. It's easier to see the dependencies between steps in the algorithm that way
  • In a few places there are some utilities in array_utils or attribute_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
  • There are some common variable naming patterns like skipping "index" or "range" suffixes, following those might help to keep this code consistent looking with other curve code
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): - The main function would probably be easier to read if some of the larger loops were split to separate functions. It's easier to see the dependencies between steps in the algorithm that way - In a few places there are some utilities in `array_utils` or `attribute_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 - There are some common variable naming patterns like skipping "index" or "range" suffixes, following those might help to keep this code consistent looking with other curve code
@ -0,0 +77,4 @@
ob_eval, *obact, drawing_index);
/* Compute some useful curves geometry data. */
CurvesGeometry &src = drawing.strokes_for_write();
Member

Looks like src can be declared const

Looks like `src` can be declared const
amelief marked this conversation as resolved
@ -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();
Member
    const OffsetIndices<int> src_points_by_curve = src.points_by_curve();
```cpp const OffsetIndices<int> src_points_by_curve = src.points_by_curve(); ```
amelief marked this conversation as resolved
@ -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];
Member
const IndexRange src_curve_points

It's standard not to include "range" in the variable name, since that info is in the type name anyway

``` const IndexRange src_curve_points ``` It's standard not to include "range" in the variable name, since that info is in the type name anyway
amelief marked this conversation as resolved
@ -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) {
Member

We found that using src_point instead of src_point_index simplifies reading this sort of code, since things get shorter-- conceptually then, the "point" is just the index anyway. Same for curves.

We found that using `src_point` instead of `src_point_index` simplifies reading this sort of code, since things get shorter-- conceptually then, the "point" is just the index anyway. Same for curves.
amelief marked this conversation as resolved
@ -0,0 +135,4 @@
}
});
if (src_cyclic[src_curve_index]) {
Member

+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

+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
Author
Member

We can even create a function isect_segment_sphere_v2 that's analog to isect_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.

We can even create a function `isect_segment_sphere_v2` that's analog to `isect_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.
Author
Member

I finally decided to make it a class function member, with mouse position and radius as class attributes.

I finally decided to make it a class function member, with mouse position and radius as class attributes.
amelief marked this conversation as resolved
@ -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) <=
Member

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

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
amelief marked this conversation as resolved
@ -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(
Member

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!

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!
Author
Member

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.

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.
Author
Member

Actually, it was way easier than expected !

Actually, it was way easier than expected !
amelief marked this conversation as resolved
@ -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) {
Member

I guess this is just a parallel copy, in that case, array_utils::copy() should do this already :)

I guess this is just a parallel copy, in that case, `array_utils::copy()` should do this already :)
amelief marked this conversation as resolved
@ -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) {
Member

Great job keeping the attribute interpolation simple and reusable. Because of that, this can be even simpler and use some existing utilities:

      bke::attribute_math::gather(attribute.src, dst_to_src_curve_index, attribute.dst.span);
Great job keeping the attribute interpolation simple and reusable. Because of that, this can be even simpler and use some existing utilities: ```cpp bke::attribute_math::gather(attribute.src, dst_to_src_curve_index, attribute.dst.span); ```
amelief marked this conversation as resolved
@ -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",
Member

dst.attributes_for_write() -> dst_attributes

`dst.attributes_for_write()` -> `dst_attributes`
amelief marked this conversation as resolved
@ -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];
Member

IndexRange dst_curve_range -> const IndexRange dst_points

More canonical variable naming

`IndexRange dst_curve_range` -> `const IndexRange dst_points` More canonical variable naming
amelief marked this conversation as resolved
@ -0,0 +390,4 @@
}
/* Copy/Interpolate point attributes. */
const Array<int> src_points_to_curve = src.point_to_curve_map();
Member

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

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
amelief marked this conversation as resolved
@ -0,0 +426,4 @@
}
/* Set the new geometry. */
drawing.geometry.wrap() = dst;
Member
drawing.geometry.wrap() = std::move(dst);

Otherwise this is probably doing a copy

```cpp drawing.geometry.wrap() = std::move(dst); ``` Otherwise this is probably doing a copy
amelief marked this conversation as resolved
@ -0,0 +427,4 @@
/* Set the new geometry. */
drawing.geometry.wrap() = dst;
drawing.tag_positions_changed();
Member

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 point

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 point
amelief marked this conversation as resolved
Falk David force-pushed gpv3-erase-operator from afa1596bb1 to bcbb9e2fb9 2023-07-14 11:06:42 +02:00 Compare
Hans Goudey changed title from WIP: GPv3 : Hard Eraser tool to WIP: GPv3: Hard Eraser tool 2023-07-16 05:21:02 +02:00
Amélie Fondevilla force-pushed gpv3-erase-operator from 3faf281b51 to 2206b69828 2023-07-17 16:44:32 +02:00 Compare
Amélie Fondevilla changed title from WIP: GPv3: Hard Eraser tool to GPv3: Hard Eraser tool 2023-07-17 17:32:40 +02:00
Author
Member

Hi @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.

Hi @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.
Falk David reviewed 2023-07-18 10:33:43 +02:00
@ -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]);
Member

Instead of storing the index and mu value inside of one float, I think it makes sense to use std::pair instead. That way we know we won't run into rounding issues resulting in wrong indices.

Instead of storing the `index` and `mu` value inside of one float, I think it makes sense to use `std::pair` instead. That way we know we won't run into rounding issues resulting in wrong indices.
filedescriptor marked this conversation as resolved
Member

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.

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.
Jacques Lucke reviewed 2023-07-18 13:42:01 +02:00
@ -0,0 +61,4 @@
*/
int intersections_with_segment(const float2 &point,
const float2 &point_after,
float &mu0,
Member

Use r_ prefix for output parameters.

Use `r_` prefix for output parameters.
amelief marked this conversation as resolved
Member

@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.

@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.
Amélie Fondevilla force-pushed gpv3-erase-operator from b5341db958 to 2ac9ac6477 2023-07-20 09:49:42 +02:00 Compare
Author
Member

@SietseB Erasing on active layer only is now an option.
Default is false (=erase on all editable drawings).

@SietseB Erasing on active layer only is now an option. Default is false (=erase on all editable drawings). <video src="/attachments/633d187a-1981-4112-ae20-c4d9617f6662" title="erase_active_layer.mp4" controls></video>
Member

Very nice!

Very nice!
Falk David requested changes 2023-07-20 12:00:18 +02:00
Falk David left a comment
Member

Some small comments + one issue.

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
Member

This should probably an inline function.

This should probably an `inline` function.
amelief marked this conversation as resolved
@ -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) {
Member

Maybe execute_eraser_on_drawing

Maybe `execute_eraser_on_drawing`
amelief marked this conversation as resolved
@ -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);
Member

This will crash if the drawing_index is -1 (e.g. the active layer doesn't have a drawing at the current frame).

This will crash if the `drawing_index` is -1 (e.g. the active layer doesn't have a drawing at the current frame).
amelief marked this conversation as resolved
Amélie Fondevilla force-pushed gpv3-erase-operator from d95cd725a5 to f42f33ffe0 2023-07-20 13:04:48 +02:00 Compare
Falk David approved these changes 2023-07-20 13:53:52 +02:00
Falk David left a comment
Member

I think this is ready to be merged now. Further refactors of the code can be done in main.

I think this is ready to be merged now. Further refactors of the code can be done in `main`.
Falk David merged commit 924aa0ef07 into main 2023-07-20 13:56:29 +02:00
Falk David referenced this issue from a commit 2023-07-20 13:56:30 +02:00
Falk David deleted branch gpv3-erase-operator 2023-07-20 13:56:31 +02:00
First-time contributor

@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
image
, 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
image
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.

@amelief while you're working on eraser, i saw the video above https://projects.blender.org/blender/blender/pulls/110063#issuecomment-983303 and came to my mind how eraser can interact with the new cap ![image](/attachments/ff441bc9-7700-40f9-9a67-c6f970ae5a70) , 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 ![image](/attachments/7fe0ca0f-b286-41a9-aa72-55a3094b059d) 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.
Member

@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.

@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.
First-time contributor

ooooh nice to heard it, good lucks

ooooh nice to heard it, good lucks
Sign in to join this conversation.
No reviewers
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110063
No description provided.