GPv3: Curve to Points node #113634

Looking at it again, the call to geometry.replace_instances(pointcloud_instances); inside the modify_geometry_sets loop does seem suspect, since there might be sub-instances that will still be processed by the loop. It might be ugly, but I worry we'd have to add to the existing instances in the geometry set rather than replace them. @JacquesLucke may have a better idea too.

I would suggest making propagate_attributes_from_layer_to_instances into a free function that just takes the source and destination attribute accessors, to make it clearer what data it's using.

Looking at it again, the call to `geometry.replace_instances(pointcloud_instances);` inside the `modify_geometry_sets` loop does seem suspect, since there might be sub-instances that will still be processed by the loop. It might be ugly, but I worry we'd have to add to the existing instances in the geometry set rather than replace them. @JacquesLucke may have a better idea too. I would suggest making `propagate_attributes_from_layer_to_instances` into a free function that just takes the source and destination attribute accessors, to make it clearer what data it's using.

source/blender/blenkernel/intern/geometry_set.cc Outdated

						
				@ -608,0 +608,4 @@

				void GeometrySet::propagate_attributes_from_layer_to_instances(

				    const AnonymousAttributePropagationInfo &propagation_info)

				{

				  Map<AttributeIDRef, AttributeKind> attributes_to_propagate;

Hans Goudey commented

2023-10-13 10:02:19 +02:00

I'd suggest a couple changes here, I wrote it out below just to make sure it would work

Avoid gather_attributes_for_propagation and work at a slightly lower abstraction level, since we're really just dealing with two attribute accessors here
Also handle the case where the source attribute doesn't have sharing info or isn't a span, just to avoid making that assumption in too many unnecessary places in case we want to change those things in the future

  const AttributeAccessor src_attributes = this->get_grease_pencil()->attributes();
  MutableAttributeAccessor dst_attributes =
      this->get_instances_for_write()->attributes_for_write();

  src_attributes.for_all([&](const AttributeIDRef &id, const AttributeMetaData meta_data) {
    if (id.is_anonymous() && !propagation_info.propagate(id.anonymous_id())) {
      return true;
    }
    const GAttributeReader src = src_attributes.lookup(id, ATTR_DOMAIN_LAYER);
    if (src.sharing_info && src.varray.is_span()) {
      const AttributeInitShared init(src.varray.get_internal_span().data(), *src.sharing_info);
      if (dst_attributes.add(id, ATTR_DOMAIN_INSTANCE, meta_data.data_type, init)) {
        return true;
      }
    }
    GSpanAttributeWriter dst = dst_attributes.lookup_or_add_for_write_only_span(
        id, ATTR_DOMAIN_INSTANCE, meta_data.data_type);
    if (!dst) {
      return true;
    }
    array_utils::copy(src.varray, dst.span);
    dst.finish();
    return true;
  });

I'd suggest a couple changes here, I wrote it out below just to make sure it would work - Avoid `gather_attributes_for_propagation` and work at a slightly lower abstraction level, since we're really just dealing with two attribute accessors here - Also handle the case where the source attribute doesn't have sharing info or isn't a span, just to avoid making that assumption in too many unnecessary places in case we want to change those things in the future ```cpp const AttributeAccessor src_attributes = this->get_grease_pencil()->attributes(); MutableAttributeAccessor dst_attributes = this->get_instances_for_write()->attributes_for_write(); src_attributes.for_all([&](const AttributeIDRef &id, const AttributeMetaData meta_data) { if (id.is_anonymous() && !propagation_info.propagate(id.anonymous_id())) { return true; } const GAttributeReader src = src_attributes.lookup(id, ATTR_DOMAIN_LAYER); if (src.sharing_info && src.varray.is_span()) { const AttributeInitShared init(src.varray.get_internal_span().data(), *src.sharing_info); if (dst_attributes.add(id, ATTR_DOMAIN_INSTANCE, meta_data.data_type, init)) { return true; } } GSpanAttributeWriter dst = dst_attributes.lookup_or_add_for_write_only_span( id, ATTR_DOMAIN_INSTANCE, meta_data.data_type); if (!dst) { return true; } array_utils::copy(src.varray, dst.span); dst.finish(); return true; }); ```

dfelinto marked this conversation as resolved

Dalai Felinto force-pushed grease-nodes-curve-to-points from 529f96a821 to c75ced06cb

2023-10-13 10:09:44 +02:00

Compare

Dalai Felinto commented

2023-10-13 10:48:49 +02:00

~~@HooglyBoogly I incorporated your changes (thanks for the code by the way), see if I got it right what you meant (I made the function static now).~~

~~As for the geometry.replace_instances this is what we do already (in main) for the curves. So if this is wrong I suspect it is wrong in main itself too.~~

~~@HooglyBoogly I incorporated your changes (thanks for the code by the way), see if I got it right what you meant (I made the function static now).~~ ~~As for the `geometry.replace_instances` this is what we do already (in main) for the curves. So if this is wrong I suspect it is wrong in main itself too.~~

Dalai Felinto commented

2023-10-13 10:56:49 +02:00

Ah I see the difference. For the curves we are not mingling with the instances.