-
Notifications
You must be signed in to change notification settings - Fork 328
[Feature] Enhanced MaskEditor to an Image Canvas #4361
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
base: main
Are you sure you want to change the base?
Conversation
… Still need to figure out how to load the painted pixels onto RGB Canvas when the MaskEditor is re-opened after painting and saving
… them as part of the image. Maybe a combinedCanvas?
@christian-byrne @webfiltered Tagging you in case you have a chance to take a look. |
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.
I just tried it. There seems to be a problem where the resulting layers are still split into 2 images in the load image preview. I tried it in both chrome and firefox and just painting on the canvas with the paint tool results in 2 images in the load image node
after reloading, it seems to be combined:
If you run the workflow, the correct image seems to be handed over. The core process seems to work, just the previews in the load image node are wrong
UI fixes, not that important:
when the user selects the mask or paint tool, auto select the mask or image layer. It does not make sense for the user paint/mask without a selected layer
Thanks @trsommer. I think with the current infrastructure across the frontend, for the user to be able to edit the painting after closing out and re-opening the interface, the node has to have the images separate. @duckcomfy Changing the rendering of the LoadImage node doesn't seem to work well. Let me know if you have any other ideas. |
Thank you for your quick review @trsommer. |
@duckcomfy As it stands, in app.ts, we are pushing RGBCanvas and CombinedCanvas to node.imgs. This is important because (if I remember correctly) when the MaskEditor is initialized, it accesses its images and adds the relevant painted pixels to the paint layer. That functionality is, as far as I can see with the current infrastructure, the only way to allow for continued editing. We cannot leverage the mask’s logic because it would not work across all RGB colors. It is interesting that upon refresh of the page, the node defaults to the combined, flattened image. My guess is that it is displaying the last saved image in the input/clipspace folder. I’m happy to discuss on Discord for easier communication if you’d like (@batman.76) |
I gave this a quick try as well. Seems to work well enough and definitely solves a pain point with ComfyUI. I did find a bug: the brush shape seems to be stuck to circular. If I select the rectangular brush, it'll show a rectangular outline but still make circular marks. |
Also comes with a refactor of the associated code to be more DRY and maintainable. In order to explain the new UX let me first explain what I'll call the different tools, as the names we previously used are now ambiguous. - "Mask Pen": Previously named "Pen" internally, this is an existing tool lets you paint a mask on the image. - "Paint Pen": New tool which lets you paint colored pixels on the image. - "Eraser": Exising tool which lets you erase parts of your mask. As of @brucew4yn3rp's PR, you can also erase parts of your painted pixels, depending on which layer is active. - "Mask Bucket": Previously named "Paint Bucket" internally, this is an existing tool which lets you take a mask that's just an outline, and fill in the inside of your mask outline with a full masked area. Equivalent functionality for the Paint layer is NOT implemented. - "Mask Color Fill": Previously named "Color Select" internally, this is an existing tool which can turn pixels into a masked area, based on color similarity in contiguous pixels on the base image. Equivalent functionality for the Paint layer is NOT implemented. New UX: - The "Mask" layer is now selected when you open the Mask Editor, instead of no layer selected. - Selecting the "Mask Pen", "Mask Bucket", or "Mask Color Fill" tool automatically selects the Mask layer. - Selecting the "Paint Pen" tool automatically selects the Paint layer. - Selecting the "Mask" layer while the "Paint Pen" tool is selected automatically selects the "Mask Pen" tool. - Selecting the "Paint" layer while the "Mask Pen", "Mask Bucket", or "Mask Color Fill" tool is selected automatically selects the "Paint Pen" tool. - Changing the selected layer while the "Eraser" tool is selected has no additional effect, because Eraser supports both layers. - Switching to the "Eraser" tool has no additional effect, because Eraser supports both layers.
… 'Activate Layer' buttons are only visible when the Eraser tool is selected, for which they are used to discern which layer is being erased. Otherwise, the buttons are hidden and a blue border reflects which layer is currently active.
…ts & improve the padding consistency when that happens
fix: allow maskeditor right-pane to have a scrollbar on small viewports & improve the padding consistency when that happens
Thanks, @webfiltered and @PabloWiedemann. The vue-tsc version, reversions requested, and nits have been fixed. In terms of UX, please see a more comprehensive description of changes below. To preface, it is important to have the Activate Layer buttons specifically for the eraser tool. As you mentioned, it automatically updates the layer based on the selected tool, but when using the eraser tool, the user must be able to discern which layer they are erasing. For convenience, there is the added functionality of right clicking while using either paint or mask to erase on that layer. For better coherence of use, the Mask Blending options have been moved to their own container, similar to the Mask Opacity slider. Layer Interface Reorganization: The structure of the different layer containers is now standardized. There is a checkbox to hide/show the associated pixels, and an activation button for each of the mask and paint layers.
Activate Button Behavior:
Active Layer Visual Feedback:
Scrollbar
ComfyUI.Image.Canvas.mp4 |
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.
Looks pretty good! Have some follow-ups but nothing major. The biggest is a comment on app.ts
about node.imgs
.
Have also added suggestions which you can just click commit on if that works for you. If any tests fail because of them, feel free to ping and I can resolve.
…iewed images)" This reverts commit c6eb0ba.
…e their url from the main image url, which also ensures the layers can be opened even if the page is refreshed
refactor: address review items on main Image Editor PR
fix: add back change to ensure node preview is only the combined image
Hi @webfiltered, thanks for the attentiveness on this - I know there's lots going on. All of your comments have been addressed in the latest commits. We discussed offline a way to circumvent the need to store images within the node. Therefore, we no longer need the Originally, I had implemented the storing of images in Now, the timestamp has been unified across all the related files, allowing us to derive the names of each layer's saved file from the final filename that gets saved to the node/workflow. As a byproduct, this allows editability to persist through refreshes because deriving the filenames is static (as opposed to This seems to work well, and without the additional clog in retaining images within the node, which you mentioned may be a strain for future maintainability. To that end, @duckcomfy has also added a |
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
@brucew4yn3rp once your PR gets merged, would you please write into me at [email protected]? i'd love to compensate you as a thank you for your significant effort here! |
@trsommer
Revamped the MaskEditor UI and changed it to an Image Canvas. The context menu name has also been renamed accordingly.
Cleaner PR of 2921
The interface now supports drawing on the image itself (on the image layer) in addition to covering the area with a mask. Users can select a color or use an eyedropper tool for color matching. This is a powerful tool to guide the image generation process post-masking (e.g., inpainting). Speaking with ComfyUI users for months, this has been highly desired addition.
The tools on the left sidebar have been updated accordingly. The top one, is the existing mask brush, and has a matching mask logo. The second one is the image painting brush.
Some relevant context from the old PR
Layer buttons have been implemented. Also, right click will only apply to erasing whichever tool is activated. Additionally, the eraser tool will only erase on the active layer. Selecting the mask tool automatically activates the mask layer, and selecting the Paint tool automatically activates the Paint layer.
Thank you very much for your help in getting this over the line @duckcomfy. I had some issues with the prior PR so decided to close it and initiate a new, cleaner PR.