These nodes make it easier to switch between vector attributes and
float attributes.
Example usage:
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- combine_separate_xyz (branched from master)
- Build Status
Buildable 12746 Build 12746: arc lint + arc unit
Event Timeline
Comment Actions
This looks quite good already, I just added a couple of notes inline.
| source/blender/nodes/geometry/nodes/node_geo_attribute_combine_xyz.cc | ||
|---|---|---|
| 45 | I think this should be float by default. | |
| 87 | The formatting is wrong, please run clang-format (make format). | |
| 137 | You can configure your editor so that automatically inserts newlines at the end of files. | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_separate_xyz.cc | ||
| 24 | Should be called "Vector". | |
| 66 | Comments should end with a .. Same below. | |
Comment Actions
Like Jacques said, the patch already looks pretty good. My comments are more on the "cleanup" side of things.
| source/blender/blenkernel/intern/node.cc | ||
|---|---|---|
| 4787–4788 | Really small thing, but do you mind sorting these alphabetically? | |
| source/blender/editors/space_node/drawnode.c | ||
| 3391 ↗ | (On Diff #33574) | New line between functions for consistency. |
| source/blender/makesdna/DNA_node_types.h | ||
| 1206 | Another picky request, comments should end like this: . */ | |
| source/blender/nodes/CMakeLists.txt | ||
| 156–157 | These can be alphabetically sorted too | |
| source/blender/nodes/NOD_geometry.h | ||
| 63–64 | And same here. | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_combine_xyz.cc | ||
| 48 | New line between functions | |
| 86 | Usually see this with parentheses rather than brackets, like float3(x, y, z); | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_separate_xyz.cc | ||
| 60 | You can get the span once for the input and pass it as an argument here. The difference is small, but it avoids calling it three times. | |
| 86 | No need for this float3 here. | |
| 97 | This method is fine, but I like using ->get_span_for_write_only<float>(), which just reads a bit better and faster in my opinion. | |
| 106 | I guess this works (haven't tested the patch yet), but I think these should be using apply_span_and_save instead. | |
Comment Actions
- Merge master after alphabitize commit
- Cleanup: Clang format
- Cleanup: Remove unecessary order change in CMakeLists.txt
- Cleanup: Use __func__ for allocation string
- Cleanup: Adjust argument to span instead of attribute
Comment Actions
Lots of places currently assume that the only domain is "point" currently, but I think we should consider addressing that.
Here's what I'd propose.
- If the output attribute already exists, use that domain.
- If it doesn't exist, use the domain of the input attribute(s) (if there are multiple use the highest priority).
- If none of the input attributes exist, use a the default domain, point.
Now that I've written that it's clear it's outside the scope of this patch. Something to address soon though, or we're going to end up with more and more places to fix.
For now, it would be good to add a comment that at least says "for now"
/* The result domain is always point for now. */ const AttributeDomain result_domain = ATTR_DOMAIN_POINT;
Since we're not really using separate domains yet it's not essential right now, but it's one fewer thing to fix down the line.
| source/blender/blenkernel/intern/node.cc | ||
|---|---|---|
| 4787–4788 | Oh, oops, I should have clarified I just mean your new functions. Thanks anyway though. I just committed that cleanup separately. | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_combine_xyz.cc | ||
| 83 | Can use results.index_range() here, it's a bit shorter. | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_separate_xyz.cc | ||
| 77 | I think this should not have a default and skip the rest of the function if the attribute is not found. | |
Comment Actions
- Cleanup: Add comments about attribute domain.
- Get rid of default value for the input attribute.
| source/blender/nodes/geometry/nodes/node_geo_attribute_separate_xyz.cc | ||
|---|---|---|
| 97 |
Not really sure what you mean by this, because I'm using the get_span_for_write_only already. Maybe I changed it without noticing it. | |
Comment Actions
Looks good to me!
I think having the default as float is fine.
Made a test manipulating some coordinates of scattered points