Geometry Nodes: Add initializer for attribute creation
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Apr 22 2021, 12:31 AM.

Details

Reviewers

Jacques Lucke (JacquesLucke)

Maniphest Tasks

T87597: Geometry Nodes PointScale node not working

Commits

rBd1ccc5b9694b: Geometry Nodes: Add initializer for attribute creation

Summary

Previously we always had to set attribute values after creating the
attribute. This patch adds an initializer argument to attribute_try_create
which can fill it in a few ways, which are explained in code comments.

I tested the "move" initializer with the following code in the attribute
fill node. I'm sure there's a better way to do it, but I just wanted to
make sure it worked.

const float value = params.get_input<float>("Value_001");
float *values = (float *)MEM_malloc_arrayN(
    component.attribute_domain_size(result_domain), sizeof(float), __func__);
for (const int i : IndexRange(component.attribute_domain_size(result_domain))) {
values[i] = value;
}
component.attribute_try_create(
    attribute_name, result_domain, data_type, AttributeInitMove(values));

This fixes T87597.

Diff Detail

Repository

rB Blender

Branch

attribute-create-init (branched from master)

Build Status

Buildable 14171
Build 14171: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Apr 22 2021, 12:31 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B14160: Diff 36390.Apr 22 2021, 12:32 AM

Hans Goudey (HooglyBoogly) mentioned this in D11018: Fix T87597: Point scale node does not work after recent refactor.Apr 22 2021, 12:32 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Hans Goudey (HooglyBoogly) added inline comments.Apr 22 2021, 12:34 AM

source/blender/blenkernel/intern/attribute_access.cc
413	It's sort of annoying to have to duplicate the logic from above in this file, but I think it keeps it simpler actually, and is preferable to overly generalizing such a function.

Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.Apr 22 2021, 1:15 AM

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 22 2021, 10:39 AM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/BKE_geometry_set.hh
125	Doing manual memory management here feels dangerous. That could easily lead to a double free if the `AttributeInitMove` is copied for whatever reason. I think you should just free the data directly where you set `free_data` to true now.
source/blender/blenkernel/intern/attribute_access.cc
265	Use `GVArray::materialize_to_unitialized`. I haven't done it yet, but this function should be optimized like `VArray::materialize_to_uninitialized_impl`.
1000	`const GVArray_For_SingleValueRef default_varray{args...}` No need to specify the long type name twice.

This revision now requires changes to proceed.Apr 22 2021, 10:39 AM

Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Apr 22 2021, 3:23 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/BKE_geometry_set.hh
125	You're right, just freeing it there is much simpler. It's still manually memory management, but at least it's less complicated.
source/blender/blenkernel/intern/attribute_access.cc
1000	Thanks, good point. I keep forgetting about that.

Rename enum to AttributeInit::Type
Improve freeing of buffer
Use materialize_to_unitialized
Don't specify varray name twice

Harbormaster completed remote builds in B14171: Diff 36414.Apr 22 2021, 3:23 PM

Jacques Lucke (JacquesLucke) accepted this revision.Apr 22 2021, 3:38 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/BKE_geometry_set.hh
82	I think the `AttributeInit::` is not necessary. Also you might want to use `enum class` instead of just `enum` above.
source/blender/blenkernel/intern/attribute_access.cc
272	unnecessary newlines