debkbanerji / lego-art-remix

Powerful computer vision assisted Lego mosaic creator ยท Over 1 million images created (so far!)
https://lego-art-remix.com
GNU General Public License v3.0
275 stars 66 forks source link

Fix synchronous steps #96

Open twjasa opened 1 year ago

twjasa commented 1 year ago

Changes:

debkbanerji commented 1 year ago

Thanks for looking into this!

I like the idea of cleaning up the setTimeouts while possible but I do have some questions. (For context, many of these exist because browsers handles drawing of images in a way where it's difficult to detect completion. The setTimeouts are a simple-ish solution but are obviously kind of hacky)

  1. What is the purpose of the skipStep2 flag in initializeCropper? Why are we running step 2 within this and then not running this after step 1? There are no guarantees about how long the browser may take to draw stuff or how the cropper works under the hood, so this could lead to a painful race condition down the line. (in the pull request code, we're essentially running step 1 after step 2 is run in the initializeCropper function, which seems like it could cause issues)
  2. When removing all of these setTimeouts, how do we know that the browser will finish drawing the pixels to the canvases? (we're reliant on waiting a split second so we can read them back in the next steps)
  3. Overall, how did you test these changes? (making sure the right steps respond to the right setting changes, etc.)
twjasa commented 1 year ago

Hey @debkbanerji no problem ๐Ÿ˜„ Thanks to you for this tool and for the response.

  1. I noticed that I was a bit confused about the flow of the app ๐Ÿ˜…. Now I think I have a clearer understanding of how is working, I removed all flags.
  2. I removed the call of runStep1 in handleInputImage and leave initializeCropper (which calls runStep1 -> runStep2->runStep3....) inside of inputImage.onload and that did the trick. Also because now all steps are running one after another I deleted some steps that were repeated.
  3. I basically test it in all browsers (except safari) with these steps:
    • Load image
    • check that all steps are fine
    • move the crop
    • check again
    • resize crop
    • check again
    • move resolution
    • check again I did not check the depth feature. After these changes I noticed that also the performance improved a lot. Check this out (left is fix-synchronous-steps and right is master):

https://user-images.githubusercontent.com/12849040/204348765-2620b046-b543-4066-8368-3f56be4a1efa.mp4

twjasa commented 1 year ago

bump :(