add Brush Designer ViewModel, preview, and app wiring#59
add Brush Designer ViewModel, preview, and app wiring#59
Conversation
cka-dev
commented
Apr 15, 2026
cka-dev
commented
- BrushDesignerViewModel with proto editing, GZIP persistence, and live preview
- PreviewPane with drawing canvas, brush color/size controls, and tip preview
- BrushDesignerTopBar with brush library (stock + Cahier custom brushes), palette, import/export
- Navigation wiring, Settings screen update (Node Graph UI coming soon)
- Shared components: NumericField, BrushDesignerComponents, BrushDesignerTab
- ViewModel and layout tests
- Placeholder controls pane (tabbed editor in follow-up PR)
- BrushDesignerViewModel with proto editing, GZIP persistence, and live preview - PreviewPane with drawing canvas, brush color/size controls, and tip preview - BrushDesignerTopBar with brush library (stock + Cahier custom brushes), palette, import/export - Navigation wiring, Settings screen update (Node Graph UI coming soon) - Shared components: NumericField, BrushDesignerComponents, BrushDesignerTab - ViewModel and layout tests - Placeholder controls pane (tabbed editor in follow-up PR)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Brush Designer' feature, allowing users to create, test, and export custom brushes. It includes UI components for color picking, numeric input, and previewing strokes, along with the necessary ViewModel logic for managing brush properties and persistence. Several critical issues were identified regarding incorrect GZIP handling when decoding brush data, which would prevent brushes from loading correctly. Additionally, a library version downgrade was noted.
| GZIPOutputStream(baos).use { it.write(rawBytes) } | ||
|
|
||
| ByteArrayInputStream(baos.toByteArray()).use { inputStream -> | ||
| BrushFamily.decode(inputStream) | ||
| } |
There was a problem hiding this comment.
The BrushFamily.decode method expects raw protobuf bytes, not gzipped bytes. In the previewBrushFamily flow, the proto.toByteArray() are first gzipped into baos, and then BrushFamily.decode is incorrectly called with these gzipped bytes. This will prevent the brush family from being decoded correctly, leading to previewBrushFamily always being null or invalid.
val rawBytes = proto.toByteArray()
ByteArrayInputStream(rawBytes).use { inputStream ->
BrushFamily.decode(inputStream)
}There was a problem hiding this comment.
This is not true. decode expected a Gzip compressed proto
| val baos = ByteArrayOutputStream() | ||
| stockBrush.encode(baos) | ||
|
|
||
| val gzippedBytes = baos.toByteArray() | ||
| ByteArrayInputStream(gzippedBytes).use { inputStream -> | ||
| GZIPInputStream(inputStream).use { gzipStream -> | ||
| val rawBytes = gzipStream.readBytes() | ||
| val loadedProto = ProtoBrushFamily.parseFrom(rawBytes) | ||
| repository.updateActiveBrushProto(loadedProto) | ||
| clearCanvas() | ||
| } | ||
| } |
There was a problem hiding this comment.
The loadStockBrush function incorrectly uses GZIP compression/decompression for in-memory conversion. The stockBrush.encode(baos) call already produces raw protobuf bytes. The subsequent GZIPOutputStream and GZIPInputStream are unnecessary and will lead to incorrect decoding, as BrushFamily.decode expects raw bytes. A simpler and correct approach is to directly convert the BrushFamily object to ProtoBrushFamily using stockBrush.toProto().
| val baos = ByteArrayOutputStream() | |
| stockBrush.encode(baos) | |
| val gzippedBytes = baos.toByteArray() | |
| ByteArrayInputStream(gzippedBytes).use { inputStream -> | |
| GZIPInputStream(inputStream).use { gzipStream -> | |
| val rawBytes = gzipStream.readBytes() | |
| val loadedProto = ProtoBrushFamily.parseFrom(rawBytes) | |
| repository.updateActiveBrushProto(loadedProto) | |
| clearCanvas() | |
| } | |
| } | |
| val loadedProto = stockBrush.toProto() | |
| repository.updateActiveBrushProto(loadedProto) | |
| clearCanvas() |
There was a problem hiding this comment.
Also not true: encode returns a Gziped proto.
| ByteArrayInputStream(entity.brushBytes).use { inputStream -> | ||
| val family = BrushFamily.decode(inputStream) |
There was a problem hiding this comment.
The BrushFamily.decode method is being called with entity.brushBytes, which contains gzipped protobuf data. However, BrushFamily.decode expects raw protobuf bytes. The raw bytes (rawProtoBytes) are already extracted from the GZIP stream earlier in the use block. Passing gzipped bytes to BrushFamily.decode will cause a decoding error and prevent custom brushes from loading correctly.
| ByteArrayInputStream(entity.brushBytes).use { inputStream -> | |
| val family = BrushFamily.decode(inputStream) | |
| ByteArrayInputStream(rawProtoBytes).use { inputStream -> | |
| val family = BrushFamily.decode(inputStream) |
| /* | ||
| * | ||
| * * | ||
| * * * Copyright 2026 Google LLC. All rights reserved. | ||
| * * * | ||
| * * * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * * * you may not use this file except in compliance with the License. | ||
| * * * You may obtain a copy of the License at | ||
| * * * | ||
| * * * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * * * | ||
| * * * Unless required by applicable law or agreed to in writing, software | ||
| * * * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * * * See the License for the specific language governing permissions and | ||
| * * * limitations under the License. | ||
| * * | ||
| * | ||
| */ |
There was a problem hiding this comment.
(nit) Should the licenses be indented with 1, 2, or 3 stars? I'm noticing differing patterns across this file, app/src/main/java/com/example/cahier/developer/brushdesigner/ui/BrushDesignerComponents.kt, and app/src/main/java/com/example/cahier/core/navigation/CahierNavGraph.kt, for example.
| }, | ||
| actions = { | ||
| BrushLibraryMenu( | ||
| onLoadStockBrush = onLoadStockBrush, |
There was a problem hiding this comment.
If onLoadStockBrush and onLoadCahierBrush can always take the same value as the argument, we can omit the second parameter and just have BrushLibraryMenu take onLoadBrush (and maybe rename above, to clarify that we use one generic load function for both stock and Cahier custom brushes)
| /** Raw numeric values with a configurable step */ | ||
| fun standard( | ||
| min: Float, | ||
| max: Float, | ||
| step: Float = 0.01f | ||
| ): NumericLimits = | ||
| NumericLimits( | ||
| min = min, | ||
| max = max, | ||
| step = step | ||
| ) | ||
| } |
There was a problem hiding this comment.
(nit) Functionally this constructor wrapper is making step an optional parameter; since displayUnit and unitScale are already optional parameters, why not also make step optional above (= 0.01f)? Then this code can be deleted, and callers can change their call from NumericLimits.standard(...) to NumericLimits(...).
| * dialog for exact value entry. | ||
| * | ||
| * Stateless: all state is passed in via [value] and changes are emitted | ||
| * via [onValueChanged] in real (proto) units. |
There was a problem hiding this comment.
"(proto)" is a bit unclear -- below, the displayed value is passed to limits.toRealValue(), so another parameter to NumericField (limits) is controlling what "real units" means. I would change this to say:
via [onValueChanged] in real units as described by [limits].
(could also reference toRealValue or unitScale in the comment, but just clarifying to limits is probably sufficient)
| textInputValue = displayValue.toString() | ||
| showTextInput = true | ||
| }, | ||
| onClick = {} |
There was a problem hiding this comment.
Maybe worth having a single click on title also open text input, just for increased discoverability? Though maybe this is too easy to open by accident.
| onLoadStockBrush(StockBrushes.dashedLine()) | ||
| expanded = false | ||
| } | ||
| ) |
There was a problem hiding this comment.
(nit) Could reduce code here by mapping the names to the stock brushes, and then creating the DropdownMenuItems in a forEach loop, as is done for ColorSelector in app/src/main/java/com/example/cahier/developer/brushdesigner/ui/PreviewPane.kt ?
| tiltRadians = 0.3f, | ||
| orientationRadians = 0f |
There was a problem hiding this comment.
(nit) Varying tilt and orientation could be useful for having the preview demonstrate more complex behaviors, as pressure does?
| initialValue = null | ||
| ) | ||
|
|
||
| private val autoSaveFile = File(context.cacheDir, "autosave.brush") |
There was a problem hiding this comment.
Note that if we change the default file extension in strings, this should change too. If not, no action is needed.
| try { | ||
| val bitmap = context.contentResolver.openInputStream(uri)?.use { | ||
| BitmapFactory.decodeStream(it) | ||
| } ?: return@launch | ||
|
|
||
| textureStore?.loadTexture(textureId, bitmap) | ||
|
|
||
| val builder = repository.activeBrushProto.value.toBuilder() | ||
|
|
||
| val baos = ByteArrayOutputStream() | ||
| bitmap.compress(Bitmap.CompressFormat.PNG, 100, baos) | ||
| builder.putTextureIdToBitmap( | ||
| textureId, | ||
| ByteString.copyFrom(baos.toByteArray()) | ||
| ) |
There was a problem hiding this comment.
Instead of packing the bitmaps into the proto manually here, and manually syncing them when you load a brush with syncProtobufTexturesToStore, you could use the AndroidBrushFamilySerialization.encode/decode methods which handle this logic of storing bitmaps in proto from some texture bitmap store, and populating a texture bitmap store based on the proto for you: https://developer.android.com/reference/kotlin/androidx/ink/storage/AndroidBrushFamilySerialization
Same goes for other such cases or serialization/deserialization throughout the PR.
Since the API expects a BrushFamily, this does require going through a BrushFamily object in order to de/serialize. But, I would usually expect that to be ok, since you generally need to keep a BrushFamily equivalent to the BrushFamilyProto around in order to have the live previews.
| /** | ||
| * Adds a "Smooth Dynamics" behavior: | ||
| * Logic: Source (Input) -> Damping (Smoothing) -> Target (Output) | ||
| */ | ||
| fun addSmoothedBehavior( | ||
| sourceType: BrushBehavior.Source, | ||
| targetType: BrushBehavior.Target, | ||
| dampingSeconds: Float = 0.1f | ||
| ) { | ||
| val source = BrushBehavior.Node.newBuilder().setSourceNode( | ||
| BrushBehavior.SourceNode.newBuilder() | ||
| .setSource(sourceType) | ||
| .setSourceValueRangeStart(0f) | ||
| .setSourceValueRangeEnd(1f) | ||
| .setSourceOutOfRangeBehavior(BrushBehavior.OutOfRange.OUT_OF_RANGE_CLAMP) | ||
| ).build() | ||
|
|
||
| val damping = BrushBehavior.Node.newBuilder().setDampingNode( | ||
| BrushBehavior.DampingNode.newBuilder() | ||
| .setDampingSource(BrushBehavior.ProgressDomain.PROGRESS_DOMAIN_TIME_IN_SECONDS) | ||
| .setDampingGap(dampingSeconds) | ||
| ).build() | ||
|
|
||
| val target = BrushBehavior.Node.newBuilder().setTargetNode( | ||
| BrushBehavior.TargetNode.newBuilder() | ||
| .setTarget(targetType) | ||
| .setTargetModifierRangeStart(0.5f) | ||
| .setTargetModifierRangeEnd(1.5f) | ||
| ).build() | ||
|
|
||
| addBehavior(listOf(source, damping, target)) | ||
| } | ||
|
|
||
| /** | ||
| * Adds a "Random Jitter" behavior: | ||
| * Logic: Noise (Random) -> Target (Output) | ||
| */ | ||
| fun addJitterBehavior(targetType: BrushBehavior.Target) { | ||
| val noise = BrushBehavior.Node.newBuilder().setNoiseNode( | ||
| BrushBehavior.NoiseNode.newBuilder() | ||
| .setSeed(kotlin.random.Random.nextInt()) | ||
| .setVaryOver( | ||
| BrushBehavior.ProgressDomain.PROGRESS_DOMAIN_DISTANCE_IN_CENTIMETERS | ||
| ) | ||
| .setBasePeriod(0.5f) | ||
| ).build() | ||
|
|
||
| val target = BrushBehavior.Node.newBuilder().setTargetNode( | ||
| BrushBehavior.TargetNode.newBuilder() | ||
| .setTarget(targetType) | ||
| .setTargetModifierRangeStart(0.8f) | ||
| .setTargetModifierRangeEnd(1.2f) | ||
| ).build() | ||
|
|
||
| addBehavior(listOf(noise, target)) | ||
| } | ||
|
|
There was a problem hiding this comment.
It doesn't make a ton of sense to me to have these behavior builders which take in a source/target enums and build everything else, since sensible values for range start/end for source and target are heavily dependent on what the source is and what the target is, partially because the units are different but also because the ranges are different. Furthermore, prefab behaviors should probably have ranges that are configured to a reasonable value for the given source/target, which will likely differ depending on the behavior.
I'd recommend having functions which build the specific behaviors we want to include as prefabs rather than generic builders. It's a bit more verbose, but we can always stick it in a separate file for prefab behaviors.
2 participants
