GPv3: Draw tool: "Draw on back" option #120894
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#120894
Loading…
Reference in New Issue
No description provided.
Delete Branch "casey-bianco-davis/blender:GPv3-Draw-on-back"
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?
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.
@ -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
@ -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)@ -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.
Did a pass on this, thanks for working on it.
@ -193,0 +199,4 @@
return;
}
const bke::CurvesGeometry temporal_copy = bke::CurvesGeometry(curves);
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" ?
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.
I think the problem is thatcopy_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..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.@ -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);
See my comment for
create_blank_curve
.@ -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);
... != 0;
@ -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);
... != 0;
@ -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();
const int active_curve ...
@ -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];
Looks like this line can be removed and integrated in the line below:
@ -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);
const Scene *
@ -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);
... != 0;
@ -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);
See my comment for
create_blank_curve
.@ -193,0 +210,4 @@
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
copy_attributes_group_to_group(src_attributes,
I'd expect
bke::
to be necessary hereSeems to be needed, does not compile without.
GPv3: Draw on back option.to GPv3: Draw tool: "Draw on back" option@ -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
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?
Draw on back
means that the draw operator draws into a stroke that needs to be the first curve in theCurvesGeometry
. So we need to make space for that first curve. This is what this is doing.Ah, thanks!
I don't think this is resolved, there is still the double copy of attribute arrays.
@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];
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.
@ -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;
The attributes for the last point and curve are uninitialized now. See
fill_attribute_range_default
This is expected I think. The function only makes the space so that the rest of the draw tool can fill in the attributes.
I added a comment to make this more clear.
@blender-bot build
@ -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 /