-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Sketching + Inpainting Capabilities to Gradio #2144
Conversation
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2144-all-demos |
* fix scaling on sketch + bg img * tweaks * ketch updates * cursor style
As discussed with @pngwn, |
@pngwn -- copying over from Slack for visibility: I was testing the sketch/painting PR, and it seems like our old Sketchpad isn't working anymore. Specifically, it used to be that something like this:
Or using string shortcuts, something like:
would create a black and white sketchpad (e.g. for handwritten recognition) However, if I run this code now, I get nothing showing up and a bunch of JS errors in the console: We should confirm that all 5 of the use cases mentioned in the issue work before merging this in. |
@abidlabs This should be ready for review now. I made to make some changes to I've added more demos to There is currently a discrepancy between the Do |
Awesome! Will go through these. The shortcuts do exist, but right now as |
This looks really fantastic @pngwn! I updated the (1) There are two cases in which the behavior is weird, and that is if (2) As you noted, I made some other changes:
If we can fix point (1), then I think this is good to merge! |
I'm not sure anyone will actually use this. Although it doesn't do much harm, it does add another option which makes the docs more overwhelming. I think it would be okay to leave it as is and potentially add a flag when we do make any changes. We can make a hard break from some of these APIs in 4.0, likewise we could just add the flag and remove in 4.0 when we clean up some of the API. I'll take a look at the webcam thing, it is a little strange but I think I know what is causing it, however, I'm not sure how easy it will be to fix because we don't have a flipped version of the image on the frontend, and if we do flip on the frontend we will need to make we don't flip on the backend as well. Will take a look at this today. |
Sounds good, I actually felt similarly (that it was adding too many
options) and didn’t end up pushing the force_dict (forgot to edit the
GitHub comment).
…On Fri, Sep 16, 2022 at 12:24 AM pngwn ***@***.***> wrote:
I added a parameter in the component called "force_dict". If set to True,
it preprocesses the data in all cases to be a dictionary with keys image
and mask. I hope this is not more confusing to users.
I'm not sure anyone will actually use this. Although it doesn't do much
harm, it does add another option which makes the docs more overwhelming. I
think it would be okay to leave it as is and potentially add a flag when we
do make any changes. We can make a hard break from some of these APIs in
4.0, likewise we could just add the flag and remove in 4.0 when we clean up
some of the API.
I'll take a look at the webcam thing, it is a little strange but I think I
know what is causing it, however, I'm not sure how easy it will be to fix
because we don't have a flipped version of the image on the frontend, and
if we do flip on the frontend we will need to make we *don't* flip on the
backend as well. Will take a look at this today.
—
Reply to this email directly, view it on GitHub
<#2144 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANSE6OWNIPRRVUOJ3IKDELV6QOBDANCNFSM6AAAAAAQBTLZXU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The @abidlabs What do you mean by this? Why do we need a class just for MNIST demos (toy examples)
The one thing that tripped me up is that you have to click on the dropper icon to change color. I would think that you can just change color by clicking anywhere on the color spectrum. |
Yeah this is a remnant from the early days of Gradio, in the early days we wanted to show how easy it was to get started with Gradio, so we had a shortcut ("sketchpad") specifically for MNIST models. We should have dropped in Gradio 3.0, but forgot. I think we are going to need to keep this now for backwards compatibility reasons, but we can file an issue to drop it in 4.0.
Nice catch @freddyaboulton! |
Noticed a bug --> if you use the ImagePaint mode (i.e. source="upload", tool="color-sketch"), then after you upload an image and start painting, you have no way of clearing and uploading a new image. If you clear the image, it becomes like a regular color-sketch component and you cannot upload an image. Also @pngwn do you know if this PR fixes #1961? It might be worth including that fix in this PR while we're working on the sktechpad component |
In addition, I find the brush behavior unintuitive in one regard: that you have to move your cursor outside of the entire brush circle in order to get the brush to move. Instead, the center of the brush should just track the cursor. |
Additional feedback from users:
+1 on the eraser suggestion |
Hello ! I got some new observations to report, i'll go straight to the points:
Solution: add a clean canvas button on the right to only clean drawings layer, not background layer containing the uploaded img
That's it, for the moment ;) |
For the eraser feature, i made a good working one for my animation app, using p5js
You can take a look on my work here : https://editor.p5js.org/fffiloni/sketches/LPSfkKlQ6 And if you plan to add an animation gradio block in the future, i would be glad to help ! :) |
Additional feedback from @hysts (from Discord)
|
I've updated this issue to gather feedback on any changes that are out of scope for this PR, would be good to get everyone's thoughts: #466 |
Pushed some changes.
The gradio "Clear" button at the bottom doesn't work as intended for all sketches, i'm looking into this as well as the issue with Tabs, I think they are related. |
Hi, thanks for working on this! To add to @abidlabs' comment on the brush behavior: it is indeed a bit unintuitive that the cursor does not track the center of the brush. It makes it particularly hard to go over the borders or even the icons/buttons from the tool itself (i.e. the color or brush size picker), since once the cursor is off the canvas the painting stops and we need a new click inside the canvas to continue. Looking forward to testing the evolutions on this PR :) |
@eduardocarvp I agree, the 'lazy brush' behaviour was originally designed to help draw smoother curves with a mouse but it might be too lazy but should probably be toggleable from the gradio API or GUI. I have listed the ability to disable/ toggle this feature in #466 as it is out of scope for this PR. |
This PR should be ready for final testing now. |
Although this PR doesn't address every single feature requested in the linked issues above, I've created a new issue to track feature requests for the Image component, so they can all be closed with this PR (which addresses most of the requests anyway). |
This is really nice @pngwn! I tested the A) the first time you use a sketchpad or a color-sketchpad, it tends to add a gray background to your sketch: When I would click the submit button, I found that the output would be all black: If you clear the sketchpad and start drawing again, the sketchpads work fine. -- B) 5(a) and 5(b) have the opposite problem: the first time, they work fine with an image. But after you clear the first image, they no longer correctly upload an image. If you try to upload an image again, it will just flicker and then become all white: -- C) The example in 3(b) doesn't seem to work (nothing happens when you click Submit). 3(a) works fine though. Haven't noticed any other bugs while testing! |
I can't reproduce the issues with 2a locally: 3b was broken because there was another instance of it on the page (#2320) i've fixed the demo. Undo was buggy, fixed that now i think. Will push to see if there was an issue with the spaces deploy becaus egetting different results locally + in spaces isn't ideal. Looking into 5a + 5b. |
I'm getting different results locally vs the spaces deploy, and I'm not sure what is going on. It could be the tabs the deployed demos are embedded in or it could be something related to the environment. Not sure. |
5a + 5b should work okay now. Still not sure how to repro the issue with transparent canvases. |
I've removed the lazy brush as well but kept some degree of path smoothing. Should be a little more intuitive now. |
Confirming that, besides the aforementioned issue with 2(a)/2(b)/4(a)/4(b), all other issues look good! |
🥳 LGTM |
Hello, All of this is going in a great direction. Do you plan to add full interactivity support? What I mean is interacting not only with the input image, but also with an output image that can be resubmitted. It could be useful for interactive image segmentation demos (between other research fields that have "interactive" in their name). The hardest to fulfill requirement is to maintain a state from one user interaction to the next. Should I open a new issue to discuss this? |
What use cases does the Image component need to serve?
On the Python side, users want…
This PR adds support for all of these (plus a few minor ones like webcam + mask/sketch). To see all of the different ways the
Image
component can now be used, go to: https://huggingface.co/spaces/gradio-pr-deploys/pr-2144-all-demos and click onblocks_mask
or use the beta releasegradio==3.4.0b
to testThe
blocks_mask
demo has the code snippets for all of the different modes. For example, for image upload + color sketching, you would something like:Fixes: #1721
Fixes: #2060
Fixes: #2124
Fixes: #2030
Fixes: #1174
Fixes: #2312
Fixes: #2224
Fixes #2295