Add support for attributes storage in simulation state #107133
No reviewers
Labels
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 project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#107133
Loading…
Reference in New Issue
No description provided.
Delete Branch "LukasTonne/blender:geometry-nodes-simulation-attribute-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
@ -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)
Feels like it would be simpler to just change the values and then compare them to a copy afterwards to detect what has changed.
You're right, combining states first and then comparing is a lot shorter.
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 aBUFFER_FOR_CPP_TYPE_VALUE
inside of it. TheCPPType
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:
@ -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.
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;
Rather than using
int
, you should be able to use theENUM_OPERATORS
macro to define the necessary operations for the enum type@ -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))
{
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));
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> *>(
auto *
->const auto *
@ -15,0 +84,4 @@
}
static void copy_next_simulation_state(lf::Params ¶ms,
int index,
int index
->const int index
@ -15,0 +85,4 @@
static void copy_next_simulation_state(lf::Params ¶ms,
int index,
short socket_type,
use
eNodeSocketDatatype
instead ofshort
@ -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:
I'd hope this could use the socket type's existing
geometry_nodes_cpp_type
callback.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 theeNodeSocketDatatype
, then get thebNodeSocketType
from theidname
, then get the CPPType.Add support for attributes storage in simulation state.to Add support for attributes storage in simulation stateThe 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 {
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)
Most of these methods/constructors seem unnecessary. A simple constructor that takes a
T value
that is then moved intodata_
should be good enough. (Callingstd::move
on a const reference as is currently done here does not make sense anyway)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;
}
If this is really need (and I'm not sure why it is right now), consider adding
NonCopyable
andNonMovable
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];
Can use
input_socket(index)
instead ofinput_sockets()[index]
@ -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))
{
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);
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)
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.
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.
Yeah, the
sync_field_states
currently assumes that ifis_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 ifis_always_single
orrequires_single
is true thenis_single
also must be true - is that correct?I'll pull out the data storage into another branch to discuss separately.
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.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 connectionsis_single
is propagated L->R to inputs via links and to outputs via internal dependencies.is_always_single
is not propagated and just reflectsInputSocketFieldType::None
andOutputSocketFieldType::None
is_field_source
is not propagated and just reflectsOutputSocketFieldType::FieldSource
So the syncing method should probably only affect
requires_single
andis_single
.is_always_single
would never change and is only dependent on socket type.is_field_source
is always false, as you suggested.Yes that sounds correct.
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.
@ -157,2 +157,4 @@
* single value. */
bool requires_single = false;
bool operator==(const SocketFieldState &other)
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
Don't actually need them any more since i only look at
requires_single
now, but will keep it in mind for later.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.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.
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.
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.
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.
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 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.
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, whileis_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:
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 {
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);
Use functional style cast for enums:
FieldStateSyncResult(0)
. Might be even better to simply add aNone
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(
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. */
[...] caused by simulation input/output nodes.
@ -15,0 +44,4 @@
params.set_output(index + 1, src);
}
else {
// if (const void *src = state_item.data()) {
remove dead code
@ -82,0 +122,4 @@
copy_next_simulation_state(
params,
i,
static_cast<eNodeSocketDatatype>(simulation_items_[i].socket_type),
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))) {
functional style cast, few more cases below
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)) {
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) {
Implicit cast to bool does not work with
enum class
unfortunately (suggested by Jacques above). Functional cast should be fine though.@ -15,0 +29,4 @@
}
}
static void copy_next_simulation_state(lf::Params ¶ms,
Maybe I'm being stupid, but I don't get what's meant by "next" here.
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.
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.@ -15,0 +36,4 @@
{
const CPPType &cpptype = get_simulation_item_cpp_type(socket_type);
/* TODO */
Might as well include what has to be done in the TODO comment