Float, Vector and Color data only.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- master
- Build Status
Buildable 13941 Build 13941: arc lint + arc unit
Event Timeline
Comment Actions
The code looks quite good already, just a few smaller comments
| source/blender/nodes/geometry/nodes/node_geo_attribute_curve_mapping.cc | ||
|---|---|---|
| 59 | No need to use ELEM with only one value | |
| 66 | These magic numbers are pretty ugly, but I understand the curvemapping code is old and might not have defines or enums for these things. | |
| 104 | Looks like this can be const | |
| 166 | This can go at the top of the file, right under the layout callback. | |
| 181 | I would drop the ing from the node's name. I think it's redundant and makes it sound less "active" | |
| source/blender/nodes/geometry/nodes/node_geo_attribute_curve_mapping.cc | ||
|---|---|---|
| 66 | Yes, no enums for these. It's setting the active curve when changing data type. | |
Comment Actions
Address comments.
Note: No Factor was added. Shader vector node has factor but compositor vector node doesn't.
Comment Actions
Looks good, I'd like to make sure it matches @Simon Thommes (simonthommes)'s expectations for such a node first though.
| source/blender/nodes/geometry/nodes/node_geo_attribute_curve_map.cc | ||
|---|---|---|
| 146 ↗ | (On Diff #35975) | BLI_assert_unreachable() |
Comment Actions
Thanks for implementing this! Absolutely fantastic node to have :)
A few remarks:
- I like how RGB and XYZ are sharing curves. I feel like for Float it would make more sense though to share the curve with Color: C rather than X and R
- Default type should simply be Float in my opinion
- Not a big fan of how the default curve for Color starts in [-1,-1], should rather be [0,0] imo. Same for the view clipping. Would it be possible to disconnect the clipping options for Vector/Float and Color, so that color is in [0,1]? Maybe the view could be reset on switching, if Use Clipping is enabled. I don't know how easy it is to customize this kind of stuff. I imagine it's out of scope.
- It would be fantastic if for procedural modeling we could have the same advanced spline handle control that we got on the custom profile of the bevel modifier. That helps a lot with precise control. Not sure how viable that is either. @Hans Goudey (HooglyBoogly) should know :)
- I noticed that there is some stepping going on. I would have assumed that it's possible to calculate the resulting values directly from the spline parameters, but that's just an artifact of how blender stores this kind of curves, right? Would be great to get those ironed out in general, but I don't expect that to be possible.
None of these remarks are really blocking, but I would be interested in how viable these things are.
Comment Actions
Changed
- Default type should simply be Float in my opinion
Changed
- Not a big fan of how the default curve for Color starts in [-1,-1], should rather be [0,0] imo. Same for the view clipping. Would it be possible to disconnect the clipping options for Vector/Float and Color, so that color is in [0,1]? Maybe the view could be reset on switching, if Use Clipping is enabled. I don't know how easy it is to customize this kind of stuff. I imagine it's out of scope.
The only easy solution is to either have a separate node for color data type or to make use of a second curve for color data. Changing the clipping affects the other data modes which is equally unintuitive.
- It would be fantastic if for procedural modeling we could have the same advanced spline handle control that we got on the custom profile of the bevel modifier. That helps a lot with precise control. Not sure how viable that is either. @Hans Goudey (HooglyBoogly) should know :)
CurveProfile could be a separate node but I guess @Hans Goudey (HooglyBoogly) can comment on this.
- I noticed that there is some stepping going on. I would have assumed that it's possible to calculate the resulting values directly from the spline parameters, but that's just an artifact of how blender stores this kind of curves, right? Would be great to get those ironed out in general, but I don't expect that to be possible.
It seems the table size is hardcoded to 256.
None of these remarks are really blocking, but I would be interested in how viable these things are.
Finally, should we add a Factor socket or leave this out?
Comment Actions
Yeah, ideally they would be the same, but I made the CurveProfile widget quite a while ago when the prospect of extending the existing code was a bit too daunting. Eventually they could be combined and we could have the handles, etc. in here.
Comment Actions
Rather than try and fit float, vector and color onto a single curve, use two curves. One for float & vector. One for color.
Comment Actions
Okay, thanks for the responses!
Looks good to me for now then. I don't really think there is all that much benefit of fitting XYZ and RGB into one curve set anyways now that I think about it.
Comment Actions
@Hans Goudey (HooglyBoogly) can you check the code again? Since the original acceptance, I changed the code to use two curves and then another revision after the attribute refactor by Jacques.