Details
Diff Detail
- Repository
- rB Blender
- Branch
- temp-attribute-transfer-node (branched from master)
- Build Status
Buildable 14225 Build 14225: arc lint + arc unit
Event Timeline
Working great in general!
Couple of small notes:
- the Auto setting should also consider what domains are available in the main geometry, or at least throw a warning that the automatically selected domain does not exist. Transferring a face attribute onto a point cloud with the auto setting results in the node doing nothing. Auto should fall back on the point domain in this case.
- the Nearest Face Interpolated option really only makes sense for source attributes on the point domain. I think for now it is fine how that behaves, just something to consider in the future.
- the Topology mapping should probably follow this first iteration soon (not as part of this ptach). I assume that this would be the most straight forward mapping mode to implement, but I can see it being very useful for a generally more branched attribute workflow.
- Merge branch 'master' into temp-attribute-transfer-node
- fallback to point domain for point clouds
Mostly minor comments inline. I have a few higher level questions though.
Caching BVH trees?
This is more of a random musing: It's hard to tell if the functions retrieving a BVH from a mesh can also cache the result.
If so, it might make sense to pursue that, to better support using multiple of these nodes in a row. On the other hand, that still has to duplicate a fair amount of work, so multi-input sockets are probably a better way to address that.
Naming
I'm not sure there's a better option, but "Nearest Face Interpolated" naming just makes me a bit sad, it's so long and hard to understand for anyone who hasn't spent time thinking about it.
Actually, maybe we could separate the method it uses to find a surface point from the interpolation method, like this:
| Nearest | Raycast | Topology |
| Direct Data | Face Interpolation | Edge Interpolation | etc.. |
That naming isn't idea, and maybe it doesn't work, but I think it's worth thinking more about the interface here, since we're establishing a base for a very common need.
Parallelization / Instances
Parallelizing some of the loops is easily done in the future, what's less easily done later is supporting instances in these nodes.
Do you think that's prohibitively complicated for such a node? I think it might be, at least with current BVH utility functions. And there may only be performance benefits for very complex source geometry.
I'm mostly just curious about your thoughts.
Docs
Side note (obviously not blocking for this patch) this node is going to need some great documentation, especially when we add more options.
| source/blender/makesdna/DNA_node_types.h | ||
|---|---|---|
| 1817 | If you drop the ing it would be a bit shorter ; ) | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_transfer.cc | ||
| 18–20 | For include order, I recently made all of the node files consistent: BLI, DNA, BKE_, UI, internal Does that order make sense? IMO it's nice to keep the consistency. | |
| 66 | from_attribute isn't very helpful here, and it's really the fact that there might be multiple attributes that makes this function necessary. I suggest get_result_domain_and_data_type | |
| 88 | This can use the function I added the other day, attribute_get_meta_data | |
| 352 |
| |
| 358 | Is this really necessary? I haven't noticed code doing it elsewhere. Or, the goal is to make sure the node does something when there is no source attribute? Either way, it doesn't seem obvious and probably deserves a comment. | |
| 521–529 | It doesn't matter that much, but this logic / naming struck me as a bit odd. How about this? const AttributeDomain input_domain = (AttributeDomain)storage.domain; CustomDataType data_type; AttributeDomain auto_domain; get_domain_and_data_type_from_attribute( src_geometry, dst_component, src_name, &data_type, &auto_domain); const AttributeDomain result_domain = (input_domain != ATTR_DOMAIN_AUTO) ? auto_domain : input_domain; | |
| 531 | I didn't notice this before, but shouldn't attribute_get_for_read return a const VArray? Maybe I'm not understanding something. Seems like the variable could be const anyway. | |
BVH trees are already cached at the mesh level (the same might not be true for point clouds yet).
I'm fine with different naming as long as it does not force us to do multiple bvh tree lookups for different cases even when there is only a single mesh. E.g. just saying "nearest" could mean nearest face/edge/point which we can't really check for efficiently in one query.
I don't really understand how to apply your naming suggestions to this specific patch.
Wouldn't do it in this patch, but I think that we can build a bvh tree that includes all the instances in the future. bvh trees should support multiple separate objects relatively efficiently.
| source/blender/nodes/geometry/nodes/node_geo_attribute_transfer.cc | ||
|---|---|---|
| 18–20 | Personally, I don't see any value in enforcing this order. It does not save me time reading the code and does it make it easier to write (more on the contrary, because I have to check other files for the order). So if you want to enforce it fine, but I wouldn't do it. | |
| 352 | Yeah, we just don't have it for the point distribute yet either. Haven't really looked what's the best way to do the interpolation. We might have to compute the distance to all edges of the polygon and then mix with weights that are inversely proportional to these distances? | |
| 358 | Will add a comment, we might have to do this because the attribute array might be malloced and contain uninitialized data. | |
| 531 | Enforcing constness of referenced data by making the reference const does not work unfortunately. This is why there is Span and MutableSpan, as well as VArray and VMutableArray. It's easier to see for spans, but the same applies to virtual arrays. When you have a const Span you can always just copy it to create a Span that is not const, so the const does not actually protect the referenced data. The data referenced by a VArray is const by design, so it does not matter whether the VArray itself is const. | |
- Merge branch 'master' into temp-attribute-transfer-node
- rename input
- shorten name
- reorder includes
- use attribute_get_meta_data
- cleanup
I'd still like to propose a slightly different interface here rather than one large enum, but since that would be easy to change with versioning, and it might even be easier to talk about when the initial version is already in master, I won't consider that blocking.
Generally the code looks good. In the future it may take some care to avoid the confusion of combinatorial explosion ; P
| source/blender/nodes/geometry/nodes/node_geo_attribute_transfer.cc | ||
|---|---|---|
| 416 | It looks like the only calls to these functions always have an empty r_positions argument. Maybe that's an oversight? But probably room for future ideas? Either way, I'm not sure it should be in this version of the patch. | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_transfer.cc | ||
|---|---|---|
| 416 | Err right, the position output is only used by the nearest-face-interpolated mode currently. I wanted to extract these functions to another file at some point. It seems like they could be useful more generally. | |