Geometry Nodes: Add a 'Group ID' input to the Fill Curve node #114048

Merged
Hans Goudey merged 17 commits from Douglas-Paul/blender:group-id-for-fill-curve-node into main 2023-11-14 11:23:28 +01:00
1 changed files with 173 additions and 30 deletions

View File

@ -31,6 +31,11 @@ static void node_declare(NodeDeclarationBuilder &b)
{
b.add_input<decl::Geometry>("Curve").supported_type(
{GeometryComponent::Type::Curve, GeometryComponent::Type::GreasePencil});
b.add_input<decl::Int>("Group ID")
.supports_field()
Douglas-Paul marked this conversation as resolved
Review

.default_value(0) is unnecessary, that's the implicit default already.

`.default_value(0)` is unnecessary, that's the implicit default already.
.hide_value()
.description(
"An index used to group curves together. Filling is done separately for each group");
b.add_output<decl::Geometry>("Mesh");
}
@ -75,43 +80,178 @@ static meshintersect::CDT_result<double> do_cdt(const bke::CurvesGeometry &curve
return result;
}
/* Converts the CDT result into a Mesh. */
static Mesh *cdt_to_mesh(const meshintersect::CDT_result<double> &result)
static meshintersect::CDT_result<double> do_cdt_with_mask(const bke::CurvesGeometry &curves,
const CDT_output_type output_type,
const IndexMask &mask)
{
const int vert_len = result.vert.size();
const int edge_len = result.edge.size();
const int face_len = result.face.size();
int loop_len = 0;
for (const Vector<int> &face : result.face) {
loop_len += face.size();
}
const OffsetIndices points_by_curve = curves.evaluated_points_by_curve();
const Span<float3> positions = curves.evaluated_positions();
Mesh *mesh = BKE_mesh_new_nomain(vert_len, edge_len, face_len, loop_len);
MutableSpan<float3> positions = mesh->vert_positions_for_write();
mesh->edges_for_write().copy_from(result.edge.as_span().cast<int2>());
MutableSpan<int> face_offsets = mesh->face_offsets_for_write();
MutableSpan<int> corner_verts = mesh->corner_verts_for_write();
int vert_len = 0;
mask.foreach_index([&](const int i) { vert_len += points_by_curve[i].size(); });
for (const int i : IndexRange(result.vert.size())) {
positions[i] = float3(float(result.vert[i].x), float(result.vert[i].y), 0.0f);
}
int i_loop = 0;
for (const int i : IndexRange(result.face.size())) {
face_offsets[i] = i_loop;
for (const int j : result.face[i].index_range()) {
corner_verts[i_loop] = result.face[i][j];
i_loop++;
meshintersect::CDT_input<double> input;
input.need_ids = false;
input.vert.reinitialize(vert_len);
input.face.reinitialize(mask.size());
Array<int> offsets_data(mask.size() + 1);
const OffsetIndices points_by_curve_masked = offset_indices::gather_selected_offsets(
Douglas-Paul marked this conversation as resolved Outdated

When writing this sort of code, we're always looking out for how we can make the serial part boil down to just integer addition. Here's my attempt at that:

  Array<int> offsets_data(mask.size() + 1);
  const OffsetIndices points_by_curve_masked = offset_indices::gather_selected_offsets(
      points_by_curve, mask, offsets_data);

  mask.foreach_index(GrainSize(1024), [&](const int src_curve, const int dst_curve) {
    const IndexRange src_points = points_by_curve[src_curve];
    const IndexRange dst_points = points_by_curve_masked[dst_curve];

    for (const int i : src_points.index_range()) {
      const int src = src_points[i];
      const int dst = dst_points[i];
      input.vert[dst] = double2(positions[src].x, positions[src].y);
    }

    input.face[dst_curve].resize(src_points.size());
    MutableSpan<int> face_verts = input.face[dst_curve];
    array_utils::fill_index_range<int>(input.face[dst_curve], dst_points.start());
  });

Not tested, but I think it works logically. In #111061 I'm also refactoring the CDT code to use something more like the OffsetIndices format, which would remove the need for the "vector of vectors" approach which is much slower.

When writing this sort of code, we're always looking out for how we can make the serial part boil down to just integer addition. Here's my attempt at that: ```cpp Array<int> offsets_data(mask.size() + 1); const OffsetIndices points_by_curve_masked = offset_indices::gather_selected_offsets( points_by_curve, mask, offsets_data); mask.foreach_index(GrainSize(1024), [&](const int src_curve, const int dst_curve) { const IndexRange src_points = points_by_curve[src_curve]; const IndexRange dst_points = points_by_curve_masked[dst_curve]; for (const int i : src_points.index_range()) { const int src = src_points[i]; const int dst = dst_points[i]; input.vert[dst] = double2(positions[src].x, positions[src].y); } input.face[dst_curve].resize(src_points.size()); MutableSpan<int> face_verts = input.face[dst_curve]; array_utils::fill_index_range<int>(input.face[dst_curve], dst_points.start()); }); ``` Not tested, but I think it works logically. In #111061 I'm also refactoring the CDT code to use something more like the `OffsetIndices` format, which would remove the need for the "vector of vectors" approach which is much slower.

Thank you, this makes a lot of sense. Just like "thinking with Portals", it will take some practice to get used to "thinking with offsets".

This inspired me to similarly re-implement cdts_to_mesh() using offsets.

Thank you, this makes a lot of sense. Just like "thinking with Portals", it will take some practice to get used to "thinking with offsets". This inspired me to similarly re-implement `cdts_to_mesh()` using offsets.
points_by_curve, mask, offsets_data);
mask.foreach_index(GrainSize(1024), [&](const int src_curve, const int dst_curve) {
const IndexRange src_points = points_by_curve[src_curve];
const IndexRange dst_points = points_by_curve_masked[dst_curve];
for (const int i : src_points.index_range()) {
const int src = src_points[i];
const int dst = dst_points[i];
input.vert[dst] = double2(positions[src].x, positions[src].y);
}
input.face[dst_curve].resize(src_points.size());
array_utils::fill_index_range<int>(input.face[dst_curve], dst_points.start());
});
meshintersect::CDT_result<double> result = delaunay_2d_calc(input, output_type);
return result;
}
static Array<meshintersect::CDT_result<double>> do_group_aware_cdt(
const bke::CurvesGeometry &curves,
const CDT_output_type output_type,
const Field<int> &group_index_field)
{
const bke::GeometryFieldContext field_context{curves, ATTR_DOMAIN_CURVE};
fn::FieldEvaluator data_evaluator{field_context, curves.curves_num()};
data_evaluator.add(group_index_field);
data_evaluator.evaluate();
const VArray<int> curve_group_ids = data_evaluator.get_evaluated<int>(0);
if (curve_group_ids.is_single()) {
return {do_cdt(curves, output_type)};
Douglas-Paul marked this conversation as resolved Outdated

This can have a special case for curve_group_ids.is_single() to avoid building selections.

A simpler way to do this that will help optimization in the future is using a combination of VectorSet<int> and IndexMask::from_groups. There are some examples of that already.

This can have a special case for `curve_group_ids.is_single()` to avoid building selections. A simpler way to do this that will help optimization in the future is using a combination of `VectorSet<int>` and `IndexMask::from_groups`. There are some examples of that already.

This can have a special case for curve_group_ids.is_single() to avoid building selections.

Yeah, that had seemed like too much of a corner case to be worth adding special handling for, but I suppose it does make sense since it's low complexity.

A simpler way to do this that will help optimization in the future is using a combination of VectorSet<int> and IndexMask::from_groups. There are some examples of that already.

Okay, I made the changes based off of how I see things being done in node_geo_index_of_nearest.cc. I'm not sure I agree it's simpler, but I understand how it's likely more efficient.

> This can have a special case for `curve_group_ids.is_single()` to avoid building selections. Yeah, that had seemed like too much of a corner case to be worth adding special handling for, but I suppose it does make sense since it's low complexity. > A simpler way to do this that will help optimization in the future is using a combination of `VectorSet<int>` and `IndexMask::from_groups`. There are some examples of that already. Okay, I made the changes based off of how I see things being done in `node_geo_index_of_nearest.cc`. I'm not sure I agree it's _simpler_, but I understand how it's likely more efficient.

Right, good point. Less so about simplicity I guess, more about making the pattern recognizable and making it use common code, so we can optimize them all at once at some point.

Right, good point. Less so about simplicity I guess, more about making the pattern recognizable and making it use common code, so we can optimize them all at once at some point.
}
/* The delaunay triangulation doesn't seem to return all of the necessary edges, even in
const VArraySpan<int> group_ids_span(curve_group_ids);
const int domain_size = group_ids_span.size();
VectorSet<int> group_indexing(group_ids_span);
const int groups_num = group_indexing.size();
IndexMaskMemory mask_memory;
Array<IndexMask> group_masks(groups_num);
Douglas-Paul marked this conversation as resolved
Review

This can be done in the constructor:

  const VectorSet<int> group_indexing(group_ids_span);
  const int groups_num = group_indexing.size();

This can be done in the constructor: ```cpp const VectorSet<int> group_indexing(group_ids_span); const int groups_num = group_indexing.size(); ```
Review

I was going to let you know these lines were copied from node_geo_index_of_nearest.cc so you could apply the same fix there, but I see you already did that.

I was going to let you know these lines were copied from `node_geo_index_of_nearest.cc` so you could apply the same fix there, but I see [you already did that](https://projects.blender.org/blender/blender/commit/df4df75317f93516fe7079a455604bc66aa4409b).
IndexMask::from_groups<int>(
IndexMask(domain_size),
mask_memory,
[&](const int i) {
const int group_id = group_ids_span[i];
return group_indexing.index_of(group_id);
},
group_masks);
Array<meshintersect::CDT_result<double>> cdt_results(groups_num);
/* The grain size should be larger as each group gets smaller. */
const int avg_group_size = domain_size / groups_num;
const int grain_size = std::max(8192 / avg_group_size, 1);
threading::parallel_for(IndexRange(groups_num), grain_size, [&](const IndexRange range) {
for (const int group_index : range) {
const IndexMask &mask = group_masks[group_index];
cdt_results[group_index] = do_cdt_with_mask(curves, output_type, mask);
}
});
return cdt_results;
}
/* Converts multiple CDT results into a single Mesh. */
static Mesh *cdts_to_mesh(const Span<meshintersect::CDT_result<double>> results)
{
/* Converting a single CDT result to a Mesh would be simple because the indices could be re-used.
* However, in the general case here we need to combine several CDT results into a single Mesh,
* which requires us to map the original indices to a new set of indices.
* In order to allow for parallelization when appropriate, this implementation starts by
* determining (for each domain) what range of indices in the final mesh data will be used for
* each CDT result. The index ranges are represented as offsets, which are referred to as "group
* offsets" to distinguish them from the other types of offsets we need to work with here.
* Since it's likely that most invocations will only have a single CDT result, it's important
* that case is made as optimal as feasible. */
Array<int> vert_groups_data(results.size() + 1);
Array<int> edge_groups_data(results.size() + 1);
Array<int> face_groups_data(results.size() + 1);
Array<int> loop_groups_data(results.size() + 1);
threading::parallel_for(results.index_range(), 1024, [&](const IndexRange results_range) {
for (const int i_result : results_range) {
const meshintersect::CDT_result<double> &result = results[i_result];
vert_groups_data[i_result] = result.vert.size();
Douglas-Paul marked this conversation as resolved Outdated

We should try to avoid duplicating the logic in cdt_to_mesh.
A better approach is probably creating the mesh, then filling each slice corresponding to a CDT result. Then the 1 mesh case is handled by that by default.

Also, you can pass const Span<meshintersect::CDT_result rather than a const reference to a vector.

We should try to avoid duplicating the logic in `cdt_to_mesh`. A better approach is probably creating the mesh, then filling each slice corresponding to a CDT result. Then the 1 mesh case is handled by that by default. Also, you can pass `const Span<meshintersect::CDT_result` rather than a const reference to a vector.

We should try to avoid duplicating the logic in cdt_to_mesh.
A better approach is probably creating the mesh, then filling each slice corresponding to a CDT result. Then the 1 mesh case is handled by that by default.

I definitely agree it's best to avoid duplicating the logic, but I don't see a way to combine them without a bit of a performance hit to the presumably much more common case of a single group.

Could you elaborate on what you mean by "filling each slice corresponding to a CDT result"? It sounds like you're talking about looping over the CDT results and adding each to the mesh, but that's what cdts_to_mesh() is already doing. I could eliminate cdt_to_mesh(), but again I'm not sure if that's worth the added complexity for the simple cases.

Also, you can pass const Span<meshintersect::CDT_result rather than a const reference to a vector.

Ah, yes, thank you. It will be a while before things like that jump out at me.

> We should try to avoid duplicating the logic in `cdt_to_mesh`. > A better approach is probably creating the mesh, then filling each slice corresponding to a CDT result. Then the 1 mesh case is handled by that by default. I definitely agree it's best to avoid duplicating the logic, but I don't see a way to combine them without a bit of a performance hit to the presumably much more common case of a single group. Could you elaborate on what you mean by "filling each slice corresponding to a CDT result"? It sounds like you're talking about looping over the CDT results and adding each to the mesh, but that's what `cdts_to_mesh()` is already doing. I could [eliminate `cdt_to_mesh()`](https://projects.blender.org/Douglas-Paul/blender/commit/dfa046b9223e50d163ac3b6da7a6974d99d7a491), but again I'm not sure if that's worth the added complexity for the simple cases. > Also, you can pass `const Span<meshintersect::CDT_result` rather than a const reference to a vector. Ah, yes, thank you. It will be a while before things like that jump out at me.

Yes, I'd probably just remove cdt_to_mesh and make sure cdts_to_mesh isn't doing too much extra work in the 1 mesh case. But yeah, this can be changed later too.

Yes, I'd probably just remove `cdt_to_mesh` and make sure `cdts_to_mesh` isn't doing too much extra work in the 1 mesh case. But yeah, this can be changed later too.
edge_groups_data[i_result] = result.edge.size();
face_groups_data[i_result] = result.face.size();
int loop_len = 0;
for (const Vector<int> &face : result.face) {
loop_len += face.size();
}
loop_groups_data[i_result] = loop_len;
}
});
const OffsetIndices vert_groups = offset_indices::accumulate_counts_to_offsets(vert_groups_data);
const OffsetIndices edge_groups = offset_indices::accumulate_counts_to_offsets(edge_groups_data);
const OffsetIndices face_groups = offset_indices::accumulate_counts_to_offsets(face_groups_data);
const OffsetIndices loop_groups = offset_indices::accumulate_counts_to_offsets(loop_groups_data);
Mesh *mesh = BKE_mesh_new_nomain(vert_groups.total_size(),
edge_groups.total_size(),
face_groups.total_size(),
loop_groups.total_size());
MutableSpan<float3> all_positions = mesh->vert_positions_for_write();
MutableSpan<int2> all_edges = mesh->edges_for_write();
MutableSpan<int> all_face_offsets = mesh->face_offsets_for_write();
MutableSpan<int> all_corner_verts = mesh->corner_verts_for_write();
threading::parallel_for(results.index_range(), 1024, [&](const IndexRange results_range) {
for (const int i_result : results_range) {
const meshintersect::CDT_result<double> &result = results[i_result];
const IndexRange verts_range = vert_groups[i_result];
const IndexRange edges_range = edge_groups[i_result];
const IndexRange faces_range = face_groups[i_result];
const IndexRange loops_range = loop_groups[i_result];
MutableSpan<float3> positions = all_positions.slice(verts_range);
for (const int i : result.vert.index_range()) {
positions[i] = float3(float(result.vert[i].x), float(result.vert[i].y), 0.0f);
}
MutableSpan<int2> edges = all_edges.slice(edges_range);
for (const int i : result.edge.index_range()) {
edges[i] = int2(result.edge[i].first + verts_range.start(),
result.edge[i].second + verts_range.start());
}
MutableSpan<int> face_offsets = all_face_offsets.slice(faces_range);
MutableSpan<int> corner_verts = all_corner_verts.slice(loops_range);
int i_face_corner = 0;
for (const int i_face : result.face.index_range()) {
face_offsets[i_face] = i_face_corner + loops_range.start();
for (const int i_corner : result.face[i_face].index_range()) {
corner_verts[i_face_corner] = result.face[i_face][i_corner] + verts_range.start();
i_face_corner++;
}
}
}
});
/* The delaunay triangulation doesn't seem to return all of the necessary all_edges, even in
* triangulation mode. */
BKE_mesh_calc_edges(mesh, true, false);
BKE_mesh_smooth_flag_set(mesh, false);
return mesh;
}
static void curve_fill_calculate(GeometrySet &geometry_set, const GeometryNodeCurveFillMode mode)
static void curve_fill_calculate(GeometrySet &geometry_set,
const GeometryNodeCurveFillMode mode,
const Field<int> &group_index)
{
const CDT_output_type output_type = (mode == GEO_NODE_CURVE_FILL_MODE_NGONS) ?
CDT_CONSTRAINTS_VALID_BMESH_WITH_HOLES :
@ -120,8 +260,9 @@ static void curve_fill_calculate(GeometrySet &geometry_set, const GeometryNodeCu
const Curves &curves_id = *geometry_set.get_curves();
const bke::CurvesGeometry &curves = curves_id.geometry.wrap();
if (curves.curves_num() > 0) {
const meshintersect::CDT_result<double> results = do_cdt(curves, output_type);
Mesh *mesh = cdt_to_mesh(results);
const Array<meshintersect::CDT_result<double>> results = do_group_aware_cdt(
curves, output_type, group_index);
Mesh *mesh = cdts_to_mesh(results);
geometry_set.replace_mesh(mesh);
}
geometry_set.replace_curves(nullptr);
@ -140,8 +281,9 @@ static void curve_fill_calculate(GeometrySet &geometry_set, const GeometryNodeCu
if (src_curves.curves_num() == 0) {
continue;
}
const meshintersect::CDT_result<double> results = do_cdt(src_curves, output_type);
mesh_by_layer[layer_index] = cdt_to_mesh(results);
const Array<meshintersect::CDT_result<double>> results = do_group_aware_cdt(
src_curves, output_type, group_index);
mesh_by_layer[layer_index] = cdts_to_mesh(results);
}
if (!mesh_by_layer.is_empty()) {
InstancesComponent &instances_component =
@ -172,12 +314,13 @@ static void curve_fill_calculate(GeometrySet &geometry_set, const GeometryNodeCu
static void node_geo_exec(GeoNodeExecParams params)
{
GeometrySet geometry_set = params.extract_input<GeometrySet>("Curve");
Field<int> group_index = params.extract_input<Field<int>>("Group ID");
const NodeGeometryCurveFill &storage = node_storage(params.node());
const GeometryNodeCurveFillMode mode = (GeometryNodeCurveFillMode)storage.mode;
geometry_set.modify_geometry_sets(
[&](GeometrySet &geometry_set) { curve_fill_calculate(geometry_set, mode); });
[&](GeometrySet &geometry_set) { curve_fill_calculate(geometry_set, mode, group_index); });
params.set_output("Mesh", std::move(geometry_set));
}