Fix generateOutput called multiple times

[rishabhshuklax]

Concerns this Fixes #1575 Fixes #1515 Fixes #1457 Fixes #1532 (this)

Note: This PR is a work-in-progress

This PR fixes generateOutput called multiple times when extramanipulation is used.

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

• [x] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm run test-all
• [x] code is in uniquely-named feature branch and has no merge conflicts
• [x] PR is descriptively titled
• [x] ask @publiclab/is-reviewers for help, in a comment below
• [x] 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]

This PR is work in progress, At the moment Resize, Crop & Constrained Crop module doesn't work correctly unless used with a setTimeout to generateOutput, This PR solves problem of QR not generating after merging #1532 and allows text-overlay module to work correctly in Node #1518 (finally!!!!)

I appreciate all the help I can get because I am stuck now :(

@jywarren @HarshKhandeparkar @publiclab/is-reviewers

[rishabhshuklax]

I removed all the instances of setTimeout and added changePixel method to the modules, this makes them work without a problem, but I don't know why this is happening, for some reason code is async and when changePixel is added it runs after extramanipulation(which it shouldn't) and generates output, so if generate output is even removed from extramanipulation it would work fine, This type of behaviour shouldn't be in our code, this should work fine even when changePixel is not added!! And why is extraManipulation even called before changePixel? https://github.com/publiclab/image-sequencer/blob/f6d53163b92b8678903956647cddf65b020e0b56/src/modules/_nomodule/PixelManipulation.js#L235-L245

This piece of code seems to be the most likely culprit, so i set useWasm option on histogram to false and this was the output

:confused: The reason to this seems to be this: https://github.com/publiclab/image-sequencer/blob/f6d53163b92b8678903956647cddf65b020e0b56/src/modules/_nomodule/PixelManipulation.js#L279-L283 I think we should add a more if block here like this:

if(!options.extraManipulation)
            generateOutput();

This works fine!!

@HarshKhandeparkar you were working on #1515? have you found the solution to this?? We need to take care of that async code to fix this problem!

[rishabhshuklax]

Ok, so i changed pixelManipulation such that generateOutput will be called only once and extraManipulation will be called only after changePixel. But, as of now resize and crop modules are not working expectedly, I'm trying to figure out why.

[rishabhshuklax]

At the moment these modules are not working

• ~~canvas-resize (this is also not working on beta site)~~
• ~~colorbar(will be fixed when crop module is fixed)~~
• constrained-crop(will be fixed when crop module is fixed)
• ~~crop~~
• resize
• rotate

[rishabhshuklax]

Canvas Resize is now fixed :tada: and I've also added an option to change the background color of the new canvas :smile:

[rishabhshuklax]

Crop Module is also fixed :tada:

So, since crop is fixed hence colorbar is also fixed there is minor change to be made in constrained crop and it will be fixed too :smile:

[rishabhshuklax]

AddQr test was failing because it's benchmark was wrong, probably changed in #1550, I've updated it, It should pass now!

[rishabhshuklax]

All modules are working fine now :tada:

[codecov[bot]]

Codecov Report

Merging #1564 (10322c3) into main (853e719) will decrease coverage by 0.21%. The diff coverage is 62.84%.

@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
- Coverage   66.67%   66.46%   -0.22%     
==========================================
  Files         130      132       +2     
  Lines        2686     2821     +135     
  Branches      433      440       +7     
==========================================
+ Hits         1791     1875      +84     
- Misses        895      946      +51     

Impacted Files	Coverage Δ
src/Modules.js	100.00% <ø> (ø)
src/modules/Crop/Crop.js	100.00% <ø> (ø)
src/modules/WebglDistort/Module.js	2.29% <0.00%> (ø)
src/modules/ColorHalftone/Module.js	3.63% <3.63%> (ø)
src/modules/BlobAnalysis/Module.js	25.00% <25.00%> (-5.77%)	:arrow_down:
src/modules/_nomodule/PixelManipulation.js	42.74% <53.84%> (+2.06%)	:arrow_up:
src/modules/Average/Module.js	100.00% <100.00%> (ø)
src/modules/Blur/Module.js	100.00% <100.00%> (ø)
src/modules/CanvasResize/Module.js	100.00% <100.00%> (+12.50%)	:arrow_up:
src/modules/ColorHalftone/index.js	100.00% <100.00%> (ø)
... and 25 more

[rishabhshuklax]

@publiclab/is-reviewers @jywarren @HarshKhandeparkar please review this!!

[rishabhshuklax]

@jywarren could you please take a look at this!

[jywarren]

Wow this is epic! Uh, i guess I'd like to get reviews from others too - and, it looks like tests are failing, no? Thank you!

[rishabhshuklax]

Wow this is epic! Uh, i guess I'd like to get reviews from others too - and, it looks like tests are failing, no? Thank you!

Tests are failing for Gifs(canvas-resize), I'll fix that in a follow up, I'll need @HarshKhandeparkar's help, since this PR changes lot of files so I am stuck solving merge conflicts most of the time :sweat_smile:

[jywarren]

Ah, we can slow down merges a little bit while you wrap it up if you don't think it'll be long? Sorry about that, it's tough! Thank you!

We will need to pass all tests to merge it, though. Let's all pitch in and we'll hopefully get this sorted quick! Thanks again!

[rishabhshuklax]

Only Canvas-Resize is failing as of now(and that too because I changed a bit how its implemented and added a new feature link so that only needs a benchmark update) I think edge-detect is not working correctly for some reason, I have already opened a issue for that #1576 maybe it happened in one the PRs where GPU.js was bumped? At that time tests for Gifs were not there hence we weren't able to see it, I'll try to solve this as soon as possible, Thanks!

[rishabhshuklax]

@jywarren everything is fixed now :tada: :smile:

[rishabhshuklax]

@jywarren can we get this merged soon so as to avoid any merge conflicts as this changes a lot of files!

[jywarren]

Yes, thank you! Almost there!!!

[harshkhandeparkar]

This one is slightly outdated but looks like it can be merged.

[gitpod-io[bot]]

[rishabhshuklax]

@HarshKhandeparkar It is rebased review it again. @jywarren can we get this merge soon as this solves a really big bug, thanks!

[jywarren]

Hi all - merging https://github.com/publiclab/image-sequencer/pull/1546 next but let's try to get this merged after that? My apologies for this needing rebases so many times. I'll mark this as a priority and we can aim to have this be the next merge to avoid any more conflicts. How does that sound, @publiclab/is-maintainers @publiclab/is-core-maintainers ?

[jywarren]

Gosh @blurry-x-face a ton of your PRs are finally getting merged! Your work is dominating the upcoming release: https://github.com/publiclab/image-sequencer/projects/4

[jywarren]

Hi @HarshKhandeparkar hope you're well, did you have a chance to look at my suggestions? If you agree we could go ahead and accept them...

[harshkhandeparkar]

I am fine, ty! How are you?

I did go through the suggestions but I was waiting for Rishabh. Do you think we should just commit those and merge or wait for him?

[rishabhshuklax]

Hi @jywarren @HarshKhandeparkar I was a bit busy with university, I'll fix the PR ASAP, ty :)

[rishabhshuklax]

@HarshKhandeparkar tests for edge detect are failing for some reason do you have any idea?

[harshkhandeparkar]

Can you upload the expected and generated outputs?

[rishabhshuklax]

@HarshKhandeparkar

What I am getting: What is expected:

Should I just update the benchmark?

[harshkhandeparkar]

New output has slightly thicker edges at some places. Maybe some defaults changed or some options are wrong?