This feature resolves a small but important papercut as it resolves an issue where the grid primitive is used to instantiate data using the Point Instance node. In particular, it allows the grid primitive to generate either a line or a point which I would refer to as the degenerate cases for a 'mesh grid' but valid cases when creating a 'point grid'.

Use-case would be when one wants to generate a node to fill/instantiate geometry along both the X and Y axis, but in certain cases only generate a single row or a single object. In 2.93 one can handle these cases separately, either by constructing a separate node group (to instantiate along a line) or conditionally add a line primitive when only generating a single row. These are cumbersome solutions polluting the node tree, creating the line/point using the grid primitive would be a preferable and general solution to this problem.

Node design

It might be relevant to discuss the design for a 'point grid' primitive before providing a review of the differential even if this might not be the optimal place. As the patch already exists, I thought it would be appropriate to discuss possible designs here even if they might not align with the original author(s) intention.

Point grid as a separate mode

Generating a point grid could be a separate mode (similar to the line primitive). This would be beneficial as it avoids generating grid meshes with no faces for the degenerate cases and simultaneously improve performance when generating Point grids. The only downside would be if there is a use-case where one wants the faces/edges as well (rather then only points), such cases could always be resolved by generating a second 'mesh grid' primitive and could be preferential since my guess is that it's a rare case.

3D grid

This is slightly out of scope, but relevant for the point grid concept. A point grid mode could be designed for creating 3D point grid(s) rather then 2D grids. Use-case would then be when one wants to fill a 3D space with objects. However this might preferentially be resolved by improving the 'Point distribute' node instead. Personally I'm not in favor of using the distribution node since the node is currently designed for face distribution and the use-cases are quite distinct.

Code review

There are currently issues in the proposed changes and relevant parts in the original code.

Generated lines/points are offset

Generating edges and lines in the current diff are offset from origin. In my view this is incorrect, a line does not have a width (or height) and the parameter should not affect placement of the vertices. To implement this change, one needs to consider the series and its initial condition when generating the coordinates. Doing so one would acknowledge that lines 82 and 83 generate undefined behavior as for a line or point they divide by 0:

const float dx = size_x / edges_x;
const float dy = size_y / edges_y;

Looking careful there is also still an assert on the first line in the function to ensure this case does not occur:

BLI_assert(verts_x > 1 && verts_y > 1);

Cumultative floating point bug

This is not a problem with the diff itself as it is a bug in the original code, but it's related to the problem above. Coordinate values are created by repeated addition of floating point numbers, this is not appropriate as a significant error will accumulate over multiple additions. This is not obvious from small vertex counts, but it produces clearly visible errors when creating large grids (say 10k width, 10k verts along X). One should address this bug when addressing the previous issue, short description of this problem is available at https://floating-point-gui.de/errors/propagation/

Code duplication

To determine the edge flag, the same if/else statement is inserted in two loops. This is not only code duplication, it also adds unnecessary overhead as the condition needs to be evaluated in each iteration. A proper solution would be to determine the flag value before the loop and set the pre-determined flag value in the loop, this would remove both duplication and unnecessary evaluating an if/else condition.

if (edges_x == 0 || edges_y == 0) {
  edge.flag = ME_LOOSEEDGE;
}
else {
  edge.flag = ME_EDGEDRAW | ME_EDGERENDER;
}

Subjective concern(s)

This is more a question of code quality with regards the original code then anything else. For loops used in the file use IndexRange iterators, these operate on int64_t and thus there is a lot of implicit conversions going on since all other integer values are of int type. This should not be that important except for possible compiler warnings or a minimal overhead. Is there a good reason for this or should it be changed? To me it looks weird but it's a subjective opinion and one could argue for either case.

Summary

To summarizing my concerns for the current patch iteration.

• Possibility to discuss/resolve this issue from the perspective of a point grid mode.
• Issue with a coordinate offset for the degenerate cases should be resolved.
• The error accumulation bug should also be resolved when resolving previous point.
• Code duplication for the edge flag should be addressed.

A working copy resolving these issues is available below, note that the solution still may be imperfect. It would also be interesting to hear an opinion from @Hans Goudey (HooglyBoogly) regarding these concerns. I could also be happy to resolve these issues if appropriate.

BR

Mattias

@Mattias Fredriksson (Osares) Wow, that was maybe the most thorough comment I think I've read on here, nice work.

I agree that a "Point Primitive" 3D grid node might be a better solution here. I think it's just a bit simpler that way-- each node has a smaller set of concerns and less complexity.
A 3D point grid primitive would be very simple to code anyway. It could also have a "Fill" option as well, to control whether to generate points on the inside of the space.
Would you be interested in creating that 3D grid node in a new "Point Primitive" category?

As for the other issues, I agree about the floating point accumulation issue. More recently I've been avoiding that by multiplying with a delta, I just didn't think of that here.
Do you want to submit a separate patch (a new diff) with that change split out?

@Hans Goudey (HooglyBoogly)

With regards to the floating point error accumulation, I can make a separate patch for that. Noticed the issue was present in other primitives so creating a new diff to address all of them at the same time wouldn't hurt.

With regards to a 3D point grid, I can create a patch for it. I'm just concerned about the design, as a user I wouldn't want to many nodes/categories cluttering the window and there are already more categories in the geo node editor then in the shading editor. Another problem is that mesh/point primitives would have similar names and do similar things. In the grid case, the mesh grid node is called 'Grid' so the point primitive would be called 'Point grid'? It feels the problem is a design problem rather than an actual implementation issue (particularly since one can generalize the code for generating vertex data for the grid to avoid duplication). I think I saw something about a weekly meeting tomorrow in blender.chat? Could try to check it out.

BR

Mattias

I agree the that generating info messages are unnecessary but they are only tagged as info and do not convey that something is wrong. There are therefore some reasoning for keeping them as they provide visual information to the user that it does in fact output an empty, it is not always obvious that inputs are 0 when provided by another node/node tree and it could be relevant to help users debug their node tree. However, I also agree that this visual information is not always preferred as it also distracts the user when the behavior is expected and a user might then expect all nodes to highlight that the output is empty (and issues would become 'traceable' in the tree). I do make the assumption that trees will utilize empty behavior and the tracing just become a distraction and hides true errors, but one could introduce a filter for different warning levels or provide a visualizer highlighting empty nodes (which would be independent and make such messages superfluous). To summarize, I believe the behavior should be determined by the design and be consistent with other nodes, and I agree that in this case it should not output any message since the other nodes do not.

Will upload a new diff.

Cheers