Port shader Voronoi to GN
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- master
- Build Status
Buildable 17866 Build 17866: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
This is in a working state but I'm happy if one of the core devs takes it over as it may be more efficient than a code review.
Comment Actions
Lots of warnings: P2468.
Would be nice if you could provide a test file to see that it matches cycles.
Still thinking about how the amount of code can be reduced a bit...
| source/blender/blenlib/BLI_noise.hh | ||
|---|---|---|
| 58 | Use r_color and r_w as parameter names. | |
| source/blender/blenlib/intern/noise.cc | ||
| 928 | Should use const whenever possible. | |
| 956 | There is a division by zero here when smoothness is zero. | |
Comment Actions
Address comments. All the modes are working but there is a bug causing different output to Cycles/Eevee.
File used for validating noise:
Comment Actions
Fixed bug and compatibilty with Cycles. The hash_float_to_float functions in noise.cc work differently but are named the same. Added compatible hash functions with shd_ prefix.
Comment Actions
I see that the noise functions are different, my question is whether we need to different noise functions with the same interface, or if we can get rid of some of the functions I added in rBd8a5b768f0763dab725401460e229787d080476a instead.
Also, all of the math functions like distance/len/... shouldn't be defined in noise.cc, but in their corresponding header.
Comment Actions
For the Voronoi/White Noise, we need the compatible hash functions. I don't know if they are slower/faster/better than the ones you added. The ones you added are in use in node_geo_distribute_points_on_faces.cc.
Comment Actions
Move math functions out of noise.cc
Following file was used to compare the output with Cycles.
The same hash functions are used in D12719: Geometry Nodes: Add White Noise texture.
| source/blender/blenlib/BLI_float4.hh | ||
|---|---|---|
| 83 | Add an assert that checks that f != 0.0f. | |
| 133 | This has to handle the case when a is zero. | |
| source/blender/blenlib/BLI_noise.hh | ||
| 54 | Did you consider removing these functions now? They are only used in the distribute node, but it is fine when we change the distribution still. The goal is to not need the shd prefix in the functions below. | |
| 71 | Use a header like /* -------------------------------------------------------------------- * Voronoi noise. */ | |
| 72 | The 1d, 2d, ... doesn't have to be part of the function name if I'm not mistaken. This can be handled by c++ overloading. This is also used in the perlin noise functions below. | |
| source/blender/blenlib/intern/noise.cc | ||
| 863–864 | Use const whenever possible here. | |
| 1221 | Hm, we should be careful with using enums defined in DNA in blenlib... | |
| 1224 | Use a switch statement and use BLI_assert_unreachable when an invalid metric is passed in. | |
Comment Actions
@Omar Emara (OmarSquircleArt) I've added you as a reviewer as the Voronoi code is originally your patch.
Comment Actions
Some warnings: P2500.
A simple way to reduce the duplication a little bit for now is to add some lambdas like:
auto get_vector = [&](int param_index) -> const VArray<float3> & {
return params.readonly_single_input<float3>(param_index, "Vector");
};Same for the other parameters. These lambdas can then be used in all the different cases.
More things can be improved in the future, but with this change I think this is good enough.
| source/blender/nodes/shader/nodes/node_shader_tex_voronoi.cc | ||
|---|---|---|
| 256 | This is not exactly what I meant unfortunately. Sorry for being not precise enough. You'd still have to do this above the loops: const VArray<float3> &vectors = get_vec(0); /* ... */ MutableSpan<float> r_distance = get_r_distance(4); | |
Comment Actions
Use lambdas for ouputs and use param++ for socket number.
Use more verbose variable names. E.g. r_pos > r_position.
Comment Actions
When Smoothness is set to exactly, the result seems unexpected. The values become much larger suddenly. Not sure if this is the same in shading.
Besides that, looks good in my tests.
| source/blender/nodes/shader/nodes/node_shader_tex_voronoi.cc | ||
|---|---|---|
| 241 | Should be called get_r_distance, instead of set_distance. Same for the other lambdas. | |
Comment Actions
Fix issue with smoothness. Bizarrely the function seems to rely on non-safe division. Since this is the same code as shader, this could be looked at later.