Curves: Add extrude operator #116354
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#116354
Loading…
Reference in New Issue
No description provided.
Delete Branch "laurynas/blender:curves-extrude-op"
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?
Adds extrude operator to new curves. Press key E in edit mode to invoke.
Demo shows extrusion on endpoint NURBS curve.
extrude.blend
It works correctly on Bezier curves also, but result is weird as there is no way to control Bezier handles interactively.
Currently operator works same way as in old curves, but effect is a bit inconsistent in cyclic curves.
cyclic.blend
Maybe it is reasonable to unify end point extrusion with inner point extrusion in cyclics?
PS
Algorithms idea is same as in https://archive.blender.org/developer/D15524.
Making quick progress, nice! I left some big-picture comments for now, didn't test it yet.
@ -0,0 +12,4 @@
namespace blender::ed::curves {
struct CurveCopy {
I'd suggest doing a "array of structs" to "struct of arrays" transformation and split this struct into separate arrays for each member. That would have a few benefits here:
selection.foreach_range
,OffsetIndices
,array_utils::gather
, etc.Well
CurveCopy
is refactored, but I'll have to think deeper regarding abstractions. Somehow I didn't noticeselection.foreach_range
exists, only it might change a lot.By the way I was thinking about using
OffsetIndices
, but comment says "References an array of ascending indices."Here I have "non decreasing indices". Will it work?
Not totally sure what the requirements are here, didn't look that in depth yet. But an array like
5, 5, 6, 8, 21, 21
will work; repeating offsets are okay, it just means that group is empty.I need to express ranges like
[0, 2][2, 2][2, 4]
. Ranges always share end points with a neighbor.Copied to destination indexes would result in
[0, 1, 2, 2, 2, 3, 4]
.This would be extrusion of point 2 in 5 point curve
[0, .., 4]
.Because of sharing neighbor I currently express this as
[0, 2, 2, 4]
and iterate with(i, i + 1)
.So for destination I see use of
OffsetIndices
, but for source indexes not sure.Ideally if to handle multiple curves in one data structure. Extrusion of points (2) and (7, 8) in curves
[0, .., 4][5, .., 9]
could look like:a.
[0, 2][2, 2][2, 4][5, 7][7, 8][8, 9]
. Here sharing endpoint rule is broken.b.
[0, 2][2, 2][2, 7][7, 8][8, 9]
. This would reduce copying count, but because of special cases is very hard to implement. Before I had a working version, I gave up on this after few days of trying.Any Thoughts?
Just checked code of
OffsetIndices
. It uses second index as exclusive, in this situation both inclusive needed.OffsetIndices
calculatesIndexRange
size bysize = end - begin;
, I needsize = last - first + 1;
.Can't think of any smart way to restructure data to make use of
OffsetIndices
...That's fine, no need to force it! Thanks for taking a look though.
@ -0,0 +70,4 @@
}
static int sel_to_copy_ints(const IndexMask selection,
const Span<int> &offsets,
Pass
Span
by value, otherwise it's like a pointer to a pointer. AndIndexMask
is passed by const reference :PI don't have a good explanation for how it happened :)
@ -0,0 +144,4 @@
Object *obedit = CTX_data_edit_object(C);
Curves *curves_id = static_cast<Curves *>(obedit->data);
const eAttrDomain selection_domain = eAttrDomain(curves_id->selection_domain);
if (selection_domain == ATTR_DOMAIN_POINT) {
Flip the condition and un-indent the rest of the function, same below with the empty selection check
@ -0,0 +152,4 @@
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
const int max = selection.size() * 2 + curves.curves_num() * 2;
int *const all_copy_intervals = static_cast<int *>(
Use
Array
rather than manual allocations@ -0,0 +164,4 @@
bke::CurvesGeometry new_curves(new_points_count, curves.curves_num());
MutableSpan<int> offsets = new_curves.offsets_for_write();
const MutableSpan<int8_t> new_types = new_curves.curve_types_for_write();
Unused types variables here
@ -0,0 +167,4 @@
const MutableSpan<int8_t> new_types = new_curves.curve_types_for_write();
const VArray<int8_t> old_types = curves.curve_types();
CustomData_copy(
Let's use the attribute API fully, without
CustomData
. That might require a bit more boilerplate, but that's easy to clean up in the future, and we have a high level goal of replacingCustomData
storage internally@ -0,0 +179,4 @@
}
bke::MutableAttributeAccessor new_attributes = new_curves.attributes_for_write();
bke::MutableAttributeAccessor old_attributes = curves.attributes_for_write();
I guess
old_attributes
could be abke::AttributeAccessor
since those values don't have to be changed?Couldn't find how to do attribute value copying from
bke::AttributeAccessor
toMutableAttributeAccessor
in chunks.Like with
type.copy_assign_n(src.data(), dst.data(), dst.size());
You can use
const GVArraySpan src_data = *src.lookup(name);
to make sure you have a span of source data@ -0,0 +193,4 @@
const int last = copy.intervals[i + 1];
const int size = last - first + 1;
copy_attributes(old_attributes, new_attributes, first, d, size);
Usually these algorithms are structured to only iterate over all the attributes once. So the first part will build mappings from input data to result data. In this case, that might be a combination of instructions for copying contiguous sections of data, and some data for which indices the copied points are from.
That structure can significantly improve performance, since it works much better with the cache of CPUs, since we access less data at once. It might also help separate concerns too.
@ -0,0 +217,4 @@
void CURVES_OT_extrude(wmOperatorType *ot)
{
ot->name = "Extrude";
ot->idname = __func__;
I think leaving
ot->description
blank will give some warning somewhere in the futureCurves extrude operatorto WIP: Curves extrude operatorWIP: Curves extrude operatorto Curves extrude operatorFunctionality wise this is working great. I didn't have the time (or energy, I guess) to go over the logic completely. But I'll leave some opinions here anyway :)
CurvesExtrusion
class doesn't seem necessary, compared to implementing the same logic with free functions that have the necessary arguments themselves. It's not a big deal, but generally it's easiest to work on this sort of code if the arguments are clear and functions are separated.d
variable that's used as a counter in the result points. Maybe that could be precomputed. Again, not a big deal for now, but multithreaded copying like this can often make a noticeable difference.@ -0,0 +156,4 @@
curve_intervals_[interval_offset_ + ins] = last_elem;
ins++;
}
// check for extrusion from one point
Comment style
@ -0,0 +208,4 @@
const Span<int> old_offsets = curves.offsets();
CurvesExtrusion curves_copy(old_offsets, extruded_points);
bke::CurvesGeometry new_curves(curves_copy.curve_offsets().last(), curves.curves_num());
The
bke::curves::copy_only_curve_domain
utility inBKE_curves_utils.hh
takes care of creating new curves and copying over the curve domain attributes for you.@ -0,0 +214,4 @@
offsets.copy_from(curves_copy.curve_offsets());
int d = 0;
const int *curve_intervals = curves_copy.curve_intervals();
Using a Span still makes sense here probably, rather than a raw pointer
@ -0,0 +247,4 @@
const GVArraySpan src = (*src_attributes.lookup(id, meta_data.domain));
if (meta_data.domain == ATTR_DOMAIN_POINT) {
d = 0;
This variable could be declared here, and could have a better name besides
d
that reflected what it was doingNo multithreading yet. I'll think about it next year :)
Done.
@ -0,0 +13,4 @@
namespace blender::ed::curves {
/* Stores information need to create new curves by copying data from original ones. */
struct CurvesCopy {
This is getting more subjective now, and I think the PR is getting close. But my one larger remaining comment is that bundling all this data in a single struct (
CurvesCopy
) doesn't really help the situation. It may be clearer to declare each array as its needed in the caller. That gives a nice order of execution to the reader, and clarifies which parts of the algorithm need which data.Sometimes that will make it easier to separate different parts of the algorithm into separate loops too. For example, maybe some data can be calculated in parallel while other data can't. Not sure if that applies here.
@ -0,0 +121,4 @@
void finish_curve(int &curve_index,
int &interval_offset,
int ins,
Could you expand
ins
to a name that helps explain the code to the reader? Generally this sort of contraction makes this sort of thing more complex@ -0,0 +144,4 @@
curve_index++;
}
void finish_curve_or_shallow_copy(int &curve_index,
Add
static
to functions that aren't exposed to the header@ -0,0 +146,4 @@
void finish_curve_or_shallow_copy(int &curve_index,
int &interval_offset,
int ins,
It's not totally clear what "shallow copy" means here. Usually I'd imagine that copying some data structure but not any data it owns itself.
I meant here: copy curve as is or copy curve without modifications. Maybe finish_curve_or_full_copy ?
Full copy sounds good!
@ -0,0 +216,4 @@
{
Object *obedit = CTX_data_edit_object(C);
Curves *curves_id = static_cast<Curves *>(obedit->data);
const bke::AttrDomain selection_domain = bke::AttrDomain(curves_id->selection_domain);
Just realized this should handle multi-object edit mode too. There's an example of that in
CURVES_OT_delete
, or other operators around there.@ -0,0 +227,4 @@
return OPERATOR_FINISHED;
}
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
Looks like this can be a const pointer, since the new curves are created separately
@ -0,0 +238,4 @@
bke::CurvesGeometry new_curves = bke::curves::copy_only_curve_domain(curves);
new_curves.resize(curve_offsets.last(), curves.curves_num());
MutableSpan<int> offsets = new_curves.offsets_for_write();
If you create the new curves before
calc_curves_extrusion
, you can copy the new offsets directly into the new curves' offsets array. If you don't know the number of points at that point, you can resize just the point domain after.@ -0,0 +246,4 @@
curves_copy.curve_intervals.size()};
bke::GSpanAttributeWriter selection = ensure_selection_attribute(
new_curves, selection_domain, CD_PROP_BOOL);
We know the domain is
bke::AttrDomain::Point
at this point.I noticed that this won't copy the float vs. bool status of the input selection though. Since these curves are completely new, it will always create a boolean selection.
It would be pretty fancy to be able to copy the float values from the input curves as well. But if you don't feel inspired to do that, it's not a big deal either :)
Don't know why selection is made with two possible types, but extrude shouldn't change the type.
In sculpt mode there's "soft" selection which uses floats. Any value greater than zero is considered selected in edit mode.
I was going to ask if other values besides 1.0f are possible, but decided that this can not be :)
Good that you told this.
@ -0,0 +249,4 @@
new_curves, selection_domain, CD_PROP_BOOL);
threading::parallel_for(curves.curves_range(), 256, [&](IndexRange curves_range) {
for (const int c : curves_range) {
c
->curve
@ -0,0 +288,4 @@
});
attribute.dst.finish();
}
new_curves.update_curve_types();
This should be unnecessary, since
copy_only_curve_domain
copies this info@ -0,0 +290,4 @@
}
new_curves.update_curve_types();
curves_id->geometry.wrap() = new_curves;
std::move(new_curves)
to avoid a copyCurves extrude operatorto Curves: Add extrude operatorNot sure if this can be reused for GPv3 extrude operator.
It's hard to tell. I had to search what GP stands for :)
I'd don't know code basis for GP, but if it's based on
CurvesGeometry
then should be.From UI side also I'm not sure what extrude should do on a stroke. I couldn't not find how to select single point.
Maybe there is an architecture document to read?
GPv3: new grease pencil architecture (3rd generation)
It uses CurvesGeometry but curve type is
CURVE_TYPE_POLY
https://developer.blender.org/docs/features/grease_pencil/architecture/ :)
Curve type doesn't matter, extrude treats all curves the same. At least at this stage.
From functional side or user perspective GP extrude in Blender 4.0 does different things
than extrude in old curves. It doesn't extend stroke, but creates new ones.
Extrude in this PR is equivalent to one of old curves.
So if in 4.0 GP extrude does what it should, then this one will not do the work.