Add support for attributes storage in simulation state #107133

Merged
Member

Support all geometry node socket types in the simulation zone nodes. This allows iterating on attributes (or single values) without having to write them into named geometry attributes after each iteration. Each socket type has its own storage buffer, using ValueOrField<> storage for the primitive types.

Field types of the simulation input and output node sockets need to be synchronized because they represent the same pieces of data. This can create a cyclic dependency which complicates the update_field_inferencing function. The simple topological sorting for propagating output field constraints (right-to-left) and input field state (left-to-right) is not sufficient any more, because now updating a simulation input node also modifies the paired output node, and vice versa. To solve this issue, the two propagation functions now can perform several passes, if changing a simulation node modifies their counterpart. Infinite updates are prevented by checking for actual changes: each update can only simplify the field state, until no more changes occur. A single simulation zone can cause at most 1 additional update, more than that may happen when simulation zones are nested.

Resolves #106218

Support all geometry node socket types in the simulation zone nodes. This allows iterating on attributes (or single values) without having to write them into named geometry attributes after each iteration. Each socket type has its own storage buffer, using `ValueOrField<>` storage for the primitive types. Field types of the simulation input and output node sockets need to be synchronized because they represent the same pieces of data. This can create a cyclic dependency which complicates the `update_field_inferencing` function. The simple topological sorting for propagating output field constraints (right-to-left) and input field state (left-to-right) is not sufficient any more, because now updating a simulation input node also modifies the paired output node, and vice versa. To solve this issue, the two propagation functions now can perform several passes, if changing a simulation node modifies their counterpart. Infinite updates are prevented by checking for actual changes: each update can only simplify the field state, until no more changes occur. A single simulation zone can cause at most 1 additional update, more than that may happen when simulation zones are nested. Resolves #106218
Lukas Tönne added 6 commits 2023-04-19 16:15:12 +02:00
Lukas Tönne added this to the Nodes & Physics project 2023-04-19 16:15:26 +02:00
Lukas Tönne requested review from Jacques Lucke 2023-04-19 16:15:42 +02:00
Lukas Tönne requested review from Hans Goudey 2023-04-19 16:15:42 +02:00
Lukas Tönne added 1 commit 2023-04-19 16:27:30 +02:00
Jacques Lucke reviewed 2023-04-19 16:27:46 +02:00
@ -259,0 +269,4 @@
* Afterwards both field states will be the same.
* @return eFieldStateSyncResult flags indicating which field states have changed.
*/
static int sync_field_states(SocketFieldState &a, SocketFieldState &b)
Member

Feels like it would be simpler to just change the values and then compare them to a copy afterwards to detect what has changed.

Feels like it would be simpler to just change the values and then compare them to a copy afterwards to detect what has changed.
Author
Member

You're right, combining states first and then comparing is a lot shorter.

You're right, combining states first and then comparing is a lot shorter.
LukasTonne marked this conversation as resolved
Hans Goudey requested changes 2023-04-19 16:29:42 +02:00
Hans Goudey left a comment
Member

I don't know exactly how it would work, but I get the sense that a large amount of this templating and switch cases shouldn't be necessary if we make more use of the CPPType system.

For example, instead of templating SimulationStateItem, it could get a BUFFER_FOR_CPP_TYPE_VALUE inside of it. The CPPType callbacks could be used to copy/move data into and out of the state. I wouldn't expect any noticeable overhead from doing it that way, since we're processing items one at a time anyway-- it would just replace switch cases with function calls.

The large switch cases could be replaced by something more like:

  1. Get item socket type
  2. get socket CPPType
  3. use CPPType callback to copy/move value
I don't know exactly how it would work, but I get the sense that a large amount of this templating and switch cases shouldn't be necessary if we make more use of the `CPPType` system. For example, instead of templating `SimulationStateItem`, it could get a `BUFFER_FOR_CPP_TYPE_VALUE` inside of it. The `CPPType` callbacks could be used to copy/move data into and out of the state. I wouldn't expect any noticeable overhead from doing it that way, since we're processing items one at a time anyway-- it would just replace switch cases with function calls. The large switch cases could be replaced by something more like: 1. Get item socket type 2. get socket CPPType 3. use CPPType callback to copy/move value
@ -259,0 +267,4 @@
/**
* Compare both field states and select the most compatible.
* Afterwards both field states will be the same.
* @return eFieldStateSyncResult flags indicating which field states have changed.
Member

Blender's doxygen syntax uses \return rather than @return

Blender's doxygen syntax uses `\return` rather than `@return`
@ -259,0 +271,4 @@
*/
static int sync_field_states(SocketFieldState &a, SocketFieldState &b)
{
int res = 0;
Member

Rather than using int, you should be able to use the ENUM_OPERATORS macro to define the necessary operations for the enum type

Rather than using `int`, you should be able to use the `ENUM_OPERATORS` macro to define the necessary operations for the enum type
LukasTonne marked this conversation as resolved
@ -447,0 +603,4 @@
/* Find reverse dependencies and resolve conflicts, which may require another pass. */
if (propagate_special_data_requirements(tree, *node, field_state_by_socket_id))
{
Member

clang format

I highly recommend configuring your editor to format the file when you save, it saves me a lot of time

clang format I highly recommend configuring your editor to format the file when you save, it saves me a lot of time
@ -15,0 +19,4 @@
{
T *data = params.try_get_input_data_ptr_or_request<T>(index);
if (data != nullptr) {
params.set_output(index + 1, std::move(*data));
Member

Might be worth adding a comment explaining the + 1 here, so the reader doesn't have to look elsewhere to figure it out.

Might be worth adding a comment explaining the `+ 1` here, so the reader doesn't have to look elsewhere to figure it out.
@ -15,0 +77,4 @@
int index,
const bke::sim::SimulationStateItem &state_item)
{
if (auto *typed_state_item = dynamic_cast<const bke::sim::TypedSimulationStateItem<T> *>(
Member

auto * -> const auto *

`auto *` -> `const auto *`
@ -15,0 +84,4 @@
}
static void copy_next_simulation_state(lf::Params &params,
int index,
Member

int index -> const int index

`int index` -> `const int index`
LukasTonne marked this conversation as resolved
@ -15,0 +85,4 @@
static void copy_next_simulation_state(lf::Params &params,
int index,
short socket_type,
Member

use eNodeSocketDatatype instead of short

use `eNodeSocketDatatype` instead of `short`
@ -96,14 +136,142 @@ NodeSimulationItem *simulation_item_add_from_socket(NodeGeometrySimulationOutput
const CPPType &get_simulation_item_cpp_type(const NodeSimulationItem &item)
{
switch (item.socket_type) {
case SOCK_FLOAT:
Member

I'd hope this could use the socket type's existing geometry_nodes_cpp_type callback.

I'd hope this could use the socket type's existing `geometry_nodes_cpp_type` callback.
Author
Member

Yeah that works too. I just wanted to keep it simple, considering we have to do a bunch of type switches elsewhere anyway. Have to get the type idname from the eNodeSocketDatatype, then get the bNodeSocketType from the idname, then get the CPPType.

Yeah that works too. I just wanted to keep it simple, considering we have to do a bunch of type switches elsewhere anyway. Have to get the type `idname` from the `eNodeSocketDatatype`, then get the `bNodeSocketType` from the `idname`, then get the CPPType.
Hans Goudey changed title from Add support for attributes storage in simulation state. to Add support for attributes storage in simulation state 2023-04-19 16:34:22 +02:00
Jacques Lucke requested changes 2023-04-19 16:36:55 +02:00
Jacques Lucke left a comment
Member

The overall approach seems reasonable, I still have to look at it in more detail though.

The overall approach seems reasonable, I still have to look at it in more detail though.
@ -15,2 +15,3 @@
class GeometrySimulationStateItem : public SimulationStateItem {
template <typename T>
class TypedSimulationStateItem : public SimulationStateItem {
Member

I think for all the trivial data types, having this templated type can make sense (although I'm not sure if a generic version would be easier). Currently, I don't think that this should be used for GeometrySet. It feels a bit like a wrong abstraction there if it needs to be specialized below.

I think for all the trivial data types, having this templated type can make sense (although I'm not sure if a generic version would be easier). Currently, I don't think that this should be used for `GeometrySet`. It feels a bit like a wrong abstraction there if it needs to be specialized below.
@ -20,2 +24,3 @@
public:
GeometrySimulationStateItem(GeometrySet geometry) : geometry_(std::move(geometry)) {}
TypedSimulationStateItem() = default;
TypedSimulationStateItem(const T &data)
Member

Most of these methods/constructors seem unnecessary. A simple constructor that takes a T value that is then moved into data_ should be good enough. (Calling std::move on a const reference as is currently done here does not make sense anyway)

Most of these methods/constructors seem unnecessary. A simple constructor that takes a `T value` that is then moved into `data_` should be good enough. (Calling `std::move` on a const reference as is currently done here does not make sense anyway)
Author
Member

TBH i just added a bunch of common constructors without considering the details, just to get it working.

TBH i just added a bunch of common constructors without considering the details, just to get it working.
@ -22,0 +35,4 @@
TypedSimulationStateItem(const U &data)
{
data_ = data;
}
Member

If this is really need (and I'm not sure why it is right now), consider adding NonCopyable and NonMovable as base class.

If this is really need (and I'm not sure why it is right now), consider adding `NonCopyable` and `NonMovable` as base class.
@ -259,0 +337,4 @@
{
int res = 0;
for (const int i : output_node.input_sockets().index_range()) {
const bNodeSocket *input_socket = input_node.input_sockets()[i];
Member

Can use input_socket(index) instead of input_sockets()[index]

Can use `input_socket(index)` instead of `input_sockets()[index]`
LukasTonne marked this conversation as resolved
@ -329,0 +471,4 @@
/* Find reverse dependencies and resolve conflicts, which may require another pass. */
if (propagate_special_data_requirements(tree, *node, field_state_by_socket_id))
{
Member

Apply clang format.

Apply clang format.
@ -107,0 +254,4 @@
copy_typed_simulation_state_output<GeometrySet>(params, index, state_item);
break;
case SOCK_COLLECTION:
copy_typed_simulation_state_output<Collection *>(params, index, state_item);
Member

Better don't support these ID types yet. They need special handling that we don't have to get into now. (have to avoid dangling pointers in the cache and stuff like that)

Better don't support these ID types yet. They need special handling that we don't have to get into now. (have to avoid dangling pointers in the cache and stuff like that)
Member

I found one correctness issue. The position input here should have a dot in the socket because it's currently a single value but can be a field.
image

I found one correctness issue. The position input here should have a dot in the socket because it's currently a single value but can be a field. ![image](/attachments/6259641c-0ade-4cf3-a4b0-f667fc77b797)
Member

Also note, we likely won't be able to store fields directly in the simulation state. Instead we'll have to evaluate them on the geometry and store the result as anonymous attributes. This is because we can't really serialize fields to store them on disk when baking.

I'd be fine with only handling the UI part in this patch (so the field inferencing). These sockets can just be ignored during evaluation for now. I think it would be easier to solve that in a separate patch.

Also note, we likely won't be able to store fields directly in the simulation state. Instead we'll have to evaluate them on the geometry and store the result as anonymous attributes. This is because we can't really serialize fields to store them on disk when baking. I'd be fine with only handling the UI part in this patch (so the field inferencing). These sockets can just be ignored during evaluation for now. I think it would be easier to solve that in a separate patch.
Author
Member

I found one correctness issue. The position input here should have a dot in the socket because it's currently a single value but can be a field.

Yeah, the sync_field_states currently assumes that if is_single is false for one of the states it should be false for both (i.e. if one is a field the other is too). I guess this flag does not really need to propagate? The only requirement would be that if is_always_single or requires_single is true then is_single also must be true - is that correct?

> I found one correctness issue. The position input here should have a dot in the socket because it's currently a single value but can be a field. Yeah, the `sync_field_states` currently assumes that if `is_single` is false for one of the states it should be false for both (i.e. if one is a field the other is too). I guess this flag does not really need to propagate? The only requirement would be that if `is_always_single` or `requires_single` is true then `is_single` also must be true - is that correct?
Author
Member

I'd be fine with only handling the UI part in this patch (so the field inferencing). These sockets can just be ignored during evaluation for now. I think it would be easier to solve that in a separate patch.

I'll pull out the data storage into another branch to discuss separately.

> I'd be fine with only handling the UI part in this patch (so the field inferencing). These sockets can just be ignored during evaluation for now. I think it would be easier to solve that in a separate patch. I'll pull out the data storage into another branch to discuss separately.
Member

I guess this flag does not really need to propagate? The only requirement would be that if is_always_single or requires_single is true then is_single also must be true - is that correct?

Sounds correct.
I wonder if is_field_source has to be propagated. Right now it doesn't feel like it should, because a simulation output is never a field source, it only outputs a field when one of the inputs is a field.

> I guess this flag does not really need to propagate? The only requirement would be that if is_always_single or requires_single is true then is_single also must be true - is that correct? Sounds correct. I wonder if `is_field_source` has to be propagated. Right now it doesn't feel like it should, because a simulation output is never a field source, it only outputs a field when one of the inputs is a field.
Author
Member

I wonder if is_field_source has to be propagated

It's not obvious from the state struct what gets propagated where, but if i'm not mistaken:

  • requires_single is propagated R->L to outputs via links and to inputs via internal dependencies.
    Confusingly any implicit inputs also force dependent outputs to become requires_single = false regardless of downstream connections
  • is_single is propagated L->R to inputs via links and to outputs via internal dependencies.
  • is_always_single is not propagated and just reflects InputSocketFieldType::None and OutputSocketFieldType::None
  • is_field_source is not propagated and just reflects OutputSocketFieldType::FieldSource

So the syncing method should probably only affect requires_single and is_single. is_always_single would never change and is only dependent on socket type. is_field_source is always false, as you suggested.

> I wonder if is_field_source has to be propagated It's not obvious from the state struct what gets propagated where, but if i'm not mistaken: - `requires_single` is propagated R->L to outputs via links and to inputs via internal dependencies. Confusingly any implicit inputs also force dependent outputs to become `requires_single = false` regardless of downstream connections - `is_single` is propagated L->R to inputs via links and to outputs via internal dependencies. - `is_always_single` is not propagated and just reflects `InputSocketFieldType::None` and `OutputSocketFieldType::None` - `is_field_source` is not propagated and just reflects `OutputSocketFieldType::FieldSource` So the syncing method should probably only affect `requires_single` and `is_single`. `is_always_single` would never change and is only dependent on socket type. `is_field_source` is always false, as you suggested.
Member

Yes that sounds correct.

Confusingly any implicit inputs also force dependent outputs to become requires_single = false regardless of downstream connections

Technically this can go either way (and we did have it in the other direction at some point in the past). Right propagating field-status L->R takes priority over propagating single-requirements R->L. The effect of this is that the invalid link is the one that connects to the is_always_single socket.

Note how in this image both links could be considered the invalid one. A benefit of the current behavior is that it more closely matches how things are actually evaluated. The output of the Add node is indeed a field that can be used as a field elsewhere, even if it is connected to something that requires a single value.
image

Yes that sounds correct. > Confusingly any implicit inputs also force dependent outputs to become requires_single = false regardless of downstream connections Technically this can go either way (and we did have it in the other direction at some point in the past). Right propagating field-status L->R takes priority over propagating single-requirements R->L. The effect of this is that the invalid link is the one that connects to the `is_always_single` socket. Note how in this image both links could be considered the invalid one. A benefit of the current behavior is that it more closely matches how things are actually evaluated. The output of the Add node is indeed a field that can be used as a field elsewhere, even if it is connected to something that requires a single value. ![image](/attachments/fa6a2fff-1254-4a9a-96d0-2b82776984d5)
Lukas Tönne added 1 commit 2023-04-20 10:49:37 +02:00
60f1e7fe76 Simplified field state syncing.
Only the `requires_single` flag actually needs propagation.
`is_single` flag remains local to either node, depending on whether
a single-value input is connected or not.
`is_always_single` flag is dependent only on the socket type.
`is_field_source` is always false for the simulation zone sockets.
Lukas Tönne added 1 commit 2023-04-20 11:16:28 +02:00
Lukas Tönne added 5 commits 2023-04-20 11:55:38 +02:00
Jacques Lucke reviewed 2023-04-20 11:59:12 +02:00
@ -157,2 +157,4 @@
* single value. */
bool requires_single = false;
bool operator==(const SocketFieldState &other)
Member
Prefer implementing these as functions taking two arguments explicitly. https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c161-use-nonmember-functions-for-symmetric-operators
Author
Member

Don't actually need them any more since i only look at requires_single now, but will keep it in mind for later.

Don't actually need them any more since i only look at `requires_single` now, but will keep it in mind for later.
Lukas Tönne added 1 commit 2023-04-20 12:16:55 +02:00
Lukas Tönne added 1 commit 2023-04-20 12:28:54 +02:00
Lukas Tönne added 2 commits 2023-04-20 14:37:46 +02:00
Author
Member

I reverted the changes to SimulationStateItem and the templated copy functions. There are dummy functions now that simply output the default value based on the CPPType. This can be implemented in a separate patch.

I reverted the changes to `SimulationStateItem` and the templated copy functions. There are dummy functions now that simply output the default value based on the CPPType. This can be implemented in a separate patch.
Lukas Tönne added 1 commit 2023-04-20 14:52:48 +02:00
Lukas Tönne added 1 commit 2023-04-20 15:39:53 +02:00
Lukas Tönne added 1 commit 2023-04-20 15:46:23 +02:00
Member

Now this socket seems to be wrong. It should be a field. Currently it would be possible to connect this socket to e.g. a cube node without any visible warning in the ui.
image

Now this socket seems to be wrong. It should be a field. Currently it would be possible to connect this socket to e.g. a cube node without any visible warning in the ui. ![image](/attachments/ae183887-4df1-4159-9f13-71f803b0aacb)
Member

Am I missing something or is this now breaking simulations completely? I'd expect that the existing files don't just break with this patch.

Am I missing something or is this now breaking simulations completely? I'd expect that the existing files don't just break with this patch.
Author
Member

Yeah i removed all the copying of state item data in the process of removing the templated types. Could add that back for the geometry i suppose.

Yeah i removed all the copying of state item data in the process of removing the templated types. Could add that back for the geometry i suppose.
Lukas Tönne added 1 commit 2023-04-20 17:48:12 +02:00
Author
Member

Am I missing something or is this now breaking simulations completely?

Should be fixed, now geometry sockets should be working again. Other data types just use a dummy state item that doesn't currently store any data.

> Am I missing something or is this now breaking simulations completely? Should be fixed, now geometry sockets should be working again. Other data types just use a dummy state item that doesn't currently store any data.
Lukas Tönne added 1 commit 2023-04-20 18:04:03 +02:00
Author
Member

Now this socket seems to be wrong. It should be a field. Currently it would be possible to connect this socket to e.g. a cube node without any visible warning in the ui.
image

Ok so my understanding was wrong that is_single would not need propagating. If one of the two zone input sockets has a field connected it becomes non-single, which then needs to propagate to the paired node input, and then schedule another iteration.

Should be ok now.

> Now this socket seems to be wrong. It should be a field. Currently it would be possible to connect this socket to e.g. a cube node without any visible warning in the ui. > ![image](/attachments/ae183887-4df1-4159-9f13-71f803b0aacb) Ok so my understanding was wrong that `is_single` would not need propagating. If one of the two zone input sockets has a field connected it becomes non-single, which then needs to propagate to the paired node input, and then schedule another iteration. Should be ok now.
Member

image

Now we are back to this situation.. To be clear, in this setup I expect the Position input on the Simulation Input node to have a dot in the socket. All the other position sockets should just be fields.

> ![image](/attachments/6259641c-0ade-4cf3-a4b0-f667fc77b797) Now we are back to this situation.. To be clear, in this setup I expect the Position input on the Simulation Input node to have a dot in the socket. All the other position sockets should just be fields.
Lukas Tönne added 1 commit 2023-04-21 10:10:49 +02:00
Author
Member

Sorry, it's kinda hard to figure out the correct behavior here. I think i've fixed it now by syncing only the output field states. This should work, because the requires_single flag on inputs is then propagated in the normal way in the following pass, while is_single for the inputs is only determined by connections of the socket.

Connecting a field to inputs keeps the dot in the other node:




Connecting a single value to inputs or a field to outputs keeps it a single value with field option (does this dot thing have a name?):




Single value on outputs restricts to single values everywhere:


Additional field connection then causes errors as expected:


Sorry, it's kinda hard to figure out the correct behavior here. I think i've fixed it now by syncing _only_ the output field states. This should work, because the `requires_single` flag on inputs is then propagated in the normal way in the following pass, while `is_single` for the inputs is only determined by connections of the socket. Connecting a field to inputs keeps the dot in the other node: <a target="_blank" href="/attachments/6ea1af8c-fb32-4421-a748-68f7da5cd5cb"> <img src="/attachments/6ea1af8c-fb32-4421-a748-68f7da5cd5cb" width="50%"> </a> <a target="_blank" href="/attachments/a325f526-4d1f-421d-8eb1-d46ecf99eb81"> <img src="/attachments/a325f526-4d1f-421d-8eb1-d46ecf99eb81" width="50%"> </a> Connecting a single value to inputs or a field to outputs keeps it a single value with field option (does this dot thing have a name?): <a target="_blank" href="/attachments/3e5737cb-bdf0-4cd7-a69c-a0efaafb6011"> <img src="/attachments/3e5737cb-bdf0-4cd7-a69c-a0efaafb6011" width="50%"> </a> <a target="_blank" href="/attachments/9da35048-b7fc-4a96-a6c1-877d3331cf36"> <img src="/attachments/9da35048-b7fc-4a96-a6c1-877d3331cf36" width="50%"> </a> Single value on outputs restricts to single values everywhere: <a target="_blank" href="/attachments/cae240f0-26e7-4b29-bfe8-b0c5f14f9e07"> <img src="/attachments/cae240f0-26e7-4b29-bfe8-b0c5f14f9e07" width="50%"> </a> Additional field connection then causes errors as expected: <a target="_blank" href="/attachments/35e35687-94fe-42c9-bc58-5a31d38315cc"> <img src="/attachments/35e35687-94fe-42c9-bc58-5a31d38315cc" width="50%"> </a>
Jacques Lucke approved these changes 2023-04-21 11:00:28 +02:00
Jacques Lucke left a comment
Member

Behavior of the field inferencing looks good to me now, thanks. Only got some minor code style comments now.

Behavior of the field inferencing looks good to me now, thanks. Only got some minor code style comments now.
@ -257,2 +257,4 @@
}
/** Result of syncing two field states. */
enum eFieldStateSyncResult {
Member

enum class FieldStateSyncResult

`enum class FieldStateSyncResult`
@ -259,0 +275,4 @@
const bool requires_single = a.requires_single || b.requires_single;
const bool is_single = a.is_single && b.is_single;
eFieldStateSyncResult res = static_cast<eFieldStateSyncResult>(0);
Member

Use functional style cast for enums: FieldStateSyncResult(0). Might be even better to simply add a None item to the enum.

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast

Use functional style cast for enums: `FieldStateSyncResult(0)`. Might be even better to simply add a `None` item to the enum. https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast
@ -259,0 +327,4 @@
const NodeGeometrySimulationInput &data = *static_cast<NodeGeometrySimulationInput *>(
node.storage);
if (const bNode *output_node = tree.node_by_id(data.output_node_id)) {
eFieldStateSyncResult sync_result = simulation_nodes_field_state_sync(
Member

const, same below

`const`, same below
@ -266,2 +361,2 @@
for (const bNode *node : toposort_result) {
const FieldInferencingInterface &inferencing_interface = *interface_by_node[node->index()];
while (true) {
/* Node updates may require sevaral passes due to cyclic dependencies. */
Member

[...] caused by simulation input/output nodes.

`[...] caused by simulation input/output nodes.`
@ -15,0 +44,4 @@
params.set_output(index + 1, src);
}
else {
// if (const void *src = state_item.data()) {
Member

remove dead code

remove dead code
LukasTonne marked this conversation as resolved
@ -82,0 +122,4 @@
copy_next_simulation_state(
params,
i,
static_cast<eNodeSocketDatatype>(simulation_items_[i].socket_type),
Member

functional style cast for enums

functional style cast for enums
@ -66,3 +109,3 @@
const bNodeSocket &socket)
{
if (socket.type != SOCK_GEOMETRY) {
if (!is_simulation_item_type_supported(static_cast<eNodeSocketDatatype>(socket.type))) {
Member

functional style cast, few more cases below

functional style cast, few more cases below
LukasTonne marked this conversation as resolved
Lukas Tönne added 3 commits 2023-04-21 12:21:06 +02:00
Lukas Tönne requested review from Hans Goudey 2023-04-21 12:21:18 +02:00
Hans Goudey requested changes 2023-04-21 21:50:40 +02:00
Hans Goudey left a comment
Member

Could you mark which inline comments have been resolved? Currently that doesn't work in the "conversations" view, but it does work in the "files changed" tab.

EDIT: Actually, trying on my own PR I see this might not be possible. Oh well. This is close I think!

Could you mark which inline comments have been resolved? Currently that doesn't work in the "conversations" view, but it does work in the "files changed" tab. EDIT: Actually, trying on my own PR I see this might not be possible. Oh well. This is close I think!
@ -259,0 +331,4 @@
if (const bNode *output_node = tree.node_by_id(data.output_node_id)) {
const eFieldStateSyncResult sync_result = simulation_nodes_field_state_sync(
node, *output_node, field_state_by_socket_id);
if ((bool)(sync_result & eFieldStateSyncResult::CHANGED_B)) {
Member

Functional cast for bool: (bool)... -> bool(...)

Though I often see implicit conversions used for cases like this, which seems fine IMO:
if (sync_result & eFieldStateSyncResult::CHANGED_B) {

Functional cast for `bool`: `(bool)...` -> `bool(...)` Though I often see implicit conversions used for cases like this, which seems fine IMO: `if (sync_result & eFieldStateSyncResult::CHANGED_B) {`
Author
Member

Implicit cast to bool does not work with enum class unfortunately (suggested by Jacques above). Functional cast should be fine though.

Implicit cast to bool does not work with `enum class` unfortunately (suggested by Jacques above). Functional cast should be fine though.
LukasTonne marked this conversation as resolved
@ -15,0 +29,4 @@
}
}
static void copy_next_simulation_state(lf::Params &params,
Member

Maybe I'm being stupid, but I don't get what's meant by "next" here.

Maybe I'm being stupid, but I don't get what's meant by "next" here.
Author
Member

Was just an ad-hoc name: it's copying from the simulation state into the output parameter for the next iteration. I'll add some comments.

Was just an ad-hoc name: it's copying from the simulation state into the output parameter for the next iteration. I'll add some comments.
Author
Member

Renamed the functions and added comments for clarity. Also the copy_simulation_state_to_output_param function is now shared by input and output node, they both need to do this: input node when preparing the next iteration, output node when outputting the sim result.

Renamed the functions and added comments for clarity. Also the `copy_simulation_state_to_output_param` function is now shared by input and output node, they both need to do this: input node when preparing the next iteration, output node when outputting the sim result.
LukasTonne marked this conversation as resolved
@ -15,0 +36,4 @@
{
const CPPType &cpptype = get_simulation_item_cpp_type(socket_type);
/* TODO */
Member

Might as well include what has to be done in the TODO comment

Might as well include what has to be done in the TODO comment
LukasTonne marked this conversation as resolved
Lukas Tönne added 2 commits 2023-04-24 11:02:59 +02:00
Lukas Tönne added 1 commit 2023-04-24 11:05:16 +02:00
Lukas Tönne requested review from Hans Goudey 2023-04-24 11:08:31 +02:00
Hans Goudey approved these changes 2023-04-24 14:14:34 +02:00
Lukas Tönne added 2 commits 2023-04-24 17:55:00 +02:00
Lukas Tönne merged commit c13005fed9 into geometry-nodes-simulation 2023-04-24 18:22:22 +02:00
Lukas Tönne deleted branch geometry-nodes-simulation-attribute-support 2023-04-24 18:22:24 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107133
No description provided.