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

feat: Object Detection Sample #58

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ashikka
Copy link
Contributor

@ashikka ashikka commented Dec 2, 2021

This PR closes #55

What does it do?

This photoshop plugin sample is capable of detecting objects and recognizing them in images using 3rd-party APIs.

How to run the project?

Please refer to the README in the project for more information.

Sample Preview

Check out the sample preview here

Copy link
Contributor

@pklaschka pklaschka left a comment

Choose a reason for hiding this comment

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

First of all: Thank you very much for your contribution(/-s)! 😊! Here's my review for this PR:

General review

  • Change Request The PR currently includes the Unsplash Plugin Sample. It seems that the object detection branch is based on the Unsplash plugin sample. Since the Unsplash plugin requires further review, I would request that you remove the Unsplash sample plugin from this PR so that we can focus at the PR's topic here and I'll add an additional, dedicated review for the Unsplash Plugin over in feat: Added Unsplash plugin sample #52 to make it easier to understand/read.

Code review

  • nice to have it might have made sense to extract the highlight boxes to React-based code, i.e., have a state with a model describing where the boxes should be and then use the React-style { boxes.map(box => <div [...] />)} in the JSX to make it a little bit easier to read, but not a big deal

Functional Review

  • Change Request Please add instructions to the README.md on running the server in the backend folder
  • Installed dependencies using npm install in both the sample's root and the backend folder -> please mention that this has to get run in backend, as well, in the README.md.
  • Dependency installation was successful
  • Change Request backend/package.json should include a start script to make it easier to run.
  • Ran node . in the backend folder to continue functional test
  • Ran npm run build in sample's root folder, as per README.md => success
  • Added plugin using UXP Developer tool
  • Ran plugin in Photoshop
  • Success: Object Detection and result display worked
  • nice to have A button to use the current Photoshop document (instead of an aribitrary file) would be nice to include some "native" functionality, but out of scope for this PR => not affecting review

Conclusion

Overall, this looks great and I'm looking forward to merging it. Before we do, please change these three things:

  1. Remove the unsplash-plugin-sample from this PR; this gets a dedicated review in feat: Added Unsplash plugin sample #52
  2. Add a start script running node . to the backend/package.json
  3. Mention installing dependencies for and running the backend in the sample's root README.md

Thank you in advance 🙂

@pklaschka pklaschka added enhancement New feature or request New sample labels Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request New sample
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object detection Plugin Sample
2 participants