Geometry Nodes: use lazy-function directly for switch node #105696

Jacques Lucke · 2023-03-12T18:40:31+01:00

Jacques Lucke commented

2023-03-12 18:40:31 +01:00

This changes the Switch node so that it is implemented directly as a lazy-function, instead of as a normal geometry node which uses GeoNodeExecParams. This improves the design of the layered execution api, where different nodes can be implemented at a proper different abstraction level. The simplest kinds of nodes are implemented as multi-function, then there is GeoNodeExecParams and more specialized nodes are implemented as lazy-function. The switch node is special in the sense that it currently needs extra behavior in the lazy-function graph generation anyway.

GeoNodeExecParams can be simplified as well, because the Switch node was the only one that used the lazy_ methods.

This changes the Switch node so that it is implemented directly as a lazy-function, instead of as a normal geometry node which uses `GeoNodeExecParams`. This improves the design of the layered execution api, where different nodes can be implemented at a proper different abstraction level. The simplest kinds of nodes are implemented as multi-function, then there is `GeoNodeExecParams` and more specialized nodes are implemented as lazy-function. The switch node is special in the sense that it currently needs extra behavior in the lazy-function graph generation anyway. `GeoNodeExecParams` can be simplified as well, because the Switch node was the only one that used the `lazy_` methods.

Jacques Lucke added 5 commits 2023-03-12 18:40:48 +01:00

ad826277f0 replace switch node implementation

739da48325 remove dead code

72e741c3b8 cleanup

ea4143cd92 cleanup

ba2a12dc0e fix

Jacques Lucke referenced this pull request

2023-03-12 18:46:07 +01:00

WIP: Functions: process multiple elements at once in the lazy-function api #105697

Jacques Lucke requested review from Lukas Tönne 2023-03-12 18:46:24 +01:00

Jacques Lucke requested review from Hans Goudey 2023-03-12 18:46:24 +01:00

Iliya Katushenock commented

2023-03-12 19:17:33 +01:00

This PR removes the ability to make ALL other nodes lazy, according to some design, partially described in the RP description: #105264.
All nodes can be lazy and this would be very useful as part of the desire for geometry nodes to be much more scalable through assets. This would imply the need to reduce the overhead on the abstraction of node groups, not all of the results of which can always be used.

This PR removes the ability to make ALL other nodes lazy, according to some design, partially described in the RP description: https://projects.blender.org/blender/blender/pulls/105264. All nodes can be lazy and this would be very useful as part of the desire for geometry nodes to be much more scalable through assets. This would imply the need to reduce the overhead on the abstraction of node groups, not all of the results of which can always be used.

Jacques Lucke commented

2023-03-12 21:36:25 +01:00

It does not remove the ability to add lazy behavior to nodes, it does change how it would be done though. It might be ok to add a different API for this back at some point, but I don't like the current approach. There shouldn't be a need for a per node type boolean to indicate that it supports lazyness. Maybe that can somehow be integrated with node declarations.

Hans Goudey approved these changes 2023-03-13 14:44:11 +01:00

Hans Goudey left a comment

Looks good to me. The node is easier to understand now, and much less annoying boilerplate. I share a bit of the concern about how these lazy features can be implemented in most other nodes now, but the big picture you described seems fine.

source/blender/nodes/NOD_geometry_nodes_lazy_function.hh

						
				@ -221,6 +221,10 @@ class GeometryNodesLazyFunctionLogger : public fn::lazy_function::GraphExecutor:

				                               const lf::Context &context) const override;

				};

				namespace node_geo_switch_cc {

Hans Goudey commented

2023-03-13 14:35:24 +01:00

I'm not sure how I feel about these unity build namespaces leaking to separate files. Maybe the public function from that file should be in the blender::nodes namespace like other cases. Or blender::nodes::geometry? (though that could be done elsewhere too)

I'm not sure how I feel about these unity build namespaces leaking to separate files. Maybe the public function from that file should be in the `blender::nodes` namespace like other cases. Or `blender::nodes::geometry`? (though that could be done elsewhere too)

source/blender/nodes/geometry/nodes/node_geo_switch.cc Outdated

						
				@ -208,3 +185,1 @@

				    return;

				  }

				  bool switch_value = params.get_input<bool>("Switch_001");

				  static const int false_input_index = 1;

Hans Goudey commented

2023-03-13 14:37:38 +01:00

static constexpr?

`static constexpr`?

source/blender/nodes/geometry/nodes/node_geo_switch.cc Outdated

						
				@ -219,0 +193,4 @@

				    params.set_input_unused(input_to_ignore);

				    void *value_to_forward = params.try_get_input_data_ptr_or_request(input_to_forward);

				    if (value_to_forward == nullptr) {

				      /* True again when the value is available. */

Hans Goudey commented

2023-03-13 14:38:57 +01:00

True again -> Try again?

`True again` -> `Try again`?

source/blender/nodes/geometry/nodes/node_geo_switch.cc Outdated

						
				@ -287,3 +225,1 @@

				    default:

				      BLI_assert_unreachable();

				      break;

				    GField output_field{std::make_shared<FieldOperation>(

Hans Goudey commented

2023-03-13 14:40:30 +01:00

How about this?

    GField output_field{FieldOperation::Create(
        switch_multi_function,
        {std::move(condition), std::move(false_field), std::move(true_field)})};

How about this? ```c++ GField output_field{FieldOperation::Create( switch_multi_function, {std::move(condition), std::move(false_field), std::move(true_field)})}; ```

Lukas Tönne approved these changes 2023-03-14 10:30:22 +01:00

Lukas Tönne left a comment

Looks good, i have nothing substantial to add. A better API for lazy functions that doesn't rely on logic in GeometryNodesLazyFunctionGraphBuilder would be nice, but can be a separate task.

Looks good, i have nothing substantial to add. A better API for lazy functions that doesn't rely on logic in `GeometryNodesLazyFunctionGraphBuilder` would be nice, but can be a separate task.

Jacques Lucke added 4 commits 2023-03-14 14:04:22 +01:00

1fe934f980 Merge branch 'main' into switch-node-separate-api

b7ebfdc451 don't use file namespace outside of file

22305ea695 use constexpr

15dc530bc4 cleanup

Jacques Lucke merged commit 258678916f into main

2023-03-14 14:09:39 +01:00

Jacques Lucke deleted branch switch-node-separate-api

2023-03-14 14:09:39 +01:00

Jacques Lucke referenced this issue from a commit

2023-03-14 14:09:40 +01:00

Geometry Nodes: use lazy-function directly for switch node