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

Mockup speed enhancement #123

Merged
merged 17 commits into from
Aug 30, 2024
Merged

Conversation

YayunHuang
Copy link
Contributor

@YayunHuang YayunHuang commented Aug 28, 2024

@YayunHuang YayunHuang requested a review from pkong-ds August 28, 2024 09:31
@YayunHuang YayunHuang assigned pkong-ds and unassigned pkong-ds Aug 28, 2024
@YayunHuang
Copy link
Contributor Author

@pkong-ds I'm still removing some scripts that are no longer used, but I think you can see if there are any big issues with this update first

@YayunHuang YayunHuang force-pushed the 179-mockup-speed-enhancement branch from d723a25 to 60f9213 Compare August 28, 2024 09:50
@YayunHuang YayunHuang force-pushed the 179-mockup-speed-enhancement branch from 60f9213 to 9958292 Compare August 28, 2024 09:52
@YayunHuang YayunHuang changed the title [WIP] Mockup speed enhancement Mockup speed enhancement Aug 28, 2024
Copy link
Collaborator

@pkong-ds pkong-ds left a comment

Choose a reason for hiding this comment

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

Thanks, seems many comments. Can huddle if any misunderstanding

public/scripts/mockup_worker.js Outdated Show resolved Hide resolved
public/scripts/mockup_worker.js Outdated Show resolved Hide resolved
@@ -72,68 +33,93 @@ function getMaxWorkers() {
return navigator.hardwareConcurrency;
}

function runPreviewWorker(worker, imageUpload) {
function runWorker(worker, imageUpload, orientationIndex, mode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the API still be runPreviewWorker and runWorker as 2 function?

That way the logic is contained inside each function, and no need to if (preview) else (generate)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same logic - can we also refactor as functions like onPreviewError, onPreviewSuccess?

Even better - different files for different complex logic

I planned to separate this holy 1000-line file into multiple files yesterday, but realized

  • need to move upload.js into src/pages first ; which means
  • need to change all CDN into package :sadfrog:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkong-ds yes, I think it's better if we can do this after changing all CDN into package, and event typescript

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per offline discussion, let's do the refactoring while upload.js is moved

public/scripts/upload.js Outdated Show resolved Hide resolved
public/scripts/upload.js Outdated Show resolved Hide resolved
public/scripts/upload.js Outdated Show resolved Hide resolved
public/scripts/upload.js Show resolved Hide resolved
public/scripts/upload.js Outdated Show resolved Hide resolved
public/scripts/upload.js Outdated Show resolved Hide resolved
mockup_package/mockup/image.py Outdated Show resolved Hide resolved
@YayunHuang YayunHuang force-pushed the 179-mockup-speed-enhancement branch from e21d973 to 0d4190b Compare August 29, 2024 04:46
@YayunHuang YayunHuang force-pushed the 179-mockup-speed-enhancement branch from e74aea9 to a6d94ac Compare August 29, 2024 08:11
@YayunHuang YayunHuang force-pushed the 179-mockup-speed-enhancement branch from f3b9d6e to 23d9fcd Compare August 29, 2024 09:48
@YayunHuang
Copy link
Contributor Author

@pkong-ds all updated

@pkong-ds pkong-ds force-pushed the 179-mockup-speed-enhancement branch from e4bcf46 to 1bbb947 Compare August 29, 2024 19:17
@pkong-ds
Copy link
Collaborator

Thanks for addressing all the comments 👍 I pushed 2 commits to fix some minor stuff.

But I observe a bug (or expected behavior?) when testing this branch locally - if I upload more than 5 files, some orientations are lost.

I can reproduce this very consistently. Can you help check check? 🙏

6 images uploaded, only 8 files output

Screen.Recording.2024-08-30.at.3.17.56.AM.mp4

@YayunHuang
Copy link
Contributor Author

@pkong-ds Fixed! I found that's because we add event listener in runPreviewWorker and runWoker but didn't remove them so some worker have multiple event listener so some mockups were replaced with incorrect images, fix by add {once: true} in addEventListener

Copy link
Collaborator

@pkong-ds pkong-ds left a comment

Choose a reason for hiding this comment

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

@YayunHuang Thanks a lot for your work🙏 🙏 LGTM

@pkong-ds pkong-ds merged commit b219d58 into oursky:main Aug 30, 2024
1 check passed
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.

2 participants