Details

Reviewers

Maniphest Tasks

T84297: Expose more built-in data as attributes
T85880: Expose "normals" built-in attribute

Commits

rBf117ea262463: Geometry Nodes: Expose vertex normals as an attribute

Summary

This attribute exposes mesh vertex normals as an attribute for use with
the attribute nodes. Since the normal vector stored in vertices is only
a cache of data computable from the surrounding faces, the attribute is
read-only.

Initially I was hoping this could be a write-able attribute, but it
doesn't work consistently because the data is treated strictly as a
cache in the code. Custom normals will be stored on the corner domain,
an attribute which will be called corner_normal.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Feb 26 2021, 5:35 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B13125: Diff 34440.Feb 26 2021, 5:35 AM

Add checks in join node and realize instances code

Harbormaster completed remote builds in B13126: Diff 34442.Feb 26 2021, 5:37 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Feb 26 2021, 5:38 AM

Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).

Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.

Hans Goudey (HooglyBoogly) added a task: T85880: Expose "normals" built-in attribute.Feb 26 2021, 5:41 AM

The code looks mostly good, except for a rather large constness and threading issue which really needs to be resolved first..

source/blender/blenkernel/intern/attribute_access.cc
1379	Need to be very careful here. Const-casting the geometry component seems dangerous here, also because `MeshComponent::get_for_write` might make a copy of the mesh. It would be better to only cast away const from the `Mesh *`. Furthermore, to make this work, `BKE_mesh_calc_normals` needs to become threadsafe. Currently it is not protected by a mutex, so different threads could enter it on the same mesh at the same time.
source/blender/blenkernel/intern/geometry_set_instances.cc
382	We should probably store this list somewhere else, but that can be changed separately.

This revision now requires changes to proceed.Feb 26 2021, 1:59 PM

Add a mutex so multiple threads don't write normals from this function at the same time

As noted in the code, this uses a global mutex rather than a per-mesh mutex. I thought of
using a mutex stored on MeshComponent, but then two mesh components might share the same
mesh, so that probably wouldn't work. Note that I've tried a few things, but I haven't been
able to trigger a problem without this lock though. Maybe there are better ways to solve
this, I'm not sure.

Harbormaster completed remote builds in B13201: Diff 34671.Mar 2 2021, 8:48 PM

Hans Goudey (HooglyBoogly) added a task: T84297: Expose more built-in data as attributes.Mar 2 2021, 8:58 PM

This locking feels really weak to me, but we might be lucky in the case of vertex normals that it won't cause any issues. In the long run we really need a more general solution to cached derived mesh data.
Given that vertex normals are exposed as readonly attribute, we can still change the internal implementation later on if necessary.