From cf771807b7997949611dbf76b43150592c9977cb Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Fri, 29 Oct 2021 09:28:31 +0200 Subject: Geometry Nodes: do cache invalidation after writing attributes This is a better and more general fix for T92511 and T92508 than the ones that I committed before. Previously, we tagged caches dirty when first accessing attributes. This led to incorrect caches when under some circumstances. Now cache invalidation is part of `OutputAttribute.save()`. A nice side benefit of this change is that it may make things more efficient in some cases, because we don't invalidate caches when they don't have to be invalidated. Differential Revision: https://developer.blender.org/D13009 --- source/blender/blenkernel/BKE_attribute_access.hh | 2 + source/blender/blenkernel/BKE_spline.hh | 2 + .../blender/blenkernel/intern/attribute_access.cc | 43 ++++++++++++--- .../blenkernel/intern/attribute_access_intern.hh | 4 +- source/blender/blenkernel/intern/curve_eval.cc | 7 +++ .../blenkernel/intern/geometry_component_curve.cc | 63 +++++++++++++--------- .../intern/geometry_component_instances.cc | 18 ++++--- .../blenkernel/intern/geometry_component_mesh.cc | 2 +- 8 files changed, 98 insertions(+), 43 deletions(-) (limited to 'source') diff --git a/source/blender/blenkernel/BKE_attribute_access.hh b/source/blender/blenkernel/BKE_attribute_access.hh index 38f5497ed92..cf19eb29a0e 100644 --- a/source/blender/blenkernel/BKE_attribute_access.hh +++ b/source/blender/blenkernel/BKE_attribute_access.hh @@ -183,6 +183,8 @@ struct WriteAttributeLookup { GVMutableArrayPtr varray; /* Domain the attributes lives on in the geometry. */ AttributeDomain domain; + /* Call this after changing the attribute to invalidate caches that depend on this attribute. */ + std::function tag_modified_fn; /* Convenience function to check if the attribute has been found. */ operator bool() const diff --git a/source/blender/blenkernel/BKE_spline.hh b/source/blender/blenkernel/BKE_spline.hh index 97e0d8415a5..8509b730709 100644 --- a/source/blender/blenkernel/BKE_spline.hh +++ b/source/blender/blenkernel/BKE_spline.hh @@ -570,6 +570,8 @@ struct CurveEval { blender::Array evaluated_point_offsets() const; blender::Array accumulated_spline_lengths() const; + void mark_cache_invalid(); + void assert_valid_point_attributes() const; }; diff --git a/source/blender/blenkernel/intern/attribute_access.cc b/source/blender/blenkernel/intern/attribute_access.cc index 1ea7f522bef..01b53baf237 100644 --- a/source/blender/blenkernel/intern/attribute_access.cc +++ b/source/blender/blenkernel/intern/attribute_access.cc @@ -360,7 +360,7 @@ GVArrayPtr BuiltinCustomDataLayerProvider::try_get_for_read( return as_read_attribute_(data, domain_size); } -GVMutableArrayPtr BuiltinCustomDataLayerProvider::try_get_for_write( +WriteAttributeLookup BuiltinCustomDataLayerProvider::try_get_for_write( GeometryComponent &component) const { if (writable_ != Writable) { @@ -397,10 +397,14 @@ GVMutableArrayPtr BuiltinCustomDataLayerProvider::try_get_for_write( data = new_data; } + std::function tag_modified_fn; if (update_on_write_ != nullptr) { - update_on_write_(component); + tag_modified_fn = [component = &component, update = update_on_write_]() { + update(*component); + }; } - return as_write_attribute_(data, domain_size); + + return {as_write_attribute_(data, domain_size), domain_, std::move(tag_modified_fn)}; } bool BuiltinCustomDataLayerProvider::try_delete(GeometryComponent &component) const @@ -925,7 +929,7 @@ blender::bke::WriteAttributeLookup GeometryComponent::attribute_try_get_for_writ const BuiltinAttributeProvider *builtin_provider = providers->builtin_attribute_providers().lookup_default_as(attribute_id.name(), nullptr); if (builtin_provider != nullptr) { - return {builtin_provider->try_get_for_write(*this), builtin_provider->domain()}; + return builtin_provider->try_get_for_write(*this); } } for (const DynamicAttributesProvider *dynamic_provider : @@ -1249,6 +1253,20 @@ static void save_output_attribute(OutputAttribute &output_attribute) varray.get(i, buffer); write_attribute.varray->set_by_relocate(i, buffer); } + if (write_attribute.tag_modified_fn) { + write_attribute.tag_modified_fn(); + } +} + +static std::function get_simple_output_attribute_save_method( + const blender::bke::WriteAttributeLookup &attribute) +{ + if (!attribute.tag_modified_fn) { + return {}; + } + return [tag_modified_fn = attribute.tag_modified_fn](OutputAttribute &UNUSED(attribute)) { + tag_modified_fn(); + }; } static OutputAttribute create_output_attribute(GeometryComponent &component, @@ -1293,14 +1311,21 @@ static OutputAttribute create_output_attribute(GeometryComponent &component, /* Builtin attribute is on different domain. */ return {}; } + GVMutableArrayPtr varray = std::move(attribute.varray); if (varray->type() == *cpp_type) { /* Builtin attribute matches exactly. */ - return OutputAttribute(std::move(varray), domain, {}, ignore_old_values); + return OutputAttribute(std::move(varray), + domain, + get_simple_output_attribute_save_method(attribute), + ignore_old_values); } /* Builtin attribute is on the same domain but has a different data type. */ varray = conversions.try_convert(std::move(varray), *cpp_type); - return OutputAttribute(std::move(varray), domain, {}, ignore_old_values); + return OutputAttribute(std::move(varray), + domain, + get_simple_output_attribute_save_method(attribute), + ignore_old_values); } const int domain_size = component.attribute_domain_size(domain); @@ -1324,7 +1349,11 @@ static OutputAttribute create_output_attribute(GeometryComponent &component, } if (attribute.domain == domain && attribute.varray->type() == *cpp_type) { /* Existing generic attribute matches exactly. */ - return OutputAttribute(std::move(attribute.varray), domain, {}, ignore_old_values); + + return OutputAttribute(std::move(attribute.varray), + domain, + get_simple_output_attribute_save_method(attribute), + ignore_old_values); } /* Allocate a new array that lives next to the existing attribute. It will overwrite the existing diff --git a/source/blender/blenkernel/intern/attribute_access_intern.hh b/source/blender/blenkernel/intern/attribute_access_intern.hh index 5cedcf69953..140498bdb01 100644 --- a/source/blender/blenkernel/intern/attribute_access_intern.hh +++ b/source/blender/blenkernel/intern/attribute_access_intern.hh @@ -87,7 +87,7 @@ class BuiltinAttributeProvider { } virtual GVArrayPtr try_get_for_read(const GeometryComponent &component) const = 0; - virtual GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const = 0; + virtual WriteAttributeLookup try_get_for_write(GeometryComponent &component) const = 0; virtual bool try_delete(GeometryComponent &component) const = 0; virtual bool try_create(GeometryComponent &UNUSED(component), const AttributeInit &UNUSED(initializer)) const = 0; @@ -267,7 +267,7 @@ class BuiltinCustomDataLayerProvider final : public BuiltinAttributeProvider { } GVArrayPtr try_get_for_read(const GeometryComponent &component) const final; - GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const final; + WriteAttributeLookup try_get_for_write(GeometryComponent &component) const final; bool try_delete(GeometryComponent &component) const final; bool try_create(GeometryComponent &component, const AttributeInit &initializer) const final; bool exists(const GeometryComponent &component) const final; diff --git a/source/blender/blenkernel/intern/curve_eval.cc b/source/blender/blenkernel/intern/curve_eval.cc index 8eec7f5dfab..0e3da9e0789 100644 --- a/source/blender/blenkernel/intern/curve_eval.cc +++ b/source/blender/blenkernel/intern/curve_eval.cc @@ -160,6 +160,13 @@ blender::Array CurveEval::accumulated_spline_lengths() const return spline_lengths; } +void CurveEval::mark_cache_invalid() +{ + for (SplinePtr &spline : splines_) { + spline->mark_cache_invalid(); + } +} + static BezierSpline::HandleType handle_type_from_dna_bezt(const eBezTriple_Handle dna_handle_type) { switch (dna_handle_type) { diff --git a/source/blender/blenkernel/intern/geometry_component_curve.cc b/source/blender/blenkernel/intern/geometry_component_curve.cc index d2a404b860a..d3c3fcc1e67 100644 --- a/source/blender/blenkernel/intern/geometry_component_curve.cc +++ b/source/blender/blenkernel/intern/geometry_component_curve.cc @@ -433,7 +433,7 @@ class BuiltinSplineAttributeProvider final : public BuiltinAttributeProvider { return as_read_attribute_(*curve); } - GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const final + WriteAttributeLookup try_get_for_write(GeometryComponent &component) const final { if (writable_ != Writable) { return {}; @@ -442,7 +442,7 @@ class BuiltinSplineAttributeProvider final : public BuiltinAttributeProvider { if (curve == nullptr) { return {}; } - return as_write_attribute_(*curve); + return {as_write_attribute_(*curve), domain_}; } bool try_delete(GeometryComponent &UNUSED(component)) const final @@ -1122,7 +1122,7 @@ template class BuiltinPointAttributeProvider : public BuiltinAttribu return point_data_gvarray(spans, offsets); } - GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const override + WriteAttributeLookup try_get_for_write(GeometryComponent &component) const override { CurveEval *curve = get_curve_from_component_for_write(component); if (curve == nullptr) { @@ -1133,25 +1133,30 @@ template class BuiltinPointAttributeProvider : public BuiltinAttribu return {}; } + std::function tag_modified_fn; + if (update_on_write_ != nullptr) { + tag_modified_fn = [curve, update = update_on_write_]() { + for (SplinePtr &spline : curve->splines()) { + update(*spline); + } + }; + } + MutableSpan splines = curve->splines(); if (splines.size() == 1) { - if (update_on_write_) { - update_on_write_(*splines[0]); - } - return std::make_unique( - get_mutable_span_(*splines.first())); + return {std::make_unique( + get_mutable_span_(*splines.first())), + domain_, + std::move(tag_modified_fn)}; } Array offsets = curve->control_point_offsets(); Array> spans(splines.size()); for (const int i : splines.index_range()) { spans[i] = get_mutable_span_(*splines[i]); - if (update_on_write_) { - update_on_write_(*splines[i]); - } } - return point_data_gvarray(spans, offsets); + return {point_data_gvarray(spans, offsets), domain_, tag_modified_fn}; } bool try_delete(GeometryComponent &component) const final @@ -1223,7 +1228,7 @@ class PositionAttributeProvider final : public BuiltinPointAttributeProvider::try_get_for_write(component); } - /* Changing the positions requires recalculation of cached evaluated data in many cases. - * This could set more specific flags in the future to avoid unnecessary recomputation. */ - for (SplinePtr &spline : curve->splines()) { - spline->mark_cache_invalid(); - } + auto tag_modified_fn = [curve]() { + /* Changing the positions requires recalculation of cached evaluated data in many cases. + * This could set more specific flags in the future to avoid unnecessary recomputation. */ + curve->mark_cache_invalid(); + }; Array offsets = curve->control_point_offsets(); - return std::make_unique< - fn::GVMutableArray_For_EmbeddedVMutableArray>( - offsets.last(), curve->splines(), std::move(offsets)); + return {std::make_unique< + fn::GVMutableArray_For_EmbeddedVMutableArray>( + offsets.last(), curve->splines(), std::move(offsets)), + domain_, + tag_modified_fn}; } }; @@ -1281,7 +1289,7 @@ class BezierHandleAttributeProvider : public BuiltinAttributeProvider { offsets.last(), curve->splines(), std::move(offsets), is_right_); } - GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const override + WriteAttributeLookup try_get_for_write(GeometryComponent &component) const override { CurveEval *curve = get_curve_from_component_for_write(component); if (curve == nullptr) { @@ -1292,10 +1300,15 @@ class BezierHandleAttributeProvider : public BuiltinAttributeProvider { return {}; } + auto tag_modified_fn = [curve]() { curve->mark_cache_invalid(); }; + Array offsets = curve->control_point_offsets(); - return std::make_unique< - fn::GVMutableArray_For_EmbeddedVMutableArray>( - offsets.last(), curve->splines(), std::move(offsets), is_right_); + return { + std::make_unique< + fn::GVMutableArray_For_EmbeddedVMutableArray>( + offsets.last(), curve->splines(), std::move(offsets), is_right_), + domain_, + tag_modified_fn}; } bool try_delete(GeometryComponent &UNUSED(component)) const final diff --git a/source/blender/blenkernel/intern/geometry_component_instances.cc b/source/blender/blenkernel/intern/geometry_component_instances.cc index d02121b44a6..5fe77000519 100644 --- a/source/blender/blenkernel/intern/geometry_component_instances.cc +++ b/source/blender/blenkernel/intern/geometry_component_instances.cc @@ -398,15 +398,16 @@ class InstancePositionAttributeProvider final : public BuiltinAttributeProvider transforms); } - GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const final + WriteAttributeLookup try_get_for_write(GeometryComponent &component) const final { InstancesComponent &instances_component = static_cast(component); MutableSpan transforms = instances_component.instance_transforms(); - return std::make_unique>( - transforms); + return { + std::make_unique>(transforms), + domain_}; } bool try_delete(GeometryComponent &UNUSED(component)) const final @@ -443,13 +444,14 @@ class InstanceIDAttributeProvider final : public BuiltinAttributeProvider { return std::make_unique>(instances.instance_ids()); } - GVMutableArrayPtr try_get_for_write(GeometryComponent &component) const final + WriteAttributeLookup try_get_for_write(GeometryComponent &component) const final { InstancesComponent &instances = static_cast(component); if (instances.instance_ids().is_empty()) { return {}; } - return std::make_unique>(instances.instance_ids()); + return {std::make_unique>(instances.instance_ids()), + domain_}; } bool try_delete(GeometryComponent &component) const final diff --git a/source/blender/blenkernel/intern/geometry_component_mesh.cc b/source/blender/blenkernel/intern/geometry_component_mesh.cc index 6091d3f3dab..c3e39c0b2cb 100644 --- a/source/blender/blenkernel/intern/geometry_component_mesh.cc +++ b/source/blender/blenkernel/intern/geometry_component_mesh.cc @@ -1210,7 +1210,7 @@ class NormalAttributeProvider final : public BuiltinAttributeProvider { return std::make_unique>>(std::move(normals)); } - GVMutableArrayPtr try_get_for_write(GeometryComponent &UNUSED(component)) const final + WriteAttributeLookup try_get_for_write(GeometryComponent &UNUSED(component)) const final { return {}; } -- cgit v1.2.3