Fixes crop module bug #1019

Divy123 · 2019-04-16T19:04:33Z

Fixes #987

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 test
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

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!

codecov · 2019-04-16T19:13:18Z

Codecov Report

Merging #1019 into main will increase coverage by 2.89%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #1019      +/-   ##
==========================================
+ Coverage   34.08%   36.98%   +2.89%     
==========================================
  Files          97       99       +2     
  Lines        1778     1836      +58     
  Branches      278      287       +9     
==========================================
+ Hits          606      679      +73     
+ Misses       1172     1157      -15

Impacted Files	Coverage Δ
examples/lib/defaultHtmlStepUi.js	`14.47% <0%> (ø)`	⬆️
src/modules/Crop/Crop.js	`100% <100%> (ø)`
src/util/ParseInputCoordinates.js	`73.33% <0%> (ø)`
src/modules/Crop/Module.js	`91.3% <0%> (+82.6%)`	⬆️

Divy123 · 2019-04-16T19:14:50Z

@jywarren please review.
I will be adding UI tests once its ok to you.

Divy123 · 2019-04-16T19:42:14Z

Divy123 · 2019-04-16T19:42:45Z

@Divy123 please have a look!!

jywarren · 2019-04-16T19:56:03Z

Oh this looks good; can you test out the exact reproducible crop parameters from the original issue? Thank you!

…

On Tue, Apr 16, 2019 at 3:43 PM Slytherin ***@***.***> wrote: @Divy123 <https://github.com/Divy123> please have a look!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ7czxXIvbt_rsaXnnxGE4OP36GiOks5vhie2gaJpZM4czgAY> .

Divy123 · 2019-04-16T20:08:58Z

If I am getting it correct, it means to have a UI test with these coordinates as input:

@jywarren

jywarren · 2019-04-17T14:39:38Z

Ah, sorry, actually the even older one, but it runs on the default image: #213 This had been reliably reproducing the error: https://sequencer.publiclab.org/examples/#steps=crop{x:127|y:127|w:266|h:266} However, now maybe we are getting a new error, unfortunately... i'll file this separately, but i see `Uncaught TypeError: Cannot read property 'description' of undefined` when i use this URL.

…

On Tue, Apr 16, 2019 at 4:09 PM Slytherin ***@***.***> wrote: If I am getting it correct, it means to have a UI test with these coordinates as input: [image: Screenshot from 2019-04-17 01-37-46] <https://user-images.githubusercontent.com/31225007/56240584-8a765700-60b1-11e9-88be-f438447aa1a8.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJyZNPg1uC3UirCh7Ag5AuIzTWgIaks5vhi3agaJpZM4czgAY> .

jywarren · 2019-04-17T14:43:32Z

OK, it was a cache issue related to #605 #953 -- now there's no error; however, it doesn't crop:

In any case this should be fixed by your code, no? Can you try these dimensions specifically? Thank you so much!!!

jywarren · 2019-04-17T14:44:48Z

And, shall we try adding a module test to protect this feature in the future? We could do a simple looks-like comparison like this:

image-sequencer/test/core/modules/contrast.js

Lines 27 to 40 in 6fa8b1b

    
           test('Contrast module works correctly', function(t) { 
        
             sequencer.run({mode:'test'}, function(out) { 
        
               var result = sequencer.steps[2].output.src 
        
               base64Img.imgSync(result, target, 'result') 
        
               base64Img.imgSync(benchmark, target, 'benchmark') 
        
               result = './test_outputs/result.png' 
        
               benchmark = './test_outputs/benchmark.png' 
        
               looksSame(result, benchmark, function(err, res) { 
        
                 if (err) console.log(err) 
        
                 t.equal(res.equal, true) 
        
                 t.end() 
        
               }) 
        
             }) 
        
           })

vibhorgupta-gh · 2019-04-17T14:46:31Z

@jywarren

OK, it was a cache issue related to #605 #953

I'm not so sure. I was still able to reproduce this, on certain values of x and y. I'll recheck and post back here. I was looking into #213 and it's pretty confusing as to why the image is being altered. I'll try and see if it indeed is just a cache issue.

harshkhandeparkar · 2019-04-17T14:53:00Z

Maybe it is because of the util functions which are included. I think this only occurs when the offset values are included

Divy123 · 2019-04-17T14:58:26Z

In any case this should be fixed by your code, no? Can you try these dimensions specifically? Thank you so much!!!

There are some issues related to crop module involving the way pixels are manipulated that's why these problems are arising.
Doing some changes here.

And, shall we try adding a module test to protect this feature in the future? We could do a simple looks-like comparison like this:

Sure @jywarren!!

Divy123 · 2019-04-17T15:02:54Z

@harshkhandeparkar which of the util functions are you talking about?
What I am able to see this error is happening when x coordinates are changed.

When we change y ones, there are no errors its perfectly working.

harshkhandeparkar · 2019-04-17T15:07:54Z

Probably parseCornerCoordinates. I don't know why I feel like the util func is a prob but I do.

Divy123 · 2019-04-17T15:13:13Z

To this, actually the problem existed even before the utility was added. You can refer #213 is an year old issue. Also the func only generates the exact coordinates if user enters in %. The code you can see and is pretty simple. I don't think there might be a bug. @harshkhandeparkar if you think there is can you point out please towards the line where it can be.

jywarren · 2019-04-17T15:15:55Z

Sorry, to clarify:

OK, it was a cache issue related to #605 #953

I meant that the js error was a cache issue, not the color problems in the crop module. Thanks!

jywarren · 2019-04-17T15:17:15Z

And there are some good clues on what's causing the color issue... sometimes it's missing a blue channel, sometimes a red channel! So, it seems likely due to a 4-color-channel pixel manipulation error of some kind.

Divy123 · 2019-04-17T15:17:16Z

@harshkhandeparkar this is the o/p as seen after removing the utility function. I think there is something other to it.

Divy123 · 2019-04-17T15:18:22Z

@jywarren the problem is arising when we change the 'x' coordinates and not in case of y coordinates.
What d you think can be a probable reason for this?

vibhorgupta-gh · 2019-04-17T15:20:46Z

Yes, @Divy123 I noticed the same in the afternoon. Have been trying to figure it out. I tried looking for issues with the jquery plugin itself, couldn't find one pertaining to this.

@jywarren thoughts on using some other well-maintained library for rewriting the module?

Divy123 · 2019-04-17T15:23:15Z

I don't think this is related to imageareaselect. There is somewhat missing in pixel manipulation.
Figuring it out, will try to come up with a solution soon.

harshkhandeparkar · 2019-04-17T15:25:58Z

@Divy123 no I don't have any doubts. Wait I'll look into the rest of the code in a min.

Sent from my OnePlus3 using FastHub

harshkhandeparkar · 2019-04-17T15:29:42Z

Ah look at this file https://github.com/publiclab/image-sequencer/blob/main/src/modules/Crop/Crop.js
It creates a new unit8Array and saves the pixels itself. This is an old module. I think we should try using the extraManipulation func of PixelManipulation API.

harshkhandeparkar · 2019-04-17T15:30:13Z

@Divy123 it doesn't even use PixelManipulation properly.

Sent from my OnePlus3 using FastHub

Divy123 · 2019-04-17T15:41:52Z

@harshkhandeparkar It does not matter if it uses old or new functionality, it should be correct.

Looking into it now.

Thanks!!

harshkhandeparkar · 2019-04-17T15:52:43Z

Maybe the older functionality didn't include the 4th channel, alpha. Anyways, updating to the latest is to be done in any case.

Divy123 · 2019-04-17T21:42:34Z

@jywarren working as desired now!!
Please have a look!!
Will add the test if it's ok to you!!

jywarren · 2019-04-17T22:44:37Z

oh this is super. Yes! a test would be wonderful!!!

…

On Wed, Apr 17, 2019 at 5:43 PM Slytherin ***@***.***> wrote: [image: ezgif com-video-to-gif] <https://user-images.githubusercontent.com/31225007/56322821-ab10e080-6187-11e9-9a21-91fdfa66432d.gif> @jywarren <https://github.com/jywarren> working as desired now!! Please have a look!! Will add the test if it's ok to you!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J6DN34S4IGQKCEBCHTPQ6KUVANCNFSM4HGOAAMA> .

harshkhandeparkar · 2019-04-18T03:27:34Z

What was the problem @Divy123.

Divy123 · 2019-04-18T10:22:04Z

@harshkhandeparkar there were a ceratin number of problems:

We were using incorrect parent of input
Somehow the css for imageareaselect was missing
Input was not triggered by change in imageareaselect
The pixels were manipulated incorrectly. I mean the logic used was partially incorrect.

vibhorgupta-gh · 2019-04-18T10:49:10Z

@Divy123 great catch :D

harshkhandeparkar · 2019-04-18T11:20:44Z

@Divy123 why don't you update this to use PixelManipulation API? That will also make it future proof.

harshkhandeparkar · 2019-04-18T11:20:57Z

And a lot simpler to read and manage.

Divy123 · 2019-04-18T11:29:39Z

I think restructuring can be made in another PR of all those modules that run on older modules.
Thanks !!

Divy123 · 2019-04-18T11:31:31Z

The code-cov increases by 2.89% here after adding the test.
Hurraayy 🎉 🎉
@jywarren please have a look!!

jywarren · 2019-04-18T11:40:49Z

Awesome!!!

jywarren · 2019-04-18T11:40:57Z

Great work!!!

Fixes crop module bug

80e53b4

Divy123 added the work-in-progress label Apr 17, 2019

Fixes crop functionality

5a70c55

vibhorgupta-gh requested a review from jywarren April 18, 2019 10:49

Add looks like test for crop module

6c6504c

jywarren merged commit 2be7a3d into publiclab:main Apr 18, 2019

jywarren mentioned this pull request Apr 18, 2019

Colorimetry plan #979

Open

vibhorgupta-gh mentioned this pull request Apr 19, 2019

Crop Module not working correctly #213

Closed

Fixes crop module bug #1019

Fixes crop module bug #1019

Conversation

Divy123 commented Apr 16, 2019 • edited by vibhorgupta-gh Loading

codecov bot commented Apr 16, 2019 • edited Loading

Codecov Report

Divy123 commented Apr 16, 2019

Divy123 commented Apr 16, 2019

Divy123 commented Apr 16, 2019

jywarren commented Apr 16, 2019 via email

Divy123 commented Apr 16, 2019 • edited Loading

jywarren commented Apr 17, 2019 via email

jywarren commented Apr 17, 2019

jywarren commented Apr 17, 2019

vibhorgupta-gh commented Apr 17, 2019 • edited Loading

harshkhandeparkar commented Apr 17, 2019

Divy123 commented Apr 17, 2019

Divy123 commented Apr 17, 2019

harshkhandeparkar commented Apr 17, 2019

Divy123 commented Apr 17, 2019

jywarren commented Apr 17, 2019

jywarren commented Apr 17, 2019

Divy123 commented Apr 17, 2019

Divy123 commented Apr 17, 2019

vibhorgupta-gh commented Apr 17, 2019

Divy123 commented Apr 17, 2019

harshkhandeparkar commented Apr 17, 2019

harshkhandeparkar commented Apr 17, 2019

harshkhandeparkar commented Apr 17, 2019

Divy123 commented Apr 17, 2019

harshkhandeparkar commented Apr 17, 2019

Divy123 commented Apr 17, 2019

jywarren commented Apr 17, 2019 via email

harshkhandeparkar commented Apr 18, 2019

Divy123 commented Apr 18, 2019

vibhorgupta-gh commented Apr 18, 2019

harshkhandeparkar commented Apr 18, 2019

harshkhandeparkar commented Apr 18, 2019

Divy123 commented Apr 18, 2019

Divy123 commented Apr 18, 2019 • edited Loading

jywarren commented Apr 18, 2019

jywarren commented Apr 18, 2019

Divy123 commented Apr 16, 2019 •

edited by vibhorgupta-gh

Loading

codecov bot commented Apr 16, 2019 •

edited

Loading

Divy123 commented Apr 16, 2019 •

edited

Loading

vibhorgupta-gh commented Apr 17, 2019 •

edited

Loading

Divy123 commented Apr 18, 2019 •

edited

Loading