GPv3: Draw tool: "Draw on back" option #120894

Merged
Falk David merged 16 commits from casey-bianco-davis/blender:GPv3-Draw-on-back into main 2024-05-03 18:04:53 +02:00

This adds the Draw on back option to GPv3.

Note:
In the legacy system the stroke being drawn would be displayed in front and only moved to the back upon finishing the operator.
Whereas this will display the stroke on back throughout operator.

This adds the `Draw on back` option to GPv3. Note: In the legacy system the stroke being drawn would be displayed in front and only moved to the back upon finishing the operator. Whereas this will display the stroke on back throughout operator.
casey-bianco-davis added 1 commit 2024-04-21 19:10:35 +02:00
casey-bianco-davis added the
Module
Grease Pencil
Interest
Grease Pencil
labels 2024-04-21 19:11:27 +02:00
casey-bianco-davis added this to the Grease Pencil project 2024-04-21 19:11:29 +02:00
casey-bianco-davis requested review from Falk David 2024-04-21 19:11:43 +02:00
casey-bianco-davis added 2 commits 2024-04-23 03:03:08 +02:00
Iliya Katushenock removed the
Module
Grease Pencil
Interest
Grease Pencil
labels 2024-04-23 12:05:09 +02:00
Iliya Katushenock reviewed 2024-04-23 12:12:39 +02:00
@ -177,0 +177,4 @@
static void create_blank_curve(bke::CurvesGeometry &curves, const bool on_back)
{
if (on_back) {
const bke::CurvesGeometry TempCurve = bke::CurvesGeometry(curves);

TempCurve -> temporal_copy

`TempCurve` -> `temporal_copy`
casey-bianco-davis marked this conversation as resolved
@ -177,0 +192,4 @@
bke::AttrDomain::Point,
{},
{},
OffsetIndices<int>(Array<int>({0, curves.points_num() - 1})),

from:
Span<int>{0, temporal_copy.points_num()}
and to:
Span<int>{1, temporal_copy.points_num() + 1}
(Cast to OffsetIndices will be implicit, life time of {1, temporal_copy.points_num() + 1} initializer list will be enough to perform the copy)

from: `Span<int>{0, temporal_copy.points_num()}` and to: `Span<int>{1, temporal_copy.points_num() + 1}` (Cast to `OffsetIndices` will be implicit, life time of `{1, temporal_copy.points_num() + 1}` initializer list will be enough to perform the copy)
casey-bianco-davis marked this conversation as resolved
@ -177,0 +205,4 @@
IndexMask(IndexRange(1)),
attributes);
}
else {

Invert if statement, handle this part at first, and next code can be out of nesting.

Invert if statement, handle this part at first, and next code can be out of nesting.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 3 commits 2024-04-24 03:17:53 +02:00
casey-bianco-davis added 1 commit 2024-04-25 03:28:48 +02:00
Falk David requested changes 2024-04-25 17:11:48 +02:00
Dismissed
Falk David left a comment
Member

Did a pass on this, thanks for working on it.

Did a pass on this, thanks for working on it.
@ -193,0 +199,4 @@
return;
}
const bke::CurvesGeometry temporal_copy = bke::CurvesGeometry(curves);
Member

I'm a bit sceptical about creating a full copy of the data and then calling curves.resize. That means we have basically two identical copies of the same data, where our resized curves just have uninitialized data at the end (the new curve).
@HooglyBoogly Would the best way be to just create a new CurvesGeometry with the right size and write the entire data to it?
In any case, we should create only one copy of the data (since we know the final size ahead of time).

PS: I think you mean "temporary copy" ?

I'm a bit sceptical about creating a full copy of the data and then calling `curves.resize`. That means we have basically two identical copies of the same data, where our resized curves just have uninitialized data at the end (the new curve). @HooglyBoogly Would the best way be to just create a new `CurvesGeometry` with the right size and write the entire data to it? In any case, we should create only one copy of the data (since we know the final size ahead of time). PS: I think you mean "temporary copy" ?
Member

A new CurvesGeometry should just be created from scratch with its "size" constructor, without copying the old curves. Then it should be filled with the right attributes and offsets, and returned by value. The input curves should be const.

Or alternatively the curves should just be resized in place without this temporary copy. Not sure why that isn't possible TBH.

A new `CurvesGeometry` should just be created from scratch with its "size" constructor, without copying the old curves. Then it should be filled with the right attributes and offsets, and returned by value. The input curves should be const. Or alternatively the curves should just be resized in place without this temporary copy. Not sure why that isn't possible TBH.
Member

I think the problem is that copy_attributes_group_to_group can't use the same source and destination attributes accessors? Or at least that might have been the thinking.
EDIT: Looks like copy_attributes_group_to_group would be just fine even if the attributes come from the same place..

~~I think the problem is that `copy_attributes_group_to_group` can't use the same source and destination attributes accessors? Or at least that might have been the thinking.~~ EDIT: Looks like `copy_attributes_group_to_group` would be just fine even if the attributes come from the same place..
Member

Then copy_attributes_group_to_group shouldn't be used here. If the utility doesn't work for a certain situation it's better not to use it than to make code more complicated and slower just to avoid duplication. Chances are there's another way to do those things anyway.

Then `copy_attributes_group_to_group` shouldn't be used here. If the utility doesn't work for a certain situation it's better not to use it than to make code more complicated and slower just to avoid duplication. Chances are there's another way to do those things anyway.
casey-bianco-davis marked this conversation as resolved
@ -193,0 +241,4 @@
const int active_curve = curves.curves_range().first();
const int last_active_point = curves.points_by_curve()[active_curve].last();
const bke::CurvesGeometry temporal_copy = bke::CurvesGeometry(curves);
Member

See my comment for create_blank_curve.

See my comment for `create_blank_curve`.
casey-bianco-davis marked this conversation as resolved
@ -238,2 +313,4 @@
const float start_opacity = ed::greasepencil::opacity_from_input_sample(
start_sample.pressure, brush_, scene_, settings_);
Scene *scene = CTX_data_scene(&C);
const bool on_back = (scene->toolsettings->gpencil_flags & GP_TOOL_FLAG_PAINT_ONBACK);
Member

... != 0;

` ... != 0;`
casey-bianco-davis marked this conversation as resolved
@ -405,2 +486,4 @@
const float opacity = ed::greasepencil::opacity_from_input_sample(
extension_sample.pressure, brush_, scene_, settings_);
Scene *scene = CTX_data_scene(&C);
const bool on_back = (scene->toolsettings->gpencil_flags & GP_TOOL_FLAG_PAINT_ONBACK);
Member

... != 0;

`... != 0;`
casey-bianco-davis marked this conversation as resolved
@ -408,3 +491,4 @@
bke::CurvesGeometry &curves = drawing_->strokes_for_write();
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
int active_curve = on_back ? curves.curves_range().first() : curves.curves_range().last();
Member

const int active_curve ...

`const int active_curve ... `
casey-bianco-davis marked this conversation as resolved
@ -439,2 +525,2 @@
curves.offsets_for_write().last() = curves.points_num();
const IndexRange curve_points = curves.points_by_curve()[curves.curves_range().last()];
extend_curve(curves, on_back, new_points_num);
curve_points = curves.points_by_curve()[active_curve];
Member

Looks like this line can be removed and integrated in the line below:

const IndexRange new_points = curves.points_by_curve()[active_curve].take_back(new_points_num);
Looks like this line can be removed and integrated in the line below: ```Cpp const IndexRange new_points = curves.points_by_curve()[active_curve].take_back(new_points_num); ```
casey-bianco-davis marked this conversation as resolved
@ -612,3 +700,2 @@
{
const int stroke_index = drawing.strokes().curves_range().last();
const IndexRange points = drawing.strokes().points_by_curve()[stroke_index];
Scene *scene = CTX_data_scene(&C);
Member

const Scene *

`const Scene *`
casey-bianco-davis marked this conversation as resolved
@ -613,2 +701,2 @@
const int stroke_index = drawing.strokes().curves_range().last();
const IndexRange points = drawing.strokes().points_by_curve()[stroke_index];
Scene *scene = CTX_data_scene(&C);
const bool on_back = (scene->toolsettings->gpencil_flags & GP_TOOL_FLAG_PAINT_ONBACK);
Member

... != 0;

`... != 0;`
casey-bianco-davis marked this conversation as resolved
@ -645,0 +746,4 @@
const int active_curve = curves.curves_range().first();
const int last_active_point = curves.points_by_curve()[active_curve].last();
const bke::CurvesGeometry temporal_copy = bke::CurvesGeometry(curves);
Member

See my comment for create_blank_curve.

See my comment for `create_blank_curve`.
casey-bianco-davis marked this conversation as resolved
Hans Goudey reviewed 2024-04-25 17:17:43 +02:00
@ -193,0 +210,4 @@
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
copy_attributes_group_to_group(src_attributes,
Member

I'd expect bke:: to be necessary here

I'd expect `bke::` to be necessary here
Author
Member

Seems to be needed, does not compile without.

Seems to be needed, does not compile without.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 6 commits 2024-04-27 03:01:24 +02:00
casey-bianco-davis added 2 commits 2024-04-29 04:31:19 +02:00
casey-bianco-davis requested review from Falk David 2024-04-29 04:43:46 +02:00
Falk David changed title from GPv3: Draw on back option. to GPv3: Draw tool: "Draw on back" option 2024-04-29 10:25:49 +02:00
Iliya Katushenock reviewed 2024-04-29 11:02:19 +02:00
@ -193,0 +208,4 @@
offsets[i + 1] = offsets[i] + 1;
}
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();

Here is the same issue as in #117910

Here is the same issue as in #117910
Member

I think this is an important point actually. Because the attribute arrays are likely shared, at least with undo steps, this involves two copies instead of one (the resize).

It would actually be helpful to know what this operator is supposed to do. Maybe that could be mentioned in the description? Looks like it moves points forward in the arrays? Maybe reusing the geometry reordering code would be the way to go?

I think this is an important point actually. Because the attribute arrays are likely shared, at least with undo steps, this involves two copies instead of one (the resize). It would actually be helpful to know what this operator is supposed to do. Maybe that could be mentioned in the description? Looks like it moves points forward in the arrays? Maybe reusing the geometry reordering code would be the way to go?
Member

Draw on back means that the draw operator draws into a stroke that needs to be the first curve in the CurvesGeometry. So we need to make space for that first curve. This is what this is doing.

`Draw on back` means that the draw operator draws into a stroke that needs to be the first curve in the `CurvesGeometry`. So we need to make space for that first curve. This is what this is doing.
Member

Ah, thanks!

Ah, thanks!
Member

I don't think this is resolved, there is still the double copy of attribute arrays.

I don't think this is resolved, there is still the double copy of attribute arrays.
Member

@HooglyBoogly Could you specify where? I don't see it.

@HooglyBoogly Could you specify where? I don't see it.

Copy of shader data: bke::GSpanAttributeWriter dst = attributes.lookup_for_write_span(id);
Copy of shifted data: span_data[i + new_points_num] = span_data[i];

Copy of shader data: `bke::GSpanAttributeWriter dst = attributes.lookup_for_write_span(id);` Copy of shifted data: `span_data[i + new_points_num] = span_data[i];`
Member

This is a tricky situation. Since this is such a common operation, ideally we would optimize for the case where the attribute arrays are shared and the case where they aren't.

When the arrays are shared, creating a new CurvesGeometry from scratch and copying the data into it at the same time as the shifting is the best solution.

When the arrays aren't shared, theoretically it's best to resize them in place. This way we can use future amortized growth of attribute arrays to avoid constant reallocation and copying. But it also reduces peak memory usage since we don't have two sets of arrays allocated at the same time at any point.

For now I think this is fine actually, I've read over it again and it seems a fine place to start. I'll bet this area will change a bit in the future, considering there are other optimizations (like amortized growth) to apply here.

This is a tricky situation. Since this is such a common operation, ideally we would optimize for the case where the attribute arrays are shared and the case where they aren't. When the arrays are shared, creating a new `CurvesGeometry` from scratch and copying the data into it at the same time as the shifting is the best solution. When the arrays aren't shared, theoretically it's best to resize them in place. This way we can use future amortized growth of attribute arrays to avoid constant reallocation and copying. But it also reduces peak memory usage since we don't have two sets of arrays allocated at the same time at any point. For now I think this is fine actually, I've read over it again and it seems a fine place to start. I'll bet this area will change a bit in the future, considering there are other optimizations (like amortized growth) to apply here.
Hans Goudey reviewed 2024-04-29 14:00:01 +02:00
@ -193,0 +195,4 @@
if (!on_back) {
const int num_old_points = curves.points_num();
curves.resize(curves.points_num() + 1, curves.curves_num() + 1);
curves.offsets_for_write().last(1) = num_old_points;
Member

The attributes for the last point and curve are uninitialized now. See fill_attribute_range_default

The attributes for the last point and curve are uninitialized now. See `fill_attribute_range_default`
Member

This is expected I think. The function only makes the space so that the rest of the draw tool can fill in the attributes.

This is expected I think. The function only makes the space so that the rest of the draw tool can fill in the attributes.
Author
Member

I added a comment to make this more clear.

I added a comment to make this more clear.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 1 commit 2024-04-29 19:31:26 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
a72baed3c5
Cleanup: Add comments.
Hans Goudey approved these changes 2024-05-01 14:04:21 +02:00
Member

@blender-bot build

@blender-bot build
Iliya Katushenock reviewed 2024-05-02 15:10:06 +02:00
@ -646,0 +782,4 @@
i < curves.points_num() - rem_points_num;
i++)
{
span_data[i] = span_data[i + rem_points_num];

Any objections to do not use https://en.cppreference.com/w/cpp/string/byte/memmove for usually trivial types of attributes?
@HooglyBoogly /

Any objections to do not use https://en.cppreference.com/w/cpp/string/byte/memmove for _usually_ trivial types of attributes? @HooglyBoogly /
Falk David approved these changes 2024-05-03 18:04:02 +02:00
Falk David merged commit 3f15c244f4 into main 2024-05-03 18:04:53 +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
4 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#120894
No description provided.