Simulation nodes: UI for simulation state items in sidebar #106919

Merged
Member

Selecting the input or output simulation node now shows a new panel in the node sidebar with a list of simulation state items. Items can be added, removed, or moved up and down the list. This updates the sockets on both simulation input and output nodes. This is similar to node group interfaces, except that simulation state items only have one list which generates both input and output sockets.

Resolves #105723

image

Selecting the input or output simulation node now shows a new panel in the node sidebar with a list of simulation state items. Items can be added, removed, or moved up and down the list. This updates the sockets on both simulation input and output nodes. This is similar to node group interfaces, except that simulation state items only have one list which generates both input and output sockets. Resolves #105723 ![image](/attachments/204be0ce-2e29-4205-af73-0fd71fb91ef9)
Lukas Tönne added 13 commits 2023-04-13 21:17:25 +02:00
Lukas Tönne added this to the Nodes & Physics project 2023-04-13 21:17:40 +02:00
Lukas Tönne changed title from Resolve #105723: UI for simulation state items in sidebar to WIP Resolve #105723: UI for simulation state items in sidebar 2023-04-13 21:20:01 +02:00
Lukas Tönne reviewed 2023-04-13 21:26:59 +02:00
@ -572,6 +572,15 @@ class NodeTreeMainUpdater {
return true;
}
}
/* Check paired simulation zone nodes. */
Author
Member

Connecting the __extend__ socket updates both input and output node, but only because of the blanket NTREE_CHANGED_LINK handling which updates all nodes in the tree when a link is changed.

Property changes of the output node don't update the paired input node on their own, so i had to add this special case check. Any better suggestions would be welcome.

Connecting the `__extend__` socket updates both input and output node, but only because of the blanket `NTREE_CHANGED_LINK` handling which updates _all nodes in the tree_ when a link is changed. Property changes of the output node don't update the paired input node on their own, so i had to add this special case check. Any better suggestions would be welcome.
LukasTonne marked this conversation as resolved
Lukas Tönne reviewed 2023-04-13 21:29:58 +02:00
@ -39,6 +39,13 @@ using namespace blender;
using blender::fn::ValueOrField;
using blender::nodes::SocketDeclarationPtr;
extern "C" void ED_node_type_draw_color(const char *idname, float *r_color);
Author
Member

Exposing socket type colors through ED_node_type_draw_color adds another bad level call. Since the same file already has other UI functions used this way it doesn't make things any worse at least.

Exposing socket type colors through `ED_node_type_draw_color` adds another bad level call. Since the same file already has other UI functions used this way it doesn't make things any worse at least.
LukasTonne marked this conversation as resolved
Lukas Tönne reviewed 2023-04-13 21:32:10 +02:00
@ -55,2 +59,2 @@
if (STREQ(item.name, name)) {
return true;
for (const NodeSimulationItem &item : Span(args.sim->items, args.sim->items_num)) {
if (&item != args.item) {
Author
Member

This function is now also used to check for uniqueness of items that are already in the list. This check avoids detecting duplicates from the same socket.

This function is now also used to check for uniqueness of items that are _already_ in the list. This check avoids detecting duplicates from the same socket.
Lukas Tönne reviewed 2023-04-13 21:34:18 +02:00
@ -27,3 +27,2 @@
std::unique_ptr<decl::Geometry> decl = std::make_unique<decl::Geometry>();
decl->name = item.name;
decl->identifier = item.name;
decl->name = item.name ? item.name : "";
Author
Member

Assigning nullptr to a std::string is undefined behavior. We also ensure the item.name is never empty now, but it's better not to rely on that alone and check the char * as well.

Assigning nullptr to a `std::string` is undefined behavior. We also ensure the `item.name` is never empty now, but it's better not to rely on that alone and check the `char *` as well.
Hans Goudey changed title from WIP Resolve #105723: UI for simulation state items in sidebar to WIP: Simulation nodes: UI for simulation state items in sidebar 2023-04-14 04:01:50 +02:00
Lukas Tönne added 1 commit 2023-04-14 10:14:40 +02:00
Lukas Tönne added 2 commits 2023-04-14 12:39:14 +02:00
e920bbede4 Generate unique identifiers for simulation state items.
This helps ensure that socket links get remapped when other properties
like name or type change. The identifier is immutable and should remain
unique over the entire lifetime of the simulation node.
Lukas Tönne added 2 commits 2023-04-14 13:58:26 +02:00
Lukas Tönne changed title from WIP: Simulation nodes: UI for simulation state items in sidebar to Simulation nodes: UI for simulation state items in sidebar 2023-04-14 14:03:52 +02:00
Lukas Tönne requested review from Jacques Lucke 2023-04-14 14:04:04 +02:00
Lukas Tönne requested review from Hans Goudey 2023-04-14 14:04:11 +02:00
Member

I wonder if we could put the list in the existing Node sidebar tab instead of adding a new one. That might be a more consistent/expected place for it. What do you think about that?

I wonder if we could put the list in the existing `Node` sidebar tab instead of adding a new one. That might be a more consistent/expected place for it. What do you think about that?
Author
Member

I wonder if we could put the list in the existing Node sidebar tab instead of adding a new one. That might be a more consistent/expected place for it. What do you think about that?

Would be a simple matter of changing the bl_category = "Simulation" of the panel. No strong opinion on this.

> I wonder if we could put the list in the existing `Node` sidebar tab instead of adding a new one. That might be a more consistent/expected place for it. What do you think about that? Would be a simple matter of changing the `bl_category = "Simulation"` of the panel. No strong opinion on this.
Lukas Tönne reviewed 2023-04-15 15:02:53 +02:00
@ -195,0 +224,4 @@
bl_label = "Add State Item"
bl_options = {'REGISTER', 'UNDO'}
default_socket_type = 'GEOMETRY'
Author
Member

Could support a type selection enum here, with a dropdown menu invoke.

Could support a type selection enum here, with a dropdown menu invoke.
Lukas Tönne reviewed 2023-04-15 15:03:19 +02:00
@ -195,0 +258,4 @@
class NODE_OT_simulation_zone_item_move(SimulationZoneOperator, Operator):
'''Move a simulation state item up or down in list'''
Author
Member

"in the list"

"in **the** list"
LukasTonne marked this conversation as resolved
Hans Goudey requested changes 2023-04-18 22:11:47 +02:00
Hans Goudey left a comment
Member
  • This works pretty well in general, it's nice to have control over this.
  • I do think this should be in the "Node" sidebar tab rather than a new one
  • I'd propose changing the panel title from "State Items" to "Simulation State"
  • Adding a "color" property to SimulationStateItem really doesn't feel like the right way to draw them in the UI. That information should probably only be accessible from the socket types. Could you retrieve the color from the socket type instead in Python?
  • Do you imagine the new socket_type_items enum items array being different from the existing socket items? If not, we could reuse the item array with a filter function.
  • I get this error in the terminal (I might have just resolved the merge conflict wrong though).
Traceback (most recent call last):
  File "/home/hans/blender-git/build/bin/3.6/scripts/startup/bl_ui/space_node.py", line 1014, in draw
    props.direction = 'UP'
AttributeError: 'NODE_OT_simulation_zone_item_move' object has no attribute 'direction'
- This works pretty well in general, it's nice to have control over this. - I do think this should be in the "Node" sidebar tab rather than a new one - I'd propose changing the panel title from "State Items" to "Simulation State" - Adding a "color" property to `SimulationStateItem` really doesn't feel like the right way to draw them in the UI. That information should probably only be accessible from the socket types. Could you retrieve the color from the socket type instead in Python? - Do you imagine the new `socket_type_items` enum items array being different from the existing socket items? If not, we could reuse the item array with a filter function. - I get this error in the terminal (I might have just resolved the merge conflict wrong though). ``` Traceback (most recent call last): File "/home/hans/blender-git/build/bin/3.6/scripts/startup/bl_ui/space_node.py", line 1014, in draw props.direction = 'UP' AttributeError: 'NODE_OT_simulation_zone_item_move' object has no attribute 'direction' ```
@ -28,2 +30,2 @@
decl->name = item.name;
decl->identifier = item.name;
decl->name = item.name ? item.name : "";
decl->identifier = buf;
Member

How about decl->identifier = "Item_" + std::to_string(item.identifier);?

How about `decl->identifier = "Item_" + std::to_string(item.identifier);`?
Author
Member

Hah, i always forget how this number-to-string stuff works in C++, somehow find it easier to remember old BLI.

Hah, i always forget how this number-to-string stuff works in C++, somehow find it easier to remember old BLI.
LukasTonne marked this conversation as resolved
@ -55,2 +61,2 @@
if (STREQ(item.name, name)) {
return true;
for (const NodeSimulationItem &item : Span(args.sim->items, args.sim->items_num)) {
if (&item != args.item) {
Member

This can be checked outside of the loop with Span::contains_ptr()

This can be checked outside of the loop with `Span::contains_ptr()`
Author
Member

I'm not sure what you mean here.
I'm checking the iterator here to avoid comparing it to itself, to prevent fake name collision. Otherwise having an existing item "XY" and entering the name "XY" would turn it into "XY.001".

I'm not sure what you mean here. I'm checking the iterator here to avoid comparing it to itself, to prevent fake name collision. Otherwise having an existing item "XY" and entering the name "XY" would turn it into "XY.001".
Author
Member

Also note i've added C++ spans to the NodeGeometrySimulationOutput now to make this a bit nicer.

Also note i've added C++ spans to the `NodeGeometrySimulationOutput` now to make this a bit nicer.
Member

Ah okay, didn't get the context fully here. Thanks.

Ah okay, didn't get the context fully here. Thanks.
HooglyBoogly marked this conversation as resolved
@ -336,2 +322,4 @@
nodeRegisterType(&ntype);
}
bNode *NOD_geometry_simulation_output_find_node_by_data(bNodeTree *ntree,
Member

NOD_geometry_simulation_output_find_node_by_data is unused, and it doesn't seem like we should need this. If we do, I'd keep it local to RNA.

`NOD_geometry_simulation_output_find_node_by_data` is unused, and it doesn't seem like we should need this. If we do, I'd keep it local to RNA.
Author
Member

Downside of putting this in RNA is that we don't have access to the C++ functions like ntree->nodes_by_type there.

Downside of putting this in RNA is that we don't have access to the C++ functions like `ntree->nodes_by_type` there.
Author
Member

Removed the unused function though.

Removed the unused function though.
@ -338,0 +336,4 @@
bNode *NOD_geometry_simulation_output_find_node_by_item(bNodeTree *ntree,
const NodeSimulationItem *item)
{
for (bNode *node : ntree->nodes_by_type("GeometryNodeSimulationOutput")) {
Member

These functions should call ensure_topology_cache() before they use it.

These functions should call `ensure_topology_cache()` before they use it.
@ -338,0 +367,4 @@
bool NOD_geometry_simulation_output_contains_item(NodeGeometrySimulationOutput *sim,
const NodeSimulationItem *item)
{
const int index = item - sim->items;
Member

Implement with Span::contains_ptr()

Implement with `Span::contains_ptr()`
@ -338,0 +374,4 @@
NodeSimulationItem *NOD_geometry_simulation_output_get_active_item(
NodeGeometrySimulationOutput *sim)
{
if (sim->active_index >= 0 && sim->active_index < sim->items_num) {
Member

A consistency suggestion: switch to return early in failure case, which is more common, and use index_range.contains():

if (!IndexRange(sim->items_num).contains(sim->active_index)) {
  return nullptr;
}
return &sim->items[sim->active_index];
A consistency suggestion: switch to return early in failure case, which is more common, and use `index_range.contains()`: ``` if (!IndexRange(sim->items_num).contains(sim->active_index)) { return nullptr; } return &sim->items[sim->active_index]; ```
@ -338,0 +392,4 @@
NodeSimulationItem *NOD_geometry_simulation_output_find_item(NodeGeometrySimulationOutput *sim,
const char *name)
{
for (const int i : blender::IndexRange(sim->items_num)) {
Member
for (NodeSimulationItem &item : blender::Span(sim->items, sim->items_num)) {
    if (STREQ(item.name, name)) {
      return &item;
    }
}
``` for (NodeSimulationItem &item : blender::Span(sim->items, sim->items_num)) { if (STREQ(item.name, name)) { return &item; } } ```
@ -338,0 +414,4 @@
{
NodeSimulationItem *old_items = sim->items;
sim->items = MEM_cnew_array<NodeSimulationItem>(sim->items_num + 1, __func__);
for (const int i : blender::IndexRange(0, index)) {
Member

IndexRange(0, index) -> IndexRange(index)

`IndexRange(0, index)` -> `IndexRange(index)`
Lukas Tönne added 1 commit 2023-04-20 15:52:08 +02:00
Lukas Tönne added 3 commits 2023-04-20 16:28:56 +02:00
Lukas Tönne added 1 commit 2023-04-20 16:31:56 +02:00
Author
Member

Adding a "color" property to SimulationStateItem really doesn't feel like the right way to draw them in the UI. That information should probably only be accessible from the socket types. Could you retrieve the color from the socket type instead in Python?

Socket types are messy and complicated: the color there is a registerable property, which requires it to be an instance property and can't be a static method. Adding a property to a type can't be done with RNA, to my knowledge, it would have to be a custom-built python module, which seems overkill for this purpose.

We'd also need to have a direct way to map a SimulationStateItem to a socket class first, something like a socket_idname method that returns a string and then do a bpy.types lookup. I would rather keep the RNA simple and do this internally.

> Adding a "color" property to SimulationStateItem really doesn't feel like the right way to draw them in the UI. That information should probably only be accessible from the socket types. Could you retrieve the color from the socket type instead in Python? Socket types are messy and complicated: the color there is a registerable property, which requires it to be an instance property and can't be a static method. Adding a property to a type can't be done with RNA, to my knowledge, it would have to be a custom-built python module, which seems overkill for this purpose. We'd also need to have a direct way to map a `SimulationStateItem` to a socket class first, something like a `socket_idname` method that returns a string and then do a `bpy.types` lookup. I would rather keep the RNA simple and do this internally.
Lukas Tönne added 2 commits 2023-04-20 17:07:24 +02:00
Lukas Tönne added 1 commit 2023-04-20 18:16:30 +02:00
Lukas Tönne added 4 commits 2023-04-21 11:56:27 +02:00
Lukas Tönne added 1 commit 2023-04-21 12:00:52 +02:00
Lukas Tönne requested review from Hans Goudey 2023-04-21 12:02:27 +02:00
Member

Seems generally fine, will leave the final review to hans.

Seems generally fine, will leave the final review to hans.
Jacques Lucke refused to review 2023-04-21 16:43:56 +02:00
Hans Goudey requested changes 2023-04-21 21:39:44 +02:00
Hans Goudey left a comment
Member

For the color thing, I think I still prefer clarity/simplicity in the API and pushing the complexity to an RNA helper function. Just seems like SimulationStateItem should not be at all concerned with its color in the UI, that's the job of the node system socket types in general. But it's not a huge deal, and if you really prefer it that way that's okay.

The rest of my comments are just small things.

For the color thing, I think I still prefer clarity/simplicity in the API and pushing the complexity to an RNA helper function. Just seems like `SimulationStateItem` should not be at all concerned with its color in the UI, that's the job of the node system socket types in general. But it's not a huge deal, and if you really prefer it that way that's okay. The rest of my comments are just small things.
@ -192,6 +193,97 @@ class NODE_OT_add_simulation_zone(NodeAddOperator, Operator):
return {'FINISHED'}
class SimulationZoneOperator():
Member

What do you think about adding these operators to geometry_nodes.py instead of node.py? They seem specific to geometry nodes.

What do you think about adding these operators to `geometry_nodes.py` instead of `node.py`? They seem specific to geometry nodes.
Author
Member

Didn't realize we had a dedicated operator py file for geonodes, will move it there.

Didn't realize we had a dedicated operator py file for geonodes, will move it there.
@ -195,0 +208,4 @@
@classmethod
def poll(cls, context):
space = context.space_data
# needs active node editor and a tree to add nodes to
Member

Comment style

Comment style
@ -195,0 +233,4 @@
# Remember index to move the item
dst_index = min(node.active_index + 1, len(state_items))
# Empty name so it is based on the type only
Member

Comment style: periods at the end of comments

Comment style: periods at the end of comments
@ -9790,0 +9973,4 @@
RNA_def_struct_ui_text(srna, "State Items", "Collection of simulation state items");
func = RNA_def_function(srna, "new", "rna_NodeGeometrySimulationOutput_items_new");
RNA_def_function_ui_description(func, "Add a state item to this simulation zone");
Member

state item -> item for UI text. "State" is mostly redundant for users in this context.

`state item` -> `item` for UI text. "State" is mostly redundant for users in this context.
@ -26,0 +36,4 @@
bool NOD_geometry_simulation_output_item_socket_type_supported(eNodeSocketDatatype socket_type);
/** Set a unique item name.
* @return True if the unique name differs from the original name.
Member

Blender uses syntax like \return rather than @return. Also for multi-line function docstring comments, the /** should be on its own line at the start.

Blender uses syntax like `\return` rather than `@return`. Also for multi-line function docstring comments, the `/**` should be on its own line at the start.
@ -338,0 +324,4 @@
return blender::Span<NodeSimulationItem>(items, items_num);
}
blender::MutableSpan<NodeSimulationItem> NodeGeometrySimulationOutput::items_mutable_span()
Member

I guess this is slightly different, but usually a method like this is called items_for_write(). Either way is fine with me.

I guess this is slightly different, but usually a method like this is called `items_for_write()`. Either way is fine with me.
Author
Member

We already have NodeSimulationItem *items for the array, so Span<NodeSimulationItem> items() is a conflict, otherwise items + items_for_write would work.

We already have `NodeSimulationItem *items` for the array, so `Span<NodeSimulationItem> items()` is a conflict, otherwise `items` + `items_for_write` would work.
Author
Member

I opted for items_span_for_write now, if that's ok for you.

I opted for `items_span_for_write` now, if that's ok for you.
@ -338,0 +501,4 @@
if (from_index == to_index) {
return;
}
else if (from_index < to_index) {
Member

else after return

else after return
Lukas Tönne added 1 commit 2023-04-24 11:14:37 +02:00
Author
Member

For the color thing, I think I still prefer clarity/simplicity in the API and pushing the complexity to an RNA helper function.

Problem is the std_node_socket_colors list is currently part of the editor code, so it requires this hacky bad-level call to access. I wouldn't mind getting rid of that, but that's out of scope here.

> For the color thing, I think I still prefer clarity/simplicity in the API and pushing the complexity to an RNA helper function. Problem is the `std_node_socket_colors` list is currently part of the editor code, so it requires this hacky bad-level call to access. I wouldn't mind getting rid of that, but that's out of scope here.
Lukas Tönne added 7 commits 2023-04-24 11:48:36 +02:00
Lukas Tönne added 1 commit 2023-04-24 12:06:23 +02:00
Lukas Tönne requested review from Hans Goudey 2023-04-24 12:08:58 +02:00
Hans Goudey approved these changes 2023-04-24 14:15:48 +02:00
Lukas Tönne merged commit 8446b2cad1 into geometry-nodes-simulation 2023-04-24 14:26:28 +02:00
Lukas Tönne deleted branch geometry-nodes-simulation-item-ui 2023-04-24 14:26:30 +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#106919
No description provided.