I'm wondering what to do about the stability of points through time. The regular point distribute node tries to handle this to the best of it's ability but VDB's Point Scatter API does not handle this. With the grid distribution mode it should be rather stable for free (as long as the volume is using the "size" mode) and all you would have to do is hash the position to create an "id" attribute, the random distribution mode presents more of an issue.

Looks close, the instances stuff isn't quite right though.

Investigated the issue with instance transforms. This code previously just uses VDB's "indexToWorld" call in order to convert voxel space coordinates to world space coordinates, to do this vdb must be aware of the transform. To solve this I apply the transform of the instances to the distributed points, It appears that non-instances always have an identity matrix transform which is nice because I can just rely on the VDB's transforms for them. I think I got it mostly working with the one caveat that since instances don't contribute to the VDB's "grid.voxelSize()" and messes up the spacing a bit on grid mode. Chasing it down the rabbit hole it appears to be stored in a Vec3d in openvdb::ScaleMap when the grid is initiated and I don't know how or if it would be possible to reconstruct spacing info and under what scenario we should since the only indicator would be the identity matrix from the "non-instance" on gather instances, which is still more than possible with actual instances.

In D10506#271978, @Hans Goudey (HooglyBoogly) wrote:

Almost there! I think it would be great to get this into 2.93.

Don't worry about getting it in 2.93 if there are still things left on the table that could be done better. It would be likely better anyways to deliver this alongside other useful volume manipulation tools like mesh to volume, texture displace, volume filter (dialate/erode/blur), and volume combine/boolean. I feel like the community could produce a lot of stuff with just that. On it's own only using Points to Volume has already gotten some initial complaints in early feedback.

Without looking at this for longer, I'm not quite sure how to take into account the instance transforms during the scattering (rather than after for moving the generated points). But I think it's essential, since without it the transform node won't change the result as expected.

In other words, these two instances of the same volume should not produce the same number of points, the larger one should be filled at the same density and produce more points.

This would work out of the box if the indexToWorld and voxelSize calls were valid in their information. Unfortunately instances have stacked another layer of transformations on top of the grid's world space transform, making indexToWorld semi-local-space but only for instances. This is what threw a wrench in it for us as I didn't take into account that volumes could be instanced like this and the API cannot work with what it doesn't know. The idea was that by setting up the spacing to be the same as the voxel size in a volume create node you could get each point to line up perfectly with the center of each voxel, I felt this would be the most desired behavior. I thought that geometry sets would only ever contain a single volume that would be piped into the volume component, it seemed straightforward up until this point when it became a mess of coordinate space stuff.

If you don't have ideas for this, I'm happy to help and look into it more tomorrow.

I would appreciate that! I'm kind of out of ideas for this. it feels like openvdb::tools::NonUniformPointScatter needs to be pulled apart to get more mileage out of it anyways. I have thought of some ways that it could be made more stable (asuming the volume was stable) by making the random seed for each cell a hash of the center's indexToWorld, and that was one of my main priorities until instances came into the mix... Now my head just hurts.

It appears it got reverted somehow... Don't know how that happened.

This isn't quite right, since theoretically a set_group can have multiple transforms (though we aren't using that ability yet).

It actually doesn't look like there is a need to store the grids and transforms in a temporary vector anyway. Also, take a look at the way the point distribute node does this. The way it's structured (storing an array of vectors (one for each instance) makes it relatively easy to parallelize the distribution for every instance.
[...]
Storing an array of vectors for the position isn't essential for the initial version of this node, I'd be fine if you didn't include that. But scattering for every transform in the array is important.

I'll have to think more about this. Making multiple streams that can be merged sounds good for parallelism although I do have to point out that having more than one volume is already an edge case and might not be worth it. When it comes to for transform in set.transforms: I don't really know how to work with that. How do you apply multiple transforms to a single instance of a volume? From what I have observed the first transform will only ever be relevant.

(Realized when attempting to quote the original it created a new inline comment that I cannot edit nor delete, ignore that.)

I do have to point out that having more than one volume is already an edge case and might not be worth it

I'm not sure I'd call it an edge case. Instances will be an important optimization used in many places during execution, especially when direct instancing of a geometry set becomes possible.
They already are actually, if you take in input from another object and split it with two wires to transform it twice, etc.

From what I have observed the first transform will only ever be relevant.

Again, this is true for now, but only for the near future.

How do you apply multiple transforms to a single instance of a volume?

Looking over the Grid class quickly, copyReplacingTransform seems like the best way to do this, shouldn't be too hard!

I would suggest structuring this like the point distribute node.
Or another method would be to define a function that iterates over all of the density grids in the geometry set and pass each method as a function argument with its various inputs.
Maybe something like the following?

static void distribution_callback(
    Span<GeometryInstanceGroup> set_groups,
    const StringRef density_grid_name,
    FunctionRef<void(const openvdb::FloatGrid::ConstPtr, Vector<float3>)> func,
    MutableSpan<Vector<float3>> positions_all)
{
  int i_instance = 0;
  for (const GeometryInstanceGroup &set_group : set_groups) {
    const GeometrySet &geometry_set = set_group.geometry_set;
    const Volume *volume = geometry_set.get_volume_for_read();
    VolumeGrid *density_grid = BKE_volume_grid_find(volume, density_grid_name.data());
    if (density_grid == nullptr) {
      i_instance++;
      continue;
    }
    const openvdb::FloatGrid::ConstPtr grid = openvdb::gridConstPtrCast<openvdb::FloatGrid>(
        BKE_volume_grid_openvdb_for_read(volume, density_grid));
    if (!grid) {
      i_instance++;
      continue;
    }

    for (const float4x4 &transform : set_group.transforms) {
      /* Get copy of grid pointer that has its transform multiplied by this one. */
      func(grid, positions_all[i_instance]);
      i_instance++;
    }
  }
}

With this in the calling function:

/* Remove any set inputs that don't contain a volume, to avoid checking later on. */
for (int i = set_groups.size() - 1; i >= 0; i--) {
  const GeometrySet &set = set_groups[i].geometry_set;
  if (!set.has_volume()) {
    set_groups.remove_and_reorder(i);
  }
}

int instances_len = 0;
for (GeometryInstanceGroup &set_group : set_groups) {
  instances_len += set_group.transforms.size();
}

Array<Vector<float3>> positions_all(instances_len);

Just checked the patch for the first time and it looks great!
I do have some concerns from a usability standpoint, regarding the context of this node.

I understand that the decision was made to focus on distribution in voxel rather than mesh volumes and I agree with that decision.
But still, I don't see a good reason why this shouldn't work on a default cube, from the user's perspective:

So, I think, along with this patch we should prioritize the following as well:

• T86838: Mesh to Volume Node
• T86970: Separate Components Node
• Implicit Mesh to Volume conversion for this node (arguable)

Otherwise, I think, the workflow is highly incomplete.

To the node itself:
Random:

• For the Point Distribute node we laid a strong focus on stability. Here there seems to be quite a bit of fluctuation when adjusting the density parameter / density of the input volume.

Grid:

• I find it confusing that both the method and the density attribute are called Grid, as what defines the distribution grid is the spacing vector.
• Alternatively I would expect the option to place a single point per voxel. But I agree with the design that this should additionally solved in a Volume to Point node later on.
• The positioning of the point grid seems to rely on the voxel grid. That should probably not be the case.

Just chatted about this with Hans, Jacques and Pablo and there was a consensus that we should probably rename the Grid attribute to Density.

We also agreed that at least getting T86838: Mesh to Volume Node done before this node would be a good idea to allow a proper workflow. @Kenzie (kenziemac130) would you maybe be up to tackle that one as well?

I understand that the stability of the distribution is probably out of scope for this patch, so you can disregard that for now.

In D10506#276878, @Simon Thommes (simonthommes) wrote:

I forgot to mention one thing:
The default spacing of (0.3,0.3,0.3) seems a bit arbitrary to me. I would just put it at (1.0,1.0,1.0) that is equivalent to the default density of 1 and also means less points by default which is good when dealing with large volumes.

I agree that 0.3 is weird. It was chosen to line up with the default spacing of the Points to Volume node. If the spacing is the same on both nodes you end up with a point at the center of each voxel. I can change it if you want but at defaults a spacing of one meter it might be a little too coarse.

In D10506#276871, @Simon Thommes (simonthommes) wrote:

Just checked the patch for the first time and it looks great!
I do have some concerns from a usability standpoint, regarding the context of this node.

I understand that the decision was made to focus on distribution in voxel rather than mesh volumes and I agree with that decision.
But still, I don't see a good reason why this shouldn't work on a default cube, from the user's perspective:

In fact, the only way to generate volume information to be used with this node within geometry nodes right now is the Points to Volume node.

So, I think, along with this patch we should prioritize the following as well:

• T86838: Mesh to Volume Node
• T86970: Separate Components Node
• Implicit Mesh to Volume conversion for this node (arguable)

Otherwise, I think, the workflow is highly incomplete.

I agree it's pretty incomplete, user feedback has revealed that as well. As for implicit conversion I will have to think about that one... There's a lot that could go wrong there without the user ever realizing the data has been turned into a volume and that they are the ones in control of how that volume is defined. I can easily see most people not even realizing that the node works best or at all off of a volume input. The fact that volumes have been lumped into a bucket of "Geometry" is already been a headache in this patch and I wish there was a better way to communicate the desired input type to the user.

To the node itself:
Random:

• For the Point Distribute node we laid a strong focus on stability. Here there seems to be quite a bit of fluctuation when adjusting the density parameter / density of the input volume.

Yeah, that's unfortunately an effect of VDB's point scatter API not being stable. I was considering re-implementing it here and use a hash of the voxel's centroid position in order to improve stability a bit when voxel size is stable.

Grid:

• I find it confusing that both the method and the density attribute are called Grid, as what defines the distribution grid is the spacing vector.

The naming of "Grid" was to make it the same as the volume distribution mode on the old particle system. Spacing was ultimately chosen over resolution after discussing it.

• Alternatively I would expect the option to place a single point per voxel. But I agree with the design that this should additionally solved in a Volume to Point node later on.

This was already a source of headache, the best solution I could think of is to make the spacing sync up perfectly when the ***** to Volume node's voxel size and the Point Distribute Volume spacing lined up perfectly.

• The positioning of the point grid seems to rely on the voxel grid. That should probably not be the case.

[...]

As mentioned in the previous answer the position of each point is meant to sync up with the grid when both the spacing and voxel size are in the same sweet spot. This point position is being offset based on the min extents of the voxels bounding box which is being modified when you change the amount. If you set the Points to Volume to size this problem goes away.

Again, not the most ideal solution but in the discussion this was one of the only ways I could see myself covering this use case as well with minimal issues. I agree it does seem rather unintuitive though.

Just chatted about this with Hans, Jacques and Pablo and there was a consensus that we should probably rename the Grid attribute to Density.

Sounds good for now, although I should point out that this patch was designed to accommodate VDB distance fields in the future as well (hence the "density" in point_scatter_density_grid()) so that attribute name won't always be correct. Something that blender right now overlooks and doesn't support is that VDB does more than just "fog volumes".

We also agreed that at least getting T86838: Mesh to Volume Node done before this node would be a good idea to allow a proper workflow. @Kenzie (kenziemac130) would you maybe be up to tackle that one as well?

This one has turned out to be a lot more work than initially thought and I am hesitant to bite off any more than I can personally chew. I am not really good with git workflow either so I would have to figure out how to juggle the different patches. I don't want to over-promise anything here.

I understand that the stability of the distribution is probably out of scope for this patch, so you can disregard that for now.

I was hoping to get around to that earlier but then some shenanigans with volumes being instanced and breaking down blender's abstraction and layers then bubbling that data back down into VDB by making short-lived shallow copies of the volume. Which I did not expect to be part of this patch either and this technical debt will make writing volume nodes in the future a lot more painful. As me and @Hans Goudey (HooglyBoogly) found out: Volume to Mesh is incomplete and doesn't deal with this either, which is why it looks soo unassuming and clean.

Thanks for the response!

In D10506#276972, @Kenzie (kenziemac130) wrote:

I agree that 0.3 is weird. It was chosen to line up with the default spacing of the Points to Volume node. If the spacing is the same on both nodes you end up with a point at the center of each voxel. I can change it if you want but at defaults a spacing of one meter it might be a little too coarse.

It is just as coarse as the default density of 1 for the random distribution. And, as I said, I think it is good to have the default be less computationally intensive.

I agree it's pretty incomplete, user feedback has revealed that as well. As for implicit conversion I will have to think about that one... There's a lot that could go wrong there without the user ever realizing the data has been turned into a volume and that they are the ones in control of how that volume is defined. I can easily see most people not even realizing that the node works best or at all off of a volume input. The fact that volumes have been lumped into a bucket of "Geometry" is already been a headache in this patch and I wish there was a better way to communicate the desired input type to the user.

I agree that this point needs some further discussion and it shouldn't be part of this patch, but that discussion should follow shortly after having this node and the Mesh to Volume node. I think from a user perspective it totally makes sense to be able to quickly scatter points in a mesh volume without an explicit conversion first. There could easily just be an info popover that informs the user when the implicit conversion is happening. The explicit workflow with additional control would, of course, also be possible.

The naming of "Grid" was to make it the same as the volume distribution mode on the old particle system. Spacing was ultimately chosen over resolution after discussing it.

I don't have a problem with the name of the distribution method being Grid. Just that the same name is used for the density grid of the volume input that is used for distribution, because the properties of the generated point grid are not actually defined by the volume grid. That is confusing.
I would actually do this the same way as with the surface distribution node, calling it Density Attribute.

Sounds good for now, although I should point out that this patch was designed to accommodate VDB distance fields in the future as well (hence the "density" in point_scatter_density_grid()) so that attribute name won't always be correct. Something that blender right now overlooks and doesn't support is that VDB does more than just "fog volumes".

That sounds to me like it would either be a new distribution method or an additional, optional input attribute. So I don't think it collides with naming this Density Attribute.

This was already a source of headache, the best solution I could think of is to make the spacing sync up perfectly when the ***** to Volume node's voxel size and the Point Distribute Volume spacing lined up perfectly.

This is not necessary for now, I think it would be another node anyways.

As mentioned in the previous answer the position of each point is meant to sync up with the grid when both the spacing and voxel size are in the same sweet spot. This point position is being offset based on the min extents of the voxels bounding box which is being modified when you change the amount. If you set the Points to Volume to size this problem goes away.

Not sure I'm understanding correctly. When I set the Resolution Mode of the Points to Volume to Size the same problem persists.
I understand where the problem is coming from, but I don't agree with the current behavior. I think for this distribution node it makes sense to completely disconnect the points position of the generated grid from the incoming volume grid. Lining them up perfectly should be an additional use-case, solved buy either another node or mode imo.

This one has turned out to be a lot more work than initially thought and I am hesitant to bite off any more than I can personally chew. I am not really good with git workflow either so I would have to figure out how to juggle the different patches. I don't want to over-promise anything here.

That's fine, don't feel obligated to take that over, I'm sure we'll find somebody to do this. @Jacques Lucke (JacquesLucke) did the modifier for this iirc, so maybe he would be a simple task for him?

I wish there was a better way to communicate the desired input type to the user.

One thing I've been doing in mockups is naming the fields "Mesh" or "Volume" or "Curve" instead of "Geometry". We haven't "officially" agreed on that, but I think it's a nice solution to this.

I would actually do this the same way as with the surface distribution node, calling it Density Attribute.

IMO the "Attribute" at the end is a bit too long, and I don't really think it's necessary, it goes without saying that a string field refers to an attribute in geometry nodes.

I was hoping to get around to that earlier but then some shenanigans with volumes being instanced and breaking down blender's abstraction and layers then bubbling that data back down into VDB by making short-lived shallow copies of the volume. Which I did not expect to be part of this patch either and this technical debt will make writing volume nodes in the future a lot more painful.

Hopefully in the future we can find some way to abstract this that makes it less painful.

In D10506#277104, @Hans Goudey (HooglyBoogly) wrote:

One thing I've been doing in mockups is naming the fields "Mesh" or "Volume" or "Curve" instead of "Geometry". We haven't "officially" agreed on that, but I think it's a nice solution to this.

I would still prefer it if we had implicit conversion, honestly. Then the type should not matter. But for when we explicitly have only one possible input component that is being used that sounds like a good idea!

I would actually do this the same way as with the surface distribution node, calling it Density Attribute.

IMO the "Attribute" at the end is a bit too long, and I don't really think it's necessary, it goes without saying that a string field refers to an attribute in geometry nodes.

I agree, but having two inputs both called Density doesn't sound like a good solution to me either. We could call the attribute input Mask. I remember that we decided against that, but I can't see a great option right now.

Hopefully in the future we can find some way to abstract this that makes it less painful.

Agreed :D

Since there is some discussion about the naming I will settle with the following:

Grid distribution will not be renamed to density because: a: "density" is not at all descriptive of how the points will be scattered, points placed in a density could easily take on other shapes than a grid. b: this is not consistent with blender's previous naming convention.

The "Grid" socket will be renamed to "Density" because: It is the most descriptive name under "random" (although "mask" would technically be better for "grid") and users likely don't know what a VDB grid is at first glance.

Implicit conversion is up on the table for @Simon Thommes (simonthommes) to propose a solution for in the future. One issue there is that a density attribute would likely be lost when being converted from interpolated values on flat triangles to voxels. How would the resolution of the internal volume be determined? Would the user be provided an interface here similar to mesh to volume? Is that newly created volume just thrown away? If the mesh volume is used to scatter points how will this differ avoid the unpredictability issues of the previous system? These are some of the problems that came up when drafting this one that would need to be solved, personally I decided it wasn't worth it.

I have to be honest: making the grid offset constant when changing the resolution of the grid is an issue that I find pretty overwhelming. I'm not very good at describing the math behind how it currently works but there isn't really a reference space here to work with to make this easy or clean. Grid distribution works in VDB index space and is made scale dependent based on the voxel size. Transforming it into a world space before scattering like requested would likely mean figuring out how to transform/rotate the bounding boxes of each voxel/tile, packing that into a grid, itterating and sampling against the original bounding box. The goal line feels like it just keeps moving further as I approach it and another rewrite is needed. I'm getting pretty burned out on this patch and I have to admit if I can't figure that one out with my limited math skills I will likely just have to call it and resign from this task. Hopefully the groundwork or discussion that was laid would still be useful.

You've done a great job of bringing the patch this far and I think it would be great if you could bring it all the way into master as well. But, of course, you're not obligated to do that and I can understand that when there are regularly more requests made, that it can just be too much.

Maybe I should put my feedback more into perspective:
I'm just trying to push for what I think would be the optimal behaviour of a feature, without regarding the implications on a development level because I'm simply missing that perspective. So, I'm sorry if I'm putting up barriers that are too much of a challenge.

If fixing the grid in position is something that requires too much effort, I don't think it's a blocking issue. I can totally see the use-case of when precision is required explicitly done by a 3D point grid input node and a point separation, once we have the needed operations.
But if this is just a matter of experience with the vdb code or space transformation, I'm sure another developer that has dealt with something similar before would be happy to help out. For me it's difficult to gauge what's needed. @Hans Goudey (HooglyBoogly)
do you have an idea or know the right person to point to?

The naming sounds good! To be clear, I think there was a misunderstanding regarding the name of the Grid distribution method, I was not proposing to change that. If somebody else was doing that maybe I missed it.

About the implicit conversion I totally agree that there is more to be figured out and it will also be a question for the geometry node system in general. My idea would be just a very bare-bones solution for ease-of-use, that simply uses a fixed resolution and displays an info popover to explain to the user that there is an explicit way of doing this.
But I agree this should be part of another task and the specifics should be discussed then, but thanks for already laying out your thoughts regarding that topic!

In D10506#277350, @Simon Thommes (simonthommes) wrote:

You've done a great job of bringing the patch this far and I think it would be great if you could bring it all the way into master as well. But, of course, you're not obligated to do that and I can understand that when there are regularly more requests made, that it can just be too much.

I'll try to finish up the initial patch in an acceptable state. This area will likely have a lot that could be improved in the future. As for the drifting of points when resolution changes I have not experienced that being a significant problem in trade off for lining up with the volume's contents. Usually the user wants the size of a volume's voxels remains stable anyways. If for some reason the user is not working with constant voxel sizes: the user can repack them with the bounding box, some math, and attribute remap node.

So I will set myself the following concrete goals:

• Rename the Grid attribute to Density.
• Handle instance transforms like native VDB transforms.
• Improve the stability of distribute random under constant voxel size and introduce the ID attribute.

Hopefully this will provide a good enough initial foundation for the node to be improved in the future. If there are any small changes like name changes, fixes, or updates, I will be happy to add them but I think I would like to lock off any more rewrites past that point.

Thank you soo much for everyone's feedback and continued work in the surrounding areas! Things have been difficult and demotivating recently but I would like to return to this patch after D10906 lands for BKE_volume_grid_shallow_transform. Thanks again.

In D10506#278218, @Kenzie (kenziemac130) wrote:

Thank you soo much for everyone's feedback and continued work in the surrounding areas! Things have been difficult and demotivating recently but I would like to return to this patch after D10906 lands for BKE_volume_grid_shallow_transform. Thanks again.

Note: It seems it was committed.