Details

Reviewers

Summary

Add the Simple subdivision option to Geometry nodes. It was added as a new node instead of part of the existing subdivision node because of future backend changes to the Simple option. (See T85584)

Diff Detail

Repository

rB Blender

Branch

simple_subdivision (branched from master)

Build Status

Buildable 12936
Build 12936: arc lint + arc unit

Event Timeline

Eitan Traurig (EitanSomething) requested review of this revision.Feb 12 2021, 7:48 PM

Eitan Traurig (EitanSomething) created this revision.

Harbormaster completed remote builds in B12862: Diff 33941.Feb 12 2021, 7:48 PM

Eitan Traurig (EitanSomething) edited the summary of this revision. (Show Details)Feb 12 2021, 7:50 PM

Eitan Traurig (EitanSomething) added a project: Geometry Nodes.

Do we really need OpenSubdiv for the simple subdivide mode?

source/blender/nodes/geometry/nodes/node_geo_simple_subdivision_surface.cc
113	Remember to remove dead code.

Remove dead code

Harbormaster completed remote builds in B12865: Diff 33945.Feb 12 2021, 8:47 PM

Eitan Traurig (EitanSomething) marked an inline comment as done.Feb 12 2021, 8:47 PM

Leon Schittek (lone_noel) awarded a token.Feb 12 2021, 9:53 PM

Remove unnecessary Use Crease option

Harbormaster completed remote builds in B12868: Diff 33948.Feb 12 2021, 10:26 PM

Eitan Traurig (EitanSomething) edited the summary of this revision. (Show Details)Feb 12 2021, 10:27 PM

Eitan Traurig (EitanSomething) edited the summary of this revision. (Show Details)

Remove unnecessary Boundary Smooth and Smooth UVs options.

Harbormaster completed remote builds in B12878: Diff 33969.Feb 14 2021, 1:12 AM

Eitan Traurig (EitanSomething) edited the summary of this revision. (Show Details)Feb 14 2021, 1:51 AM

Wouldn't it be better to call it "Subdivide"? Similar to Edit Mesh tool.

Change 10029 -> 1029

Harbormaster completed remote builds in B12889: Diff 33986.Feb 15 2021, 4:22 AM

Florian Kamenz (TimBrown) awarded a token.Feb 15 2021, 8:58 AM

clang format

Harbormaster completed remote builds in B12936: Diff 34068.Feb 16 2021, 9:30 PM

Pretty close! Since I think Dalai mentioned a while ago that there were some different plans for simple subdivision (don't remember details?), maybe it's best if we quickly run this idea by @Sergey Sharybin (sergey).

source/blender/nodes/NOD_geometry.h
55	Words in function names (and variable names) should be ordered from least to most important. `register_node_type_geo_simple_subdivision_surface` -> `register_node_type_geo_subdivision_surface_simple`
source/blender/nodes/NOD_static_types.h
274–275	We just add new nodes at the bottom here, in the same order as the definitions of `GEO_NODE_*`

This revision now requires changes to proceed.Feb 17 2021, 5:34 AM

@Hans Goudey (HooglyBoogly), the idea was to move simple subdivision type from the Subdiv modifier and have it as a dedicated modifier. The reason for that is that then it is possible to use faster for the specific task BMesh based implementation. We really shouldn't use OpenSubdiv for simple subdivision, as it leads to a huge amount of computational overhead.

@Sergey Sharybin (sergey) Thanks!

@Eitan Traurig (EitanSomething), do you want to take that on? It means that this node could look more like the MESH_OT_subdivide operator. It would have to convert mesh to BMesh first of course.
I would also be fine with saving that for a future improvement, I don't have a strong opinion.

I don’t have enough time to take it on at the moment

Rename simple_subdivision_surface to subdivision_surface_simple

Harbormaster completed remote builds in B12950: Diff 34095.Feb 17 2021, 3:35 PM

Should I rename node_geo_simple_subdivision_surface.cc to node_geo_subdivision_surface_simple.cc

Eitan Traurig (EitanSomething) marked 2 inline comments as done.Feb 17 2021, 3:36 PM

In D10409#263289, @Eitan Traurig (EitanSomething) wrote:

Should I rename node_geo_simple_subdivision_surface.cc to node_geo_subdivision_surface_simple.cc

Yes, same with the node type define.