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

Added bodySegmentation.getPartsId() #219

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

alanvww
Copy link
Member

@alanvww alanvww commented Oct 14, 2024

@shiffman @ziyuan-linn
As we discussed in ml5js/ml5-website-v02-docsify#88 & #139 , this PR is to wrap the palette in a public method called getPartsId() within the bodySegmentation. This method will return an object containing all the constants as key-value pairs, similar to the getSkeleton() method in BodyPose.
CleanShot 2024-10-14 at 15 06 00@2x
As we are removing the properties section from the model documentation page, this public method ensures that the data remains accessible to all users and aligns with the consistency of other models.

@shiffman
Copy link
Member

Fantastic! This will go along well with #211 as well! I hope to add that soon as I will be recording a faceMesh video tutorial next.

@shiffman shiffman requested a review from gohai October 15, 2024 15:19
@shiffman
Copy link
Member

@gohai since this continues your work from #139 I thought I'd check with you to review before merging. Thank you!

@shiffman shiffman mentioned this pull request Oct 15, 2024
@gohai
Copy link
Member

gohai commented Oct 19, 2024

This looks good to me @alanvww.

One stylistic nitpick: in the example, one line is missing the trailing semicolon

In #139, we added two constants to MediaPipeSelfieSegmentation as well. I am thinking it's probably best to remove them with the change now.

this.BACKGROUND = 0;

@alanvww
Copy link
Member Author

alanvww commented Oct 19, 2024

This looks good to me @alanvww.

One stylistic nitpick: in the example, one line is missing the trailing semicolon

In #139, we added two constants to MediaPipeSelfieSegmentation as well. I am thinking it's probably best to remove them with the change now.

this.BACKGROUND = 0;

Thanks, @gohai! I’ll commit the changes soon. Regarding the constants in SelfieSegmentation, I double-check that there aren’t any current examples using them. However, even if we remove them, it might be a good idea to add more documentation about those properties? @shiffman @MOQN

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.

Looks great to me! I agree we can remove the selfie segmentation constants. Normally we might "deprecate" the property, but I did a search and they appear nowhere in the examples or documentation so I think it's good to remove.

examples/bodySegmentation-select-body-parts/sketch.js Outdated Show resolved Hide resolved
src/BodySegmentation/index.js Outdated Show resolved Hide resolved
@alanvww
Copy link
Member Author

alanvww commented Oct 21, 2024

@gohai @shiffman Thanks for the reviews! I’ve just made the changes to the code format, removing the selfie segmentation constants.

@shiffman
Copy link
Member

Fantastic work, thank you! Merging! I may have some more thoughts or questions when I get to recording a video tutorial about this, stay tuned!

@shiffman shiffman merged commit cb4118f into ml5js:main Oct 22, 2024
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.

3 participants