Brush tool improvements - #173
StickmanRed wants to merge 55 commits into
Conversation
There was a problem hiding this comment.
First of all I wanna say this is awesome, thanks for putting in all this effort. It works well but I wanna see if we can improve the experience. It's a long and scary list but I think these are all important to address.
Clarity
- "Reso" (resolution) doesn't really make sense as a name from the user's perspective. As devs we know why it's resolution, but it would be nice if resolution was renamed to something that everyone can understand. My suggestion would be "quality".
- Spread should not have a button to toggle a slider between 0.01 and the max, it should just be a slider between 0 and the max. Also, it's called "scatter" in one place and "spread" in another, so let's make sure that the name is standardized.
- We should consider removing some of the preset shapes and/or putting them in a dropdown. While they're nice to have, it creates the illusion of so many buttons that it's overwhelming. The plus sign also makes me think that it's a button to add a new brush shape, which is not how it works. I think that, at minimum, we should remove the rough, scatter, and plus. The dropdown is so that we can hide away all the buttons when we don't really need them.
- The "delete brush" button should be grayed out and non-interactive if there is only one brush.
- There are a bunch of buttons that should be moved or removed.
- In the brush selector, the "add brush" and "upload brush" buttons should be related. This could mean putting them in a dropdown where all the other brushes are, or 2 large buttons where "edit brush" is now.
- Also in the brush selector, "edit brush" and "delete brush" should be similar. In my opinion it's weird that the edit button is big while the delete button is small. They should probably go together at the bottom.
- The way that "delete brush" and "delete shape" are both trash can buttons in the same location is really confusing. The previous checkbox already has my suggestion for delete brush. For delete shape, it really shouldn't be in the same row of buttons as everything else since it serves a completely different function. I don't know where exactly it should go... but not where it is now.
- When editing a brush, the pressure, relative size, and mode buttons should not be there. They are accessible when outside of the brush editor because they can change at any time without "altering the fundamental brush". Therefore, the brush editor should not show these, as it's only for the "important" settings.
Customizability
- Some shapes like the thin rectangle rotate to follow the cursor as it moves, but other brushes like the square don't. This makes me think that the rotation thing is defined in the brush shape, which doesn't make sense. I think there should be 3 rotation options: set offset angle (default is zero), follow mouse (with offset angle), and randomize angle.
Visual Improvements
- When dragging the sliders, the preview flickers quite a bit, which is distracting. Hopefully there's a way to remove the flickering?
- I found some inconsistencies in the shapes. The sparkle in the preview is in an "X" shape, but it's in a "+" shape everywhere else. The moon looks different in all 3 places: on the canvas it's closest to a "C" shape, on the button it's not as sharp, and in the preview it's cut weirdly.
- When using a big brush size, the brush path before releasing the mouse is pretty low resolution. Is it possible to increase the resolution so that it looks nicer?
- The triangle shape is not equilateral... I don't know, that kinda bugs me. (If we fix this, make the centroid/center of mass the middle of the shape if that's even possible)
Safeguards
- We should have preset brushes that cannot be significantly modified or deleted. For the standard brush, the only customizable settings should what we offer in the current version of Candlestick, plus maybe the resolution/smoothing.
- It's a little dangerous to be able to delete some of the fundamental shapes, especially the circle. I toyed with deleting stuff and I had to clear my cookies to bring back the default shapes. I'd say that the default shapes should not be allowed to be deleted, or at bare minimum the circle.
- There should be a cancel button to exit the brush config without saving it.
I was aiming for a shorter name to not have it cut off and reso sounded catchy lol
It started as a button, later turned to a slider, then I increased lower limit to 0.01 because having it at 0 bugged something— that said it makes a lot more sense to have it be just a slider correct yeh and I'll just turn it off if it's 0.
You're right! Idk how I missed that ahh few weeks back I called it scatter later in the code I just decided to call it spread when I made the slider 😓
A dropdown is possible but an easier option I was actually considering is horizontal scroll 🤔
Good point here this I can do
Agree with you there
|
|
Note to self: Allow drag and dropping |
|
Few more things I think we should change:
I'll do a couple of these really quick tonight. I'll see what else I can do over this upcoming week. |
brush is a normal circle brush brushSize+1 is removed, since that is a remnant of the old wick brush default brush rotation is "fixed" brush rotation dropdown reordered to "fixed", "path", "random" to follow the natural order from no-rotation to all-rotation "new brush" tooltip renamed to "create"
BaronAWC
left a comment
There was a problem hiding this comment.
Code-related comments:
Main comments here are about moving code out of Inspector.jsx and into either Brush.js or a new file(s).
Additionally, in the code itself, we should standardize terms like spread/scatter, resolution/quality, etc.
| // Default brushes — structured like a parsed .cbrush JSON for easy migration later. | ||
| // Each field matches the keys stored/loaded by saveBrush / applyBrush. | ||
| // FIXME: these "default" values are in 3 places (search for this same comment) | ||
| const DEFAULT_BRUSHES = [ |
There was a problem hiding this comment.
I feel like this default brush doesn't have to be in Inspector.jsx, it makes more sense either in Brush.js or possibly somewhere else
| }, | ||
| ]; | ||
|
|
||
| const BRUSH_SHAPES = [ |
There was a problem hiding this comment.
I would imagine this is redundant, since the brush shapes exist somewhere else. When creating a new brush shape, the editor also knows how to create the preview, so I'd imagine it's doable for these default shapes too.
| svg: <><circle cx="14" cy="14" r="4"/><circle cx="7" cy="8" r="3"/><circle cx="21" cy="8" r="2.5"/><circle cx="7" cy="20" r="3"/><circle cx="21" cy="20" r="2.5"/></> }, | ||
| ]; | ||
|
|
||
| const ROTATION_MODE_OPTIONS = [ |
There was a problem hiding this comment.
These only seem to be used once, so I wonder if it makes more sense to hardcode
| } | ||
|
|
||
| // BRUSH PREVIEW— we're literally drawing a brush stroke here -H.A. | ||
| drawBrushPreview = () => { |
There was a problem hiding this comment.
Could the brush preview go to a separate file? It's not really suitable in the inspector file
| if (this.brushFileInputRef.current) this.brushFileInputRef.current.click(); | ||
| } | ||
|
|
||
| handleBrushFileImport = (e) => { |
There was a problem hiding this comment.
Import/export could probably go into a different file - doesn't make sense in inspector
| @@ -70,10 +70,6 @@ class ToolSettings extends Component { | |||
| return ( | |||
| <div className='settings-input-container'> | |||
| {this.renderBrushSize()} | |||
There was a problem hiding this comment.
we may not even need brush size here, since it's in the inspector now
| ) | ||
| } | ||
|
|
||
| renderBrushResolution = () => { |
| } | ||
|
|
||
| // Crescent needs compositing — handle separately | ||
| if (shape === 'crescent') { |
There was a problem hiding this comment.
Can the brush shapes go into a separate file?
|
I quickly fixed the default brush settings in one final place, which should make it consistent with the other 3 places. (Speaking of which, we should try to only have it in one place) Also, I'm not sure how to reproduce this, but sometimes the fill color of the brush preview will disappear entirely, leaving just the outline. Has anyone else encountered this? |
I think we should try to move all the brush shape-drawing logic into an engine file, maybe |
Features