-
Notifications
You must be signed in to change notification settings - Fork 359
Fixed Square Brush, Improve Brush Hardness and Smoothing Precision #4519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed Square Brush, Improve Brush Hardness and Smoothing Precision #4519
Conversation
… improved the effect of smoothing precision
|
@trsommer @webfiltered Quickly following up; this is a much smaller, addendum, PR that improves functionality to 4361 |
@christian-byrne @webfiltered Can you please review this PR when you have a chance? |
Hi @webfiltered @christian-byrne Following up on the review of this patch as it fixes and improves functionality of the new Image Canvas. Shouldn’t be too much in terms of changes to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work fixing the square brush hardness issue! The solution correctly addresses the core problem where square brushes were becoming circular when hardness < 1.
Key Improvements:
- Fixes square brush rendering with proper hardness control
- Improves smoothing precision algorithm with better granular control
- Maintains backward compatibility
- Clear separation between RGB and mask brush logic
I've added specific inline comments focusing on performance optimizations and code quality improvements. The most important suggestions are:
- Performance: Consider optimizing the 10-fill loop in
drawSoftSquare
- Code Quality: Leverage existing
colorUtil
functions for safer color manipulation
The changes work correctly as-is - these are optimization suggestions for better maintainability and performance.
9936142
to
78572a8
Compare
Thank you for your review @christian-byrne. I've replied to your comments and pushed some updates accordingly. |
da61da7
to
0d1711e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance concern with the drawSoftSquare function - is it creating a new canvas and manipulating ImageData for every brush stroke when using soft square brushes?
That seems liek it would be significantly slower than the existing gradient-based rendering for other brush types. Could we consider using cached brush textures or optimizing the ImageData approach to avoid creating new canvases on each stroke?
The fix for square brush hardness is great, but the performance impact could cause lag during drawing for users of soft rectangular brushes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const tempCanvas = document.createElement('canvas')
const tempCtx = tempCanvas.getContext('2d')
// ... manual pixel manipulation loop
0d1711e
to
18ad6b3
Compare
Thanks @christian-byrne. I've fixed brush rendering anti-aliasing and performance issues.
Additionally, It seems that @trsommer has reacted with a thumbs up to Kevin's message here in regards to adding me as codeowner. Could you / the team please do so when you have a chance? |
Sure, could you make a PR to add yourself to the CODEOWNERS file? |
Done - can you please approve changes if they look good to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. I tested it and it works nicely. What are your thoughts on adding a unit test 😄? You don't have to.
src/extensions/core/maskeditor.ts
Outdated
@@ -2049,9 +2050,15 @@ class BrushTool { | |||
rgbCtx: CanvasRenderingContext2D | null = null | |||
initialDraw: boolean = true | |||
|
|||
private static brushTextureCache = new Map<string, HTMLCanvasElement>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using an LRU cache? We have a library we use in some other places, it can be used like:
import QuickLRU from '@alloc/quick-lru'
private static brushTextureCache = new QuickLRU<string, HTMLCanvasElement>({
maxSize: 8 // Reasonable limit for brush texture variations?
})
I can leverage QuickLRU but I'm getting the following error when doing
|
Does it still happen when using node 24?
|
It does still happen when using v24.4.0 I bypassed the dompurify issue by going to ComfyUI_frontend\src\types and creating a file But there are still issues with the packages. This is likely stemming from the switch to pnpm in the latest frontend updates. For example, the
Are you not seeing the errors on your end?
Might be easier to chat on discord to not sidetrack this thread? I sent you a DM and also posted in the frontend channel of the Comfy server. |
No, I am not. Have you tried deleeting the node_modules folder and reinstalling? rm -rf node_modules pnpm-lock.yaml
pnpm install Can also try rm -rf node_modules pnpm-lock.yaml
pnpm install --shamefully-hoist |
@christian-byrne Fixed the issue. The frontend’s switch to pnpm resulted in much longer file paths due to its symlinked Just pushed an update that uses QuickLRU for the brushTextureCache. If the changes look good to you, can you approve instead of @trsommer? I'm not sure that he is active on this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR does the following:
Fixed.Smoothing.Precision.and.Soft.Square.Brush.mp4
┆Issue is synchronized with this Notion page by Unito