Curves: Transform Bezier handles #120222
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#120222
Loading…
Reference in New Issue
No description provided.
Delete Branch "laurynas/blender:transform-bezier-handles"
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?
Allows user to transform Bezier handles.
Curves: Transform Bezier handlesto WIP: Curves: Transform Bezier handles@ -82,3 +133,2 @@
else {
selection_per_object[i] = ed::curves::retrieve_selected_points(curves, memory);
tc.data_len = selection_per_object[i].size();
tc.data_len = std::reduce(
This call to
std::reduce
does not compile for me. It seems like the lambda has an invalid signature.If to comment it, does another
std::reduce
in the same file compile?Lambda's return value type is actually
int64_t
, it should match init value type, which isint
.Can you try casting return expression to
int
?If it's not the reason, I'll rewrite it without
reduce
as it is not used in Blender with that function parameter and I don't know what else could cause this.Both calls to
reduce
don't work for me. Was just testing this on compiler explorer: https://godbolt.org/z/bGcj89ejvNote that it compiles with
-stdlib=libc++
but not without.Better just rewrite it.
Do you consider this patch done or have you planned any further changes? If it's done from your side right now, please remove the "WIP" prefix.
Not quite yet.
I've commented call
curves.calculate_bezier_auto_handles();
. With it Blender crashes in random places if to move Bezier handles. Sometimes inVector
destructor, sometimes on buffer swap. I am stuck on this.Also had intentions to look more on two aligned handles constraint, but it requires more investigation on legacy curves code and I'm a bit busy these days.
If to take these two out from scope it is done.
@JacquesLucke I've root of the problem mentioned above, maybe you could look it through. It can be observed in
main
branch also. Intransform_convert_curves.cc
replacecurves.calculate_bezier_auto_handles()
withcurves.positions_for_write()
. After few frames of moving selected vertex Blender should crash.In the scope of
TransConvertType_Curve.create_trans_data
call methodhandle_positions_left_for_write
is called and it's return value is left referenced byTransInfo.data_container.data.loc
s. Latter after few frames in the scope ofTransConvertType_Curve.recalcData_curve
call methodhandle_positions_left_for_write
gets called again, but layer'ssharing_info
has 2 strong users and layer gets copied and latter deleted (probably when second strong user gets deleted) though layer's data is still referenced byTransInfo
.I don't fully understand all mechanisms behind, but came up with workaround
a3d10274b1
. Same reference to layer's data is shared betweenTransConvertType_Curve.create_trans_data
andTransConvertType_Curve.recalcData_curve
during all transformation process.There is indeed a more fundamental problem in the transform code currently... I created an initial patch to address it: #120631.
I decided to leave
Aligned
and some other handle type dynamics for another patch.WIP: Curves: Transform Bezier handlesto Curves: Transform Bezier handlesMy main high level comment is that it feels like it may be easier not to put everything into a single position array. It's ok, but I wonder if it could become simpler.
@ -67,0 +72,4 @@
int offset = 0;
for (const int i : src_offsets.index_range()) {
dst_offsets[i] = offset;
offset += src_offsets[i].size() * (selection.contains(i) ? multiplier : 1);
Using
IndexMask.contains
should generally be avoided if possible. I wonder if it would be useful to have another iterator likeselection.foreach_in_range(IndexRange(...), [&](int i, bool contained) { ... })
. That could be implemented much more efficiently than callingselection.contains
for each index separately.I've refactored
expand_selected_offsets
. It would be much simpler with proposed iterator, but for now it will do.@ -96,0 +116,4 @@
curves.curves_range(),
curves_transform_data->memory);
/* Alter selection as in legacy curves bezt_select_to_transform_triple_flag(). */
if (bezier_curves[i].size()) {
Use
!.is_empty()
@ -96,0 +138,4 @@
IndexMask must_be_selected_mask = IndexMask::from_indices(must_be_selected.as_span(),
curves_transform_data->memory);
if (must_be_selected.size()) {
selection_per_attribute[i][1] = IndexMask::from_union(
/* Select bezier handles that must be transformed if the main control point is selected. */
@ -96,0 +145,4 @@
}
}
int positions_in_custom_data = 0;
Is this really a position or really just
points_to_transform_num
?@ -169,0 +233,4 @@
curves.positions_for_write(),
curves.handle_positions_left_for_write(),
curves.handle_positions_right_for_write()};
for (const int layer :
Why do we suddenly have multiple layers? Seems like this is mixing layers and selection attribute names.
@ -215,2 +288,4 @@
const blender::IndexMask &bezier_curves)
{
using namespace blender;
/* Term layer in function is used to refer Bezier curve points, left and right handles as layers
"Layer" really should not be used in this context.
I doubted about this terminology myself, but it seemed nice in a context where positions from different domains are joined into one flat array. I'll think how to remove it.
I wonder if we should just go for a much simple solution first where we just make a full copy of the position arrays. That way we can skip a lot of this complex index handling for now. That can be added separately when we see that it actually is a bottleneck.
My first impression that you are right here. I just saw that data structure from #120824 can be reused with little changes.
All indexing complexity is in
use_proportional_edit
and it actually comes fromuse_connected_only
case. For this one to work points has to be arranged in particular order. I don't know maybe that problem could be push down tocalculate_curve_point_distances_for_proportional_editing
, but for me it was easier solution.And regarding full copy that is what I'm doing. It it possible to copy only handle_positions_left/right for Bezier curves, but I quit that idea :)
@ -149,6 +149,11 @@ void gather_group_sizes(OffsetIndices<int> offsets, const IndexMask &mask, Mutab
void gather_group_sizes(OffsetIndices<int> offsets, Span<int> indices, MutableSpan<int> sizes);
OffsetIndices<int> expand_selected_offsets(const OffsetIndices<int> src_offsets,
Not sure this function is so general to be in this file\
Agree, it is not.
@ -173,6 +248,33 @@ static void recalcData_curves(TransInfo *t)
}
}
static OffsetIndices<int> expand_selected_offsets(const OffsetIndices<int> src_offsets,
Separate arrays wouldn't simplify things. I've added return value to
append_positions_to_custom_data
, now all separation is done inside of function and it is one line anyway.There is tricky part that GP layers and curves Bezier handles are treaded equally by
CurvesTransformData
. Code as is will require extra parameter (orlayer
shift in calling code) forcopy_positions_from_curves_transform_custom_data
to support GP with Bezier handles transformation. Still any ideas I came with on restructuring data would not simplify anything.Here is positions layout scheme . All complexity comes from proportional connected case. It requires points to be grouped by curves and arranged that handles are next to their point. I tried to separate this case, but only managed to complicate things more.
Finally in both proportional and normal cases 'Normal transform case' layout is used.
Overall, it feels like one needs to take a step back and maybe just rewrite the whole transform code from scratch instead of adding more complexity. Even more complexity will be necessary to handle the tilt and radius modes too.
I'll give this another test run now, and if I don't find functional issues, I'll accept this for now. I might rewrite this thing from scratch at some point (unless you still want to work on it).
Seems like a more productive way forward to get this basic functionality working on a user-level now, so we can make progress on the project overall. The transform code is well enough isolated that it seems reasonable to refactor it later on without affecting other areas.
@blender-bot build
I have a refactoring idea. It is not total rewrite, but will look better.
Curves: Transform Bezier handlesto WIP: Curves: Transform Bezier handlesPlease remove WIP from the name again once you're ready.
WIP: Curves: Transform Bezier handlesto Curves: Transform Bezier handlesAt a high level, I think the only thing I really care about is that there's no performance cost when there are no Bezier curves. I didn't thoroughly check whether that was the case, it's a bit hard to tell.
@ -95,0 +127,4 @@
if (ELEM(type_left, BEZIER_HANDLE_AUTO, BEZIER_HANDLE_ALIGN) &&
ELEM(type_right, BEZIER_HANDLE_AUTO, BEZIER_HANDLE_ALIGN))
{
must_be_selected.append(point_i);
Appending from a threaded loop isn't threadsafe
Moved to sequential
foreach_index
.@ -176,0 +259,4 @@
}
/**
* Creates map of indexes to `tc.data` representing curve in layout
indexes
->indices
@ -176,0 +265,4 @@
*/
static void fill_map(const CurveType curve_type,
MutableSpan<int> map,
IndexRange curve_points,
More the
map
argument last, use const for the others@ -240,0 +410,4 @@
const int selection_attrs_num = curve_types[curve_i] == CURVE_TYPE_BEZIER ? 3 : 1;
const IndexRange curve_points = points_by_curve[curve_i];
const int total_curve_points = selection_attrs_num * curve_points.size();
map.reinitialize(total_curve_points);
I don't think this is threadsafe (adjusting the vectors from inside this threaded loop)
@ -117,3 +116,2 @@
value_attribute,
points,
use_proportional_edit,
Span(&points, 1),
I think {points} will work?
If to ignore extra assignments in array initialization areas, I see two moments in
use_connected_only
case.Initialization of
bezier_offsets_in_td
.Second without Bezier
map
wouldn't be needed. It adds indirection to referencing.Now I kind of see that
map
could be replaced with extrafor
loop's as it consist of three starting points and walks with respective steps.I don't know if it is worth adding this noise to code.