Skip to content
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

Add paint bucket module #556

Merged
merged 2 commits into from
Jan 22, 2019
Merged

Add paint bucket module #556

merged 2 commits into from
Jan 22, 2019

Conversation

Mridul97
Copy link

Fixes #541

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@Mridul97
Copy link
Author

Mridul97 commented Dec 25, 2018

I am not sure that is this the desired result, screenshot of the output:

ss_30

Please have a look!

Copy link
Member

@tech4GT tech4GT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mridul this looks great!! Maybe we can add a color option too?

@Mridul97
Copy link
Author

Yes, I will do that!

@Mridul97
Copy link
Author

Mridul97 commented Dec 26, 2018

@tech4GT I have made the required changes. Please have a look!

Screenshot :
ss_31

@harshkhandeparkar
Copy link
Member

@Mridul97 maybe sliders for rgb values will be better?

@Mridul97
Copy link
Author

Yes, we can do that but I think many times users have exact rgb values of the color they want and may just want to copy it!

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Dec 27, 2018

@Mridul97 you are right on that one. I also have another suggestion can you add an optional alpha value?

@Mridul97
Copy link
Author

@harshkhandeparkar yes I will do that! Thanks!

@Mridul97
Copy link
Author

I have made the changes. Please have a look!
screenshot :

ss_33

Copy link
Member

@tech4GT tech4GT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay this looks great! Mridul can you look into the conflicting dist files? Maybe you need to pull the latest code to rebase?

@Mridul97
Copy link
Author

Mridul97 commented Dec 28, 2018

I have rebased the branch! Please have a look!

@jywarren
Copy link
Member

jywarren commented Jan 3, 2019

Oh cool! But i'm not sure if the module is working properly - does it fill any polygon? Like this?

image

Maybe it just needs to be configured correctly? But this is very exciting -- thank you !!!!

@Mridul97
Copy link
Author

Mridul97 commented Jan 3, 2019

Oh! got it, I will fix this. Thanks!

@Mridul97
Copy link
Author

Mridul97 commented Jan 4, 2019

@jywarren How a user will specify a polygon using x, y coordinates of starting pixel to begin filling at and a color to fill with. I mean like in the above flower pic if a user enters starting pixel, how to decide when to stop filling?

@jywarren
Copy link
Member

jywarren commented Jan 4, 2019 via email

@Mridul97
Copy link
Author

Mridul97 commented Jan 4, 2019

Oh! got it. Thanks!

@Mridul97
Copy link
Author

@jywarren Please have a look!

Original Image :-
untitled

Output Image :-
ss_42

It is not filling it completely maybe we have to apply threshold that you talked about above! 😄

@jywarren
Copy link
Member

jywarren commented Jan 15, 2019 via email

@Mridul97
Copy link
Author

Mridul97 commented Jan 16, 2019

@jywarren It seems to work similarly on aliasing and anti-aliasing images. The left image is with aliasing and right one is with anti-aliasing. Please have a look!

screenshot 110

screenshot 111

screenshot 112

@jywarren
Copy link
Member

Hm, that's a great demonstration, thanks! But it looks like it is not really filling all the way. I wonder why? Compare to my comment above: #556 (comment)

package.json Outdated
@@ -29,6 +29,7 @@
"data-uri-to-buffer": "^2.0.0",
"downloadjs": "^1.4.7",
"fisheyegl": "^0.1.2",
"flood-fill": "^0.1.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting -- if we're including in the module here, why is the pretty complex code required below? Did the module not end up working for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, The module didn't work for this, it looks like the module works only for black and white images.

@harshkhandeparkar
Copy link
Member

Maybe we can add tollerance instead of using === directly?

if (pixels.get('same params', 0) > (r*(1 - tolerance/100)) && pixels.get('same params', 0) < (r*(1 + tolerance/100))){
  
}

Tollerance can be a percentage we can set by experimentation?

@Mridul97
Copy link
Author

Maybe we can add tollerance instead of using === directly?

if (pixels.get('same params', 0) > (r*(1 - tolerance/100)) && pixels.get('same params', 0) < (r*(1 + tolerance/100))){
  
}

Tollerance can be a percentage we can set by experimentation?

Nice! I think this is same as 'Threshold' that @jywarren talked about above, maybe we can start by keeping it 5%?

@harshkhandeparkar
Copy link
Member

I think testing it at 5% will be good. Also making a new function for testing that might be good.
Maybe

function isSimilarPixel(tolerance, other_params){
return bool;
}

@Mridul97
Copy link
Author

I think testing it at 5% will be good. Also making a new function for testing that might be good.
Maybe

function isSimilarPixel(tolerance, other_params){
return bool;
}

Yes, I was also thinking to do that! Thanks!

@Mridul97
Copy link
Author

After adding threshold of 5%, @jywarren Please have a look!

ss_46

@jywarren
Copy link
Member

jywarren commented Jan 18, 2019 via email

@Mridul97
Copy link
Author

@jywarren Yes, I will rebase the branch then It is ready to be merged! :)

@harshkhandeparkar
Copy link
Member

@Mridul97 this is amazing! But does this work with other images maybe with a bit more noise?

@Mridul97
Copy link
Author

@Mridul97 this is amazing! But does this work with other images maybe with a bit more noise?

I would love to test it, do you have any such image on which we can try this out? Thanks!

@harshkhandeparkar
Copy link
Member

I don't know if this is a good example

images 1.

Can the module fill in one of those polygons?

@harshkhandeparkar
Copy link
Member

Also what will happen if there is a dot in between?

3wrxj

@Mridul97
Copy link
Author

I got these results!

ss_47

ss_48

@harshkhandeparkar
Copy link
Member

I got these results!

ss_47

ss_48

The second one looka awesome but what happened to the first? Its pretty messy. Maybe the tolerance has to be set dynamically? Maybe we can do that in a followup?

@Mridul97
Copy link
Author

Sounds good! any idea how will we set tolerance dynamically maybe take from the user?

@harshkhandeparkar
Copy link
Member

I don't really know but maybe we can somehow average the noise in lets say a rectangle around the starting pixel?

@harshkhandeparkar
Copy link
Member

Maybe get the sum of r,g,b,a values of each pixel in the rectangle and average it and subtract the sum of r,g,b,a value of starting pixel. This seems really overkill though 😅😅

@Mridul97
Copy link
Author

Even I don't really know what would be right approach for this, lets think upon it on a follow-up PR! Thanks!

@Mridul97
Copy link
Author

@jywarren I have rebased the branch! :)

@harshkhandeparkar
Copy link
Member

Or maybe the fastest way would be to subtract the r,g,b,a values of starting pixel from r,g,b,a values of the pixel adjacent to it and get a percentage for each value(i.e. r,g,b,a) then average that percentage? This might give us inconsistent and inaccurate results though.

@Mridul97
Copy link
Author

Mridul97 commented Jan 18, 2019

Or maybe the fastest way would be to subtract the r,g,b,a values of starting pixel from r,g,b,a values of the pixel adjacent to it and get a percentage for each value(i.e. r,g,b,a) then average that percentage? This might give us inconsistent and inaccurate results though.

But adjacent pixel could be same also, should we really keep the tolerance depending upon an adjacent pixel? Lets wait to hear @jywarren thoughts on this! :)

@harshkhandeparkar
Copy link
Member

But adjacent pixel could be same also,

Ya, thats why I said that it may be inconsistent. Lets hear it from @jywarren

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These suggestions might be helpful?

src/modules/PaintBucket/PaintBucket.js Show resolved Hide resolved
function isSimilar(currx, curry){
if (pixels.get(currx, curry, 0) > r*(1 - tolerance/100) && pixels.get(currx, curry, 0) < r*(1 + tolerance/100) &&
pixels.get(currx, curry, 1) > g*(1 - tolerance/100) && pixels.get(currx, curry, 1) < g*(1 + tolerance/100) &&
pixels.get(currx, curry, 2) > b*(1 - tolerance/100) && pixels.get(currx, curry, 2) < b*(1 + tolerance/100) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also maybe you can assign 1-tolerance/100 and 1+tolerance/100 to different vars?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the changes. Thanks!

@jywarren
Copy link
Member

jywarren commented Jan 18, 2019 via email

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

@Mridul97
Copy link
Author

@jywarren I have added tolerance as an user input, Please have a look!

ss

@jywarren
Copy link
Member

This looks fantastic. I'd like to request two follow up issues be opened: one to make the threshold range from 0-100, and another to propose an input type called something like "pixel" which prompts the user to click on the image and returns an xy coordinate. What do you think? We could use a crosshairs icon on a button and have a label like "Paint bucket starting point: click to select a pixel"

@jywarren jywarren merged commit 4fd814d into publiclab:main Jan 22, 2019
@jywarren
Copy link
Member

Great work on this! It might be an interesting one to write a "looks-like" test for too - a circle before and after filling could make a good comparison!

@Mridul97
Copy link
Author

@jywarren Sounds great! An improved UI will allow the user to use the module more efficiently as it is difficult for the user to enter the coordinates of the desired pixel!

Sorry, actually didn't quite understand the "looks-like" test, could you please elaborate. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants