Skip to content

[BrushGraph 5/7] Add complex node fields#64

Open
maxmmitchell wants to merge 3 commits intobrush-graph/4-nodesfrom
brush-graph/5-complex-fields
Open

[BrushGraph 5/7] Add complex node fields#64
maxmmitchell wants to merge 3 commits intobrush-graph/4-nodesfrom
brush-graph/5-complex-fields

Conversation

@maxmmitchell
Copy link
Copy Markdown

@maxmmitchell maxmmitchell commented Apr 27, 2026

Description

This is the fifth PR in the Brush Graph stack. It completes the node field editors by adding the more complex ones.

Details

  • Complex UI models for viewing and editing the fields of the remaining node types.
  • Top-level NodeFields

Dependencies

  • Target: brush-graph/4-nodes

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements UI fields for various brush graph nodes, including behavior, color, family, and texture layers. Feedback highlights missing unit conversions for angle ranges in polar target nodes and performance concerns regarding allocations within the response curve's draw loop. Other suggestions include removing unused variables, consolidating duplicated logic for source types, and ensuring developer comments are consistently available across all behavior node types.

Comment on lines +121 to +122
if (nodeCase == ProtoBrushBehavior.Node.NodeCase.TARGET_NODE ||
nodeCase == ProtoBrushBehavior.Node.NodeCase.POLAR_TARGET_NODE) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The developerComment field is currently only displayed for TARGET_NODE and POLAR_TARGET_NODE. Since developerComment is a common property of the NodeData.Behavior class, it should likely be available for all behavior node types to ensure consistency and allow users to document any node in the graph.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

developerComment exists at the Behavior level in proto, not at the node level. Therefore it only makes sense to have one comment per behavior, so the other nodes have no need for comments, it would just be duplicated and changing it would need to update all the nodes -- no good. This does present an interesting challenge for terminal nodes which take multiple inputs, thereby representing multiple behaviors -- perhaps they should surface multiple comment fields, one per input. Though each input could technically represent several behaviors, so this too is brittle. Needs more thought on how to best display.

@maxmmitchell maxmmitchell force-pushed the brush-graph/5-complex-fields branch from 618cc67 to d78bbc2 Compare April 27, 2026 13:23
@maxmmitchell maxmmitchell force-pushed the brush-graph/4-nodes branch 2 times, most recently from 4b2402e to 6f34b89 Compare April 28, 2026 06:38
@maxmmitchell maxmmitchell force-pushed the brush-graph/5-complex-fields branch from d78bbc2 to 7d9bfce Compare April 28, 2026 06:59
@maxmmitchell maxmmitchell marked this pull request as ready for review April 28, 2026 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant