GPv3: Stroke Simplify operator #110266

Falk David · 2023-07-19T17:04:38+02:00

Falk David commented

2023-07-19 17:04:38 +02:00

This adds the stroke_simplify operators.

It also implements the ramer_douglas_peucker_simplify core function.

This adds the `stroke_simplify` operators. It also implements the `ramer_douglas_peucker_simplify` core function.

Falk David requested review from Amélie Fondevilla 2023-07-19 17:12:58 +02:00

Falk David added this to the Grease Pencil project 2023-07-19 17:13:10 +02:00

Amélie Fondevilla commented

2023-07-19 18:06:48 +02:00

Looks good to me. I only have two remarks.

It looks like the case of cyclic curve is not dealt with.

In this image for example if the curve is cyclic, the last point won't be deleted, even though the last segment (3,4) is aligned with the closing segment (4,0). It's very specific though 😅 .

It seems like some variable could be const. I'll try to comment them directly on the code.

Looks good to me. I only have two remarks. 1) It looks like the case of cyclic curve is not dealt with. ![simplify_cyclic-annotated.png](/attachments/b5cc21a9-23b3-4275-bd26-e8e9e902cf00) In this image for example if the curve is cyclic, the last point won't be deleted, even though the last segment (3,4) is aligned with the closing segment (4,0). It's very specific though 😅 . 2) It seems like some variable could be const. I'll try to comment them directly on the code.

simplify_cyclic-annotated.png

90 KiB

Amélie Fondevilla requested changes 2023-07-19 18:10:24 +02:00

source/blender/editors/grease_pencil/intern/grease_pencil_edit.cc Outdated

						
				@ -394,0 +419,4 @@

				  Stack<IndexRange> stack;

				  stack.push(src.index_range());

				  while (!stack.is_empty()) {

				    IndexRange range = stack.pop();

Amélie Fondevilla commented

2023-07-19 18:08:04 +02:00

const IndexRange range = stack.pop();

`const IndexRange range = stack.pop();`

filedescriptor marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_edit.cc Outdated

						
				@ -394,0 +420,4 @@

				  stack.push(src.index_range());

				  while (!stack.is_empty()) {

				    IndexRange range = stack.pop();

				    Span<float3> slice = src.slice(range);

Amélie Fondevilla commented

2023-07-19 18:08:34 +02:00

const Span<float3> slice = src.slice(range);

`const Span<float3> slice = src.slice(range);`

filedescriptor marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_edit.cc Outdated

						
				@ -394,0 +470,4 @@

				        const Span<float3> positions = curves.positions();

				        const OffsetIndices points_by_curve = curves.points_by_curve();

				        VArray<bool> selection = *curves.attributes().lookup_or_default<bool>(

Amélie Fondevilla commented

2023-07-19 18:09:03 +02:00

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

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

Iliya Katushenock commented

2023-07-19 18:27:50 +02:00

const VArray<bool> &selection = *curves.attributes().lookup_or_default<bool>( ".selection", ATTR_DOMAIN_POINT, true);
Avoid to do extra copy/user counter increment.

`const VArray<bool> &selection = *curves.attributes().lookup_or_default<bool>( ".selection", ATTR_DOMAIN_POINT, true);` Avoid to do extra copy/user counter increment.

filedescriptor marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_edit.cc Outdated

						
				@ -394,0 +484,4 @@

				              continue;

				            }

				            Vector<IndexRange> selection_ranges = array_utils::find_all_ranges(curve_selection,