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

BodyPose cleanup #178

Merged
merged 5 commits into from
Aug 6, 2024
Merged

BodyPose cleanup #178

merged 5 commits into from
Aug 6, 2024

Conversation

ziyuan-linn
Copy link
Member

@ziyuan-linn ziyuan-linn commented Jul 25, 2024

This PR updates the comments and refactors the code of BodyPose. No change to the user API.

  • update incorrect license info: MIT License -> ml5.js License.
  • add file description and helpful links at the beginning of the file.
  • format and clean up options object JSDoc comment.
  • move mold objects/schemas for handleOptions next to the options object comments.
  • add comment description to each property of BodyPose class.
  • update various comments for clarity
  • refactor some variable names for clarity

This PR also removes the window._incrementPreload and window._decrementPreload calls to address the bug in #175.

@ziyuan-linn ziyuan-linn requested a review from alanvww July 25, 2024 00:34
@ziyuan-linn ziyuan-linn mentioned this pull request Jul 25, 2024
10 tasks
Copy link
Member

@shiffman shiffman left a comment

Choose a reason for hiding this comment

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

This looks great, thank you @ziyuan-linn! Looks like you tagged @alanvww for a review, I'm happy to merge afterwards!

@alanvww
Copy link
Member

alanvww commented Jul 26, 2024

Awesome work! Thanks for letting me know about the update, I will work on the doc ASAP.

@shiffman We are ready to merge!

@lindapaiste
Copy link
Contributor

Thoughts:

It might make sense to move functions like mirrorBoundingBox to a utility file, if we want to use that logic in multiple models.

There are some types that can be more specific, but it’s annoying if we have to write out the union of possible image types every time. The return types could also be more specific (an array of what?). We can define a type for the prediction object and instead of Promise it can be Promise<Array>. That might not be worth the effort, unless…

It would be really great if we can figure out a way to sync property descriptions between the code comments and the documentation.

@shiffman
Copy link
Member

shiffman commented Aug 6, 2024

Thank you for the feedback @lindapaiste, feel free to open any issues with proposals for these changes if you like!

@shiffman shiffman merged commit a88a3a1 into main Aug 6, 2024
@ziyuan-linn ziyuan-linn deleted the bodyPose-cleanup branch September 15, 2024 22:35
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.

4 participants