Geometry Nodes: Points to Curves node #109610
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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#109610
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "mod_moder/blender:points_to_curves"
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?
New node to converts groups of points to curves. Groups
of points defined as
Curve Group ID
attribute.Weight
in curveis used for sort points in each group. Points of result curves
propagate attributes from original points. Implicit conversion
of other geometry types is give up for now.
@blender-bot package
Package build started. Download here when ready.
WIP: Geometry Nodes: Points to Curves nodeto Geometry Nodes: Points to Curves nodeThanks for the patch. As we discussed in the module meeting, I think this is a good node to have. Some notes:
parallel_sort
currently always invokes tbb. It would be good to check if having a threshold under which tbb is not invoked improves performance when many curves are created.O(n)
instead ofO(n log n)
. Not sure if we have an implementation for that already.@ -0,0 +163,4 @@
{
GeometrySet geometry_set = params.extract_input<GeometrySet>("Points");
const Field<int> group_id_field = params.extract_input<Field<int>>("Curve Group ID");
const Field<float> length_field = params.extract_input<Field<float>>("Weight");
Should be called
weight_field
? Not sure wherelength
comes from here.Yes, still undecided on the names. In the end, it seems that the weight is more appropriate here.
Thanks for the test and feedback. I'm still in the process of improvements and fixes. Some things:
Sounds reasonable if a node can work with any geometry component. Since so far it has been decided to have it only for the point cloud, it seems that the moving is to a geometry is a little inconsistent.
Yes, I will have to check if I can still get a v-array, with information about the nature of the data, when I use
add_with_destination
.As i can see in
lib\win64_vc15\tbb\include\tbb\parallel_sort.h
, actuallystd::sort
is used::I'm not sure if there is any point in somehow artificially increasing the min size.
It's fine to move it anyway.
I mainly want the optimization for the case when sorting can be skipped because the weight is constant. Other optimizations are nice, but less important in the first version.
Yeah, you're right, should be fine not to add an extra wrapper function then.
Hope it looks close. I have not yet explored the use of another sort. At the moment, this uses a PR with group deduce functions to process groups.
It's good that you added the optimizations for a single group and a single weight now. However, I think that the whole group detection code should be moved out of this patch. I'd rather see this as a separate PR which a more clear complexity/performance trade-off. Detecting the groups can be simpler in this patch, which would make reviewing it easier + it would make reviewing and testing
BLI_group_deduce.hh
easier as well if it was a separate patch.You could even work on both patches in parallel, since the group detection code can also just be added as an optimization to the Index of Nearest (and maybe other?) node.
@ -0,0 +90,4 @@
const int domain_size = points->totpoint;
Array<int> group_ids(domain_size);
Array<float> weights(domain_size);
Looks like it shouldn't be necessary to always allocate a new array for the weights.
@ -0,0 +106,4 @@
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
curves.cyclic_for_write().fill(false);
if (weights_is_single) {
bke::copy_attributes(points->attributes(),
Does this really have the same behavior? It should propagate non-point attributes to the curve as well.
Do points have a curve-domain attributes?
Oh right, I missed that we are only converting point clouds here and not e.g. meshes.
@ -0,0 +113,4 @@
curves.attributes_for_write());
return curves_id;
}
const Array<int, 2> offset({0, domain_size});
use
std::array
for constant sized arraysLooks very close now and works well in my tests.
@ -0,0 +5,4 @@
#include "node_geometry_util.hh"
#include "BKE_attribute.hh"
#include "BKE_attribute_math.hh"
@ -0,0 +40,4 @@
const auto comparator = [&](const int index_a, const int index_b) {
const float weight_a = weights[index_a];
const float weight_b = weights[index_b];
if (UNLIKELY(weight_a == weight_b)) {
I wouldn't add
UNLIKELY
here, because it can actually be very likely.The idea is that this should not happen. If it does, then it will be slower.
@ -0,0 +47,4 @@
return weight_a > weight_b;
};
threading::parallel_for_each(offsets.index_range(), [&](const int group_index) {
Use
parallel_for
with a grain size of e.g. 256 here. Could be faster when there are many small groups. For larger groups, parallelization happens withparallel_sort
anyway. In the future (separately from this patch), we can investigate splitting the groups into approximately equally sized chunks to better handle a mix of large and small groups.@ -0,0 +58,4 @@
const Span<int> indices,
MutableAttributeAccessor dst_attributes)
{
src_attributes.for_all([&](const AttributeIDRef &id, const AttributeMetaData meta_data) {
May want to use
GeometrySet::gather_attributes_for_propagation
here. There is some extra logic for avoiding e.g. propagating string and built-in attributes.I'd like to avoid using a
GeometrySet
method for the logic of transferring data between two geometries actually, I think that function is at a higher level than is usually needed, and ends up bringing the geometry set argument to a much lower level than it should be.@ -0,0 +62,4 @@
if (id.is_anonymous() && !propagation_info.propagate(id.anonymous_id())) {
return true;
}
const bke::GAttributeReader src = src_attributes.lookup(id);
Pass
ATTR_DOMAIN_POINT
tolookup
, or add an assert thatsrc
is on the point domain.@ -0,0 +71,4 @@
const GVArraySpan src_span(src.varray);
bke::attribute_math::convert_to_static_type(meta_data.data_type, [&](auto dummy) {
using T = decltype(dummy);
array_utils::gather<T, int>(src_span.typed<T>(), indices, dst.span.typed<T>());
This can probably use
blender::bke::attribute_math::gather
which avoids the use ofconvert_to_static_type
here.@ -0,0 +78,4 @@
});
}
static void deduce_curves(const Span<int> indices_of_curves,
I don't really like the term
deduce
here. Maybe something likefind_points_by_group_index
could work as well?@ -0,0 +102,4 @@
}
});
Array<int> indices(deduplicated_groups.size());
Right now I don't fully understand why the loop above is not enough. Can you maybe provide an example for how the indices are changed here?
The groups can be ordered randomly as it's just a hash. This means that their order does not depend on anything. To make it stable even for different ordered a point cloud points, i sort the groups by it id value.
I think, as long as the output is deterministic based on the input, we don't need extra sorting. The user can do that manually if necessary. Also note that the groups are not necessarily a hash.
I mean,
VectorSet<int>.index_of()
is non stabil hash.If I remember correctly, the order of elements in
VectorSet
is deterministic based on insertion order. Did you see anything else?Yes, now, if your delete first element of point cloud, than order of result curves gonna be changed. Not sure if this kind of behavior is too useful or stabile.
In addition, this binds the result of the geometry nodes to the internal implementation of the container. Not sure if it's a good idea to do this. In the future, any changes to the order will also require support for this behavior...
I don't think there is any to make a guarantee about the order here now, as long as it is deterministic. If the input stays the same, the output stays the same, that's what matters.
That said, depending on the input element order is something we do in other nodes too afaik.
@ -0,0 +119,4 @@
}
static Curves *curve_from_points(const AttributeAccessor attributes,
const VArray<float> weights_varray,
I think this can take a reference, since this function does not take ownership of the
VArray
.@ -0,0 +196,4 @@
geometry_set.modify_geometry_sets([&](GeometrySet &geometry_set) {
const PointCloud *points = geometry_set.get_pointcloud();
Curves *curves_id = curves_from_points(
points, group_id_field, weight_field, params.get_output_propagation_info("Curves"));
Call
get_output_propagation_info
outside of themodify_geometry_sets
loop.@ -213,6 +214,7 @@ set(LIB
PRIVATE bf::intern::guardedalloc
bf_nodes
extern_fmtlib
PRIVATE bf::intern::atomic
This seems unnecessary now.
Good some smaller suggestions, but looks good to me now.
Would be great if you can build a regression test file for this node that covers all the cases.
@ -927,6 +927,13 @@ void gather_attributes(AttributeAccessor src_attributes,
const IndexMask &selection,
MutableAttributeAccessor dst_attributes);
void gather_attributes(AttributeAccessor src_attributes,
This should have some comment for why it's different from the function above.
@ -0,0 +23,4 @@
{
b.add_input<decl::Geometry>("Points")
.supported_type(GeometryComponent::Type::PointCloud)
.description("Points to build curves");
Points to generate curves from
@ -0,0 +28,4 @@
.field_on_all()
.hide_value()
.description(
"Index of curve for each point. ID groups will be converted to curves indices without "
A curve is created for every distinct group ID. All points with the same ID are put into the same curve
@ -0,0 +31,4 @@
"Index of curve for each point. ID groups will be converted to curves indices without "
"gaps");
b.add_input<decl::Float>("Weight").field_on_all().hide_value().description(
"Weight to sort points of curve");
Determines the order of points in each curve
@ -0,0 +72,4 @@
}
}
int identifiers_to_indices(MutableSpan<int> r_identifiers_to_indices)
missing
static
Very nice code!
I think there's one special case that might be missing: when the order of curve points matches the order of the input point cloud points. I think it's important to do here, because people end up using these nodes for very trivial conversions. It could also be handled by the new
bke::gather_attributes
function. I'll handle that after this is committed, since it's already done by the mesh to curve node (indices_are_full_ordered_copy
).Just three comments inline then.
@ -1036,0 +1056,4 @@
if (!dst) {
return true;
}
bke::attribute_math::convert_to_static_type(meta_data.data_type, [&](auto dummy) {
Use
attribute_math::gather
here to avoid generating this code againAh, I didn’t know about the gather versions in
attribute_math
!@ -0,0 +91,4 @@
const int domain_size = weights_varray.size();
Curves *curves_id = bke::curves_new_nomain_single(domain_size, CURVE_TYPE_POLY);
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
curves.cyclic_for_write().fill(false);
False is the default value, no need to set that here. Same below.
@ -0,0 +111,4 @@
const Field<float> &weight_field,
const bke::AnonymousAttributePropagationInfo &propagation_info)
{
if (points == nullptr) {
This null check should be handled by the caller IMO, so this function can take a reference as an argument.
I already handle cases where the order is known for a constant O (single weight or group).
But I'm not sure if doing checks for ordering (clearly not const O) is better than just sorting. Since sorting should not be so slow, but at the same time it should be called if the algorithm for checking ordering return a lack of order.
Similar for attribute propagation with the help of sharing. If the groups is single, we are already doing it. But if there are several groups, their order has its own requirements. So we will have to copy attributes in general anyway.
@ -0,0 +165,4 @@
geometry_set.modify_geometry_sets([&](GeometrySet &geometry_set) {
const PointCloud *points = geometry_set.get_pointcloud();
geometry_set.replace_curves(nullptr);
if (points != nullptr) {