Skip to content

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

Open
maxmmitchell wants to merge 15 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 15 commits intobrush-graph/3-viewmodelfrom
brush-graph/4-nodes

Conversation

@maxmmitchell
Copy link
Copy Markdown

@maxmmitchell maxmmitchell commented Apr 27, 2026 •
edited
Loading

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

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Apr 27, 2026
View reviewed changes
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
Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodePortLabels.kt Outdated
Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/fields/NodeFields.kt Outdated
ProtoBrushBehavior.Node.newBuilder()
.setToolTypeFilterNode(
ProtoBrushBehavior.ToolTypeFilterNode.newBuilder()
.setEnabledToolTypes(1 shl 3) // Stylus
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 27, 2026

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
Author

@maxmmitchell maxmmitchell Apr 28, 2026 •
edited
Loading

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.

Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeWidget.kt
@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
cka-dev
cka-dev requested changes Apr 30, 2026
View reviewed changes
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/NodePortDots.kt
import kotlin.math.roundToInt

@Composable
fun BoxScope.NodePortDots(
Copy link
Copy Markdown
Contributor

@cka-dev cka-dev Apr 30, 2026

Choose a reason for hiding this comment

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

needa a modifier

Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeWidget.kt
import kotlin.math.roundToInt

@Composable
fun NodeWidget(
Copy link
Copy Markdown
Contributor

@cka-dev cka-dev Apr 30, 2026

Choose a reason for hiding this comment

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

also needs a modifier

Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeWidget.kt
MaterialTheme.colorScheme.surfaceDim
} else if (isActiveSource || isPressed || isSelected || isInSelectedSet) {
MaterialTheme.colorScheme.primaryContainer
} else if (node.hasError) {
Copy link
Copy Markdown
Contributor

@cka-dev cka-dev Apr 30, 2026

Choose a reason for hiding this comment

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

consider switching to when statement here.

Comment thread app/src/main/java/com/example/cahier/developer/brushgraph/ui/node/NodeWidget.kt
.border(
if (isActiveSource || isPressed || isSelected || isInSelectedSet) {
2.dp
} else if (node.isDisabled) {
Copy link
Copy Markdown
Contributor

@cka-dev cka-dev Apr 30, 2026

Choose a reason for hiding this comment

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

same as above

@cka-dev cka-dev mentioned this pull request Apr 30, 2026
Open
@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 from db4a7d4 to 3fc5d33 Compare May 1, 2026 19:36
cka-dev and others added 14 commits May 4, 2026 18:24
@cka-dev @maxmmitchell
feat: add Brush Designer UI with tabbed editor
a450eb8 
    This PR adds the full Brush Designer screen accessible from Settings, including:

    UI Components:
    - BrushDesignerScreen with adaptive layout (compact/expanded)
    - Three editor tabs: Tip Shape, Paint, and Behaviors
    - NumericField with slider, ± buttons, and tap-to-edit dialog
    - EditableListWidget for managing behaviors, nodes, and texture layers
    - PreviewPane with live stroke rendering via DrawingSurface
    - TipPreview for visual tip shape feedback
    - MathCurvePreview for response curve visualization
    - NodeEditors for Source, Damping, Target, ResponseCurve, BinaryOp, etc.

    Features:
    - Proto editing via BrushDesignerViewModel
    - Import/Export .brush files (GZIP-compressed protobuf)
    - Save to Palette (Room DB integration)
    - Load from Brushes(both stock and custom brushes) or My Palette
    - Standard and Advanced Dynamics presets
    - Texture import with thumbnail preview
    - Color picker for brush paint
@cka-dev @maxmmitchell
address Gemini Code Assist comments and dix test failures
e300780
@maxmmitchell
Add converters and repository for brushgraph
6dd5ea0
@maxmmitchell
Gemini code review comments
5f97056
@maxmmitchell
Add support for caching last valid brush family + fix warnings on test
0062f73
@maxmmitchell
Support duplication respecting original nodes positioning
cb3121a
@maxmmitchell
Add ViewModel and TutorialManager for brushgraph
e0aa5a8
@maxmmitchell
Fix issues from PR 2
220b753
@maxmmitchell
Gemini comments
2afbcfc
@maxmmitchell
Support returning mapping of old ids to duplicated ids from viewmodel…
168090a 
… to UI layer
@maxmmitchell
Get brush family correctly
4f012bf
@maxmmitchell
Add Node UI infrastructure and simple fields
e867acd
@maxmmitchell
Fix breakage from PR 3
c3681c5
@maxmmitchell
Gemini review comments
7fb1de6
@maxmmitchell maxmmitchell force-pushed the brush-graph/4-nodes branch from 3fc5d33 to 7fb1de6 Compare May 4, 2026 18:27
@maxmmitchell maxmmitchell force-pushed the brush-graph/3-viewmodel branch from 8157b89 to 4f012bf Compare May 4, 2026 18:28
@maxmmitchell maxmmitchell force-pushed the brush-graph/3-viewmodel branch from 4f012bf to 4ebcb7e Compare May 5, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cka-dev cka-dev cka-dev requested changes

@miguelmontemayor miguelmontemayor Awaiting requested review from miguelmontemayor

@romanofranz romanofranz Awaiting requested review from romanofranz romanofranz is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maxmmitchell @cka-dev