Page MenuHome
Differential D11810

Cuboid Mesh Primitive Geometry Node
ClosedPublic
Actions

Authored by Rajesh Advani (rajeshja) on Jul 5 2021, 2:55 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Commits
rBb44406f9634a: Geometry Nodes: Enhance the cube mesh primitive as a cuboid
Summary
Description of the problem that is addressed in the patch.

With geometry node mesh primitives, it is currently possible to create a cube and then scale it arbitrarily along X, Y and Z axes. You can also subdivide the mesh but the number of subdivisions is equal along all axes. This means that making the basic frame for something like modular buildings isn't trivial.

Description of the proposed solution, and a motivation as to why this is the best solution.

This mesh primitive enhances the Cube mesh primitive and allows the creation of a cuboid with a configurable size and number of vertices in all 3 directions. The Cube primitive is now similar to the Grid primitive except that it works in 3 dimensions. Inspired by watching the modular building creation tutorials and looking at the demo files for the same.

The cuboid is created using the Mesh class, and so that large meshes with millions of faces are created quickly, Edges are calculated using BKE_mesh_calc_edges.

List of alternative solutions, and a motivation as to why these are undesirable

The alternative to a mesh primitive is to use multiple Grid primitives and rotate them into shape, which can get cumbersome and makes the node tree even harder to read.

Limitations of the proposed solution

As with other geometry nodes, UV maps don't work. This was tested in development using the following fragment of code, applying the Geometry nodes modifier and inspecting the UVMap of the resulting mesh.

MLoopUV *mloopuv = static_cast<MLoopUV *>(CustomData_get_layer(&mesh->ldata, CD_MLOOPUV));
if (!mloopuv) {
  mloopuv = static_cast<MLoopUV *>(CustomData_add_layer(&mesh->ldata, CD_MLOOPUV, CD_CALLOC, NULL, config.loop_count));
}

for (int i=0; i<config.loop_count; i++) {
  mloopuv[i].uv[0] = uvs[i].x;
  mloopuv[i].uv[1] = uvs[i].y;
}
Screenshots

Here is an example of the node in action.

And an example of what is possible when combined with some attribute math

Some sample files:

  1. Since the bounding box node was impacted, this file is to test that it works as before.

bounding-box-test.blend822 KBDownload

  1. Cuboid in action:

cuboid-cube.blend988 KBDownload

  1. And this one is to test versioning.

Cube-geo-node-testfile.blend820 KBDownload

Diff Detail

Repository
rB Blender
Branch
cuboid-2 (branched from master)
Build Status
Buildable 16641
Build 16641: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Jul 12 2021, 10:33 PM
Rajesh Advani (rajeshja) updated this revision to Diff 39488.EditedJul 13 2021, 5:08 PM
  • Until the cuboid needs to be used elsewhere, IMO there is no need for anything to be in a separate header file or a separate file from the node's file.

Done. Merged the external files into the node's file. The reason for splitting was less reuse (though an Add Cuboid mesh via the UI option did cross my mind), and more separation of concerns. I wanted to separate the code for the node from the code for the mesh.

  • If parallelizing this node is making it much more complicated, I'm not sure it's worth it.

I tested with and without parallel, and surprisingly the different in time taken wasn't as much as I expected. Only 16% more for 1000x1000x1000 vertices (where it matters), and 300% more for 10x10x10 (where it doesn't really matter.

Vertex configParallelized Time (ms)Non-parallel Time single loop (ms)
10x10x102.68.6
100x100x100210336
1000x100x10016502550
1000x1000x1001050015500
1000x1000x10003600042000
  • Comments should be C style comments (/* */) and should end with a period.

Done

  • Even if we have the ability to go lower than 2 vertices, the soft limit (the limits in the socket definitions) should probably be 2.

Done.
I had procedural buildings in mind when I brought minimum down to 1, and it made sense that someone might want to make a building only one room high, or one room wide. But I realized that you can still have the Group input configured to a minimum of 1 and feed that to the socket.

  • This sort of thing: int *vert_positions = new int[config.vertex_count]; should always use blender::Array rather than a bare memory allocation.

Done. No longer need these arrays as the parallelization code has been removed. But will keep this in mind for future patches.

  • Having a separate functions for every face is a lot. I wonder if that it's necessary and if reusing code a bit more would simplify things.

While building this, having separate functions to reach via the Outline in an IDE really made it much easier to debug the face generation for a given face. Combining them into a single function makes it 500+ lines long, and hard to read. The code for each face is also quite different, which makes it less fruitful to combine them.

I'll take a look at whether this is some improvement possible though. The face generation code is the hardest to read right now.

  • Testing the UV data can be done with a hack in the code to copy the generated data to a "real" UV layer (CD_MLOOPUV) with the CustomData API.

It will take me a little longer to do this, as I haven't used either of those things before.

  • Replacing Cuboid class with free-standing functions based on code review feedback.
  • Renamed CuboidInputs to CuboidConfig and removed local variables from functions.
  • Moved vert, edge and poly counting to config class.
  • Renamed draw methods to calculate instead, as per review comments from Hans.
  • Replaced word face with poly in all functions.
  • Ran make format.
  • Added comments, fixed vertex normals, and made internal functions static.
  • Moved cuboid code to node file.
  • Removed parallelization. This resulted in a 16% performance hit at 1000x1000x1000 vertices, and a 300% hit at 10x10x10 vertices.
  • Changed // comments to /* */ commends and added period at the end of each.
  • Removed unused arrays.
  • Increased minimum vertex socket values to 2 instead of 1.
  • Removed unnecessary header include of BLI_task, since parallel_for is no longer used.
Harbormaster completed remote builds in B15796: Diff 39488.Jul 13 2021, 5:08 PM
Rajesh Advani (rajeshja) updated this revision to Diff 39551.EditedJul 14 2021, 6:17 PM

@Hans Goudey (HooglyBoogly) Thanks for the CustomData tip. Found some UV calculation bugs, and fixed them.

Combining the face drawing functions definitely makes the code look more complicated and harder to debug, so I'm leaving it as is. If you think it's really important to combine them, I will do so.

Harbormaster completed remote builds in B15824: Diff 39551.Jul 14 2021, 6:17 PM
Rajesh Advani (rajeshja) updated this revision to Diff 39552.EditedJul 14 2021, 6:22 PM

Rebased to latest master.

Harbormaster completed remote builds in B15825: Diff 39552.Jul 14 2021, 6:22 PM
Hans Goudey (HooglyBoogly) added a comment.Jul 15 2021, 7:53 PM
In D11810#307114, @Rajesh Advani (rajeshja) wrote:

Combining the face drawing functions definitely makes the code look more complicated and harder to debug, so I'm leaving it as is. If you think it's really important to combine them, I will do so.

Thanks for the detailed response! It was less about wanting them all combined, and more of a shot in the dark of a way to make the code less complicated (which you would probably have a better sense about than me anyway, at this point).

Rajesh Advani (rajeshja) added a comment.EditedJul 16 2021, 12:41 AM
In D11810#307615, @Hans Goudey (HooglyBoogly) wrote:

Thanks for the detailed response! It was less about wanting them all combined, and more of a shot in the dark of a way to make the code less complicated (which you would probably have a better sense about than me anyway, at this point).

I get what you mean. I hate code that doesn't make sense the first time you read it. But in this case I'm struggling to make it simpler.

Rajesh Advani (rajeshja) updated this revision to Diff 39865.EditedJul 23 2021, 1:01 PM

Rebased to master, fixed some clang format issues, and added comments.

Harbormaster completed remote builds in B15990: Diff 39865.Jul 23 2021, 1:01 PM
Pratik Borhade (PratikPB2123) awarded a token.Jul 23 2021, 1:10 PM
Rajesh Advani (rajeshja) updated this revision to Diff 39915.EditedJul 24 2021, 3:34 PM

Replaced the edge creation code with a call to BKE_mesh_calc_edges.

There is a performance hit of the following magnitude, but the complexity of the edge calculation code is gone.

Vertex configManual Edge creation (ms)Auto Edge creation (ms)
10x10x101.52
100x100x100110135
1000x100x1008951090
1000x1000x10056006860
1000x1000x10001700020000
  • Removed edge calculations, and automatic calculation of edges using BKE_mesh_calc_edges.
  • Fixed issue where edges weren't getting created for lines. Also cleaned up unused parameters.
  • Collapsed most of the poly calculation functions into a single one.
Harbormaster completed remote builds in B16018: Diff 39915.Jul 24 2021, 3:34 PM
Rajesh Advani (rajeshja) added a subscriber: Jacques Lucke (JacquesLucke).EditedAug 2 2021, 10:37 AM

@Jacques Lucke (JacquesLucke) Here are the stats from a release build:

Vertex configManual Edge creation (ms)Auto Edge creation (ms)
10x10x100.20.35
100x100x1007.59
1000x100x1003456
1000x1000x100210375
1000x1000x100010271460

I won't consider them very scientifically captured, since the numbers sometimes tend to vary significantly between changes to the node inputs. But these are the numbers that pop up most often.

Hans Goudey (HooglyBoogly) requested changes to this revision.EditedAug 2 2021, 4:44 PM

Let's definitely stick with the auto edge calculation then. The speed improvement of manual calculation just doesn't seem worth it in this case.

I wonder if you could just call the code from the line node and the grid node in the 1D and 2D cases, that seems nicer than the redundancy here.

Some other smaller comments inline.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cuboid.cc
44 ↗(On Diff #39915)

No need for typedef.

132 ↗(On Diff #39915)

Best to take CuboidConfig as a const reference, it's not a tiny struct.

143–146 ↗(On Diff #39915)

Some unused variables here

147–153 ↗(On Diff #39915)

This logic makes me a bit skeptical honestly, since you have to loop over x*y*z vertices even though you're only working with the surface.
Maybe the alternatives are much more complex though?

148 ↗(On Diff #39915)

For loops can look like for (const int z : IndexRange(config.verts.z) {

194–202 ↗(On Diff #39915)

More edge calculation to remove here?

204–235 ↗(On Diff #39915)

Using BKE_mesh_calc_edges you shouldn't need to fill the edge values, right?

627 ↗(On Diff #39915)

It looks like normals are calculated above, is it necessary to call this? (It's another thing that can be quite slow).
If there's no way around calling this, best to remove the normal calculation above.

631 ↗(On Diff #39915)

This can be quite slow, so we shouldn't leave it in the patch besides for development.

This revision now requires changes to proceed.Aug 2 2021, 4:44 PM
Mattias Fredriksson (Osares) mentioned this in T90404: Geometry Nodes: Point Grid Primitive (3D).Aug 3 2021, 12:51 PM
Rajesh Advani (rajeshja) updated this revision to Diff 40259.EditedAug 3 2021, 12:58 PM
Rajesh Advani (rajeshja) marked 8 inline comments as done.

CuboidConfig now doesn't use typedef, and is passed as a const.
Removed unused variables.
for loops now iterate over an IndexRange.
Edge calculation completely removed.
Calling create_line_mesh and create_grid_mesh for 1D and 2D cases.
Removed normal calculation, marking normals as dirty.
Also removed mesh validation.

Harbormaster completed remote builds in B16186: Diff 40259.Aug 3 2021, 12:58 PM
Rajesh Advani (rajeshja) added a comment.EditedAug 4 2021, 6:26 AM

@Hans Goudey (HooglyBoogly)

Here's a perspective on the complexity when I try creating vertices one face at a time, rather than in the current order.

The dark blue numbers are vertex indexes, while the ones in between are edge indexes (which are now obsolete, but I have not removed them).

With this approach it is easy to calculate polys for the bottom and top faces, but the complexity increases when you move to front and back faces, and even more for left and right faces. The calculations are different for the bottom, top and in-between rows for front and back, and additionally different for 1st and last columns on the left and right faces.
There are additional cases to be handled when the number of vertices in X, Y or Z are only 2.

The resulting poly code is more complex than the current one. And the vertex code is also a little more complex than the current one. This is why I've stuck with the current approach.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cuboid.cc
147–153 ↗(On Diff #39915)

Yes, I tried creating the vertices one face at a time, but that only makes the poly code more complex, because you need to skip the common vertices on each face.

Hans Goudey (HooglyBoogly) requested changes to this revision.Aug 5 2021, 5:32 PM

Okay, that's fine. The loop over x*y*z is a bit unfortunate, but it's not a complicated loop I guess. And there is real value in keeping the rest of the code as simple as possible.

Generally this is looking quite close! I Just have some more comments about simplifying the code inline.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cuboid.cc
43–129 ↗(On Diff #40259)

This could be simplified a bit too:

  • Member variables come first
  • Count calculations return their values instead of setting the values in the class
  • Rely on assert for the 3D case, as discussed below.
class CuboidConfig {
 public:
  float size_x;
  float size_y;
  float size_z;
  int verts_x;
  int verts_y;
  int verts_z;
  int edges_x;
  int edges_y;
  int edges_z;
  int vertex_count;
  int poly_count;
  int loop_count;
  CuboidConfig(float size_x, float size_y, float size_z, int verts_x, int verts_y, int verts_z)
      : size_x(size_x),
        size_y(size_y),
        size_z(size_z),
        verts_x(verts_x),
        verts_y(verts_y),
        verts_z(verts_z),
        edges_x(verts_x - 1),
        edges_y(verts_y - 1),
        edges_z(verts_z - 1)
  {
    BLI_assert(edges_x > 0 && edges_y > 0 && edges_z > 0);
    this->vertex_count = this->get_vertex_count();
    this->poly_count = this->get_poly_count();
    this->loop_count = this->poly_count * 4;
  }

 private:
  int get_vertex_count()
  {
    const int inner_position_count = ((verts_x > 1) && (verts_y > 1) && (verts_z > 1)) ?
                                         (verts_x - 2) * (verts_y - 2) * (verts_z - 2) :
                                         0;
    return verts_x * verts_y * verts_z - inner_position_count;
  }

  int get_poly_count()
  {
    return 2 * (edges_x * edges_y + edges_y * edges_z + edges_z * edges_x);
  }
};
92–117 ↗(On Diff #40259)

No need for this edge count calculation, you can just create the mesh with totedge = 0 and the edge calculation code will take care of it.

133–140 ↗(On Diff #40259)

More code suggestions:

  • Assume the 3 dimension case with edges in every direction
  • I don't think all of the float casting is necessary
  • Use const variables
  • Use the ELEM macro
static void calculate_vertices(MutableSpan<MVert> verts, const CuboidConfig &config)
{
  const float z_bottom = -config.size_z / 2.0f;
  const float z_delta = config.size_z / config.edges_z;

  const float x_left = -config.size_x / 2.0f;
  const float x_delta = config.size_x / config.edges_x;

  const float y_front = -config.size_y / 2.0f;
  const float y_delta = config.size_y / config.edges_y;

  int vert_index = 0;

  for (const int z : IndexRange(config.verts_z)) {
    for (const int y : IndexRange(config.verts_y)) {
      for (const int x : IndexRange(config.verts_x)) {
        /* Only plot vertices on the surface of the cuboid. */
        if (ELEM(z, 0, config.edges_z) || ELEM(x, 0, config.edges_x) ||
            ELEM(y, 0, config.edges_y)) {

          const float x_pos = x_left + x_delta * x;
          const float y_pos = y_front + y_delta * y;
          const float z_pos = z_bottom + z_delta * z;
          copy_v3_v3(verts[vert_index].co, float3(x_pos, y_pos, z_pos));

          vert_index++;
        }
      }
    }
  }
}
164–191 ↗(On Diff #40259)

This function can be simplified a bit:

static void define_poly(MutableSpan<MPoly> polys,
                        MutableSpan<MLoop> loops,
                        const int poly_index,
                        const int loop_index,
                        const int vert_1,
                        const int vert_2,
                        const int vert_3,
                        const int vert_4)
{
  MPoly &poly = polys[poly_index];
  poly.loopstart = loop_index;
  poly.totloop = 4;

  MLoop &loop_1 = loops[loop_index];
  loop_1.v = vert_1;
  MLoop &loop_2 = loops[loop_index + 1];
  loop_2.v = vert_2;
  MLoop &loop_3 = loops[loop_index + 2];
  loop_3.v = vert_3;
  MLoop &loop_4 = loops[loop_index + 3];
  loop_4.v = vert_4;
}
206–207 ↗(On Diff #40259)

Declare variables const where possible

219–222 ↗(On Diff #40259)

Does this comment apply specifically to the bottom faces? It seems it doesn't. Maybe move part of it to the define_poly function?

308–310 ↗(On Diff #40259)

It looks like these three values are set in the if else statements below. So maybe it's better just to declare them like int vert1; to make that clearer.

495 ↗(On Diff #40259)
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cuboid.cc:479:3: warning: do not use 'else' after 'return' [readability-else-after-return]
  else if (config.dimensions == 2) {
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
528–529 ↗(On Diff #40259)

Use BKE_mesh_normals_tag_dirty

547 ↗(On Diff #40259)

In D11923 we settled on a wording that could be nice to use here:
Vertex count must be greater than one along each axis

Also best to use the Info message type, since it's valid to have one of these inputs less than one in some cases, like when animating the inputs.

552–553 ↗(On Diff #40259)

We generally don't have errors like this, since it's sort of arbitrary at which point to add the error.

557–560 ↗(On Diff #40259)

I recommend simplifying what the cuboid code has to do by taking the three cases from create_cuboid_mesh. Then you can only create the CuboidConfig in the three dimension case.

This makes sense semantically too. The 1D case is a line, not a cuboid, and the 2D case is a grid, not a cuboid.

This revision now requires changes to proceed.Aug 5 2021, 5:32 PM
Rajesh Advani (rajeshja) updated this revision to Diff 40433.EditedAug 7 2021, 11:20 AM
Rajesh Advani (rajeshja) marked 12 inline comments as done.

Rebased to master and fixed review comments.

  • Fixed review comments. Added const where possible. Used ELEM for multiple comparisons. And simplified cuboid code to only handle cuboids since grids and lines are handled by the code in those nodes.
Harbormaster completed remote builds in B16276: Diff 40433.Aug 7 2021, 11:20 AM
Hans Goudey (HooglyBoogly) requested changes to this revision.Aug 12 2021, 6:24 AM

Looking quite close! The inline comments are fairly picky at this point.

I did some quick performance testing. On my computer the node takes 1.6 seconds to calculate a 1000x1000x1000 cube. Just 5ms for 100x100x100 though.
I think those results are reasonable, though we could always consider optimizations in the future.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cuboid.cc
20 ↗(On Diff #40433)

Is this include necessary?

92 ↗(On Diff #40433)

We assert above that the edges values are greater than zero, no need to check that here.

126 ↗(On Diff #40433)

Picky thing, but this function's name would be more specific if it was define_quad

153 ↗(On Diff #40433)

Extra newline

157 ↗(On Diff #40433)

Here it's not clear what "cs" means. It probably doesn't need to be contracted, this variable isn't used enough that having a short name is important.

171 ↗(On Diff #40433)

Extra newline

280 ↗(On Diff #40433)

Unnecessary newline (better to keep those lines together I think, since they're all about adding the face.

328 ↗(On Diff #40433)

Same here

420 ↗(On Diff #40433)

Extra newline

This revision now requires changes to proceed.Aug 12 2021, 6:24 AM
Rajesh Advani (rajeshja) updated this revision to Diff 40627.EditedAug 12 2021, 9:33 AM
Rajesh Advani (rajeshja) marked 9 inline comments as done.

The time you're seeing for rendering large meshes is mostly the same as what I'm seeing here. And it does seem fine. If someone needs a really large mesh, I guess they can afford to wait a few seconds.

I've addressed all the comments now.

  • Removed unnecessary include.
  • Removed redundant 0 edge checks.
  • Renamed define_poly to define_quad.
  • Made variable name clearer.
  • Removed unnecessary newlines.
Harbormaster completed remote builds in B16378: Diff 40627.Aug 12 2021, 9:33 AM
Hans Goudey (HooglyBoogly) added a comment.Aug 24 2021, 6:00 PM

We talked about this briefly at the daily geometry nodes sub-module meeting. I mentioned I thought the code was satisfying enough to commit. The consensus about the other items:

  • The node can replace the existing cube node, with the "Cube" name.
  • The default size should be the same.
  • The default vertex count should be 2x2x2, so it's just a more flexible "cube".

This means this patch should include versioning:

  • The value ("default value") on the existing cube node sockets should be copied to the new sockets.
  • Links connecting to the cube's "Size" input should be duplicated. (Or there could be a single vector size input in this node. I forget it we talked about that before, I like the idea though).
Rajesh Advani (rajeshja) updated this revision to Diff 41133.EditedAug 26 2021, 7:29 PM

Moved the cuboid code to the Cube node with defaults to mimic the original Cube.

  • Copied Cuboid code to Cube node. And added versioning to ensure old files using Cube mesh primitive still work.
  • Removed duplicate code in versioning. And using PROP_TRANSLATION to make Size show up as a distance with unit of measure.
  • Removed Cuboid node.
Harbormaster completed remote builds in B16639: Diff 41133.Aug 26 2021, 7:29 PM
Rajesh Advani (rajeshja) updated this revision to Diff 41136.EditedAug 26 2021, 8:13 PM

Rebased from master.

Harbormaster completed remote builds in B16641: Diff 41136.Aug 26 2021, 8:13 PM
Rajesh Advani (rajeshja) edited the summary of this revision. (Show Details)Aug 27 2021, 7:21 PM
Rajesh Advani (rajeshja) updated this revision to Diff 41181.Aug 28 2021, 6:53 AM
  • Changed bounding box to create cube with appropriate scale directly rather than scaling later.
  • Simplified create_cuboid_mesh signature to take a float3 for the size.
Harbormaster completed remote builds in B16663: Diff 41181.Aug 28 2021, 6:53 AM
Rajesh Advani (rajeshja) edited the summary of this revision. (Show Details)Aug 28 2021, 7:14 AM
Rajesh Advani (rajeshja) edited the summary of this revision. (Show Details)
Rajesh Advani (rajeshja) edited the summary of this revision. (Show Details)Aug 28 2021, 7:16 AM
Hans Goudey (HooglyBoogly) accepted this revision.Aug 30 2021, 5:07 AM

Thanks for the hard work on this Rajesh. It works well in my tests, and the behavior reflects what we had discussed as a sub-module previously. When committing I'll do a few small cleanups:

  • Use float3 for size in CuboidConfig
  • Change order of some arguments so that the config is first and the result arguments are last.
  • Adjust whitespace in a couple places.
  • Separate creation of mesh from geo_node_mesh_primitive_cube_exec to allow returning the result.
This revision is now accepted and ready to land.Aug 30 2021, 5:07 AM
Closed by commit rBb44406f9634a: Geometry Nodes: Enhance the cube mesh primitive as a cuboid (authored by Rajesh Advani (rajeshja), committed by Hans Goudey (HooglyBoogly)). · Explain WhyAug 30 2021, 5:14 AM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rBb44406f9634a: Geometry Nodes: Enhance the cube mesh primitive as a cuboid.

Revision Contents
Changeset List

PathSize
source/
blender/
blenloader/
intern/
49 lines
nodes/
geometry/
10 lines
nodes/
2 lines
465 lines
8 lines
2 lines

Diff 41136

View Options

source/blender/blenloader/intern/versioning_300.c

Loading...
View Options

source/blender/nodes/geometry/node_geometry_util.hh

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_bounding_box.cc

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cube.cc

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_grid.cc

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_line.cc

Loading...