Skip to content

[BrushGraph 4/7] Add Node UI core and simple fields#63

Open
maxmmitchell wants to merge 6 commits intobrush-graph/3-viewmodelfrom
brush-graph/4-nodes
Open

[BrushGraph 4/7] Add Node UI core and simple fields#63
maxmmitchell wants to merge 6 commits intobrush-graph/3-viewmodelfrom
brush-graph/4-nodes

Conversation

@maxmmitchell
Copy link
Copy Markdown
Contributor

@maxmmitchell maxmmitchell commented Apr 27, 2026

Description

This is the fourth PR in the Brush Graph stack. It introduces the visual infrastructure for nodes on the canvas and the simpler field editors.

Details

  • Core UI for representing and interacting with nodes on-screen in NodeWidget and PortDot
  • NodeRegistry maintains positons of nodes (useful for "Add node between functionality") and ports (necessary for splines snapping to ports and clean edge connection)
  • UI layout data for measuring and placing ports dynamically as nodes change.
  • Algorithm to layout graph nodes in a somewhat organized/pretty manner.
  • Preview widgets to display brush at various stages in various shapes.
  • Logic to assign tooltips to all values for various types, to be utilized in a follow-up PR.
  • Logic to resolve DisplayText at UI layer.
  • Expand color scheme to include necessary "Warning" shades for highlighting nodes.
  • Add auto save functionality.
  • Dialogs to handle all pop-up logic.
  • Icon for port drag handle
  • Much of the fundamental logic for the fields which will be editable in the inspector in a future PR.
  • Some refactoring of code

Dependencies

  • Target: brush-graph/3-viewmodel

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 the UI layer for a node-based brush behavior editor, introducing specific field editors for various node types (BinaryOp, Constant, Damping, Integral, etc.) and the core visual components for nodes and ports. The feedback focuses on critical performance optimizations within the Jetpack Compose implementation, specifically regarding the use of remember to cache expensive operations like protobuf creation and the pre-calculation of values to avoid O(N^2) complexity during recomposition or high-frequency touch events. Additionally, there are suggestions to improve maintainability by replacing magic numbers with constants and removing unused state variables.

Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodePortDots.kt Outdated
Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeHeader.kt Outdated
ProtoBrushBehavior.Node.newBuilder()
.setToolTypeFilterNode(
ProtoBrushBehavior.ToolTypeFilterNode.newBuilder()
.setEnabledToolTypes(1 shl 3) // Stylus
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

Using a magic number 1 shl 3 for tool types is brittle and hard to maintain. It should be replaced with a named constant (e.g., TOOL_TYPE_STYLUS_MASK) or derived from the InputToolType enum if possible.

Copy link
Copy Markdown
Contributor Author

@maxmmitchell maxmmitchell Apr 28, 2026

Choose a reason for hiding this comment

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

I think this is OK, it's just for a default value and is in the proto, so very unlikely to change. We could always have a utility that treats the tooltypes as an enum and then handles the bitshifting under the hood when converting to proto, but maybe overkill. Regardless will be on a different PR since this file was moved to a follow-up PR.

@maxmmitchell maxmmitchell force-pushed the brush-graph/3-viewmodel branch 2 times, most recently from a4e196f to e17146a Compare April 28, 2026 05:53
@maxmmitchell maxmmitchell marked this pull request as ready for review April 28, 2026 07:58
@maxmmitchell maxmmitchell force-pushed the brush-graph/3-viewmodel branch from e17146a to cc0aecc Compare April 30, 2026 14:54
Copy link
Copy Markdown
Contributor

@cka-dev cka-dev left a comment

Choose a reason for hiding this comment

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

Main feedback on this one is to make sure all UI composables have a modifier param,

Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeWidget.kt Outdated
Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeWidget.kt Outdated
@maxmmitchell maxmmitchell force-pushed the brush-graph/3-viewmodel branch from cc0aecc to 8157b89 Compare May 1, 2026 19:36
@maxmmitchell maxmmitchell force-pushed the brush-graph/4-nodes branch 2 times, most recently from 3fc5d33 to 7fb1de6 Compare May 4, 2026 18:27
@maxmmitchell maxmmitchell force-pushed the brush-graph/3-viewmodel branch 2 times, most recently from 4f012bf to 4ebcb7e Compare May 5, 2026 20:08
@maxmmitchell maxmmitchell force-pushed the brush-graph/4-nodes branch 3 times, most recently from 8a7a026 to dbe0562 Compare May 6, 2026 18:10
@maxmmitchell maxmmitchell requested a review from cka-dev May 6, 2026 18:10
@maxmmitchell maxmmitchell force-pushed the brush-graph/4-nodes branch from dbe0562 to 1c38d5a Compare May 6, 2026 18:12
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.

2 participants