Page MenuHome
Differential D9364

Geometry Nodes: Add subdivision surface node
ClosedPublic
Actions

Authored by Léo Depoix (PiloeGAO) on Oct 27 2020, 6:15 PM.
Tags
Subscribers
Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)
Tokens
"Love" token, awarded by eversimo."Love" token, awarded by Robonnet."Love" token, awarded by Raimund58."Burninate" token, awarded by TheAngerSpecialist."Love" token, awarded by hadrien.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)
Summary

This include a new subdivision surface node to geometry nodes project.
The code is normally as fast as the subdivision surface modifier.

The new code is based on the work of @Jacques Lucke (JacquesLucke), @Hans Goudey (HooglyBoogly) and the subdivision surface modifier source code.

The subsurf node is available in the mesh category from "Add" menu in the Geometry Node Editor.

Example:

Diff Detail

Repository
rB Blender

Event Timeline

Léo Depoix (PiloeGAO) requested review of this revision.Oct 27 2020, 6:15 PM
Léo Depoix (PiloeGAO) created this revision.
Hadrien Brissaud (hadrien) awarded a token.Oct 27 2020, 6:57 PM
Hans Goudey (HooglyBoogly) requested changes to this revision.Oct 28 2020, 4:40 AM
Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.
Hans Goudey (HooglyBoogly) added reviewers: Hans Goudey (HooglyBoogly), Jacques Lucke (JacquesLucke).

Thanks for the patch! It will be nice to have subdivision as a node.

I got a crash when testing this on a plane and a cube. Although I did get it to work once. I wonder if you can reproduce it. I didn't look into why the crash happens but here's a stack trace:

blender::opensubdiv::CpuEvalOutputAPI::evaluateLimit(blender::opensubdiv::CpuEvalOutputAPI * const this, const int ptex_face_index, float face_u, float face_v, float * P, float * dPdu, float * dPdv) (/home/hans/Documents/Blender-Git/blender/intern/opensubdiv/internal/evaluator/evaluator_impl.cc:695)
(anonymous namespace)::evaluateLimit(OpenSubdiv_Evaluator * evaluator, const int ptex_face_index, const float face_u, const float face_v, float * P, float * dPdu, float * dPdv) (/home/hans/Documents/Blender-Git/blender/intern/opensubdiv/internal/evaluator/evaluator_capi.cc:101)
BKE_subdiv_eval_limit_point_and_derivatives(Subdiv * subdiv, const int ptex_face_index, const float u, const float v, float * r_P, float * r_dPdu, float * r_dPdv) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_eval.c:185)
subdiv_accumulate_vertex_normal_and_displacement(SubdivMeshContext * ctx, const int ptex_face_index, const float u, const float v, MVert * subdiv_vert) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_mesh.c:487)
subdiv_mesh_vertex_every_corner_or_edge(const SubdivForeachContext * foreach_context, void * UNUSED_tls, const int ptex_face_index, const float u, const float v, const int subdiv_vertex_index) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_mesh.c:646)
subdiv_mesh_vertex_every_corner(const SubdivForeachContext * foreach_context, void * tls, const int ptex_face_index, const float u, const float v, const int UNUSED_coarse_vertex_index, const int UNUSED_coarse_poly_index, const int UNUSED_coarse_corner, const int subdiv_vertex_index) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_mesh.c:659)
subdiv_foreach_corner_vertices_regular_do(SubdivForeachTaskContext * ctx, void * tls, const MPoly * coarse_poly, SubdivForeachVertexFromCornerCb vertex_corner, _Bool check_usage) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_foreach.c:333)
subdiv_foreach_every_corner_vertices_regular(SubdivForeachTaskContext * ctx, void * tls, const MPoly * coarse_poly) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_foreach.c:408)
subdiv_foreach_every_corner_vertices(SubdivForeachTaskContext * ctx, void * tls) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_foreach.c:430)
subdiv_foreach_single_thread_tasks(SubdivForeachTaskContext * ctx) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_foreach.c:1816)
BKE_subdiv_foreach_subdiv_geometry(Subdiv * subdiv, const SubdivForeachContext * context, const SubdivToMeshSettings * mesh_settings, const Mesh * coarse_mesh) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_foreach.c:1886)
BKE_subdiv_to_mesh(Subdiv * subdiv, const SubdivToMeshSettings * settings, const Mesh * coarse_mesh) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/subdiv_mesh.c:1229)
blender::nodes::geo_subsurf_exec(bNode * UNUSED_node, blender::nodes::GValueByName & inputs, blender::nodes::GValueByName & outputs) (/home/hans/Documents/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_subsurf.cc:88)
GeometryNodesEvaluator::execute_node(GeometryNodesEvaluator * const this, const blender::nodes::DNode & node, blender::nodes::GValueByName & node_inputs, blender::nodes::GValueByName & node_outputs) (/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_nodes.cc:242)
GeometryNodesEvaluator::compute_output_and_forward(GeometryNodesEvaluator * const this, const blender::nodes::DOutputSocket & socket_to_compute) (/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_nodes.cc:227)
GeometryNodesEvaluator::get_input_value(GeometryNodesEvaluator * const this, const blender::nodes::DInputSocket & socket_to_compute) (/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_nodes.cc:208)
GeometryNodesEvaluator::execute(GeometryNodesEvaluator * const this) (/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_nodes.cc:166)
compute_geometry(const blender::nodes::DerivedNodeTree & tree, blender::Span<blender::nodes::DOutputSocket const*> group_input_sockets, const blender::nodes::DInputSocket & socket_to_compute, blender::bke::GeometryPtr input_geometry, NodesModifierData * nmd) (/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_nodes.cc:501)
modifyMesh(ModifierData * md, const ModifierEvalContext * UNUSED_ctx, Mesh * mesh) (/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_nodes.cc:548)
BKE_modifier_modify_mesh(ModifierData * md, const ModifierEvalContext * ctx, struct Mesh * me) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/modifier.c:1004)
mesh_calc_modifiers(struct Depsgraph * depsgraph, Scene * scene, Object * ob, int useDeform, const _Bool need_mapping, const CustomData_MeshMasks * dataMask, const int index, const _Bool use_cache, const _Bool allow_shared_mesh, Mesh ** r_deform, Mesh ** r_final) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/DerivedMesh.c:1183)
mesh_build_data(struct Depsgraph * depsgraph, Scene * scene, Object * ob, const CustomData_MeshMasks * dataMask, const _Bool need_mapping) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/DerivedMesh.c:1788)
makeDerivedMesh(struct Depsgraph * depsgraph, Scene * scene, Object * ob, BMEditMesh * em, const CustomData_MeshMasks * dataMask) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/DerivedMesh.c:1925)
BKE_object_handle_data_update(Depsgraph * depsgraph, Scene * scene, Object * ob) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/object_update.c:192)
BKE_object_eval_uber_data(Depsgraph * depsgraph, Scene * scene, Object * ob) (/home/hans/Documents/Blender-Git/blender/source/blender/blenkernel/intern/object_update.c:384)
std::__invoke_impl<void, void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(void (*&)(Depsgraph *, Scene *, Object *) __f) (/usr/include/c++/10/bits/invoke.h:60)
std::__invoke<void (*&)(Depsgraph*, Scene*, Object*), Depsgraph*, Scene*&, Object*&>(void (*&)(Depsgraph *, Scene *, Object *) __fn) (/usr/include/c++/10/bits/invoke.h:95)
std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::__call<void, Depsgraph*&&, 0ul, 1ul, 2ul>(std::tuple<Depsgraph*&&>&&, std::_Index_tuple<0ul, 1ul, 2ul>)(std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> * const this, std::tuple<Depsgraph*&&> && __args) (/usr/include/c++/10/functional:416)
std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>::operator()<Depsgraph*, void>(Depsgraph*&&)(std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> * const this) (/usr/include/c++/10/functional:499)
std::__invoke_impl<void, std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>&, Depsgraph*>(std::__invoke_other, std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>&, Depsgraph*&&)(std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> & __f) (/usr/include/c++/10/bits/invoke.h:60)
std::__invoke_r<void, std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>&, Depsgraph*>(std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)>&, Depsgraph*&&)(std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> & __fn) (/usr/include/c++/10/bits/invoke.h:110)
std::_Function_handler<void (Depsgraph*), std::_Bind<void (*(std::_Placeholder<1>, Scene*, Object*))(Depsgraph*, Scene*, Object*)> >::_M_invoke(std::_Any_data const&, Depsgraph*&&)(const std::_Any_data & __functor,  __args#0) (/usr/include/c++/10/bits/std_function.h:291)
std::function<void (Depsgraph*)>::operator()(Depsgraph*) const(const std::function<void(Depsgraph*)> * const this,  __args#0) (/usr/include/c++/10/bits/std_function.h:622)
blender::deg::(anonymous namespace)::evaluate_node(const blender::deg::(anonymous namespace)::DepsgraphEvalState * state, blender::deg::OperationNode * operation_node) (/home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:114)
blender::deg::(anonymous namespace)::deg_task_run_func(TaskPool * pool, void * taskdata) (/home/hans/Documents/Blender-Git/blender/source/blender/depsgraph/intern/eval/deg_eval.cc:125)
Task::operator()() const::{lambda()#1}::operator()() const(const struct {...} * const __closure) (/home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:118)
tbb::interface7::internal::delegated_function<Task::operator()() const::{lambda()#1} const, void>::operator()() const(const tbb::interface7::internal::delegated_function<const Task::operator()() const::<lambda()>, void> * const this) (/home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:93)
tbb::interface7::internal::isolate_within_arena(tbb::interface7::internal::delegate_base&, long) (Unknown Source:0)
tbb::interface7::internal::isolate_impl<void, Task::operator()() const::{lambda()#1} const>(Task::operator()() const::{lambda()#1} const&)(const struct {...} & f) (/home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:160)
tbb::interface7::this_task_arena::isolate<Task::operator()() const::{lambda()#1}>(tbb::interface7::internal::return_type_or_void const&)(const struct {...} & f) (/home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task_arena.h:395)
Task::operator()(const Task * const this) (/home/hans/Documents/Blender-Git/blender/source/blender/blenlib/intern/task_pool.cc:118)
tbb::internal::function_task<Task>::execute(tbb::internal::function_task<Task> * const this) (/home/hans/Documents/Blender-Git/lib/linux_centos7_x86_64/tbb/include/tbb/task.h:1048)
tbb::internal::custom_scheduler<tbb::internal::IntelSchedulerTraits>::process_bypass_loop(tbb::internal::context_guard_helper<false>&, tbb::task*, long) (Unknown Source:0)
tbb::internal::custom_scheduler<tbb::internal::IntelSchedulerTraits>::local_wait_for_all(tbb::task&, tbb::task*) (Unknown Source:0)
tbb::internal::arena::process(tbb::internal::generic_scheduler&) (Unknown Source:0)
tbb::internal::market::process(rml::job&) (Unknown Source:0)
tbb::internal::rml::private_worker::run() (Unknown Source:0)
tbb::internal::rml::private_worker::thread_routine(void*) (Unknown Source:0)
libpthread.so.0!start_thread(void * arg) (/usr/src/debug/glibc-2.31-48-g64246fccaf/nptl/pthread_create.c:477)
libc.so.6!clone() (/usr/src/debug/glibc-2.31-48-g64246fccaf/sysdeps/unix/sysv/linux/x86_64/clone.S:95)
source/blender/blenkernel/BKE_node.h
1349

This is a faithful copy of the modifier code, but I would prefer using the full "SUBDIVIVISION" term here, just for consistency with the name that shows in the UI.

Same everywhere else "subsurf" is used in this patch.

source/blender/nodes/geometry/nodes/node_geo_subsurf.cc
41 ↗(On Diff #30427)

If you get rid of the trailing commas here clang format will place the whole socket on one line, which looks better in my opinion.

41 ↗(On Diff #30427)

The "Is" part is implied by the checkbox, no need to have it here.

48–49 ↗(On Diff #30427)

I'm not sure we should exposing display related settings to the node. I'd be curious to here other's opinions though.

70 ↗(On Diff #30427)

In general I prefer returning early for checks like this, like in the boolean node for example. Sorry a bit of the existing code isn't that consistent with this desire yet.

77–78 ↗(On Diff #30427)

Since you're only using the subdiv field, why not just use Subdiv subdiv_start and pass &subdiv_start to BKE_subdiv_update_from_mesh?

That would allow removing the typedef for the runtime struct at the top, which shouldn't be necessary anyway.

This revision now requires changes to proceed.Oct 28 2020, 4:40 AM
Hans Goudey (HooglyBoogly) added inline comments.Oct 28 2020, 4:41 AM
source/blender/blenkernel/BKE_node.h
1349

lol, "SUBDIVISION", not "SUBDIVIVISION"

Léo Depoix (PiloeGAO) updated this revision to Diff 30471.Oct 28 2020, 6:56 PM
Léo Depoix (PiloeGAO) marked 7 inline comments as done.

This new diff fix the problem for non-closed meshes (like planes, suzanne eye's...) and all requests from @Hans Goudey (HooglyBoogly) comments.

This also add inputs for "Boundary Smooth" and "Smooth UVs" (options available in the subdivision surface modifier).
I haven't touch to the "Optimal Display" option (just moved between "Simple" and "Use Creases"), it can be useful for viewport clarity.

Example:

Léo Depoix (PiloeGAO) updated this revision to Diff 30508.Oct 29 2020, 6:41 PM

I updated my code with latest commits.

This diff cleaned up some codes from yesterday's diff, It also introduce the WITH_OPENSUBDIV directive.
Now, if you compile the source code with Opensubdiv deactivated, the subdivision node only let you use geometry as input.

Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 29 2020, 7:04 PM

Thanks for working on this. I know that the node api is quite a moving target currently, that makes it harder.

source/blender/nodes/geometry/nodes/node_geo_subdivision.cc
27

The sockets shouldn't be removed when opensubdiv is disabled. This cause data loss.
Better just display a warning in the node ui.

109

Comments should start with a captial letter and end with a ..
Also see https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments.

121

Instead of creating a new geometry with a new mesh, this should make the incoming geometry mutable and then replace the mesh. This way other data in the geometry is not lost (e.g. when the geometry also contains volume data in the future).
I updated some other nodes similarly today.

This revision now requires changes to proceed.Oct 29 2020, 7:04 PM
Léo Depoix (PiloeGAO) updated this revision to Diff 30534.Oct 30 2020, 11:36 AM
Léo Depoix (PiloeGAO) marked 3 inline comments as done.

I made changes from @Jacques Lucke (JacquesLucke) review.

Now when you build without OpenSubdiv, the subdivision node show you a warning instead of removing inputs.

Example:

Jacques Lucke (JacquesLucke) added a comment.Oct 30 2020, 12:59 PM

It's almost done. I actually wanted to commit it already with some minor changes. However, there is a memory leak that seems quite easy to fix but maybe I'm missing something.

source/blender/blenkernel/BKE_node.h
1349

Actually, I think this should be called "subdivision surface" everywhere, instead of just "subdivision".

source/blender/editors/space_node/drawnode.c
3150 ↗(On Diff #30534)

Can use #ifndef.
Also make sure to avoid warnings due to unused variables.

static void node_geometry_buts_subdivision_surface(uiLayout *layout,
                                                   bContext *UNUSED(C),
                                                   PointerRNA *UNUSED(ptr))
{
#ifndef WITH_OPENSUBDIV
  uiItemL(layout, IFACE_("Disabled, built without OpenSubdiv"), ICON_ERROR);
#else
  UNUSED_VARS(layout);
#endif
}
source/blender/nodes/geometry/nodes/node_geo_subdivision.cc
27

Set the default level to 1. That improves the ux in my opinion.

63

Move this line down to right before the ->replace_mesh call.

65

Use geometry->get_mesh_for_read() because this mesh is not modified.

77

Looks like you should also introduce an upper level for the subdiv level like 30 or so.

99

Are you sure this line is needed? It makes the if (subdiv != subdiv_start) check fail every time. Also, I get memory leaks when this line this line exists.

Léo Depoix (PiloeGAO) updated this revision to Diff 30555.Oct 30 2020, 4:35 PM
Léo Depoix (PiloeGAO) marked 6 inline comments as done.

This new diff follows @Jacques Lucke (JacquesLucke) advice's.

I spend some times in memory leaks inspection and It should be okay now.
I also made some tests and researches for the upper level but I found nothing other that hacks so I haven't touch it.

I remain at your disposal for any advices.

Thank you

Jacques Lucke (JacquesLucke) added a comment.Oct 30 2020, 5:01 PM

Testing this, I found this weird issue:

I can't explain what is causing this, yet.

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc
57 ↗(On Diff #30555)

const int subdiv_level = clamp_i(inputs.extract<int>("Level"), 0, 30);

80 ↗(On Diff #30555)

This does not seem to be necessary, when you check for the level above.

Jacques Lucke (JacquesLucke) added inline comments.Oct 30 2020, 5:07 PM
source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc
72 ↗(On Diff #30555)

Not sure if this is related to the issue mentioned above.
Maybe one should use BKE_subdiv_new_from_mesh instead of MEM_callocN.

Léo Depoix (PiloeGAO) marked 2 inline comments as done.Oct 30 2020, 5:24 PM

I'm not able to reproduce the issue.
Maybe it may be cause by a bool I haven't set (is_adaptive from SubdivSettings) but not sure about that. I fix it and send a new diff.

source/blender/nodes/geometry/nodes/node_geo_subdivision.cc
77

I made some researches based on the subdivision surface modifier and I found nothing for that.
We can hardcode a check on node execution to avoid higher levels but I'm not sure that will be a good option.

99

After some tests, I found that line isn't needed. I tried checking memory leaks after change and the issue may be fixed.

Léo Depoix (PiloeGAO) updated this revision to Diff 30561.Oct 30 2020, 5:37 PM

This should fix the weird issue mentioned by @Jacques Lucke (JacquesLucke).
I also made changes from latest review.

Jacques Lucke (JacquesLucke) added a comment.Oct 30 2020, 5:49 PM

That seems to fix it thanks.

I just started wondering whether subdiv_start is necessary at all. Looking at what BKE_subdiv_update_from_mesh does, it seems like you could just pass in nullptr.
Then the MEM_callocN I was mentioning earlier is not necessary anymore.

Léo Depoix (PiloeGAO) updated this revision to Diff 30566.EditedOct 30 2020, 6:24 PM

Huge thanks for taking your time and pointing me all this optimisations.
Now the code should be more clean and optimised.

Léo Depoix (PiloeGAO) marked an inline comment as done.Oct 30 2020, 6:26 PM

Changes were made in latest published diff.

Hans Goudey (HooglyBoogly) accepted this revision.Oct 30 2020, 9:29 PM

This looks good to me! I still find it a bit weird to have "Optimal Display" in the node, but we can always revisit that later as part of a larger conversation about how to apply some similar settings to the evaluated geometry. For now it could be useful.

Jacques Lucke (JacquesLucke) accepted this revision.Oct 31 2020, 10:51 AM
This revision is now accepted and ready to land.Oct 31 2020, 10:51 AM
Jacques Lucke (JacquesLucke) closed this revision.Oct 31 2020, 10:56 AM

Committed in https://developer.blender.org/rBf8c52590c0f928a31d005942508b446358057794.

Thanks for the contribution!

Joseph Brandenburg (TheAngerSpecialist) awarded a token.Nov 1 2020, 3:25 PM
Raimund Klink (Raimund58) awarded a token.Nov 10 2020, 9:47 AM
Robert Windefjord (Robonnet) awarded a token.Nov 11 2020, 11:19 PM
Dspazio (Dspazio) awarded a token.Nov 12 2020, 11:54 PM
Dspazio (Dspazio) rescinded a token.
Everton Schneider (eversimo) awarded a token.Nov 13 2020, 3:33 AM

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
1 line
source/
blender/
blenkernel/
1 line
intern/
1 line
nodes/
1 line
1 line
1 line
geometry/
nodes/
132 lines

Diff 30471

View Options

release/scripts/startup/nodeitems_builtins.py

Loading...
View Options

source/blender/blenkernel/BKE_node.h

Loading...
View Options

source/blender/blenkernel/intern/node.c

Loading...
View Options

source/blender/nodes/CMakeLists.txt

Loading...
View Options

source/blender/nodes/NOD_geometry.h

Loading...
View Options

source/blender/nodes/NOD_static_types.h

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_subdivision.cc

Loading...