From a82e52102b0f7ddfe3741fccdaa9de5f480c59fe Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Wed, 21 Sep 2022 12:06:53 -0500 Subject: Mesh: Avoid uninitialized values when converting BMesh to Mesh I didn't observe this issue in practice, but since the write_only version of the attribute API isn't meant to initialize trivial types, theoretically this could be a problem if the attribute is created halfway through converting the BMesh to a Mesh. --- source/blender/blenkernel/BKE_attribute.hh | 2 ++ source/blender/bmesh/intern/bmesh_mesh_convert.cc | 12 ++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.hh b/source/blender/blenkernel/BKE_attribute.hh index 946a7d21580..b1f4039f788 100644 --- a/source/blender/blenkernel/BKE_attribute.hh +++ b/source/blender/blenkernel/BKE_attribute.hh @@ -692,6 +692,8 @@ class MutableAttributeAccessor : public AttributeAccessor { * The "only" in the name indicates that the caller should not read existing values from the * span. If the attribute is not stored as span internally, the existing values won't be copied * over to the span. + * + * For trivial types, the values in a newly created attribute will not be initialized. */ GSpanAttributeWriter lookup_or_add_for_write_only_span(const AttributeIDRef &attribute_id, const eAttrDomain domain, diff --git a/source/blender/bmesh/intern/bmesh_mesh_convert.cc b/source/blender/bmesh/intern/bmesh_mesh_convert.cc index aa8972f9d14..6f1552e6a0f 100644 --- a/source/blender/bmesh/intern/bmesh_mesh_convert.cc +++ b/source/blender/bmesh/intern/bmesh_mesh_convert.cc @@ -1279,7 +1279,7 @@ void BM_mesh_bm_to_me_for_eval(BMesh *bm, Mesh *me, const CustomData_MeshMasks * mv->flag = BM_vert_flag_to_mflag(eve); if (BM_elem_flag_test(eve, BM_ELEM_HIDDEN)) { if (!hide_vert_attribute) { - hide_vert_attribute = mesh_attributes.lookup_or_add_for_write_only_span( + hide_vert_attribute = mesh_attributes.lookup_or_add_for_write_span( ".hide_vert", ATTR_DOMAIN_POINT); } hide_vert_attribute.span[i] = true; @@ -1301,8 +1301,8 @@ void BM_mesh_bm_to_me_for_eval(BMesh *bm, Mesh *me, const CustomData_MeshMasks * med->flag = BM_edge_flag_to_mflag(eed); if (BM_elem_flag_test(eed, BM_ELEM_HIDDEN)) { if (!hide_edge_attribute) { - hide_edge_attribute = mesh_attributes.lookup_or_add_for_write_only_span( - ".hide_edge", ATTR_DOMAIN_EDGE); + hide_edge_attribute = mesh_attributes.lookup_or_add_for_write_span(".hide_edge", + ATTR_DOMAIN_EDGE); } hide_edge_attribute.span[i] = true; } @@ -1337,8 +1337,8 @@ void BM_mesh_bm_to_me_for_eval(BMesh *bm, Mesh *me, const CustomData_MeshMasks * mp->flag = BM_face_flag_to_mflag(efa); if (BM_elem_flag_test(efa, BM_ELEM_HIDDEN)) { if (!hide_poly_attribute) { - hide_poly_attribute = mesh_attributes.lookup_or_add_for_write_only_span( - ".hide_poly", ATTR_DOMAIN_FACE); + hide_poly_attribute = mesh_attributes.lookup_or_add_for_write_span(".hide_poly", + ATTR_DOMAIN_FACE); } hide_poly_attribute.span[i] = true; } @@ -1346,7 +1346,7 @@ void BM_mesh_bm_to_me_for_eval(BMesh *bm, Mesh *me, const CustomData_MeshMasks * mp->loopstart = j; if (efa->mat_nr != 0) { if (!material_index_attribute) { - material_index_attribute = mesh_attributes.lookup_or_add_for_write_only_span( + material_index_attribute = mesh_attributes.lookup_or_add_for_write_span( "material_index", ATTR_DOMAIN_FACE); } material_index_attribute.span[i] = efa->mat_nr; -- cgit v1.2.3 From 600c069e0ef5334575164e5c4d758efe7324b0c7 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Wed, 21 Sep 2022 12:19:04 -0500 Subject: Attributes: Correct implementation of typed "write_only" method The typed "lookup_or_add_for_write_only" function is meant to do the same thing as the non-typed version of the function. Instead, it still initialized values of new attribute arrays, which isn't meant to happen. Missed in 4c91c24bc7cbe2c4f97be373. I also had to correct one place that used the "write_only" function but didn't intialize all values. --- source/blender/blenkernel/BKE_attribute.hh | 4 +++- source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.hh b/source/blender/blenkernel/BKE_attribute.hh index b1f4039f788..97a8a91d0e4 100644 --- a/source/blender/blenkernel/BKE_attribute.hh +++ b/source/blender/blenkernel/BKE_attribute.hh @@ -706,7 +706,9 @@ class MutableAttributeAccessor : public AttributeAccessor { SpanAttributeWriter lookup_or_add_for_write_only_span(const AttributeIDRef &attribute_id, const eAttrDomain domain) { - AttributeWriter attribute = this->lookup_or_add_for_write(attribute_id, domain); + AttributeWriter attribute = this->lookup_or_add_for_write( + attribute_id, domain, AttributeInitConstruct()); + if (attribute) { return SpanAttributeWriter{std::move(attribute), false}; } diff --git a/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc b/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc index edf14f664c5..1f9ad9f6ea2 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc @@ -485,7 +485,7 @@ static void calculate_selection_outputs(Mesh *mesh, /* Populate "Top" selection output. */ if (attribute_outputs.top_id) { const bool face = !config.top_is_point && config.fill_type != GEO_NODE_MESH_CIRCLE_FILL_NONE; - SpanAttributeWriter selection = attributes.lookup_or_add_for_write_only_span( + SpanAttributeWriter selection = attributes.lookup_or_add_for_write_span( attribute_outputs.top_id.get(), face ? ATTR_DOMAIN_FACE : ATTR_DOMAIN_POINT); if (config.top_is_point) { @@ -501,7 +501,7 @@ static void calculate_selection_outputs(Mesh *mesh, if (attribute_outputs.bottom_id) { const bool face = !config.bottom_is_point && config.fill_type != GEO_NODE_MESH_CIRCLE_FILL_NONE; - SpanAttributeWriter selection = attributes.lookup_or_add_for_write_only_span( + SpanAttributeWriter selection = attributes.lookup_or_add_for_write_span( attribute_outputs.bottom_id.get(), face ? ATTR_DOMAIN_FACE : ATTR_DOMAIN_POINT); if (config.bottom_is_point) { @@ -518,7 +518,7 @@ static void calculate_selection_outputs(Mesh *mesh, /* Populate "Side" selection output. */ if (attribute_outputs.side_id) { - SpanAttributeWriter selection = attributes.lookup_or_add_for_write_only_span( + SpanAttributeWriter selection = attributes.lookup_or_add_for_write_span( attribute_outputs.side_id.get(), ATTR_DOMAIN_FACE); selection.span.slice(config.side_faces_start, config.side_faces_len).fill(true); -- cgit v1.2.3 From 91dd29fd45d190728f29cc2f6cf9cd5549392f61 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Wed, 21 Sep 2022 13:17:05 -0500 Subject: Attributes: Allow calling "finish" on empty accessors This removes some boilerplate when creating many optional attributes. --- source/blender/blenkernel/BKE_attribute.hh | 8 ++++-- source/blender/bmesh/intern/bmesh_mesh_convert.cc | 17 +++--------- .../blender/geometry/intern/realize_instances.cc | 30 ++++++---------------- .../nodes/node_geo_distribute_points_on_faces.cc | 9 ++----- 4 files changed, 20 insertions(+), 44 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.hh b/source/blender/blenkernel/BKE_attribute.hh index 97a8a91d0e4..7b13b8a2b09 100644 --- a/source/blender/blenkernel/BKE_attribute.hh +++ b/source/blender/blenkernel/BKE_attribute.hh @@ -264,7 +264,9 @@ template struct SpanAttributeWriter { */ void finish() { - this->span.save(); + if (this->span.varray()) { + this->span.save(); + } if (this->tag_modified_fn) { this->tag_modified_fn(); } @@ -339,7 +341,9 @@ struct GSpanAttributeWriter { void finish() { - this->span.save(); + if (this->span.varray()) { + this->span.save(); + } if (this->tag_modified_fn) { this->tag_modified_fn(); } diff --git a/source/blender/bmesh/intern/bmesh_mesh_convert.cc b/source/blender/bmesh/intern/bmesh_mesh_convert.cc index 6f1552e6a0f..399d5e1517b 100644 --- a/source/blender/bmesh/intern/bmesh_mesh_convert.cc +++ b/source/blender/bmesh/intern/bmesh_mesh_convert.cc @@ -1368,21 +1368,12 @@ void BM_mesh_bm_to_me_for_eval(BMesh *bm, Mesh *me, const CustomData_MeshMasks * } bm->elem_index_dirty &= ~(BM_FACE | BM_LOOP); - if (material_index_attribute) { - material_index_attribute.finish(); - } - assert_bmesh_has_no_mesh_only_attributes(*bm); - if (hide_vert_attribute) { - hide_vert_attribute.finish(); - } - if (hide_edge_attribute) { - hide_edge_attribute.finish(); - } - if (hide_poly_attribute) { - hide_poly_attribute.finish(); - } + material_index_attribute.finish(); + hide_vert_attribute.finish(); + hide_edge_attribute.finish(); + hide_poly_attribute.finish(); me->cd_flag = BM_mesh_cd_flag_from_bmesh(bm); } diff --git a/source/blender/geometry/intern/realize_instances.cc b/source/blender/geometry/intern/realize_instances.cc index 29a9f51c0a7..8133a685eb8 100644 --- a/source/blender/geometry/intern/realize_instances.cc +++ b/source/blender/geometry/intern/realize_instances.cc @@ -782,9 +782,7 @@ static void execute_realize_pointcloud_tasks(const RealizeInstancesOptions &opti dst_attribute.finish(); } positions.finish(); - if (point_ids) { - point_ids.finish(); - } + point_ids.finish(); } /** \} */ @@ -1107,12 +1105,8 @@ static void execute_realize_mesh_tasks(const RealizeInstancesOptions &options, for (GSpanAttributeWriter &dst_attribute : dst_attribute_writers) { dst_attribute.finish(); } - if (vertex_ids) { - vertex_ids.finish(); - } - if (material_indices) { - material_indices.finish(); - } + vertex_ids.finish(); + material_indices.finish(); } /** \} */ @@ -1406,19 +1400,11 @@ static void execute_realize_curve_tasks(const RealizeInstancesOptions &options, for (GSpanAttributeWriter &dst_attribute : dst_attribute_writers) { dst_attribute.finish(); } - if (point_ids) { - point_ids.finish(); - } - if (radius) { - radius.finish(); - } - if (resolution) { - resolution.finish(); - } - if (all_curves_info.create_handle_postion_attributes) { - handle_left.finish(); - handle_right.finish(); - } + point_ids.finish(); + radius.finish(); + resolution.finish(); + handle_left.finish(); + handle_right.finish(); } /** \} */ diff --git a/source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc b/source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc index a007f6afcc7..cdcb16985ac 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc @@ -382,13 +382,8 @@ BLI_NOINLINE static void compute_attribute_outputs(const Mesh &mesh, } ids.finish(); - - if (normals) { - normals.finish(); - } - if (rotations) { - rotations.finish(); - } + normals.finish(); + rotations.finish(); } static Array calc_full_density_factors_with_selection(const Mesh &mesh, -- cgit v1.2.3