This provide random vectors in spherical or conical distributions.
This replaces previous attribute patch D10883: Geometry Nodes: Attribute Randomize Spherical Node
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- master
- Build Status
Buildable 19241 Build 19241: arc lint + arc unit
Event Timeline
Comment Actions
This patch needs an update to latest master, but I think it's worth pursuing. It's especially useful when combined with the raycast node.
| source/blender/nodes/function/nodes/node_fn_random_spherical_value.cc | ||
|---|---|---|
| 17 ↗ | (On Diff #42885) | Remove the commented code here. |
| 116–122 ↗ | (On Diff #42885) | Why not save some lines of code by retrieving the values directly in the spherical_value call? Same with below. |
| 160 ↗ | (On Diff #42885) | This looks very similar to the spherical function. It might be nice to extract some of the lines to separate static functions with nice descriptive names-- then everything gets more readable too. |
| 197–200 ↗ | (On Diff #42885) | Could avoid a few lines with switch (static_cast<FunctionNodeRandomSphericalDistribution>(storage.distribution)) { |
Comment Actions
This seems to work very well!
I'm basically evaluating this with the use case of choosing direction for the raycast node. I'm sure there are more, but I believe that's a basic on this should be able to handle.
Using the conical version, I really wish I could choose the the up vector, I think it would make the node much more friendly to use:
What do you think about that?
Here's my test file:
I think I'd like to require a similar test file be committed to the geometry nodes tests repository. Now that the crazyness of the fields transition is over, we should start to be better about adding tests for new features.
| source/blender/nodes/function/nodes/node_fn_random_spherical_value.cc | ||
|---|---|---|
| 29–33 ↗ | (On Diff #44946) | It's not obvious what the bias input does, could you add descriptions? |
| 29–33 ↗ | (On Diff #44946) | These need N_ translation markers. |
| 63–65 ↗ | (On Diff #44946) | It seems preferable to use BLI_findlink to avoid the string comparisons. |
Comment Actions
Address comments.
Add axis to conical mode.
Used Axis rather than Direction.
This does make conical mode more useful.
Comment Actions
Radial Bias controls distribution of vector lengths between min and max radius.
On a disc, when both min/max angles are set to 90 degrees
On a hemisphere, when angles are set to 0 and 90 degrees
Comment Actions
Change Axis to Rotation as axis is less friendly for animating (180 flipping) or controlling with an empty.
Comment Actions
I'd like some artist to test this, but as far as I'm concerned it's probably close to ready. If you merge master, maybe we can make a build of the patch?
| source/blender/nodes/function/nodes/node_fn_random_spherical_vector.cc | ||
|---|---|---|
| 38 | "adjusts the distribution" is a bit vague, it doesn't really give me a sense of what changing the number in a certain direction will do. Maybe there's a better way to describe it? | |
| 84 | clamp_f -> std::clamp Also, declare a new const variable rather than changing the original. | |
Comment Actions
Address comments.
D12746 builds available here: https://builder.blender.org/download/patch/
@Simon Thommes (simonthommes) can you test the patch and provide feedback or accept the patch?
Comment Actions
Rebase to master.
Rename node from Random Spherical Vector to Spherical Vector.
After more testing it was frustrating to not be able to control the distribution of points more finely.
For this reason I have added a mode to switch between Random and Manual use cases.
Random: generates points based on ID with limits on angles and radius.
Manual: does not generate points but exposes the factors that control distribution.
Plugging Random values or other inputs allows control of the Angle, Radial and Z factors of the output vector.
From UI perspective it could use Radians but Factors work better in practice.
Factors are not clamped. In Spherical mode, the Z values wrap around the sphere.
In Conical mode, the Z values bounce between the Z angle limits.
The Tangent and Binormal axis are also exposed to align Rotations.
Comment Actions
Sorry, missed this patch for a while!
I tested it now, looking very nice, I think this will be very useful!
I have a couple of notes:
- I'm not sure the Rotation input is necessary. This can easily be done with a vector rotation afterwards. But maybe this input can just stay there and benefit from being hidden by default eventually whenever that gets a thing.
- Does the default Max angle of 36.4756dhave any significance that I'm unaware of? Otherwise I think 45d would be a good choice for a default here.
- I think, giving Min/Max Angle soft limits of 0d/180d would be nice, so the values can be more easily set to those when sliding. Most use cases wouldn't exceed those anyways.
- In the same sense, I think Min/Max Radius can be soft limited to 0.
- I'm not sure the binormal output is needed. I think users that are familiar with these terms and would use the outputs would know they can easily retrieve it with a cross product.
- Instead of the binormal, I think, a Rotation/Euler output would be useful. I see this node being used for isotropic orientation a lot (/mostly). And this output would allow a setup like this very easily:
- Personally I would even make Cone the default distribution because of this.
- In manual mode I find the Sphere parameters slightly confusing, due to multiple reasons.
- The behavior of the Z Factor I find confusing.
- I would have it interpolate with the acos(z) rather than z, as the zenith angle is also what you control in the cone distribution.
- The Z Factor correlates to the Min/Max Angle in the cone distributions, while the Angle Factor correlates to the azimuth angle that is not controllable (-> naming inconsitency).
- I would change the naming of these inputs. Either what is now the Z-factor should be the angle factor, or we go with the more widely accepted terms of Zenith/Azimuth or Theta/Phi. Maybe you have a better naming suggestion.
- The order of the inputs is not consistent with the remaining node inputs they correlate to.
- I would change it to be Radius/Zenith/Azimuth
- The behavior of the Z Factor I find confusing.
Comment Actions
I just noticed that the Rotation input was a request by @Hans Goudey (HooglyBoogly) :D
I personally don't think that this node needs this functionality itself, as I think usually you would want to use this node for relative rather than absolute transformations and this can be done with a single node afterwards.
But this is not a very crucial point for me.
Comment Actions
@Simon Thommes (simonthommes) as usual, thanks for the feedback, I'll take a look and update the node
Comment Actions
We chatted about this patch in today's meeting and agreed that the rotation input can be removed.
@Jacques Lucke (JacquesLucke) brought up a good point, that the manual mode could just be replaced with a vector map range if we had nodes to convert between spherical and cartesian space.
We should probably have those anyways, so maybe the can leave out the manual mode from this node and add nodes for space conversion.
Comment Actions
Yes, I did think perhaps it could be split out into two nodes. I'll revert manual mode changes and update the patch.
@Simon Thommes (simonthommes) do you want to create a task for spherical space with notes on naming and whether to use radians or factors?
Comment Actions
Cool, thank you!
Yes, I'll create that task. I can foresee a couple of design questions that need to be figured out as well.
Comment Actions
Revert and remove manual mode.
Add soft limits.
Add defaults as suggested.
Remove binormal socket.
Hide rotation values.
@Simon Thommes (simonthommes) I've kept Rotation socket but hidden the values.
Removing it would mean that post rotations would need to be
made to both output Vector and Tangent socket to be meaningful.
So either keep node as is with hidden values and useable Tangent
or remove Rotation and Tangent sockets.
Comment Actions
Thanks for the changes!
- I was aware, when I made the suggestion to remove the rotation, that you'd have to rotate both vector and tangent individually. I personally don't think that would be that big of a deal, the behaviour of the node itself is still well-defined.
But I'm also fine with a hidden Rotation input like you have it now. I think, I'd put it first on the node inputs though in that case. I think that would read more naturally.
- Have you considered my suggestion to add a Rotation output?
- Instead of the binormal, I think, a Rotation/Euler output would be useful. I see this node being used for isotropic orientation a lot (/mostly). And this output would allow a setup like this very easily:
That output would already encapsulate both the vector and the tangent (normalized).
- Regarding this, I just noticed that the tangent output is scaled with the radius just like the vector output.
I don't really see a need for this and I personally would expect a tangent in this context to be normalized.
Comment Actions
Nice!
For me this patch is good to go now :)
Maybe @Hans Goudey (HooglyBoogly) can just take another quick look on the code side of things, otherwise I think this can be merged.
Comment Actions
Here are the notes from a recent sub-module meeting where we discussed this patch:
- For similar reasons to the easing node patch, it may make more sense to implement this as a node group
- Inner loops can be devirtualized when more smaller functions have fewer inputs.
- Optimizing smaller functions with SIMD and GPU acceleration is in the plans, but hand-optimizing every node becomes much harder
- With things like this implemented as node groups, there is less code to maintain as we refactor things in the future.
- With a builtin "coordinate space conversion" node and the random value node, a node group should be relatively simple. It would be good to at least see what the node looks like as a node group.
So, I think that means the way to move forward is focusing on the "Compose/Decompose/Combine/Separate Vector" nodes, then implementing this as a node group(s) and seeing if the built-in node is still required.
I also wanted to say that I'm personally sorry for not having a consistent opinion about this again. Thinking about T95445: Collect node groups to ship in possible asset bundle and more about the performance aspects changed the way I viewed this.
Comment Actions
Ok, thanks for the message. It is frustrating but I'll keep this open until we have the spherical coordinate node at least.