Geometry Nodes: add Skip input to Simulation Output node #112140

Jacques Lucke · 2023-09-08T13:56:24+02:00

Jacques Lucke commented

2023-09-08 13:56:24 +02:00

This implements #112082.

The implementation adds this new functionality directly to the LazyFunction of the Simulation Output node. As shown in the graph below, the lazy function has the new Skip input as well as an additional set of inputs for the simulation state that is linked directly to the Simulation Input node.

This implements #112082. The implementation adds this new functionality directly to the `LazyFunction` of the Simulation Output node. As shown in the graph below, the lazy function has the new Skip input as well as an additional set of inputs for the simulation state that is linked directly to the Simulation Input node. ![image](/attachments/d57ee5e7-7978-4684-8659-741e139154a3) ![image](/attachments/d7a702c0-f5a6-4cfb-bf36-ad2be2869072)

image.png

38 KiB

image.png

18 KiB

image.png

92 KiB

Jacques Lucke added 6 commits 2023-09-08 13:56:37 +02:00

565795b064 add pass through socket

f5a3ce610c link up pass through sockets

89d2e98827 actually use pass through socket

588264ed8c improve naming

79c55f2019 fix

b1b1224234 fix

Jacques Lucke added 1 commit 2023-09-08 14:07:51 +02:00

ef97e1eb55 improve description

Jacques Lucke added 1 commit 2023-09-08 14:08:33 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

8c4bedca5b fix

Jacques Lucke commented

2023-09-08 14:09:02 +02:00

@blender-bot build

Jacques Lucke requested review from Lukas Tönne 2023-09-08 14:09:25 +02:00

Jacques Lucke requested review from Simon Thommes 2023-09-08 14:09:26 +02:00

Jacques Lucke requested review from Hans Goudey 2023-09-08 14:09:26 +02:00

Hans Goudey reviewed 2023-09-08 14:14:50 +02:00

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

						
				@ -688,2 +722,3 @@

				  std::optional<bke::bake::BakeState> get_bake_state_from_inputs(lf::Params &params) const

				  std::optional<bke::bake::BakeState> get_bake_state_from_inputs(

				      lf::Params &params, const bool force_pass_through) const

Hans Goudey commented

2023-09-08 14:13:14 +02:00

I find this force_pass_through input confusing. Maybe it would be unnecessary if the functions were split up a bit? That way we wouldn't have the "boolean input significantly changes how a function works" situation here.

I find this `force_pass_through` input confusing. Maybe it would be unnecessary if the functions were split up a bit? That way we wouldn't have the "boolean input significantly changes how a function works" situation here.

JacquesLucke marked this conversation as resolved

Simon Thommes approved these changes 2023-09-08 14:36:30 +02:00

Simon Thommes left a comment

I gave this a few test runs and it seems to work great. I'd still have this signed off by @dfelinto especially because of the naming. But for me this is all good to go from the user perspective 👍

Dalai Felinto commented

2023-09-08 16:31:43 +02:00

For the records (and the posterity), I think "Pass Through" may lead people to think that the entire Simulation Zone is by passed. My preference in that regard would be to name it "Skip" instead.

We are deciding this elsewhere though, and if nothing gets added here it means we sticked to "Pass Through".

For the records (and the posterity), I think "Pass Through" may lead people to think that the entire Simulation Zone is by passed. My preference in that regard would be to name it "Skip" instead. We are deciding this elsewhere though, and if nothing gets added here it means we sticked to "Pass Through".

Jacques Lucke added 2 commits 2023-09-08 16:51:29 +02:00

3c87a27ffd Merge branch 'main' into sim-muted

89b2ac7922 cleanup

Dalai Felinto commented

2023-09-08 16:55:40 +02:00

Let's go with Skip, it seems preferable among the module members.

Jacques Lucke added 1 commit 2023-09-08 17:00:49 +02:00

c09d62fd53 rename Pass Through to Skip

Jacques Lucke requested review from Dalai Felinto 2023-09-08 17:01:01 +02:00

Jacques Lucke changed title from ~~Geometry Nodes: add Pass Through input to Simulation Output node~~ to Geometry Nodes: add Skip input to Simulation Output node

2023-09-08 17:01:52 +02:00

Hans Goudey reviewed 2023-09-08 17:06:59 +02:00

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

						
				@ -517,2 +529,4 @@

				  const bNode &node_;

				  Span<NodeSimulationItem> simulation_items_;

				  int skip_input_index_;

				  int skip_inputs_offset_;

Hans Goudey commented

2023-09-08 17:06:56 +02:00

I think there needs to be some place where there's a comment about these extra links. As is, these three variable names are the best description of the design, which isn't really satisfying IMO. Looking back on the PR description it's obvious, but it took me a minute to figure out the purpose of these variables.

Maybe add a function to get these and return them as IndexRange or even Array<void *>? That would give a place to say something like "Retrieve the inputs connected directly to the simulation input node for when the "skip" is used"

I think there needs to be some place where there's a comment about these extra links. As is, these three variable names are the best description of the design, which isn't really satisfying IMO. Looking back on the PR description it's obvious, but it took me a minute to figure out the purpose of these variables. Maybe add a function to get these and return them as `IndexRange` or even `Array<void *>`? That would give a place to say something like "Retrieve the inputs connected directly to the simulation input node for when the "skip" is used"

Jacques Lucke added 1 commit 2023-09-08 17:51:30 +02:00

d94ab8e76e add comments

Hans Goudey reviewed 2023-09-08 17:57:21 +02:00

source/blender/nodes/geometry/nodes/node_geo_simulation_output.cc

						
				@ -516,6 +528,18 @@ static void mix_simulation_state(const NodeSimulationItem &item,

				class LazyFunctionForSimulationOutputNode final : public LazyFunction {

				  const bNode &node_;

				  Span<NodeSimulationItem> simulation_items_;

				  int skip_input_index_;

Hans Goudey commented

2023-09-08 17:57:21 +02:00

Sorry to get stuck on this, but I don't really understand the reasoning for storing these-- can't they be determined at runtime?

skip_input_index_ = 0
skip_inputs_offset_ = 1
solve_inputs_offset_ = simulation_items_.size() + 1

If that's right, I think it's a bit misleading to store them here rather than compute them on the fly.

Sorry to get stuck on this, but I don't really understand the reasoning for storing these-- can't they be determined at runtime? `skip_input_index_ = 0` `skip_inputs_offset_ = 1` `solve_inputs_offset_ = simulation_items_.size() + 1` If that's right, I think it's a bit misleading to store them here rather than compute them on the fly.

Jacques Lucke commented

2023-09-08 18:01:28 +02:00

The reason I store them is that this makes the code a little less brittle. It allows storing the index at the same place where it is "created". Without this, it is much easier to mess up indices when moving/adding/removing sockets.

Hans Goudey commented

2023-09-08 18:05:35 +02:00

I think asserts are another way to do that, but I don't care enough to continue this topic :P

Jacques Lucke commented

2023-09-08 18:10:20 +02:00

I don't quite see how asserts should solve this. Here it's relatively simple still, because there are only three blocks of sockets. Computing the indices on the fly becomes harder and more error prone the more such blocks there are. If this would have a measurable impact on performance, I might choose differently, but it doesn't.

Hans Goudey approved these changes 2023-09-08 18:05:43 +02:00

Dalai Felinto reviewed 2023-09-08 18:06:47 +02:00

Dalai Felinto left a comment

Approving as far as the "naming" is concerned, so I don't show as blocking here :) I haven't tested it though

Marek Smoliński commented

2023-09-08 18:19:01 +02:00

First-time contributor

IMHO:

"Pass Through" isn't very clear. It could mean, for example, the starting geometry (connected to the "Simulation Input") is returned from the "Simulation Output" (pass through the entire zone).

"Skip" seems nicer, but why not make it more explicit? "Skip This Frame"…

Alternatively: "Store [This Frame]" [or "Save[…]"], as I think that's the key feature here, limiting the cache size; otherwise a simple switch would do. This brings another issue: could the simulation still evaluate some data, output it, and yet avoid storing (saving) it to the cache? This would also play nicely with a possible future redesign, allowing to read an arbitrary previously evaluated frame, rather than just the last one…

IMHO: _"Pass Through"_ isn't very clear. It could mean, for example, the starting geometry (connected to the _"Simulation Input"_) is returned from the _"Simulation Output"_ (pass through the entire zone). _"Skip"_ seems nicer, but why not make it more explicit? _"Skip This Frame"_… Alternatively: _"Store [This Frame]"_ [or _"Save[…]"_], as I think that's the key feature here, limiting the cache size; otherwise a simple switch would do. This brings another issue: could the simulation still evaluate some data, output it, and yet avoid storing (saving) it to the cache? This would also play nicely with a possible future redesign, allowing to read an arbitrary previously evaluated frame, rather than just the last one…

Jacques Lucke commented

2023-09-08 19:33:17 +02:00

This is not a feature to limit cache size primarily. As mentioned in the design task (#112082), this is mainly a usability feature to avoid having to setup these switch nodes manually. If the data doesn't change, the cache should be deduplicated automatically to decrease memory usage.

Simulating some frames (or subframes) but not storing them when baking is an independent topic that we are working on as well.

This is not a feature to limit cache size primarily. As mentioned in the design task (#112082), this is mainly a usability feature to avoid having to setup these switch nodes manually. If the data doesn't change, the cache should be deduplicated automatically to decrease memory usage. Simulating some frames (or subframes) but not storing them when baking is an independent topic that we are working on as well.

👍 1

Jacques Lucke merged commit dd515ebc1d into main

2023-09-09 09:53:08 +02:00

Jacques Lucke referenced this issue from a commit

2023-09-09 09:53:10 +02:00

Geometry Nodes: add Skip input to Simulation Output node

Jacques Lucke deleted branch sim-muted

2023-09-09 09:53:10 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Geometry Nodes: add Skip input to Simulation Output node #112140