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 arbitrary crop module #1607

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rishabhshuklax
Copy link
Member

@rishabhshuklax rishabhshuklax commented Feb 1, 2020

Concerns #1080

Screenshot from 2020-02-01 16-37-15

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 npm run test-all
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
  • Insert-step functionality is working correct as expected.

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
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!

@rishabhshuklax
Copy link
Member Author

rishabhshuklax commented Feb 1, 2020

@tech4GT @jywarren please review this

Also could you suggest some other name for this module, I am really bad at this 😅

@codecov
Copy link

codecov bot commented Feb 1, 2020

Codecov Report

Merging #1607 into main will increase coverage by 9.64%.
The diff coverage is 61.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1607      +/-   ##
==========================================
+ Coverage   55.11%   64.76%   +9.64%     
==========================================
  Files         117      134      +17     
  Lines        2344     2784     +440     
  Branches      360      447      +87     
==========================================
+ Hits         1292     1803     +511     
+ Misses       1052      981      -71     
Impacted Files Coverage Δ
examples/lib/scopeQuery.js 18.51% <ø> (ø)
src/Modules.js 100.00% <ø> (ø)
src/modules/WebglDistort/Module.js 2.29% <0.00%> (ø)
src/ui/SetInputStep.js 12.90% <0.00%> (-1.39%) ⬇️
src/modules/ColorHalftone/Module.js 3.63% <3.63%> (ø)
examples/lib/defaultHtmlStepUi.js 11.35% <4.08%> (-0.85%) ⬇️
examples/lib/intermediateHtmlStepUi.js 11.11% <5.55%> (+0.94%) ⬆️
examples/lib/insertPreview.js 13.15% <20.00%> (-0.36%) ⬇️
src/util/getImageDimensions.js 20.00% <20.00%> (ø)
src/util/isGif.js 20.00% <20.00%> (ø)
... and 104 more

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.

Great work! 👍
But there's an issue here, we can't just crop in one step and then overlay in another, coz once you export the image you'll get the black borders again. Since the export would be rectangular. Anyway can you please run this via the cli and confirm what output are we getting. Otherwise we can just do this while overlaying. What do you think?


function changePixel(r, g, b, a, x, y) {

if(l1(x, y) * l3(x, y) <= 0 && l2(x, y) * l4(x, y) <= 0) return [r, g, b, a];
Copy link
Member

Choose a reason for hiding this comment

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

Hey is this the formula that determines whether the point is inside the quadrilateral?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes


// This function is called on every draw.
function draw(input, callback, progressObj) {
progressObj.stop(true);
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need these? I remember taking out the progress code..

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't exactly know, should I remove this?

@@ -0,0 +1,48 @@
{
"name": "arbitrary-crop",
"description": "Crops image for any arbitrary quadilateral",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a description about which way these coordinates go. Like top left, bottom right and so on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do that!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@rishabhshuklax
Copy link
Member Author

But there's an issue here, we can't just crop in one step and then overlay in another, coz once you export the image you'll get the black borders again. Since Images are rectangular only

@tech4GT I fixed that problem by making the alpha channel of other pixels to be 0, after this a meta module can be created for the problem in #1080 using this and overlay module, i've fixed the working of overlay module with transparent image in #1606, after this gets merged that problem can be easily solved by using this meta module

@rishabhshuklax
Copy link
Member Author

rishabhshuklax commented Feb 1, 2020

@tech4GT I am getting this as cli output.

image_1

These were the inputs

Screenshot from 2020-02-01 21-48-24

@rishabhshuklax
Copy link
Member Author

@tech4GT I've also added the functionality to crop the image so that the quadilateral lies in smallest possible rectangle so we don't have to compute for the extra pixels.

Screenshot from 2020-02-01 22-09-11

This is not showing desired output because extramanipulation is running before changePixel function and generating output according to it, this bug will be fixed in #1564 and it will work normally also I'd really appreciate if you could also review #1564

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.

Hey could you try running overlay after arbitrary crop in the cli. Sorry this is taking so long, just want to get this right! :D

@rishabhshuklax
Copy link
Member Author

@tech4GT I am getting this as cli output

image_2

This will be fixed when we merge #1606

To make overlay-subpart module we need to first add support for transparent image for overlay module then we need to develop arbitrary-crop and then the module described in #1080 can be easily made by making meta-module of arbitrary-crop and overlay! What do you think?

@tech4GT
Copy link
Member

tech4GT commented Feb 5, 2020

@tech4GT I am getting this as cli output

image_2

This will be fixed when we merge #1606

To make overlay-subpart module we need to first add support for transparent image for overlay module then we need to develop arbitrary-crop and then the module described in #1080 can be easily made by making meta-module of arbitrary-crop and overlay! What do you think?

Yeah, this is what I was worried about! Let's merge the other PR first then!

@rishabhshuklax
Copy link
Member Author

Yeah, this is what I was worried about! Let's merge the other PR first then!

Yes we need to first merge #1606 and then #1564 then finally this one

@gitpod-io
Copy link

gitpod-io bot commented Jul 7, 2020

@jywarren jywarren added this to the v3.7.0 milestone Oct 28, 2020
@harshkhandeparkar harshkhandeparkar removed this from the v3.7.0 milestone Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants