add tabbed brush editor with Tip Shape, Paint, and Behaviors tabs#68
add tabbed brush editor with Tip Shape, Paint, and Behaviors tabs#68
Conversation
- 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)
Migrated to PR-A API changes:
- NumericLimits.standard() → NumericLimits())
- addSmoothedBehavior/addJitterBehavior → PrefabBehaviors
- onLoadStockBrush → onLoadBrush
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
|
/gemini review |
![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, enabling users to create, customize, and manage ink brushes through a dedicated UI. The feature includes tabs for adjusting tip shape, paint and texture properties, and brush behaviors, along with controls for input models and brush layers. Users can import and export brush configurations, save them to a local palette, and test them on an interactive drawing canvas. The implementation also adds a color picker, various numeric input controls, and new drawable resources. Several high-severity issues were identified, including multiple potential NullPointerException risks in brush creation and preview logic due to non-null assertions, and a potential race condition in texture loading. Medium-severity feedback includes opportunities for improving data reactivity, using more precise MIME types for brush imports, enhancing API design for test parameters, improving code maintainability by extracting hardcoded default values into constants, optimizing brush and texture loading performance, and verifying a UI/UX change related to the custom caption bar. Additionally, several instances of hardcoded strings were noted, which should be localized for better internationalization.
| */ | ||
| @Composable | ||
| private fun PreviewToolbar( | ||
| brushSize: Float, |
There was a problem hiding this comment.
The onGetNextBrush lambda uses the non-null assertion operator !!. This can lead to a NullPointerException if activeBrush is null. It's safer to provide a fallback or handle the null case explicitly.
onGetNextBrush = { viewModel.getActiveBrush() ?: activeBrush ?: Brush.getDefaultInstance() },| if (family == null) null | ||
| else Brush.createWithComposeColor( | ||
| family = family, | ||
| color = color, | ||
| size = size, | ||
| epsilon = 0.1f | ||
| ) |
There was a problem hiding this comment.
The activeBrush combines previewBrushFamily, _brushColor, and _brushSize. The Brush.createWithComposeColor call uses a non-null assertion !! on activeBrush in the onGetNextBrush lambda in BrushDesignerScreen.kt. This could lead to a NullPointerException if previewBrushFamily is null. It's safer to provide a default Brush instance or handle the null case explicitly here.
else Brush.createWithComposeColor(
family = family,
color = color,
size = size,
epsilon = 0.1f
) ?: Brush.getDefaultInstance()| BrushFamily.decode(inputStream) { textureId, bitmap -> | ||
| bitmap?.let { textureStore?.loadTexture(textureId, it) } | ||
| textureId | ||
| } |
There was a problem hiding this comment.
The BrushFamily.decode call uses a lambda that accesses textureStore?. If textureStore is null at the time of decoding, the bitmap?.let block will not execute, meaning textures might not be loaded. While setTextureStore is called later, there's a potential race condition or an assumption that textureStore will always be available when decode is called. Ensure textureStore is initialized before previewBrushFamily starts emitting values or handle the null case more explicitly within the lambda.
| onSetTextureStore = { viewModel.setTextureStore(it) }, | ||
| onReplaceStrokes = { viewModel.replaceStrokes(it) }, | ||
| onStrokesFinished = { viewModel.onStrokesFinished(it) }, | ||
| onGetNextBrush = { viewModel.getActiveBrush() ?: activeBrush!! }, |
There was a problem hiding this comment.
The onGetNextBrush lambda returns viewModel.getActiveBrush() ?: activeBrush!!. The use of the non-null assertion operator !! here could lead to a NullPointerException if activeBrush is null. It's safer to handle the null case explicitly, perhaps by returning a default brush or logging an error, or ensuring activeBrush is never null at this point.
onGetNextBrush = { viewModel.getActiveBrush() ?: activeBrush ?: Brush.getDefaultInstance() }| onSetTextureStore = { viewModel.setTextureStore(it) }, | ||
| onReplaceStrokes = { viewModel.replaceStrokes(it) }, | ||
| onStrokesFinished = { viewModel.onStrokesFinished(it) }, | ||
| onGetNextBrush = { viewModel.getActiveBrush() ?: activeBrush!! }, |
There was a problem hiding this comment.
The onGetNextBrush lambda in the ListDetailPaneScaffold also uses the non-null assertion operator !!. This carries the same risk as noted previously. Consider handling the null case more robustly.
onGetNextBrush = { viewModel.getActiveBrush() ?: activeBrush ?: Brush.getDefaultInstance() }| val textureLayer = ProtoBrushPaint.TextureLayer.newBuilder() | ||
| .setClientTextureId(textureId) | ||
| .setMapping(ProtoBrushPaint.TextureLayer.Mapping.MAPPING_TILING) | ||
| .setBlendMode(ProtoBrushPaint.TextureLayer.BlendMode.BLEND_MODE_SRC_OVER) | ||
| .setSizeUnit(ProtoBrushPaint.TextureLayer.SizeUnit.SIZE_UNIT_BRUSH_SIZE) | ||
| .setSizeX(1.0f) | ||
| .setSizeY(1.0f) | ||
| .build() |
There was a problem hiding this comment.
The textureLayer is built with several hardcoded default values (e.g., MAPPING_TILING, BLEND_MODE_SRC_OVER, SIZE_UNIT_BRUSH_SIZE, 1.0f). Similar to the ProtoBrushTip defaults, these could be extracted into constants or a helper function for better maintainability and readability.
| val clientBrushFamilyId = _customBrushes.value | ||
| .find { it.brushFamily == currentBrushFamily }?.name |
There was a problem hiding this comment.
The clientBrushFamilyId is retrieved by finding a custom brush by its brushFamily. This lookup might be inefficient if _customBrushes.value contains a large number of brushes, as it iterates through the list and compares BrushFamily objects. Consider optimizing this lookup, perhaps by storing custom brushes in a map keyed by brushFamily or name if performance becomes an issue.
| GZIPInputStream(ByteArrayInputStream(entity.brushBytes)) | ||
| .use { gzip -> | ||
| val rawProtoBytes = gzip.readBytes() | ||
| val proto = ink.proto.BrushFamily.parseFrom(rawProtoBytes) | ||
|
|
||
| proto.textureIdToBitmapMap.forEach { (id, byteString) -> | ||
| val bitmapBytes = byteString.toByteArray() | ||
| val bitmap = | ||
| BitmapFactory.decodeByteArray( | ||
| bitmapBytes, | ||
| 0, | ||
| bitmapBytes.size | ||
| ) | ||
| if (bitmap != null) { | ||
| textureStore.loadTexture(id, bitmap) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The texture loading logic within loadCustomBrushes is nested inside GZIPInputStream.use and then iterates through proto.textureIdToBitmapMap. This process is repeated for each CustomBrushEntity. While the decodedCache helps with CustomBrush objects, the texture loading itself is not cached. If multiple brushes share textures, or if the same brush is loaded multiple times, textures might be re-decoded unnecessarily. Consider adding a texture-level cache within CahierTextureBitmapStore or optimizing this texture loading loop.
| CahierNavHost( | ||
| navController = navController, | ||
| modifier = modifier | ||
| ) |
There was a problem hiding this comment.
The removal of the Column and Row for the caption bar means the app will no longer display a custom title bar based on the current route. Ensure this change aligns with the intended UI/UX for the application, especially on platforms where custom title bars might be expected or beneficial.
| val isCompact = forceCompact | ||
| ?: (windowSizeClass?.widthSizeClass == WindowWidthSizeClass.Compact) |
There was a problem hiding this comment.
The isCompact calculation now uses a forceCompact parameter. While this is useful for testing, ensure that the default behavior (windowSizeClass?.widthSizeClass == WindowWidthSizeClass.Compact) is robust and covers all production scenarios correctly. If windowSizeClass can be null, the default isCompact would be false, which might not be the desired fallback.
maxmmitchell
left a comment
maxmmitchell
left a comment
There was a problem hiding this comment.
Reviewed for commit 3 (I assume the intended diffbase is brush-designer-pr-a, which is in the first 2 commits). Before merging I strongly recommend pulling in changes from #67 and addressing any issues which arise from the changes in proto coming with Ink 1.1.0-alpha02 (should be mostly minor name changes)
| val s = BrushBehavior.Node.newBuilder().setSourceNode( | ||
| BrushBehavior.SourceNode.newBuilder() | ||
| .setSource(BrushBehavior.Source.SOURCE_NORMALIZED_PRESSURE) | ||
| .setSourceValueRangeStart(0f).setSourceValueRangeEnd(1f) | ||
| .setSourceOutOfRangeBehavior( | ||
| BrushBehavior.OutOfRange.OUT_OF_RANGE_CLAMP | ||
| ) | ||
| ).build() | ||
| val t = BrushBehavior.Node.newBuilder().setTargetNode( | ||
| BrushBehavior.TargetNode.newBuilder() | ||
| .setTarget(BrushBehavior.Target.TARGET_SIZE_MULTIPLIER) | ||
| .setTargetModifierRangeStart(0.5f).setTargetModifierRangeEnd(1.5f) | ||
| ).build() | ||
| onAddBehavior(listOf(s, t)) |
There was a problem hiding this comment.
Should we be using the behaviors in viewmodel/PrefabBehaviors.kt here? Or, if we want to use these behaviors rather than the prefab behaviors, should these behaviors be abstracted to PrefabBehaviors.kt?
| } | ||
|
|
||
| @Composable | ||
| private fun AdvancedDynamicsSection( |
There was a problem hiding this comment.
Some of the behaviors in PrefabBehaviors.kt don't make an appearance here in the UI -- we should probably have buttons here to make them available. Or, if we don't want to include some of them, maybe delete the unused ones from PrefabBehaviors.kt.
| onTextureIdChange = { textureIdInput = it }, | ||
| onConfirm = { | ||
| if (textureIdInput.isNotBlank() && pendingTextureUri != null) { | ||
| viewModel.addCustomTexture(pendingTextureUri!!, textureIdInput) |
There was a problem hiding this comment.
(eenit) I don't think you need the !! here, since the check above ensures pendingTextureUri is not null.
| * A generic list editor that supports add, delete, duplicate, and A/B | ||
| * testing (enable/disable toggle) for ordered lists of proto items. |
There was a problem hiding this comment.
In manual testing, I'm noticing some bugs with the A/B toggle. Unchecking an item deletes it. If there is an item after the unchecked item in the list, then that item becomes unchecked in the UI. Trying to duplicate an unchecked items appears to delete the last element in the list. Unchecking an item above the unchecked item does the usual thing (deletes the item, unchecks the item below it) but also deletes the other unchecked item. Unchecking an item below an unchecked item seems to delete several items from the list. Editing any item in the list deletes the unchecked item.
| * Stateless: the canonical item list is passed in via [items], and all | ||
| * changes are emitted via [onItemsChanged] with only the enabled items. | ||
| * | ||
| * @param title section header text | ||
| * @param items current list of items (from proto) | ||
| * @param defaultItem factory default for new items | ||
| * @param onItemsChanged callback with the updated full list of items | ||
| * @param itemHeader display label for each item in collapsed view | ||
| * @param editorContent expanded editor for the selected item |
There was a problem hiding this comment.
Is this stateless? We maintain itemStates below to keep track of disabled items which won't be in the items list passed via proto.
|
|
||
| if (itemStates.size != items.size || | ||
| itemStates.zip(items).any { (s, i) -> s.item != i }) { | ||
| itemStates = items.mapIndexed { index, item -> |
There was a problem hiding this comment.
I think the logic here doesn't handle the case where itemStates contains an item which isn't in items. During a recomposition when an item in the list has been disabled, itemStates.size != items.size, and we overwrite itemStates to only contain items that are in items. So since items is only what is in the proto, and disabled items aren't in the proto, we get rid of disabled items.
I think what we want to do here is verify that every item in items is represented by a corresponding CheckableItem(it, true), and if not, add such an item at the end of the list. Though, we only need to bother with such logic if it is ever valid for new items to be passed in via items after initialization. If not (i.e., the editable list is only editable via the widget), then I'd only do anything with items when itemStates is uninitialized, otherwise, just ignore it. Alternatively, hoist this state to the viewmodel, though doing that for every single list probably makes the code much less clear.
|
|
||
| /** | ||
| * Full editor for a single [ProtoBrushPaint.TextureLayer], including all | ||
| * fields from SSA: texture ID, mapping, size unit, scale, rotation, |
There was a problem hiding this comment.
Delete reference to SSA, can just say all fields:.
| } | ||
|
|
||
| /** Returns a short human-readable description for each blend mode. */ | ||
| private fun blendModeDescription(mode: ProtoBrushPaint.TextureLayer.BlendMode): String = |
There was a problem hiding this comment.
Missing descriptions for several blend modes: dst_atop, xor, src_out, dst, src, and dst_over.
| */ | ||
| @Composable | ||
| internal fun NodeEditor( | ||
| node: BrushBehavior.Node, |
There was a problem hiding this comment.
Should these composables take a modifier param?
| Text( | ||
| stringResource(R.string.brush_designer_animation), | ||
| style = MaterialTheme.typography.labelLarge | ||
| ) | ||
| Card( | ||
| colors = CardDefaults.cardColors( | ||
| containerColor = MaterialTheme.colorScheme.tertiaryContainer.copy(alpha = 0.5f) | ||
| ), | ||
| modifier = Modifier.fillMaxWidth() | ||
| ) { | ||
| Column(modifier = Modifier.padding(12.dp)) { | ||
| NumericField( | ||
| title = stringResource(R.string.brush_designer_rows), | ||
| value = if (layer.hasAnimationRows()) layer.animationRows.toFloat() else 1f, | ||
| limits = NumericLimits(1f, 10f, 1f), | ||
| onValueChanged = { | ||
| onLayerChanged( | ||
| layer.toBuilder().setAnimationRows(it.toInt()).build() | ||
| ) | ||
| } | ||
| ) | ||
| NumericField( | ||
| title = stringResource(R.string.brush_designer_columns), | ||
| value = if (layer.hasAnimationColumns()) layer.animationColumns.toFloat() | ||
| else 1f, | ||
| limits = NumericLimits(1f, 10f, 1f), | ||
| onValueChanged = { | ||
| onLayerChanged( | ||
| layer.toBuilder().setAnimationColumns(it.toInt()).build() | ||
| ) | ||
| } | ||
| ) | ||
| NumericField( | ||
| title = stringResource(R.string.brush_designer_frames), | ||
| value = if (layer.hasAnimationFrames()) layer.animationFrames.toFloat() | ||
| else 1f, | ||
| limits = NumericLimits(1f, 64f, 1f), | ||
| onValueChanged = { | ||
| onLayerChanged( | ||
| layer.toBuilder().setAnimationFrames(it.toInt()).build() | ||
| ) | ||
| } | ||
| ) | ||
| NumericField( | ||
| title = stringResource(R.string.brush_designer_duration_seconds), | ||
| value = if (layer.hasAnimationDurationSeconds()) | ||
| layer.animationDurationSeconds else 0f, | ||
| limits = NumericLimits(0f, 5f, 0.1f), | ||
| onValueChanged = { | ||
| onLayerChanged( | ||
| layer.toBuilder().setAnimationDurationSeconds(it).build() | ||
| ) | ||
| } |
There was a problem hiding this comment.
All the animation stuff can be omitted. In a recent update this was removed from the public brush_family.proto which we will be updating to use in PR #67
2 participants
add tabbed brush editor with Tip Shape, Paint, and Behaviors tabs