Page MenuHome

Geometry Nodes: Greatly improve speed of some mesh primitives
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Mar 16 2021, 12:16 AM.
Tokens
"Love" token, awarded by wilBr."Love" token, awarded by gritche."Love" token, awarded by billreynish."Love" token, awarded by ace_dragon."Love" token, awarded by someuser."Love" token, awarded by rawalanche.

Details

Summary

The BMesh primitive operators are implemented differently than you might expect.
Instead of using the fact that the result is known and constant to their advantage,
they use operators like extrusion, removing doubles, and rotation.

Implementing the primitives directly with the Mesh data structure is much
more efficient, since it's simple to fill the result data based on the known inputs.
It also allows also skip the conversion from BMesh to Mesh after the calculations.

Speed matters a lot more for procedural operations than for the existing mesh
primitive operators accessible from the add menu, since they will be executed
for every evaluation rather than only once before a destructive workflow.

ShapeAverage Before (ms)Average After (ms)Speed Factor (x times faster)
Cylinder1.16760.313273.72
Cone4.58900.1776225.8
Sphere64213.313.5954720

The downside of this change is that there will be two implementations for
these three primitives, in these nodes and in bmo_primitive.c. One option
would be re-implementing the BMesh primitives in terms of this code, but that
would require BMesh to depend on Mesh, which isn't desired.

On the other hand, it will be easier to add new features to these nodes like
different fill types for the top and the bottom of a cylinder, rings for a cylinder,
and tagging the output with boolean attributes.

Another factor to consider is that the add mesh object operator could be implemented
with these nodes, just providing more benefit for a faster implementation.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Mar 16 2021, 12:16 AM
Hans Goudey (HooglyBoogly) created this revision.
  • Merge branch 'master' into geometry-nodes-mesh-primitives
  • Fixes and cleanup after merge
Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 29 2021, 12:42 PM

I can't apply this on master anymore.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc
286

As mentioned before, might be good to use another default name, because uv seems to have special meaning in cycles.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_uv_sphere.cc
29

You shouldn't really change defaults as part of this patch.

164

Don't compute the sin of an angle twice.

305

Change attribute name.

This revision now requires changes to proceed.Mar 29 2021, 12:42 PM
Omar Emara (OmarSquircleArt) added inline comments.
source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc
322

One can avoid computing sin and cos inside the loop by rotating the points instead of incrementing the angle as follows:

float i_cos = 1.0f
float i_sin = 0.0f
const float step_cos = std::cos(angle_delta)
const float step_sin = std::sin(angle_delta)
for (const int i : IndexRange(verts_num)) {
  // x = i_cos;
  // y = i_sin;
  // ...
  rotate_step(&i_cos, &i_sin, step_cos, step_sin);
}

void rotate_step(float *i_cos, float *i_sin, float step_cos, float step_sin) {
  const float new_cos = step_cos * *i_cos - step_sin * *i_sin;
  *i_sin = step_sin * *i_cos + step_cos * *i_sin;
  *i_cos = new_cos;
}
source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc
322

I actually tried that for the circle node, and at least in my tests it didn't improve performance in a non-negligible way. It's a cool idea though.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
  • Merge branch 'master' into geometry-nodes-mesh-primitives
  • Don't calculate sin twice, fix UVs
  • Fix UV order, sphere and cylinder height

I remember discussing the uv name before, but I don't remember exactly why the conflict wasn't okay. In my testing using the uv name is working fine.

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_uv_sphere.cc
29

I'm happy to split this out, but the new implementation supports only two rings where I don't think the previous did:

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_uv_sphere.cc
29

One 1 ring I mean, not 2.

I asked Brecht about the "uv" naming:

that's fine, that "uv" in the Cycles code doesn''t actually influence anything in the Blender integration
it's just the default name when adding a standard UV layer, but for Blender we always set the name from the UV layer being exported

That's nice that we can keep using the lowercase naming convention then.

Code wise this seems fine.

For some reason I still get different behavior when using uv or another name for the attribute.. mesh->need_attribute(scene, name) in attr_create_generic returns false for the uv attribute. Note that it does work correctly, when I change the "uv" strings in cycles code to something else.

Hmm, okay. I don't know why I wasn't noticing that in my tests.. who knows.

Since there's no particular reason, I just used "uv_map" instead.

The patch should be updated to include the edge redraw flags ME_EDGEDRAW | ME_EDGERENDER so that wireframe works. Similar to what D10878 needed to do

Right, thanks for the reminder.

I guess the cleanest way would be do have the uv attribute name as input. Then it can also be turned off by just not providing a name. Fortunately, this is easy to change with versioning.

Besides that, it seems to work well.

This revision is now accepted and ready to land.Apr 7 2021, 10:28 AM

Yes, that's a good idea! I think Ideally the name socket wouldn't be exposed by default, but you could expose it with the side-panel like animation nodes or the prototype you made earlier.