GPv3: Stroke smoothing operator #109635

Merged
Falk David merged 34 commits from amelief/blender:gpv3-blur-stroke-operator into main 2023-07-18 11:54:42 +02:00

Implementation of the smoothing operator for strokes, which works on the curves geometry inner data, including position and other attributes, such as radius (optional).

Implementation of the smoothing operator for strokes, which works on the curves geometry inner data, including position and other attributes, such as radius (optional).
Amélie Fondevilla added this to the Grease Pencil project 2023-07-03 12:59:19 +02:00
Hans Goudey requested review from Hans Goudey 2023-07-04 18:33:33 +02:00
Amélie Fondevilla force-pushed gpv3-blur-stroke-operator from 5271bc6847 to 6ca5f950f2 2023-07-05 11:54:39 +02:00 Compare
Author
Member

This PR is waiting for 109733 to land so that we can add the option to smooth radius and opacity.

This PR is waiting for [109733](https://projects.blender.org/blender/blender/pulls/109733) to land so that we can add the option to smooth radius and opacity.
Falk David force-pushed gpv3-blur-stroke-operator from e13150085a to fdcba22d38 2023-07-11 15:14:47 +02:00 Compare
Falk David changed title from WIP: GPv3 stroke smoothing operator to GPv3 stroke smoothing operator 2023-07-11 15:45:53 +02:00
Falk David requested changes 2023-07-13 13:00:33 +02:00
Falk David left a comment
Member

Some cleanup comments. Additionally this should add a menu and entry for the operator in the UI.
Add a class below VIEW3D_MT_edit_greasepencil in blender/scripts/startup/bl_ui/space_view3d.py:

class VIEW3D_MT_edit_greasepencil_stroke(Menu):
    bl_label = "Stroke"

    def draw(self, context):
       ...

Also declare it at the bottom of the file.

Then add an if-case (line 1010):

elif mode_string == EDIT_GREASE_PENCIL:
      layout.menu("VIEW3D_MT_edit_greasepencil_stroke")

Some cleanup comments. Additionally this should add a menu and entry for the operator in the UI. Add a class below `VIEW3D_MT_edit_greasepencil` in `blender/scripts/startup/bl_ui/space_view3d.py`: ``` class VIEW3D_MT_edit_greasepencil_stroke(Menu): bl_label = "Stroke" def draw(self, context): ... ``` Also declare it at the bottom of the file. Then add an if-case (line 1010): ``` elif mode_string == EDIT_GREASE_PENCIL: layout.menu("VIEW3D_MT_edit_greasepencil_stroke") ```
@ -81,0 +95,4 @@
* \{ */
template<typename T>
static Span<T> gaussian_blur_1D_ex(const bool is_cyclic,
Member

is_cyclic should be below keep_shape in the parameters.

`is_cyclic` should be below `keep_shape` in the parameters.
amelief marked this conversation as resolved
@ -81,0 +143,4 @@
* Note these weights only work because the averaging is done in relative coordinates.
*/
/* Avoid computation if the mask is empty */
Member

All comments should end in a ..

All comments should end in a `.`.
amelief marked this conversation as resolved
@ -81,0 +159,4 @@
const int64_t first_pt = curve_points.first();
const int64_t last_pt = curve_points.last();
const int64_t nb_pts = curve_points.size();
Member

nb_pts -> total_points

`nb_pts` -> `total_points`
amelief marked this conversation as resolved
@ -81,0 +172,4 @@
}
});
for (int step = iterations; step > 0; step--) {
Member

Does this need to go from iterations -> 0?

Otherwise I think something like for (const int step : IndexRange(iterations)) would be better.

Does this need to go from `iterations` -> `0`? Otherwise I think something like `for (const int step : IndexRange(iterations))` would be better.
Author
Member

Yes, that's why it loops over the indices. I can also reverse it and replace step by iterations - step inside the loop. Idk what is best.

Yes, that's why it loops over the indices. I can also reverse it and replace `step` by `iterations - step` inside the loop. Idk what is best.
Author
Member

I think I am just gonna do :

for (const int step : IndexRange(iterations)) {
    const int offset = iterations - step;

And then use offset instead of step in the loop

I think I am just gonna do : ``` for (const int step : IndexRange(iterations)) { const int offset = iterations - step; ``` And then use `offset` instead of `step` in the loop
Member

Looks good!

Looks good!
amelief marked this conversation as resolved
@ -81,0 +180,4 @@
return;
}
float w_before = float(w - w2);
Member

Could these be just double?

Could these be just `double`?
amelief marked this conversation as resolved
@ -81,0 +263,4 @@
static int grease_pencil_stroke_smooth_exec(bContext *C, wmOperator *op)
{
using namespace blender;
Scene *scene = CTX_data_scene(C);
Member

const Scene *

`const Scene *`
amelief marked this conversation as resolved
Hans Goudey requested changes 2023-07-13 17:39:45 +02:00
Hans Goudey left a comment
Member

I like where you're headed with this! It's nice to have the slightly generalized 1D Gaussian blur functions.
However, I think the generalization can go a bit further. I got a bit carried away and did some prototyping in the code:

  • The gaussian blur function only gets the data for each curve (so it's not curve or GP specific anymore)
  • The original data is only made large enough for each curve, which should approximately halve the memory usage of each attribute for the many small curves case, and help to keep the data in CPU caches
  • Attributes are gathered only with the attribute API, generalizing things and making it easier to support more data in the future
  • attribute_math::convert_to_static_type is used to generate the code necessary for each type

Hopefully that's all reasonable. Sorry to paste a bunch of text here:

void gaussian_blur_1D(const GSpan src,
                      const IndexMask &mask,
                      const int iterations,
                      const float influence,
                      const bool smooth_ends,
                      const bool keep_shape,
                      const bool is_cyclic,
                      GMutableSpan dst)
{
  bke::attribute_math::convert_to_static_type(src.type(), [&](auto dummy) {
    using T = decltype(dummy);
    if constexpr (std::is_same_any_v<T, float, float3>) { // Reduces unnecessary code generation
      gaussian_blur_1D(src.typed<T>(), ..., dst.typed<T>());
    }
  });
}

static void smooth_curve_attributes(bke::CurvesGeometry &curves,
                                    const Span<StringRef> names,
                                    const int iterations,
                                    const float influence,
                                    const bool smooth_ends,
                                    const bool keep_shape)
{
  bke::MutableAttributeAccessor attributes = curves.attributes_for_write();

  const VArray<bool> selection = *attributes.lookup_or_default<bool>(
      ".selection", ATTR_DOMAIN_POINT, true);

  Vector<bke::GSpanAttributeWriter> attributes_to_smooth;
  for (const StringRef name : names) {
    bke::GSpanAttributeWriter attribute = attributes.lookup_for_write_span(name);
    if (attribute && attribute.domain == ATTR_DOMAIN_POINT) {
      attributes_to_smooth.append(std::move(attribute));
    }
  }

  const OffsetIndices points_by_curve = curves.points_by_curve();
  const VArray<bool> cyclic = curves.cyclic();
  for (bke::GSpanAttributeWriter &attribute : attributes_to_smooth) {
    GMutableSpan data = attribute.span;
    threading::parallel_for(curves.curves_range(), 256, [&](const IndexRange range) {
      Vector<std::byte> orig_data;
      for (const int curve : range) {
        const IndexRange points = points_by_curve[curve];
        IndexMaskMemory memory;
        const IndexMask curve_mask = IndexMask::from_bools(points, selection, memory);
        if (curve_mask.is_empty()) {
          continue;
        }

        GMutableSpan curve_data = data.slice(points);
        orig_data.resize(curve_data.size_in_bytes());
        curve_data.type().copy_assign_n(curve_data.data(), orig_data.data(), points.size());

        gaussian_blur_1D(GSpan(curve_data.type(), orig_data.data(), points.size()),
                         curve_mask,
                         iterations,
                         influence,
                         smooth_ends,
                         keep_shape,
                         cyclic[curve],
                         curve_data);
      }
    });
  }

  for (bke::GSpanAttributeWriter &attribute : attributes_to_smooth) {
    attribute.finish();
  }
}
I like where you're headed with this! It's nice to have the slightly generalized 1D Gaussian blur functions. However, I think the generalization can go a bit further. I got a bit carried away and did some prototyping in the code: - The gaussian blur function only gets the data for each curve (so it's not curve or GP specific anymore) - The original data is only made large enough for each curve, which should approximately halve the memory usage of each attribute for the many small curves case, and help to keep the data in CPU caches - Attributes are gathered only with the attribute API, generalizing things and making it easier to support more data in the future - `attribute_math::convert_to_static_type` is used to generate the code necessary for each type Hopefully that's all reasonable. Sorry to paste a bunch of text here: ```cpp void gaussian_blur_1D(const GSpan src, const IndexMask &mask, const int iterations, const float influence, const bool smooth_ends, const bool keep_shape, const bool is_cyclic, GMutableSpan dst) { bke::attribute_math::convert_to_static_type(src.type(), [&](auto dummy) { using T = decltype(dummy); if constexpr (std::is_same_any_v<T, float, float3>) { // Reduces unnecessary code generation gaussian_blur_1D(src.typed<T>(), ..., dst.typed<T>()); } }); } static void smooth_curve_attributes(bke::CurvesGeometry &curves, const Span<StringRef> names, const int iterations, const float influence, const bool smooth_ends, const bool keep_shape) { bke::MutableAttributeAccessor attributes = curves.attributes_for_write(); const VArray<bool> selection = *attributes.lookup_or_default<bool>( ".selection", ATTR_DOMAIN_POINT, true); Vector<bke::GSpanAttributeWriter> attributes_to_smooth; for (const StringRef name : names) { bke::GSpanAttributeWriter attribute = attributes.lookup_for_write_span(name); if (attribute && attribute.domain == ATTR_DOMAIN_POINT) { attributes_to_smooth.append(std::move(attribute)); } } const OffsetIndices points_by_curve = curves.points_by_curve(); const VArray<bool> cyclic = curves.cyclic(); for (bke::GSpanAttributeWriter &attribute : attributes_to_smooth) { GMutableSpan data = attribute.span; threading::parallel_for(curves.curves_range(), 256, [&](const IndexRange range) { Vector<std::byte> orig_data; for (const int curve : range) { const IndexRange points = points_by_curve[curve]; IndexMaskMemory memory; const IndexMask curve_mask = IndexMask::from_bools(points, selection, memory); if (curve_mask.is_empty()) { continue; } GMutableSpan curve_data = data.slice(points); orig_data.resize(curve_data.size_in_bytes()); curve_data.type().copy_assign_n(curve_data.data(), orig_data.data(), points.size()); gaussian_blur_1D(GSpan(curve_data.type(), orig_data.data(), points.size()), curve_mask, iterations, influence, smooth_ends, keep_shape, cyclic[curve], curve_data); } }); } for (bke::GSpanAttributeWriter &attribute : attributes_to_smooth) { attribute.finish(); } } ```
@ -81,0 +291,4 @@
/* Position. */
Array<float3> curves_positions_copy;
if (smooth_position && !curves.positions().is_empty()) {
curves_positions_copy = curves.positions();
Member

You can count on the position attribute always being available

You can count on the position attribute always being available
Member

Hm the code doesn't check if the position attribute is available, only if it's empty. Can the position attribute not be empty?

Hm the code doesn't check if the `position` attribute is available, only if it's empty. Can the position attribute not be empty?
Member

Ah, sure. But maybe it makes sense to just return early at the top if the curves are empty, skipping everything

Ah, sure. But maybe it makes sense to just return early at the top if the curves are empty, skipping everything
filedescriptor marked this conversation as resolved
@ -81,0 +293,4 @@
if (smooth_position && !curves.positions().is_empty()) {
curves_positions_copy = curves.positions();
}
/* Opacity. */
Member

These comments don't really help IMO, since what they say is reading just a few words of the code. The blank line would be nice though

These comments don't really help IMO, since what they say is reading just a few words of the code. The blank line would be nice though
filedescriptor marked this conversation as resolved
@ -81,0 +294,4 @@
curves_positions_copy = curves.positions();
}
/* Opacity. */
Array<float> curves_opacities_copy;
Member

How about simplifying the naming like opacities_orig? The "curves" part of the existing name doesn't really help, and "orig" helps to make the purpose clearer (as in "why do we make a copy (which we can see with the Array))

How about simplifying the naming like `opacities_orig`? The "curves" part of the existing name doesn't really help, and "orig" helps to make the purpose clearer (as in "why do we make a copy (which we can see with the `Array`))
filedescriptor marked this conversation as resolved
@ -81,0 +304,4 @@
curves_radii_copy = drawing.radii().get_internal_span();
}
/* Cyclic. */
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
Member

offset_indices::OffsetIndices<int> -> OffsetIndices

`offset_indices::OffsetIndices<int>` -> `OffsetIndices`
filedescriptor marked this conversation as resolved
@ -81,0 +307,4 @@
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
const VArray<bool> cyclic = curves.cyclic();
/* Selection. */
bke::AttributeAccessor curves_attributes = curves.attributes();
Member
        const bke::AttributeAccessor attributes = curves.attributes();
        const VArray<bool> selection = *attributes.lookup_or_default<bool>(
            ".selection", ATTR_DOMAIN_POINT, true);
  • Skipping "curves" part of name
  • Using VArray directly since the attribute reader isn't used later on
``` const bke::AttributeAccessor attributes = curves.attributes(); const VArray<bool> selection = *attributes.lookup_or_default<bool>( ".selection", ATTR_DOMAIN_POINT, true); ``` - Skipping "curves" part of name - Using `VArray` directly since the attribute reader isn't used later on
filedescriptor marked this conversation as resolved
@ -81,0 +337,4 @@
smooth_ends,
keep_shape,
is_cyclic,
curves.positions_for_write(),
Member

These attribute access functions do string lookups, so they are a bit too expensive to call once per curve

These attribute access functions do string lookups, so they are a bit too expensive to call once per curve
filedescriptor marked this conversation as resolved
@ -62,5 +63,25 @@ void create_blank(Main &bmain, Object &object, int frame_number);
void create_stroke(Main &bmain, Object &object, float4x4 matrix, int frame_number);
void create_suzanne(Main &bmain, Object &object, float4x4 matrix, const int frame_number);
Span<float3> gaussian_blur_1D_float3(const IndexRange curve_points,
Member
  • Looks like the return value isn't used. It's also unnecessary since it just just references dst anyway
  • The name can be overloaded so only a single name is used for the different types
  • The const for all arguments passed by value is unnecessary here in the declaration (so only mask needs to have const
  • dst should be the last argument (it's just more typical to have order return arguments or changed data last
- Looks like the return value isn't used. It's also unnecessary since it just just references `dst` anyway - The name can be overloaded so only a single name is used for the different types - The `const` for all arguments passed by value is unnecessary here in the declaration (so only `mask` needs to have `const` - `dst` should be the last argument (it's just more typical to have order return arguments or changed data last
filedescriptor marked this conversation as resolved
Falk David force-pushed gpv3-blur-stroke-operator from 2134127aa0 to b34deb16f4 2023-07-14 16:02:52 +02:00 Compare
Hans Goudey changed title from GPv3 stroke smoothing operator to GPv3: Stroke smoothing operator 2023-07-16 05:10:00 +02:00
Member

@HooglyBoogly I implemented some of your suggestions above. There are some noteworthy differences though:

  • The gaussian_blur_1D does not receive an IndexMask anymore. It always blurs the entire source span. Previously, when smoothing only a portion of a curve (even with smooth_ends disabled) the result could lead to a discontinuous curve. This was because the smooth_ends option only applied to the start and end point of a curve. Now, we consider the start and end points of a selected range as the "ends". IMHO this leads to more expected results.
  • Instead of a smooth_curve_attributes function, I implemented a function that smooths a single curve attribute. This means that I can choose different smoothing parameters per attribute (which is needed in the grease pencil case, e.g. only for positions do we enable keep_shape).
@HooglyBoogly I implemented some of your suggestions above. There are some noteworthy differences though: * The `gaussian_blur_1D` does not receive an `IndexMask` anymore. It always blurs the entire source span. Previously, when smoothing only a portion of a curve (even with `smooth_ends` disabled) the result could lead to a discontinuous curve. This was because the `smooth_ends` option only applied to the start and end point of a curve. Now, we consider the start and end points of a selected range as the "ends". IMHO this leads to more expected results. * Instead of a `smooth_curve_attributes` function, I implemented a function that smooths a single curve attribute. This means that I can choose different smoothing parameters per attribute (which is needed in the grease pencil case, e.g. only for positions do we enable `keep_shape`).
Falk David requested review from Hans Goudey 2023-07-17 12:20:27 +02:00
Falk David approved these changes 2023-07-17 12:20:59 +02:00
Falk David left a comment
Member

Ready to be merged from my point of view.

Ready to be merged from my point of view.
Falk David force-pushed gpv3-blur-stroke-operator from 5eeda5e954 to 084f5b193c 2023-07-17 12:42:00 +02:00 Compare
Falk David merged commit d9277b19f3 into main 2023-07-18 11:54:42 +02:00
Falk David deleted branch gpv3-blur-stroke-operator 2023-07-18 11:54:43 +02:00
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
3 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#109635
No description provided.